Bug 1675820 - Part 6: Make DocGroup cycle-collected, r=farre,smaug

Previously, the DocGroup type was not cycle-collected, as it needed to have
references from other threads for Quantum DOM. Nowadays the only off-main-thread
use of DocGroup is for dispatching runnables to the main thread which should be
tracked using a performance counter for about:performance. This means we can
remove the DocGroup references from these dispatching callsites, only storing
the Performance Counter we're interested in, and simplify make DocGroup be
cycle-collected itself.

This fixes a leak caused by adding the WindowGlobalChild getter to
WindowContext, by allowing cycles between the document and its BrowsingContext
to be broken by the cycle-collector.

Differential Revision: https://phabricator.services.mozilla.com/D108865

docshell/base/BrowsingContextGroup.cpp

@@ -451,7 +451,8 @@ void BrowsingContextGroup::GetAllGroups(
 
 NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(BrowsingContextGroup, mContexts,
                                       mToplevels, mHosts, mSubscribers,
-                                      mTimerEventQueue, mWorkerEventQueue)
+                                      mTimerEventQueue, mWorkerEventQueue,
+                                      mDocGroups)
 
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(BrowsingContextGroup, AddRef)
 NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(BrowsingContextGroup, Release)

dom/base/DocGroup.cpp

@@ -40,16 +40,11 @@ namespace {
 // created it.
 class LabellingEventTarget final : public nsISerialEventTarget,
                                    public nsIDirectTaskDispatcher {
-  // This creates a cycle with DocGroup. Therefore, when DocGroup
-  // looses its last Document, the DocGroup of the
-  // LabellingEventTarget needs to be cleared.
-  RefPtr<mozilla::dom::DocGroup> mDocGroup;
-
  public:
   NS_DECLARE_STATIC_IID_ACCESSOR(NS_LABELLINGEVENTTARGET_IID)
 
-  explicit LabellingEventTarget(mozilla::dom::DocGroup* aDocGroup)
-      : mDocGroup(aDocGroup),
+  explicit LabellingEventTarget(mozilla::PerformanceCounter* aPerformanceCounter)
+      : mPerformanceCounter(aPerformanceCounter),
         mMainThread(
             static_cast<nsThread*>(mozilla::GetMainThreadSerialEventTarget())) {
   }
@@ -60,6 +55,7 @@ class LabellingEventTarget final : public nsISerialEventTarget,
 
  private:
   ~LabellingEventTarget() = default;
+  const RefPtr<mozilla::PerformanceCounter> mPerformanceCounter;
   const RefPtr<nsThread> mMainThread;
 };
 
@@ -80,8 +76,8 @@ LabellingEventTarget::Dispatch(already_AddRefed<nsIRunnable> aRunnable,
     return NS_ERROR_UNEXPECTED;
   }
 
-  return mozilla::SchedulerGroup::DispatchWithDocGroup(
-      mozilla::TaskCategory::Other, std::move(aRunnable), mDocGroup);
+  return mozilla::SchedulerGroup::LabeledDispatch(
+      mozilla::TaskCategory::Other, std::move(aRunnable), mPerformanceCounter);
 }
 
 NS_IMETHODIMP
@@ -125,11 +121,31 @@ namespace mozilla::dom {
 
 AutoTArray<RefPtr<DocGroup>, 2>* DocGroup::sPendingDocGroups = nullptr;
 
+NS_IMPL_CYCLE_COLLECTION_CLASS(DocGroup)
+
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DocGroup)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSignalSlotList)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBrowsingContextGroup)
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DocGroup)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mSignalSlotList)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mBrowsingContextGroup)
+
+  // If we still have any documents in this array, they were just unlinked, so
+  // clear out our weak pointers to them.
+  tmp->mDocuments.Clear();
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+
+NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(DocGroup, AddRef)
+NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(DocGroup, Release)
+
 /* static */
 already_AddRefed<DocGroup> DocGroup::Create(
     BrowsingContextGroup* aBrowsingContextGroup, const nsACString& aKey) {
   RefPtr<DocGroup> docGroup = new DocGroup(aBrowsingContextGroup, aKey);
-  docGroup->mEventTarget = new LabellingEventTarget(docGroup);
+  docGroup->mEventTarget =
+      new LabellingEventTarget(docGroup->GetPerformanceCounter());
   return docGroup.forget();
 }
 
