Bug 1717620 - ResizeObserverController shouldn't need to keep observers alive. r=smaug
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 09 Jul 2021 09:29:18 +0000
changeset 585187 ade72b0db6c019578f9a6b7438a0565de767f32e
parent 585186 a5c04de0bab31c17c38226618658a8be987170d7
child 585188 957372929e10b573c402fd0f55a4022641c85f87
push id38598
push userncsoregi@mozilla.com
push dateFri, 09 Jul 2021 21:40:06 +0000
treeherdermozilla-central@329106017f97 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1717620
milestone91.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1717620 - ResizeObserverController shouldn't need to keep observers alive. r=smaug The observers take care of unregistering when they need to. Instead, make the ResizeObservation keep the element alive just like nsMutationReceiver keeps the mutation observer alive. Differential Revision: https://phabricator.services.mozilla.com/D118477
dom/base/Document.cpp
dom/base/ResizeObserver.cpp
dom/base/ResizeObserver.h
dom/base/ResizeObserverController.cpp
dom/base/ResizeObserverController.h
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -2424,19 +2424,16 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_
        mql = static_cast<LinkedListElement<MediaQueryList>*>(mql)->getNext()) {
     if (mql->HasListeners() &&
         NS_SUCCEEDED(mql->CheckCurrentGlobalCorrectness())) {
       NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mDOMMediaQueryLists item");
       cb.NoteXPCOMChild(mql);
     }
   }
 
-  if (tmp->mResizeObserverController) {
-    tmp->mResizeObserverController->Traverse(cb);
-  }
   for (size_t i = 0; i < tmp->mMetaViewports.Length(); i++) {
     NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mMetaViewports[i].mElement);
   }
 
   // XXX: This should be not needed once bug 1569185 lands.
   for (const auto& entry : tmp->mL10nProtoElements) {
     NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mL10nProtoElements key");
     cb.NoteXPCOMChild(entry.GetKey());
@@ -2583,19 +2580,16 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Do
     mql->Disconnect();
     mql = next;
   }
 
   tmp->mPendingFrameStaticClones.Clear();
 
   tmp->mInUnlinkOrDeletion = false;
 
-  if (tmp->mResizeObserverController) {
-    tmp->mResizeObserverController->Unlink();
-  }
   tmp->mMetaViewports.Clear();
 
   tmp->UnregisterFromMemoryReportingForDataDocument();
 
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mL10nProtoElements)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_WEAK_PTR
   NS_IMPL_CYCLE_COLLECTION_UNLINK_WEAK_REFERENCE
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
--- a/dom/base/ResizeObserver.cpp
+++ b/dom/base/ResizeObserver.cpp
@@ -83,38 +83,88 @@ static nsSize GetTargetSize(Element* aTa
       default:
         size = frame->GetContentRectRelativeToSelf().Size();
     }
   }
 
   return size;
 }
 
-NS_IMPL_CYCLE_COLLECTION(ResizeObservation, mTarget)
+NS_IMPL_CYCLE_COLLECTION_CLASS(ResizeObservation)
+
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(ResizeObservation)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTarget);
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ResizeObservation)
+  tmp->Unlink(RemoveFromObserver::Yes);
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(ResizeObservation, AddRef)
 NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(ResizeObservation, Release)
 
