Bug 1328319 part 8 - Make subclasses of CounterStyle have threadsafe refcnt. r?heycam draft
authorXidorn Quan <me@upsuper.org>
Tue, 09 May 2017 13:07:49 +1000
changeset 575239 8cc762d7de9d3604a626d1d85913c3f6aa129388
parent 575238 398b60e64169902a6d99ccbc576cbf3add5952ad
child 575240 40129f084fba382fe9a4a8575cfe47f0d5f27e28
child 575248 9db9fa64a25d23b127757634f322165f24695ffb
child 575250 cc2ac81e46727788d0809249b14d6351c2096680
push id58005
push userxquan@mozilla.com
push dateWed, 10 May 2017 05:43:11 +0000
reviewersheycam
bugs1328319, 1077718, 1075336, 1077687
milestone55.0a1
Bug 1328319 part 8 - Make subclasses of CounterStyle have threadsafe refcnt. r?heycam Although their refcnt is threadsafe, their destroy functions are not, and thus the assertions there. It is not completely clear to me whether that's going to work. If not, we have two choices: 1. Stop using presshell arena for those objects, which is basically a backout of bug 1077718. This may regress security since we use kind of manual lifetime management for those objects, and the objects have vtable, which means if we make any mistake on lifetime somewhere, we may be in trouble (e.g. bug 1075336 or bug 1077687). 2. Collect all objects which are removed from the CounterStyleManager cache table into a clear list, and add another cleanup phase after we rebuild all the style structs. With that, we should be able to guarantee all objects are eventually released in the main thread. Proxy releasing to the main thread is not an option because: * we have to have PresShell in order to release the objects, but * we cannot put a strong reference of it into a release runner, thus we cannot guarantee that it outlives the release runner, because: * PresShell doesn't have threadsafe refcnt, and * you cannot hold a strong reference to it anywhere from the object, otherwise there would be a cycle reference via CounterStyleManager. MozReview-Commit-ID: 6wAdQhJ5tYY
layout/style/CounterStyleManager.cpp
layout/style/CounterStyleManager.h
--- a/layout/style/CounterStyleManager.cpp
+++ b/layout/style/CounterStyleManager.cpp
@@ -984,24 +984,25 @@ public:
   {
     return aPresContext->PresShell()->AllocateByObjectID(
         eArenaObjectID_DependentBuiltinCounterStyle, sz);
   }
 
 private:
   void Destroy()
   {
+    MOZ_ASSERT(NS_IsMainThread());
     nsIPresShell* shell = mManager->PresContext()->PresShell();
     this->~DependentBuiltinCounterStyle();
     shell->FreeByObjectID(eArenaObjectID_DependentBuiltinCounterStyle, this);
   }
 
   CounterStyleManager* mManager;
 
-  nsAutoRefCnt mRefCnt;
+  ThreadSafeAutoRefCnt mRefCnt;
   NS_DECL_OWNINGTHREAD
 };
 
 NS_IMPL_ADDREF(DependentBuiltinCounterStyle)
 NS_IMPL_RELEASE_WITH_DESTROY(DependentBuiltinCounterStyle, Destroy())
 
 /* virtual */ CounterStyle*
 DependentBuiltinCounterStyle::GetFallback()
@@ -1105,16 +1106,17 @@ public:
   {
     return aPresContext->PresShell()->AllocateByObjectID(
         eArenaObjectID_CustomCounterStyle, sz);
   }
 
 private:
   void Destroy()
   {
+    MOZ_ASSERT(NS_IsMainThread());
     nsIPresShell* shell = mManager->PresContext()->PresShell();
     this->~CustomCounterStyle();
     shell->FreeByObjectID(eArenaObjectID_CustomCounterStyle, this);
   }
 
   const nsTArray<nsString>& GetSymbols();
   const nsTArray<AdditiveSymbol>& GetAdditiveSymbols();
 
@@ -1186,17 +1188,17 @@ private:
   CounterStyle* mSpeakAsCounter;
 
   CounterStyle* mExtends;
   // This field refers to the last counter in the extends chain. The
   // counter must be either a builtin style or a style whose system is
   // not 'extends'.
   CounterStyle* mExtendsRoot;
 
-  nsAutoRefCnt mRefCnt;
+  ThreadSafeAutoRefCnt mRefCnt;
   NS_DECL_OWNINGTHREAD
 };
 
 NS_IMPL_ADDREF(CustomCounterStyle)
 NS_IMPL_RELEASE_WITH_DESTROY(CustomCounterStyle, Destroy())
 
 void
 CustomCounterStyle::ResetCachedData()
--- a/layout/style/CounterStyleManager.h
+++ b/layout/style/CounterStyleManager.h
@@ -127,17 +127,17 @@ public:
                                      bool& aIsRTL) override;
 
   virtual AnonymousCounterStyle* AsAnonymous() override { return this; }
 
   bool IsSingleString() const { return mSingleString; }
   uint8_t GetSystem() const { return mSystem; }
   const nsTArray<nsString>& GetSymbols() const { return mSymbols; }
 
-  NS_INLINE_DECL_REFCOUNTING(AnonymousCounterStyle, override)
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AnonymousCounterStyle, override)
 
 private:
   ~AnonymousCounterStyle() {}
 
   bool mSingleString;
   uint8_t mSystem;
   nsTArray<nsString> mSymbols;
 };