@@ -175,8 +191,6 @@ void DocGroup::RemoveDocument(Document* aDocument) {
 
   if (mDocuments.IsEmpty()) {
     mBrowsingContextGroup = nullptr;
-    // This clears the cycle DocGroup has with LabellingEventTarget.
-    mEventTarget = nullptr;
   }
 }
 
@@ -196,15 +210,8 @@ DocGroup::DocGroup(BrowsingContextGroup* aBrowsingContextGroup,
 }
 
 DocGroup::~DocGroup() {
+  MOZ_RELEASE_ASSERT(NS_IsMainThread());
   MOZ_RELEASE_ASSERT(mDocuments.IsEmpty());
-  MOZ_RELEASE_ASSERT(!mBrowsingContextGroup);
-
-  if (!NS_IsMainThread()) {
-    NS_ReleaseOnMainThread("DocGroup::mReactionsStack",
-                           mReactionsStack.forget());
-
-    NS_ReleaseOnMainThread("DocGroup::mArena", mArena.forget());
-  }
 
   if (mIframePostMessageQueue) {
     FlushIframePostMessageQueue();
@@ -304,23 +311,23 @@ RefPtr<PerformanceInfoPromise> DocGroup::ReportPerformanceInfo() {
 
 nsresult DocGroup::Dispatch(TaskCategory aCategory,
                             already_AddRefed<nsIRunnable>&& aRunnable) {
+  MOZ_RELEASE_ASSERT(NS_IsMainThread());
+
   if (mPerformanceCounter) {
     mPerformanceCounter->IncrementDispatchCounter(DispatchCategory(aCategory));
   }
-  return SchedulerGroup::DispatchWithDocGroup(aCategory, std::move(aRunnable),
-                                              this);
+  return SchedulerGroup::LabeledDispatch(aCategory, std::move(aRunnable),
+                                         mPerformanceCounter);
 }
 
 nsISerialEventTarget* DocGroup::EventTargetFor(TaskCategory aCategory) const {
+  MOZ_RELEASE_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!mDocuments.IsEmpty());
+
   // Here we have the same event target for every TaskCategory. The
   // reason for that is that currently TaskCategory isn't used, and
   // it's unsure if it ever will be (See Bug 1624819).
-  if (mEventTarget) {
-    return mEventTarget;
-  }
-
-  return GetMainThreadSerialEventTarget();
+  return mEventTarget;
 }
 
 AbstractThread* DocGroup::AbstractMainThreadFor(TaskCategory aCategory) {

dom/base/DocGroup.h

@@ -41,7 +41,8 @@ class DocGroup final {
  public:
   typedef nsTArray<Document*>::iterator Iterator;
 
-  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DocGroup)
+  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(DocGroup)
+  NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(DocGroup)
 
   static already_AddRefed<DocGroup> Create(
       BrowsingContextGroup* aBrowsingContextGroup, const nsACString& aKey);

dom/base/Document.cpp

@@ -2398,6 +2398,7 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(Document)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPrototypeDocument)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMidasCommandManager)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAll)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocGroup)
 
   // Traverse all our nsCOMArrays.
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPreloadingImages)
@@ -2529,6 +2530,12 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Document)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mReferrerInfo)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mPreloadReferrerInfo)
 
+  if (tmp->mDocGroup && tmp->mDocGroup->GetBrowsingContextGroup()) {
+    tmp->mDocGroup->GetBrowsingContextGroup()->RemoveDocument(
+        tmp->mDocGroup->GetKey(), tmp);
+  }
+  tmp->mDocGroup = nullptr;
+
   if (tmp->IsTopLevelContentDocument()) {
     RemoveToplevelLoadingDocument(tmp);
   }

