Bug 1445392: Remove pending slots from the docgroup on unlink. r?smaug draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 07 May 2018 19:12:29 +0200
changeset 792119 38536d12c658fd71dd13e9d123f558a606cd1312
parent 792118 8e6f0c39cea0b829f67ed9ee0cac34d2002ddc52
push id108997
push userbmo:emilio@crisal.io
push dateMon, 07 May 2018 17:25:13 +0000
reviewerssmaug
bugs1445392
milestone61.0a1
Bug 1445392: Remove pending slots from the docgroup on unlink. r?smaug This fixes the leak. However, I suspect it's not the right thing to do, given the same test-case asserts later with: mPendingMicroTaskRunnables.empty() In CycleCollectedJSContext.cpp. That microtask is posted from DocGroup::SignalSlotChange, from the UnbindFromTree that happens on the <div> during unlink, and I suspect we should prevent it, somehow. If this is a good excuse to pass down a "are we unlinking" bit to UnbindFromTree, I can do that. MozReview-Commit-ID: 6sJgMqZa80U
dom/base/DocGroup.h
dom/html/HTMLSlotElement.cpp
--- a/dom/base/DocGroup.h
+++ b/dom/base/DocGroup.h
@@ -111,16 +111,21 @@ public:
   // Return a pointer that can be continually checked to see if access to this
   // DocGroup is valid. This pointer should live at least as long as the
   // DocGroup.
   bool* GetValidAccessPtr();
 
   // Append aSlot to the list of signal slot list, if it's not in it already
   // list, and queue a mutation observer microtask.
   void SignalSlotChange(HTMLSlotElement&);
+  void RemoveSlotFromSignalList(const HTMLSlotElement& aSlot)
+  {
+    MOZ_ASSERT(mSignalSlotList.Contains(&aSlot));
+    mSignalSlotList.RemoveElement(&aSlot);
+  }
 
   void MoveSignalSlotListTo(nsTArray<RefPtr<HTMLSlotElement>>& aDest);
 
   // List of DocGroups that has non-empty signal slot list.
   static AutoTArray<RefPtr<DocGroup>, 2>* sPendingDocGroups;
 
 private:
   DocGroup(TabGroup* aTabGroup, const nsACString& aKey);
--- a/dom/html/HTMLSlotElement.cpp
+++ b/dom/html/HTMLSlotElement.cpp
@@ -34,19 +34,32 @@ HTMLSlotElement::HTMLSlotElement(already
 {
 }
 
 HTMLSlotElement::~HTMLSlotElement() = default;
 
 NS_IMPL_ADDREF_INHERITED(HTMLSlotElement, nsGenericHTMLElement)
 NS_IMPL_RELEASE_INHERITED(HTMLSlotElement, nsGenericHTMLElement)
 
-NS_IMPL_CYCLE_COLLECTION_INHERITED(HTMLSlotElement,
-                                   nsGenericHTMLElement,
-                                   mAssignedNodes)
+NS_IMPL_CYCLE_COLLECTION_CLASS(HTMLSlotElement)
+
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(HTMLSlotElement,
+                                                  nsGenericHTMLElement)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAssignedNodes)
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(HTMLSlotElement)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mAssignedNodes)
+  if (MOZ_UNLIKELY(tmp->mInSignalSlotList)) {
+    tmp->mInSignalSlotList = false;
+    if (DocGroup* docGroup = tmp->OwnerDoc()->GetDocGroup()) {
+      docGroup->RemoveSlotFromSignalList(*tmp);
+    }
+  }
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END_INHERITED(nsGenericHTMLElement)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(HTMLSlotElement)
 NS_INTERFACE_MAP_END_INHERITING(nsGenericHTMLElement)
 
 NS_IMPL_ELEMENT_CLONE(HTMLSlotElement)
 
 nsresult
 HTMLSlotElement::BindToTree(nsIDocument* aDocument,