Bug 1363596 part 1 - Merge nsCOMPtr<nsIAtom> into CounterStylePtr. r=heycam draft
authorXidorn Quan <me@upsuper.org>
Thu, 25 May 2017 17:51:36 +1000
changeset 585621 f8256885e1be8bc2523d705b10fe1796c64c4ccd
parent 585620 54634261ff9a67200ee7031dd47987f0eeaea2f3
child 585622 7eccc8ee6a73c9756c58989dd1e408361dedf74f
push id61148
push userxquan@mozilla.com
push dateSat, 27 May 2017 10:48:14 +0000
reviewersheycam
bugs1363596
milestone55.0a1
Bug 1363596 part 1 - Merge nsCOMPtr<nsIAtom> into CounterStylePtr. r=heycam MozReview-Commit-ID: D2fdlrC3yop
js/src/devtools/rootAnalysis/analyzeHeapWrites.js
layout/style/CounterStyleManager.h
layout/style/ServoBindings.cpp
layout/style/ServoBindings.h
layout/style/ServoBindings.toml
layout/style/nsStyleStruct.cpp
layout/style/nsStyleStruct.h
--- a/js/src/devtools/rootAnalysis/analyzeHeapWrites.js
+++ b/js/src/devtools/rootAnalysis/analyzeHeapWrites.js
@@ -86,16 +86,17 @@ function checkOverridableVirtualCall(ent
     if (hasThreadsafeReferenceCounts(entry, /nsCOMPtr<T>::assign_with_AddRef.*?\[with T = (.*?)\]/))
         return;
 
     // Watch for raw addref/release.
     var whitelist = [
         "Gecko_AddRefAtom",
         "Gecko_ReleaseAtom",
         /nsPrincipal::Get/,
+        /CounterStylePtr::Reset/,
     ];
     if (entry.matches(whitelist))
         return;
 
     dumpError(entry, location, "AddRef/Release on nsISupports");
 }
 
 function checkIndirectCall(entry, location, callee)