dom/script/ScriptLoader.cpp

@@ -2124,7 +2124,7 @@ namespace {
 class NotifyOffThreadScriptLoadCompletedRunnable : public Runnable {
   RefPtr<ScriptLoadRequest> mRequest;
   RefPtr<ScriptLoader> mLoader;
-  RefPtr<DocGroup> mDocGroup;
+  nsCOMPtr<nsISerialEventTarget> mEventTarget;
   JS::OffThreadToken* mToken;
 
  public:
@@ -2135,9 +2135,11 @@ class NotifyOffThreadScriptLoadCompletedRunnable : public Runnable {
       : Runnable("dom::NotifyOffThreadScriptLoadCompletedRunnable"),
         mRequest(aRequest),
         mLoader(aLoader),
-        mDocGroup(aLoader->GetDocGroup()),
         mToken(nullptr) {
     MOZ_ASSERT(NS_IsMainThread());
+    if (DocGroup* docGroup = aLoader->GetDocGroup()) {
+      mEventTarget = docGroup->EventTargetFor(TaskCategory::Other);
+    }
   }
 
   virtual ~NotifyOffThreadScriptLoadCompletedRunnable();
@@ -2150,8 +2152,8 @@ class NotifyOffThreadScriptLoadCompletedRunnable : public Runnable {
   static void Dispatch(
       already_AddRefed<NotifyOffThreadScriptLoadCompletedRunnable>&& aSelf) {
     RefPtr<NotifyOffThreadScriptLoadCompletedRunnable> self = aSelf;
-    RefPtr<DocGroup> docGroup = self->mDocGroup;
-    docGroup->Dispatch(TaskCategory::Other, self.forget());
+    nsCOMPtr<nsISerialEventTarget> eventTarget = self->mEventTarget;
+    eventTarget->Dispatch(self.forget());
   }
 
   NS_DECL_NSIRUNNABLE

layout/style/Loader.cpp

@@ -2181,6 +2181,7 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(Loader)
   for (nsCOMPtr<nsICSSLoaderObserver>& obs : tmp->mObservers.ForwardRange()) {
     ImplCycleCollectionTraverse(cb, obs, "mozilla::css::Loader.mObservers");
   }
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocGroup)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Loader)
@@ -2192,6 +2193,7 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Loader)
   }
   tmp->mInlineSheets.Clear();
   tmp->mObservers.Clear();
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocGroup)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(Loader, AddRef)

parser/html/nsHtml5StreamParser.cpp

