Bug 1596992 - Avoid leaking ResizeObserver entries without active observations for the lifetime of the document. r=smaug

By adding / removing to the observer list on observe() / unobserve()
respectively, rather than only adding on construction per spec.

See https://github.com/w3c/csswg-drafts/issues/4518 for the relevant spec issue.

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

--HG--
extra : moz-landing-system : lando

dom/base/Document.cpp

@@ -15117,12 +15117,19 @@ FlashClassification Document::DocumentFlashClassification() {
   return mFlashClassification;
 }
 
-void Document::AddResizeObserver(ResizeObserver* aResizeObserver) {
+void Document::AddResizeObserver(ResizeObserver& aObserver) {
   if (!mResizeObserverController) {
     mResizeObserverController = MakeUnique<ResizeObserverController>(this);
   }
+  mResizeObserverController->AddResizeObserver(aObserver);
+}
 
-  mResizeObserverController->AddResizeObserver(aResizeObserver);
+void Document::RemoveResizeObserver(ResizeObserver& aObserver) {
+  MOZ_DIAGNOSTIC_ASSERT(mResizeObserverController, "No controller?");
+  if (MOZ_UNLIKELY(!mResizeObserverController)) {
+    return;
+  }
+  mResizeObserverController->RemoveResizeObserver(aObserver);
 }
 
 PermissionDelegateHandler* Document::GetPermissionDelegateHandler() {

dom/base/Document.h

@@ -3965,7 +3965,8 @@ class Document : public nsINode,
   FlashClassification DocumentFlashClassification();
 
   // ResizeObserver usage.
-  void AddResizeObserver(ResizeObserver* aResizeObserver);
+  void AddResizeObserver(ResizeObserver&);
+  void RemoveResizeObserver(ResizeObserver&);
   void ScheduleResizeObserversNotification() const;
 
   // Getter for PermissionDelegateHandler. Performs lazy initialization.

dom/base/ResizeObserver.cpp

@@ -108,8 +108,9 @@ void ResizeObservation::UpdateLastReportedSize(const nsSize& aSize) {
 }
 
 // Only needed for refcounted objects.
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ResizeObserver, mOwner, mCallback,
-                                      mActiveTargets, mObservationMap)
+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ResizeObserver, mOwner, mDocument,
+                                      mCallback, mActiveTargets,
+                                      mObservationMap)
 NS_IMPL_CYCLE_COLLECTING_ADDREF(ResizeObserver)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(ResizeObserver)
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ResizeObserver)
@@ -122,31 +123,37 @@ already_AddRefed<ResizeObserver> ResizeObserver::Constructor(
     ErrorResult& aRv) {
   nsCOMPtr<nsPIDOMWindowInner> window =
       do_QueryInterface(aGlobal.GetAsSupports());
-
   if (!window) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
-  Document* document = window->GetExtantDoc();
-
-  if (!document) {
+  Document* doc = window->GetExtantDoc();
+  if (!doc) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
-  RefPtr<ResizeObserver> observer = new ResizeObserver(window.forget(), aCb);
-  document->AddResizeObserver(observer);
-
-  return observer.forget();
+  return do_AddRef(new ResizeObserver(window.forget(), doc, aCb));
 }
 
 void ResizeObserver::Observe(Element& aTarget,
                              const ResizeObserverOptions& aOptions,
                              ErrorResult& aRv) {
-  RefPtr<ResizeObservation> observation;
+  // NOTE(emilio): Per spec, this is supposed to happen on construction, but the
+  // spec isn't particularly sane here, see
+  // https://github.com/w3c/csswg-drafts/issues/4518
+  if (mObservationList.isEmpty()) {
+    MOZ_ASSERT(mObservationMap.IsEmpty());
+    if (MOZ_UNLIKELY(!mDocument)) {
+      return aRv.Throw(NS_ERROR_FAILURE);
+    }
+    mDocument->AddResizeObserver(*this);
+  }
 
-  if (mObservationMap.Get(&aTarget, getter_AddRefs(observation))) {
+  RefPtr<ResizeObservation>& observation =
+    mObservationMap.LookupForAdd(&aTarget).OrInsert([] { return nullptr; });
+  if (observation) {
     if (observation->BoxOptions() == aOptions.mBox) {
       // Already observed this target and the observed box is the same, so
       // return.
@@ -157,14 +164,17 @@ void ResizeObserver::Observe(Element& aTarget,
       // should be kept to make sure IsActive() returns the correct result.
       return;
     }
-    Unobserve(aTarget, aRv);
+    // Remove the pre-existing entry, but without unregistering ourselves from
+    // 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());
-
-  mObservationMap.Put(&aTarget, observation);
   mObservationList.insertBack(observation);
 
   // Per the spec, we need to trigger notification in event loop that
@@ -183,6 +193,11 @@ void ResizeObserver::Unobserve(Element& aTarget, ErrorResult& aRv) {
              "If ResizeObservation found for an element, observation list "
              "must be not empty.");
   observation->remove();
+  if (mObservationList.isEmpty()) {
+    if (MOZ_LIKELY(mDocument)) {
+      mDocument->RemoveResizeObserver(*this);
+    }
+  }
 }
 
 void ResizeObserver::Disconnect() {
@@ -195,7 +210,7 @@ void ResizeObserver::GatherActiveObservations(uint32_t aDepth) {
   mActiveTargets.Clear();
   mHasSkippedTargets = false;
 
-  for (auto observation : mObservationList) {
+  for (auto* observation : mObservationList) {
     if (!observation->IsActive()) {
       continue;
     }

dom/base/ResizeObserver.h

@@ -89,9 +89,11 @@ class ResizeObserver final : public nsISupports, public nsWrapperCache {
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ResizeObserver)
 
   ResizeObserver(already_AddRefed<nsPIDOMWindowInner>&& aOwner,
-                 ResizeObserverCallback& aCb)
-      : mOwner(aOwner), mCallback(&aCb) {
+                 Document* aDocument, ResizeObserverCallback& aCb)
+      : mOwner(aOwner), mDocument(aDocument), mCallback(&aCb) {
     MOZ_ASSERT(mOwner, "Need a non-null owner window");
+    MOZ_ASSERT(mDocument, "Need a non-null doc");
+    MOZ_ASSERT(mDocument == mOwner->GetExtantDoc());
   }
 
   nsISupports* GetParentObject() const { return mOwner; }
@@ -147,6 +149,8 @@ class ResizeObserver final : public nsISupports, public nsWrapperCache {
   ~ResizeObserver() { mObservationList.clear(); }
 
   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

dom/base/ResizeObserverController.cpp

@@ -82,13 +82,6 @@ void ResizeObserverController::Traverse(
 
 void ResizeObserverController::Unlink() { mResizeObservers.Clear(); }
 
-void ResizeObserverController::AddResizeObserver(ResizeObserver* aObserver) {
-  MOZ_ASSERT(aObserver,
-             "AddResizeObserver() should never be called with a null "
-             "parameter");
-  mResizeObservers.AppendElement(aObserver);
-}
-
 void ResizeObserverController::ShellDetachedFromDocument() {
   mResizeObserverNotificationHelper->Unregister();
 }
@@ -165,26 +158,19 @@ void ResizeObserverController::Notify() {
 }
 
 void ResizeObserverController::GatherAllActiveObservations(uint32_t aDepth) {
-  nsTObserverArray<RefPtr<ResizeObserver>>::ForwardIterator iter(
-      mResizeObservers);
-  while (iter.HasMore()) {
-    iter.GetNext()->GatherActiveObservations(aDepth);
+  for (ResizeObserver* observer : mResizeObservers) {
+    observer->GatherActiveObservations(aDepth);
   }
 }
 
 uint32_t ResizeObserverController::BroadcastAllActiveObservations() {
   uint32_t shallowestTargetDepth = std::numeric_limits<uint32_t>::max();
 
-  // Use EndLimitedIterator, so we handle the new-added observers (from the JS
-  // callback) in the next iteration of the while loop.
-  // Note: the while loop is in ResizeObserverController::Notify()).
-  nsTObserverArray<RefPtr<ResizeObserver>>::EndLimitedIterator iter(
-      mResizeObservers);
-  while (iter.HasMore()) {
-    RefPtr<ResizeObserver>& observer = iter.GetNext();
-
+  // Copy the observers as this invokes the callbacks and could register and
+  // unregister observers at will.
+  nsTArray<RefPtr<ResizeObserver>> observers(mResizeObservers);
+  for (auto& observer : observers) {
     uint32_t targetDepth = observer->BroadcastActiveObservations();
-
     if (targetDepth < shallowestTargetDepth) {
       shallowestTargetDepth = targetDepth;
     }
@@ -194,10 +180,8 @@ uint32_t ResizeObserverController::BroadcastAllActiveObservations() {
 }
 
 bool ResizeObserverController::HasAnyActiveObservations() const {
-  nsTObserverArray<RefPtr<ResizeObserver>>::ForwardIterator iter(
-      mResizeObservers);
-  while (iter.HasMore()) {
-    if (iter.GetNext()->HasActiveObservations()) {
+  for (auto& observer : mResizeObservers) {
+    if (observer->HasActiveObservations()) {
       return true;
     }
   }
@@ -205,10 +189,8 @@ bool ResizeObserverController::HasAnyActiveObservations() const {
 }
 
 bool ResizeObserverController::HasAnySkippedObservations() const {
-  nsTObserverArray<RefPtr<ResizeObserver>>::ForwardIterator iter(
-      mResizeObservers);
-  while (iter.HasMore()) {
-    if (iter.GetNext()->HasSkippedObservations()) {
+  for (auto& observer : mResizeObservers) {
+    if (observer->HasSkippedObservations()) {
       return true;
     }
   }

dom/base/ResizeObserverController.h

@@ -71,7 +71,15 @@ class ResizeObserverController final {
   void Unlink();
 
   void ShellDetachedFromDocument();
-  void AddResizeObserver(ResizeObserver* aObserver);
+  void AddResizeObserver(ResizeObserver& aObserver) {
+    MOZ_ASSERT(!mResizeObservers.Contains(&aObserver));
+    mResizeObservers.AppendElement(&aObserver);
+  }
+
+  void RemoveResizeObserver(ResizeObserver& aObserver) {
+    MOZ_ASSERT(mResizeObservers.Contains(&aObserver));
+    mResizeObservers.RemoveElement(&aObserver);
+  }
 
   /**
    * Schedule the notification via ResizeObserverNotificationHelper refresh
@@ -121,7 +129,7 @@ class ResizeObserverController final {
   Document* const mDocument;
 
   RefPtr<ResizeObserverNotificationHelper> mResizeObserverNotificationHelper;
-  nsTObserverArray<RefPtr<ResizeObserver>> mResizeObservers;
+  nsTArray<RefPtr<ResizeObserver>> mResizeObservers;
 };
 
 }  // namespace dom