+ResizeObservation::ResizeObservation(Element& aTarget,
+                                     ResizeObserver& aObserver,
+                                     ResizeObserverBoxOptions aBox,
+                                     WritingMode aWm)
+    : mTarget(&aTarget),
+      mObserver(&aObserver),
+      mObservedBox(aBox),
+      // This starts us with a 0,0 last-reported-size:
+      mLastReportedSize(aWm),
+      mLastReportedWM(aWm) {
+  aTarget.BindObject(mObserver);
+}
+
+void ResizeObservation::Unlink(RemoveFromObserver aRemoveFromObserver) {
+  ResizeObserver* observer = std::exchange(mObserver, nullptr);
+  nsCOMPtr<Element> target = std::move(mTarget);
+  if (observer && target) {
+    if (aRemoveFromObserver == RemoveFromObserver::Yes) {
+      IgnoredErrorResult rv;
+      observer->Unobserve(*target, rv);
+      MOZ_DIAGNOSTIC_ASSERT(!rv.Failed(),
+                            "How could we keep the observer and target around "
+                            "without being in the observation map?");
+    }
+    target->UnbindObject(observer);
+  }
+}
+
 bool ResizeObservation::IsActive() const {
   nsIFrame* frame = mTarget->GetPrimaryFrame();
   const WritingMode wm = frame ? frame->GetWritingMode() : WritingMode();
   const LogicalSize size(wm, GetTargetSize(mTarget, mObservedBox));
   return mLastReportedSize.ISize(mLastReportedWM) != size.ISize(wm) ||
          mLastReportedSize.BSize(mLastReportedWM) != size.BSize(wm);
 }
 
 void ResizeObservation::UpdateLastReportedSize(const nsSize& aSize) {
   nsIFrame* frame = mTarget->GetPrimaryFrame();
   mLastReportedWM = frame ? frame->GetWritingMode() : WritingMode();
   mLastReportedSize = LogicalSize(mLastReportedWM, aSize);
 }
 
 // Only needed for refcounted objects.
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ResizeObserver, mOwner, mDocument,
-                                      mCallback, mActiveTargets,
-                                      mObservationMap)
+NS_IMPL_CYCLE_COLLECTION_CLASS(ResizeObserver)
+
+NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(ResizeObserver)
+
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(ResizeObserver)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOwner, mDocument, mCallback,
+                                    mActiveTargets, mObservationMap);
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ResizeObserver)
+  tmp->Disconnect();
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mOwner, mDocument, mCallback, mActiveTargets,
+                                  mObservationMap);
+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+
 NS_IMPL_CYCLE_COLLECTING_ADDREF(ResizeObserver)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(ResizeObserver)
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ResizeObserver)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
 
 already_AddRefed<ResizeObserver> ResizeObserver::Constructor(
@@ -166,18 +216,19 @@ void ResizeObserver::Observe(Element& aT
     // the controller.
     observation->remove();
     observation = nullptr;
   }
 
   // FIXME(emilio): This should probably either flush or not look at the
   // writing-mode or something.
   nsIFrame* frame = aTarget.GetPrimaryFrame();
-  observation = new ResizeObservation(
-      aTarget, aOptions.mBox, frame ? frame->GetWritingMode() : WritingMode());
+  observation =
+      new ResizeObservation(aTarget, *this, aOptions.mBox,
+                            frame ? frame->GetWritingMode() : WritingMode());
   mObservationList.insertBack(observation);
 
   // Per the spec, we need to trigger notification in event loop that
   // contains ResizeObserver observe call even when resize/reflow does
   // not happen.
   aTarget.OwnerDoc()->ScheduleResizeObserversNotification();
 }
 