@@ -1256,14 +1256,15 @@ nsresult nsHtml5StreamParser::OnStartRequest(nsIRequest* aRequest) {
   // previous normal load in the history.
   mReparseForbidden = !(mMode == NORMAL || mMode == PLAIN_TEXT);
 
-  mDocGroup = mExecutor->GetDocument()->GetDocGroup();
+  mNetworkEventTarget =
+      mExecutor->GetDocument()->EventTargetFor(TaskCategory::Network);
 
   nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mRequest, &rv));
   if (NS_SUCCEEDED(rv)) {
     // Non-HTTP channels are bogus enough that we let them work with unlabeled
     // runnables for now. Asserting for HTTP channels only.
-    MOZ_ASSERT(mDocGroup || mMode == LOAD_AS_DATA,
-               "How come the doc group is still null?");
+    MOZ_ASSERT(mNetworkEventTarget || mMode == LOAD_AS_DATA,
+               "How come the network event target is still null?");
 
     nsAutoCString method;
     Unused << httpChannel->GetRequestMethod(method);
@@ -2189,8 +2190,8 @@ void nsHtml5StreamParser::MarkAsBroken(nsresult aRv) {
 
 nsresult nsHtml5StreamParser::DispatchToMain(
     already_AddRefed<nsIRunnable>&& aRunnable) {
-  if (mDocGroup) {
-    return mDocGroup->Dispatch(TaskCategory::Network, std::move(aRunnable));
+  if (mNetworkEventTarget) {
+    return mNetworkEventTarget->Dispatch(std::move(aRunnable));
   }
   return SchedulerGroup::UnlabeledDispatch(TaskCategory::Network,
                                            std::move(aRunnable));

parser/html/nsHtml5StreamParser.h

@@ -509,9 +509,9 @@ class nsHtml5StreamParser final : public nsISupports {
   nsHtml5TreeOpExecutor* mExecutor;
 
   /**
-   * The same as mExecutor->mDocument->mDocGroup.
+   * Network event target for mExecutor->mDocument
    */
-  RefPtr<mozilla::dom::DocGroup> mDocGroup;
+  nsCOMPtr<nsISerialEventTarget> mNetworkEventTarget;
 
   /**
    * The HTML5 tree builder

xpcom/tests/gtest/TestThreadMetrics.cpp

@@ -92,7 +92,8 @@ class TimedRunnable final : public Runnable {
   static nsresult DispatchWithDocgroup(nsIRunnable* aRunnable,
                                        DocGroup* aDocGroup) {
     nsCOMPtr<nsIRunnable> runnable = aRunnable;
-    runnable = new SchedulerGroup::Runnable(runnable.forget(), aDocGroup);
+    runnable = new SchedulerGroup::Runnable(runnable.forget(),
+                                            aDocGroup->GetPerformanceCounter());
     return aDocGroup->Dispatch(TaskCategory::Other, runnable.forget());
   }
 

xpcom/threads/SchedulerGroup.cpp

@@ -59,13 +59,6 @@ void SchedulerGroup::MarkVsyncRan() { gEarliestUnprocessedVsync = 0; }
 
 SchedulerGroup::SchedulerGroup() : mIsRunning(false) {}
 
-/* static */
-nsresult SchedulerGroup::DispatchWithDocGroup(
-    TaskCategory aCategory, already_AddRefed<nsIRunnable>&& aRunnable,
-    dom::DocGroup* aDocGroup) {
-  return LabeledDispatch(aCategory, std::move(aRunnable), aDocGroup);
-}
-
 /* static */
 nsresult SchedulerGroup::Dispatch(TaskCategory aCategory,
                                   already_AddRefed<nsIRunnable>&& aRunnable) {
@@ -75,11 +68,11 @@ nsresult SchedulerGroup::Dispatch(TaskCategory aCategory,
 /* static */
 nsresult SchedulerGroup::LabeledDispatch(
     TaskCategory aCategory, already_AddRefed<nsIRunnable>&& aRunnable,
-    dom::DocGroup* aDocGroup) {
+    mozilla::PerformanceCounter* aPerformanceCounter) {
   nsCOMPtr<nsIRunnable> runnable(aRunnable);
   if (XRE_IsContentProcess()) {
     RefPtr<Runnable> internalRunnable =
-        new Runnable(runnable.forget(), aDocGroup);
+        new Runnable(runnable.forget(), aPerformanceCounter);
     return InternalUnlabeledDispatch(aCategory, internalRunnable.forget());
   }
   return UnlabeledDispatch(aCategory, runnable.forget());
@@ -113,13 +106,17 @@ nsresult SchedulerGroup::InternalUnlabeledDispatch(
   return rv;
 }
 
-SchedulerGroup::Runnable::Runnable(already_AddRefed<nsIRunnable>&& aRunnable,
-                                   dom::DocGroup* aDocGroup)
+SchedulerGroup::Runnable::Runnable(
+    already_AddRefed<nsIRunnable>&& aRunnable,
+    mozilla::PerformanceCounter* aPerformanceCounter)
     : mozilla::Runnable("SchedulerGroup::Runnable"),
       mRunnable(std::move(aRunnable)),
-      mDocGroup(aDocGroup) {}
+      mPerformanceCounter(aPerformanceCounter) {}
 
-dom::DocGroup* SchedulerGroup::Runnable::DocGroup() const { return mDocGroup; }
+mozilla::PerformanceCounter* SchedulerGroup::Runnable::GetPerformanceCounter()
+    const {
+  return mPerformanceCounter;
+}
 
 #ifdef MOZ_COLLECTING_RUNNABLE_TELEMETRY
 NS_IMETHODIMP

xpcom/threads/SchedulerGroup.h

@@ -9,6 +9,7 @@
 
 #include "mozilla/RefPtr.h"
 #include "mozilla/TaskCategory.h"
+#include "mozilla/PerformanceCounter.h"
 #include "nsCOMPtr.h"
 #include "nsID.h"
 #include "nsIRunnable.h"
@@ -34,16 +35,6 @@ class DocGroup;
     }                                                \
   }
 
-// The "main thread" in Gecko will soon be a set of cooperatively scheduled
-// "fibers". Global state in Gecko will be partitioned into a series of "groups"
-// (with roughly one group per tab). Runnables will be annotated with the set of
-// groups that they touch. Two runnables may run concurrently on different
-// fibers as long as they touch different groups.
-//
-// A SchedulerGroup is an abstract class to represent a "group". Essentially the
-// only functionality offered by a SchedulerGroup is the ability to dispatch
-// runnables to the group. DocGroup, and SystemGroup are the concrete
-// implementations of SchedulerGroup.
 class SchedulerGroup {
  public:
   SchedulerGroup();
@@ -53,9 +44,9 @@ class SchedulerGroup {
   class Runnable final : public mozilla::Runnable, public nsIRunnablePriority {
    public:
     Runnable(already_AddRefed<nsIRunnable>&& aRunnable,
-             dom::DocGroup* aDocGroup);
+             mozilla::PerformanceCounter* aPerformanceCounter);
 
-    dom::DocGroup* DocGroup() const;
+    mozilla::PerformanceCounter* GetPerformanceCounter() const;
 
 #ifdef MOZ_COLLECTING_RUNNABLE_TELEMETRY
     NS_IMETHOD GetName(nsACString& aName) override;
@@ -73,7 +64,7 @@ class SchedulerGroup {
     ~Runnable() = default;
 
     nsCOMPtr<nsIRunnable> mRunnable;
-    RefPtr<dom::DocGroup> mDocGroup;
+    RefPtr<mozilla::PerformanceCounter> mPerformanceCounter;
   };
   friend class Runnable;
 
@@ -87,18 +78,14 @@ class SchedulerGroup {
 
   static void MarkVsyncRan();
 
-  static nsresult DispatchWithDocGroup(
+  static nsresult LabeledDispatch(
       TaskCategory aCategory, already_AddRefed<nsIRunnable>&& aRunnable,
-      dom::DocGroup* aDocGroup);
+      mozilla::PerformanceCounter* aPerformanceCounter);
 
  protected:
   static nsresult InternalUnlabeledDispatch(
       TaskCategory aCategory, already_AddRefed<Runnable>&& aRunnable);
 
-  static nsresult LabeledDispatch(TaskCategory aCategory,
-                                  already_AddRefed<nsIRunnable>&& aRunnable,
-                                  dom::DocGroup* aDocGroup);
-
   // Shuts down this dispatcher. If aXPCOMShutdown is true, invalidates this
   // dispatcher.
   void Shutdown(bool aXPCOMShutdown);

xpcom/threads/nsThread.cpp

@@ -974,10 +974,7 @@ mozilla::PerformanceCounter* nsThread::GetPerformanceCounterBase(
     nsIRunnable* aEvent) {
   RefPtr<SchedulerGroup::Runnable> docRunnable = do_QueryObject(aEvent);
   if (docRunnable) {
-    mozilla::dom::DocGroup* docGroup = docRunnable->DocGroup();
-    if (docGroup) {
-      return docGroup->GetPerformanceCounter();
-    }
+    return docRunnable->GetPerformanceCounter();
   }
   return nullptr;
 }