@@ -140,18 +141,18 @@ function treatAsSafeArgument(entry, varN
         // RawGeckoBorrowedNode thread-mutable parameters.
         ["Gecko_SetNodeFlags", "aNode", null],
         ["Gecko_UnsetNodeFlags", "aNode", null],
 
         // Various Servo binding out parameters. This is a mess and there needs
         // to be a way to indicate which params are out parameters, either using
         // an attribute or a naming convention.
         ["Gecko_CopyFontFamilyFrom", "dst", null],
-        ["Gecko_SetListStyleType", "aList", null],
-        ["Gecko_CopyListStyleTypeFrom", "aDst", null],
+        ["Gecko_SetCounterStyleToName", "aPtr", null],
+        ["Gecko_CopyCounterStyle", "aDst", null],
         ["Gecko_SetMozBinding", "aDisplay", null],
         [/ClassOrClassList/, /aClass/, null],
         ["Gecko_GetAtomAsUTF16", "aLength", null],
         ["Gecko_CopyMozBindingFrom", "aDest", null],
         ["Gecko_SetNullImageValue", "aImage", null],
         ["Gecko_SetGradientImageValue", "aImage", null],
         ["Gecko_SetImageOrientation", "aVisibility", null],
         ["Gecko_SetImageOrientationAsFromImage", "aVisibility", null],
--- a/layout/style/CounterStyleManager.h
+++ b/layout/style/CounterStyleManager.h
@@ -145,18 +145,28 @@ private:
 // managed by counter style manager.
 class CounterStylePtr
 {
 public:
   CounterStylePtr() : mRaw(0) {}
   CounterStylePtr(const CounterStylePtr& aOther)
     : mRaw(aOther.mRaw)
   {
-    if (IsAnonymous()) {
-      AsAnonymous()->AddRef();
+    switch (GetType()) {
+      case eCounterStyle:
+        break;
+      case eAnonymousCounterStyle:
+        AsAnonymous()->AddRef();
+        break;
+      case eUnresolvedAtom:
+        AsAtom()->AddRef();
+        break;
+      case eMask:
+        MOZ_ASSERT_UNREACHABLE("Unknown type");
+        break;
     }
   }
   CounterStylePtr(CounterStylePtr&& aOther)
     : mRaw(aOther.mRaw)
   {
     aOther.mRaw = 0;
   }
   ~CounterStylePtr() { Reset(); }
@@ -164,88 +174,139 @@ public:
   CounterStylePtr& operator=(const CounterStylePtr& aOther)
   {
     if (this != &aOther) {
       Reset();
       new (this) CounterStylePtr(aOther);
     }
     return *this;
   }
+  CounterStylePtr& operator=(CounterStylePtr&& aOther)
+  {
+    if (this != &aOther) {
+      Reset();
+      mRaw = aOther.mRaw;
+      aOther.mRaw = 0;
+    }
+    return *this;
+  }
   CounterStylePtr& operator=(decltype(nullptr))
   {
     Reset();
     return *this;
   }
+  CounterStylePtr& operator=(already_AddRefed<nsIAtom> aAtom)
+  {
+    Reset();
+    if (nsIAtom* raw = aAtom.take()) {
+      AssertPointerAligned(raw);
+      mRaw = reinterpret_cast<uintptr_t>(raw) | eUnresolvedAtom;
+    }
+    return *this;
+  }
   CounterStylePtr& operator=(AnonymousCounterStyle* aCounterStyle)
   {
     Reset();
     if (aCounterStyle) {
       CounterStyle* raw = do_AddRef(aCounterStyle).take();
       AssertPointerAligned(raw);
-      mRaw = reinterpret_cast<uintptr_t>(raw) | kAnonymousFlag;
+      mRaw = reinterpret_cast<uintptr_t>(raw) | eAnonymousCounterStyle;
     }
     return *this;
   }
   CounterStylePtr& operator=(CounterStyle* aCounterStyle)
   {
     Reset();
     if (aCounterStyle) {
       MOZ_ASSERT(!aCounterStyle->AsAnonymous());
       AssertPointerAligned(aCounterStyle);
-      mRaw = reinterpret_cast<uintptr_t>(aCounterStyle);
+      mRaw = reinterpret_cast<uintptr_t>(aCounterStyle) | eCounterStyle;
     }
     return *this;
   }
 
   operator CounterStyle*() const & { return Get(); }
   operator CounterStyle*() const && = delete;
   CounterStyle* operator->() const { return Get(); }
   explicit operator bool() const { return !!mRaw; }
   bool operator!() const { return !mRaw; }
   bool operator==(const CounterStylePtr& aOther) const
     { return mRaw == aOther.mRaw; }
   bool operator!=(const CounterStylePtr& aOther) const
     { return mRaw != aOther.mRaw; }
 
+  bool IsResolved() const { return !IsUnresolved(); }
+  inline void Resolve(CounterStyleManager* aManager);
+
 private:
   CounterStyle* Get() const
   {
-    return reinterpret_cast<CounterStyle*>(mRaw & ~kAnonymousFlag);
+    MOZ_ASSERT(IsResolved());
+    return reinterpret_cast<CounterStyle*>(mRaw & ~eMask);
   }
-  void AssertPointerAligned(CounterStyle* aPointer)
+  template<typename T>
+  void AssertPointerAligned(T* aPointer)
   {
     // This can be checked at compile time via
-    // > static_assert(alignof(CounterStyle) >= 2);
+    // > static_assert(alignof(CounterStyle) >= 4);
+    // > static_assert(alignof(nsIAtom) >= 4);
     // but MSVC2015 doesn't support using alignof on an abstract class.
     // Once we move to MSVC2017, we can replace this runtime check with
     // the compile time check above.
-    MOZ_ASSERT(!(reinterpret_cast<uintptr_t>(aPointer) & kAnonymousFlag));
+    MOZ_ASSERT(!(reinterpret_cast<uintptr_t>(aPointer) & eMask));
   }
 
-  bool IsAnonymous() const { return !!(mRaw & kAnonymousFlag); }
+  enum Type : uintptr_t {
+    eCounterStyle = 0,
+    eAnonymousCounterStyle = 1,
+    eUnresolvedAtom = 2,
+    eMask = 3,
+  };
+
+  Type GetType() const { return static_cast<Type>(mRaw & eMask); }
+  bool IsUnresolved() const { return GetType() == eUnresolvedAtom; }
+  bool IsAnonymous() const { return GetType() == eAnonymousCounterStyle; }
+  nsIAtom* AsAtom()
+  {
+    MOZ_ASSERT(IsUnresolved());
+    return reinterpret_cast<nsIAtom*>(mRaw & ~eMask);
+  }
   AnonymousCounterStyle* AsAnonymous()
   {
     MOZ_ASSERT(IsAnonymous());
     return static_cast<AnonymousCounterStyle*>(
-      reinterpret_cast<CounterStyle*>(mRaw & ~kAnonymousFlag));
+      reinterpret_cast<CounterStyle*>(mRaw & ~eMask));
   }
 
   void Reset()
   {
-    if (IsAnonymous()) {
-      AsAnonymous()->Release();
+    switch (GetType()) {
+      case eCounterStyle:
+        break;
+      case eAnonymousCounterStyle:
+        AsAnonymous()->Release();
+        break;
+      case eUnresolvedAtom:
+        AsAtom()->Release();
+        break;
+      case eMask:
+        MOZ_ASSERT_UNREACHABLE("Unknown type");
+        break;
     }
     mRaw = 0;
   }
 
-  // mRaw contains the pointer, and its last bit is used for the flag.
-  // If the flag is set, this pointer owns an AnonymousCounterStyle,
-  // otherwise, it is a weak pointer referring a named counter style
+  // mRaw contains the pointer, and its last two bits are used for type
+  // of the pointer.
+  // If the type is eUnresolvedAtom, the pointer owns a reference to an
+  // nsIAtom, and it needs to be resolved to a counter style before use.
+  // If the type is eAnonymousCounterStyle, it owns a reference to an
+  // anonymous counter style.
+  // Otherwise it is a weak pointer referring a named counter style
   // managed by CounterStyleManager.
-  static const uintptr_t kAnonymousFlag = 1;
   uintptr_t mRaw;
 };
 
 class CounterStyleManager final
 {
 private:
   ~CounterStyleManager();
 public:
@@ -290,11 +351,19 @@ public:
 private:
   void DestroyCounterStyle(CounterStyle* aCounterStyle);
 
   nsPresContext* mPresContext;
   nsDataHashtable<nsRefPtrHashKey<nsIAtom>, CounterStyle*> mStyles;
   nsTArray<CounterStyle*> mRetiredStyles;
 };
 
+void
+CounterStylePtr::Resolve(CounterStyleManager* aManager)
+{
+  if (IsUnresolved()) {
+    *this = aManager->BuildCounterStyle(AsAtom());
+  }
+}
+
 } // namespace mozilla
 
 #endif /* !defined(mozilla_CounterStyleManager_h_) */
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -1154,25 +1154,25 @@ Gecko_SetImageOrientationAsFromImage(nsS
 void
 Gecko_CopyImageOrientationFrom(nsStyleVisibility* aDst,
                                const nsStyleVisibility* aSrc)
 {
   aDst->mImageOrientation = aSrc->mImageOrientation;
 }
 
 void
-Gecko_SetListStyleType(nsStyleList* aList, nsIAtom* aName)
+Gecko_SetCounterStyleToName(CounterStylePtr* aPtr, nsIAtom* aName)
 {
-  aList->SetListStyleType(aName);
+  *aPtr = already_AddRefed<nsIAtom>(aName);
 }
 
 void
-Gecko_CopyListStyleTypeFrom(nsStyleList* aDst, const nsStyleList* aSrc)
+Gecko_CopyCounterStyle(CounterStylePtr* aDst, const CounterStylePtr* aSrc)
 {
-  aDst->CopyListStyleTypeFrom(*aSrc);
+  *aDst = *aSrc;
 }
 
 already_AddRefed<css::URLValue>
 ServoBundledURI::IntoCssUrl()
 {
   if (!mURLString) {
     return nullptr;
   }
--- a/layout/style/ServoBindings.h
+++ b/layout/style/ServoBindings.h
@@ -269,18 +269,20 @@ void Gecko_nsFont_Destroy(nsFont* dst);
 void Gecko_SetImageOrientation(nsStyleVisibility* aVisibility,
                                double aRadians,
                                bool aFlip);
 void Gecko_SetImageOrientationAsFromImage(nsStyleVisibility* aVisibility);
 void Gecko_CopyImageOrientationFrom(nsStyleVisibility* aDst,
                                     const nsStyleVisibility* aSrc);
 
 // Counter style.
-void Gecko_SetListStyleType(nsStyleList* style_struct, nsIAtom* name);
-void Gecko_CopyListStyleTypeFrom(nsStyleList* dst, const nsStyleList* src);
+// This function takes an already addrefed nsIAtom
+void Gecko_SetCounterStyleToName(mozilla::CounterStylePtr* ptr, nsIAtom* name);
+void Gecko_CopyCounterStyle(mozilla::CounterStylePtr* dst,
+                            const mozilla::CounterStylePtr* src);
 
 // background-image style.
 void Gecko_SetNullImageValue(nsStyleImage* image);
 void Gecko_SetGradientImageValue(nsStyleImage* image, nsStyleGradient* gradient);
 NS_DECL_THREADSAFE_FFI_REFCOUNTING(mozilla::css::ImageValue, ImageValue);
 mozilla::css::ImageValue* Gecko_ImageValue_Create(ServoBundledURI aURI);
 void Gecko_SetLayerImageImageValue(nsStyleImage* image,
                                    mozilla::css::ImageValue* aImageValue);
--- a/layout/style/ServoBindings.toml
+++ b/layout/style/ServoBindings.toml
@@ -339,16 +339,17 @@ structs-types = [
     "RawGeckoServoStyleRuleList",
     "RawGeckoURLExtraData",
     "RefPtr",
     "CSSPseudoClassType",
     "CSSPseudoElementType",
     "TraversalRestyleBehavior",
     "TraversalRootBehavior",
     "ComputedTimingFunction_BeforeFlag",
+    "CounterStylePtr",
     "FontFamilyList",
     "FontFamilyType",
     "FontSizePrefs",
     "GeckoFontMetrics",
     "IterationCompositeOperation",
     "Keyframe",
     "ServoBundledURI",
     "ServoElementSnapshot",
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -598,55 +598,49 @@ nsStyleOutline::CalcDifference(const nsS
 // --------------------
 // nsStyleList
 //
 nsStyleList::nsStyleList(const nsPresContext* aContext)
   : mListStylePosition(NS_STYLE_LIST_STYLE_POSITION_OUTSIDE)
 {
   MOZ_COUNT_CTOR(nsStyleList);
   if (aContext->StyleSet()->IsServo()) {
-    mListStyleType = nsGkAtoms::disc;
+    mCounterStyle = do_AddRef(nsGkAtoms::disc);
   } else {
     mCounterStyle = aContext->
       CounterStyleManager()->BuildCounterStyle(nsGkAtoms::disc);
   }
   SetQuotesInitial();
 }
 
 nsStyleList::~nsStyleList()
 {
   MOZ_COUNT_DTOR(nsStyleList);
 }
 
 nsStyleList::nsStyleList(const nsStyleList& aSource)
   : mListStylePosition(aSource.mListStylePosition)
   , mListStyleImage(aSource.mListStyleImage)
-  , mListStyleType(aSource.mListStyleType)
   , mCounterStyle(aSource.mCounterStyle)
   , mQuotes(aSource.mQuotes)
   , mImageRegion(aSource.mImageRegion)
 {
   MOZ_COUNT_CTOR(nsStyleList);
 }
 
 void
 nsStyleList::FinishStyle(nsPresContext* aPresContext)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
 
   if (mListStyleImage && !mListStyleImage->IsResolved()) {
     mListStyleImage->Resolve(aPresContext);
   }
-  if (mListStyleType) {
-    MOZ_ASSERT(!mCounterStyle);
-    mCounterStyle = aPresContext->
-      CounterStyleManager()->BuildCounterStyle(mListStyleType);
-    mListStyleType = nullptr;
-  }
+  mCounterStyle.Resolve(aPresContext->CounterStyleManager());
 }
 
 void
 nsStyleList::SetQuotesInherit(const nsStyleList* aOther)
 {
   mQuotes = aOther->mQuotes;
 }
 
@@ -704,18 +698,17 @@ nsStyleList::CalcDifference(const nsStyl
       (mQuotes || aNewData.mQuotes) &&
       GetQuotePairs() != aNewData.GetQuotePairs()) {
     return nsChangeHint_ReconstructFrame;
   }
   if (mListStylePosition != aNewData.mListStylePosition) {
     return nsChangeHint_ReconstructFrame;
   }
   if (DefinitelyEqualImages(mListStyleImage, aNewData.mListStyleImage) &&
-      (mCounterStyle == aNewData.mCounterStyle ||
-       mListStyleType != aNewData.mListStyleType)) {
+      mCounterStyle == aNewData.mCounterStyle) {
     if (mImageRegion.IsEqualInterior(aNewData.mImageRegion)) {
       return nsChangeHint(0);
     }
     if (mImageRegion.width == aNewData.mImageRegion.width &&
         mImageRegion.height == aNewData.mImageRegion.height) {
       return NS_STYLE_HINT_VISUAL;
     }
   }
@@ -3792,18 +3785,17 @@ nsStyleContentData::nsStyleContentData(c
 }
 
 bool
 nsStyleContentData::
 CounterFunction::operator==(const CounterFunction& aOther) const
 {
   return mIdent == aOther.mIdent &&
     mSeparator == aOther.mSeparator &&
-    mCounterStyle == aOther.mCounterStyle &&
-    mCounterStyleName == aOther.mCounterStyleName;
+    mCounterStyle == aOther.mCounterStyle;
 }
 
 nsStyleContentData&
 nsStyleContentData::operator=(const nsStyleContentData& aOther)
 {
   if (this == &aOther) {
     return *this;
   }
@@ -3835,23 +3827,18 @@ nsStyleContentData::Resolve(nsPresContex
   switch (mType) {
     case eStyleContentType_Image:
       if (!mContent.mImage->IsResolved()) {
         mContent.mImage->Resolve(aPresContext);
       }
       break;
     case eStyleContentType_Counter:
     case eStyleContentType_Counters: {
-      CounterFunction* counters = mContent.mCounters;
-      if (counters->mCounterStyleName) {
-        MOZ_ASSERT(!counters->mCounterStyle);
-        counters->mCounterStyle = aPresContext->CounterStyleManager()->
-          BuildCounterStyle(counters->mCounterStyleName);
-        counters->mCounterStyleName = nullptr;
-      }
+      mContent.mCounters->
+        mCounterStyle.Resolve(aPresContext->CounterStyleManager());
       break;
     }
     default:
       break;
   }
 }
 
 
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@@ -1463,45 +1463,26 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsSt
 
   imgRequestProxy* GetListStyleImage() const
   {
     return mListStyleImage ? mListStyleImage->get() : nullptr;
   }
 
   already_AddRefed<nsIURI> GetListStyleImageURI() const;
 
-  // The following two methods are called from Servo code to maintain
-  // list-style-type off main thread.
-  void SetListStyleType(nsIAtom* aType)
-  {
-    mListStyleType = aType;
-    mCounterStyle = nullptr;
-  }
-  void CopyListStyleTypeFrom(const nsStyleList& aOther)
-  {
-    mListStyleType = aOther.mListStyleType;
-    mCounterStyle = aOther.mCounterStyle;
-  }
-
   const nsStyleQuoteValues::QuotePairArray& GetQuotePairs() const;
 
   void SetQuotesInherit(const nsStyleList* aOther);
   void SetQuotesInitial();
   void SetQuotesNone();
   void SetQuotes(nsStyleQuoteValues::QuotePairArray&& aValues);
 
   uint8_t mListStylePosition;                  // [inherited]
   RefPtr<nsStyleImageRequest> mListStyleImage; // [inherited]
 
-  // mCounterStyle is the actual field for computed list-style-type.
-  // mListStyleType is only used when we are off the main thread, so we
-  // cannot safely construct CounterStyle object. FinishStyle() will
-  // use it to setup mCounterStyle and then clear it. At any time, only
-  // one of the following two fields should be non-null.
-  nsCOMPtr<nsIAtom> mListStyleType;
   mozilla::CounterStylePtr mCounterStyle;      // [inherited]
 
 private:
   RefPtr<nsStyleQuoteValues> mQuotes;   // [inherited]
   nsStyleList& operator=(const nsStyleList& aOther) = delete;
 public:
   nsRect        mImageRegion;           // [inherited] the rect to use within an image
 
@@ -3043,36 +3024,33 @@ public:
     return mContent.mString;
   }
 
   struct CounterFunction
   {
     nsString mIdent;
     // This is only used when it is a counters() function.
     nsString mSeparator;
-    // One and only one of mCounterStyle and mCounterStyleName must be
-    // non-null at any time.
     mozilla::CounterStylePtr mCounterStyle;
-    nsCOMPtr<nsIAtom> mCounterStyleName;
 
     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CounterFunction)
 
     bool operator==(const CounterFunction& aOther) const;
     bool operator!=(const CounterFunction& aOther) const {
       return !(*this == aOther);
     }
   private:
     ~CounterFunction() {}
   };
 
   CounterFunction* GetCounters() const
   {
     MOZ_ASSERT(mType == eStyleContentType_Counter ||
                mType == eStyleContentType_Counters);
-    MOZ_ASSERT(mContent.mCounters->mCounterStyle,
+    MOZ_ASSERT(mContent.mCounters->mCounterStyle.IsResolved(),
                "Counter style should have been resolved");
     return mContent.mCounters;
   }
 
   nsStyleImageRequest* GetImageRequest() const
   {
     MOZ_ASSERT(mType == eStyleContentType_Image);
     return mContent.mImage;