@@ -195,17 +246,20 @@ void ResizeObserver::Unobserve(Element& 
     if (MOZ_LIKELY(mDocument)) {
       mDocument->RemoveResizeObserver(*this);
     }
   }
 }
 
 void ResizeObserver::Disconnect() {
   const bool registered = !mObservationList.isEmpty();
-  mObservationList.clear();
+  while (auto* observation = mObservationList.popFirst()) {
+    observation->Unlink(ResizeObservation::RemoveFromObserver::No);
+  }
+  MOZ_ASSERT(mObservationList.isEmpty());
   mObservationMap.Clear();
   mActiveTargets.Clear();
   if (registered && MOZ_LIKELY(mDocument)) {
     mDocument->RemoveResizeObserver(*this);
   }
 }
 
 void ResizeObserver::GatherActiveObservations(uint32_t aDepth) {
--- a/dom/base/ResizeObserver.h
+++ b/dom/base/ResizeObserver.h
@@ -26,59 +26,52 @@
 
 namespace mozilla {
 class ErrorResult;
 
 namespace dom {
 
 class Element;
 
-}  // namespace dom
-}  // namespace mozilla
-
-namespace mozilla {
-namespace dom {
-
 // For the internal implementation in ResizeObserver. Normally, this is owned by
 // ResizeObserver.
 class ResizeObservation final : public LinkedListElement<ResizeObservation> {
  public:
   NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(ResizeObservation)
   NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(ResizeObservation)
 
-  ResizeObservation(Element& aTarget, ResizeObserverBoxOptions aBox,
-                    const WritingMode aWM)
-      : mTarget(&aTarget),
-        mObservedBox(aBox),
-        // This starts us with a 0,0 last-reported-size:
-        mLastReportedSize(aWM),
-        mLastReportedWM(aWM) {
-    MOZ_ASSERT(mTarget, "Need a non-null target element");
-  }
+  ResizeObservation(Element&, ResizeObserver&,
+                    ResizeObserverBoxOptions, WritingMode);
 
   Element* Target() const { return mTarget; }
 
   ResizeObserverBoxOptions BoxOptions() const { return mObservedBox; }
 
   /**
    * Returns whether the observed target element size differs from the saved
    * mLastReportedSize.
    */
   bool IsActive() const;
 
   /**
    * Update current mLastReportedSize with size from aSize.
    */
   void UpdateLastReportedSize(const nsSize& aSize);
 
+  enum class RemoveFromObserver : bool { No, Yes };
+  void Unlink(RemoveFromObserver);
+
  protected:
-  ~ResizeObservation() = default;
+  ~ResizeObservation() { Unlink(RemoveFromObserver::No); };
 
   nsCOMPtr<Element> mTarget;
 
+  // Weak, observer always outlives us.
+  ResizeObserver* mObserver;
+
   const ResizeObserverBoxOptions mObservedBox;
 
   // The latest recorded size of observed target.
   // Per the spec, observation.lastReportedSize should be entry.borderBoxSize
   // or entry.contentBoxSize (i.e. logical size), instead of entry.contentRect
   // (i.e. physical rect), so we store this as LogicalSize.
   LogicalSize mLastReportedSize;
   WritingMode mLastReportedWM;
@@ -146,17 +139,17 @@ class ResizeObserver final : public nsIS
    * The active observations' mLastReportedSize fields will be updated, and
    * mActiveTargets will be cleared. It also returns the shallowest depth of
    * elements from active observations or numeric_limits<uint32_t>::max() if
    * there are not any active observations.
    */
   MOZ_CAN_RUN_SCRIPT uint32_t BroadcastActiveObservations();
 
  protected:
-  ~ResizeObserver() { mObservationList.clear(); }
+  ~ResizeObserver() { Disconnect(); }
 
   nsCOMPtr<nsPIDOMWindowInner> mOwner;
   // The window's document at the time of ResizeObserver creation.
   RefPtr<Document> mDocument;
   RefPtr<ResizeObserverCallback> mCallback;
   nsTArray<RefPtr<ResizeObservation>> mActiveTargets;
   // The spec uses a list to store the skipped targets. However, it seems what
   // we want is to check if there are any skipped targets (i.e. existence).
--- a/dom/base/ResizeObserverController.cpp
+++ b/dom/base/ResizeObserverController.cpp
@@ -70,23 +70,16 @@ void ResizeObserverNotificationHelper::U
   mRegistered = false;
 }
 
 ResizeObserverNotificationHelper::~ResizeObserverNotificationHelper() {
   MOZ_RELEASE_ASSERT(!mRegistered, "How can we die when registered?");
   MOZ_RELEASE_ASSERT(!mOwner, "Forgot to clear weak pointer?");
 }
 
-void ResizeObserverController::Traverse(
-    nsCycleCollectionTraversalCallback& aCb) {
-  ImplCycleCollectionTraverse(aCb, mResizeObservers, "mResizeObservers");
-}
-
-void ResizeObserverController::Unlink() { mResizeObservers.Clear(); }
-
 void ResizeObserverController::ShellDetachedFromDocument() {
   mResizeObserverNotificationHelper->Unregister();
 }
 
 void ResizeObserverController::Notify() {
   if (mResizeObservers.IsEmpty()) {
     return;
   }
@@ -163,17 +156,19 @@ void ResizeObserverController::GatherAll
   }
 }
 
 uint32_t ResizeObserverController::BroadcastAllActiveObservations() {
   uint32_t shallowestTargetDepth = std::numeric_limits<uint32_t>::max();
 
   // Copy the observers as this invokes the callbacks and could register and
   // unregister observers at will.
-  for (auto& observer : mResizeObservers.Clone()) {
+  const auto observers =
+      ToTArray<nsTArray<RefPtr<ResizeObserver>>>(mResizeObservers);
+  for (auto& observer : observers) {
     // MOZ_KnownLive because 'observers' is guaranteed to keep it
     // alive.
     //
     // This can go away once
     // https://bugzilla.mozilla.org/show_bug.cgi?id=1620312 is fixed.
     uint32_t targetDepth =
         MOZ_KnownLive(observer)->BroadcastActiveObservations();
     if (targetDepth < shallowestTargetDepth) {
--- a/dom/base/ResizeObserverController.h
+++ b/dom/base/ResizeObserverController.h
@@ -63,20 +63,16 @@ class ResizeObserverController final {
  public:
   explicit ResizeObserverController(Document* aDocument)
       : mDocument(aDocument),
         mResizeObserverNotificationHelper(
             new ResizeObserverNotificationHelper(this)) {
     MOZ_ASSERT(mDocument, "Need a non-null document");
   }
 
-  // Methods for supporting cycle-collection
-  void Traverse(nsCycleCollectionTraversalCallback& aCb);
-  void Unlink();
-
   void AddSizeOfIncludingThis(nsWindowSizes&) const;
 
   void ShellDetachedFromDocument();
   void AddResizeObserver(ResizeObserver& aObserver) {
     MOZ_ASSERT(!mResizeObservers.Contains(&aObserver));
     mResizeObservers.AppendElement(&aObserver);
   }
 
@@ -128,15 +124,15 @@ class ResizeObserverController final {
    */
   bool HasAnySkippedObservations() const;
 
   // Raw pointer is OK because mDocument strongly owns us & hence must outlive
   // us.
   Document* const mDocument;
 
   RefPtr<ResizeObserverNotificationHelper> mResizeObserverNotificationHelper;
-  nsTArray<RefPtr<ResizeObserver>> mResizeObservers;
+  nsTArray<ResizeObserver*> mResizeObservers;
 };
 
 }  // namespace dom
 }  // namespace mozilla
 
 #endif  // mozilla_dom_ResizeObserverController_h