Bug 1551425: Part 2) Change reference to parent in `nsAttrChildContentList` from non-owning to RefPtr. r=smaug
authorMirko Brodesser <mbrodesser@mozilla.com>
Tue, 25 Jun 2019 06:56:23 +0000
changeset 480014 5a5508254aac6534c5e8368d2c546510dfa0f0a2
parent 480013 a4f89580ce4029fcd0b48f9ef4563b9728beb5d1
child 480015 ddc6f1b9c5241e7f3d1fca299b0544dea766d394
push id88433
push userapavel@mozilla.com
push dateTue, 25 Jun 2019 08:25:22 +0000
treeherderautoland@5a5508254aac [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1551425
milestone69.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 1551425: Part 2) Change reference to parent in `nsAttrChildContentList` from non-owning to RefPtr. r=smaug With a non-owning reference, a JS reference to the NodeList didn't keep its items alive. With this change, the NodeList keeps the parent node (which keeps its children alive) alive. Differential Revision: https://phabricator.services.mozilla.com/D34151
dom/base/FragmentOrElement.cpp
dom/base/nsChildContentList.h
dom/base/nsINode.cpp
--- a/dom/base/FragmentOrElement.cpp
+++ b/dom/base/FragmentOrElement.cpp
@@ -388,32 +388,27 @@ static bool NeedsScriptTraverse(nsINode*
          !aNode->HasKnownLiveWrapperAndDoesNotNeedTracing(aNode);
 }
 
 //----------------------------------------------------------------------
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAttrChildContentList)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAttrChildContentList)
 
-// If nsAttrChildContentList is changed so that any additional fields are
-// traversed by the cycle collector, then CAN_SKIP must be updated to
-// check that the additional fields are null.
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(nsAttrChildContentList)
+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(nsAttrChildContentList, mNode)
 
-// nsAttrChildContentList only ever has a single child, its wrapper, so if
-// the wrapper is known-live, the list can't be part of a garbage cycle.
+// If the wrapper is known-live, the list can't be part of a garbage cycle.
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsAttrChildContentList)
   return tmp->HasKnownLiveWrapper();
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
 
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsAttrChildContentList)
   return tmp->HasKnownLiveWrapperAndDoesNotNeedTracing(tmp);
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END
 
-// CanSkipThis returns false to avoid problems with incomplete unlinking.
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(nsAttrChildContentList)
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_END
 
 NS_INTERFACE_TABLE_HEAD(nsAttrChildContentList)
   NS_WRAPPERCACHE_INTERFACE_TABLE_ENTRY
   NS_INTERFACE_TABLE(nsAttrChildContentList, nsINodeList)
   NS_INTERFACE_TABLE_TO_MAP_SEGUE_CYCLE_COLLECTION(nsAttrChildContentList)
 NS_INTERFACE_MAP_END
--- a/dom/base/nsChildContentList.h
+++ b/dom/base/nsChildContentList.h
@@ -2,16 +2,17 @@
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef nsChildContentList_h__
 #define nsChildContentList_h__
 
+#include "mozilla/RefPtr.h"
 #include "nsISupportsImpl.h"
 #include "nsINodeList.h"   // base class
 #include "js/TypeDecls.h"  // for Handle, Value, JSObject, JSContext
 
 class nsIContent;
 class nsINode;
 
 /**
@@ -30,47 +31,41 @@ class nsAttrChildContentList : public ns
   // nsWrapperCache
   virtual JSObject* WrapObject(JSContext* cx,
                                JS::Handle<JSObject*> aGivenProto) override;
 
   // nsINodeList interface
   virtual int32_t IndexOf(nsIContent* aContent) override;
   virtual nsIContent* Item(uint32_t aIndex) override;
   uint32_t Length() override;
+  nsINode* GetParentObject() final { return mNode; }
 
-  virtual void DropReference() { mNode = nullptr; }
-
-  virtual nsINode* GetParentObject() override { return mNode; }
+  virtual void InvalidateCacheIfAvailable() {}
 
  protected:
   virtual ~nsAttrChildContentList() {}
 
  private:
   // The node whose children make up the list.
-  // This is a non-owning ref which is safe because it's set to nullptr by
-  // DropReference() by the node slots get destroyed.
-  nsINode* MOZ_NON_OWNING_REF mNode;
+  RefPtr<nsINode> mNode;
 };
 
 class nsParentNodeChildContentList final : public nsAttrChildContentList {
  public:
   explicit nsParentNodeChildContentList(nsINode* aNode)
       : nsAttrChildContentList(aNode), mIsCacheValid(false) {
     ValidateCache();
   }
 
   // nsINodeList interface
   virtual int32_t IndexOf(nsIContent* aContent) override;
   virtual nsIContent* Item(uint32_t aIndex) override;
   uint32_t Length() override;
 
-  void DropReference() override {
-    InvalidateCache();
-    nsAttrChildContentList::DropReference();
-  }
+  void InvalidateCacheIfAvailable() final { InvalidateCache(); }
 
   void InvalidateCache() {
     mIsCacheValid = false;
     mCachedChildArray.Clear();
   }
 
  private:
   ~nsParentNodeChildContentList() {}
--- a/dom/base/nsINode.cpp
+++ b/dom/base/nsINode.cpp
@@ -117,32 +117,33 @@
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
 nsINode::nsSlots::nsSlots() : mWeakReference(nullptr) {}
 
 nsINode::nsSlots::~nsSlots() {
   if (mChildNodes) {
-    mChildNodes->DropReference();
+    mChildNodes->InvalidateCacheIfAvailable();
   }
 
   if (mWeakReference) {
     mWeakReference->NoticeNodeDestruction();
   }
 }
 
 void nsINode::nsSlots::Traverse(nsCycleCollectionTraversalCallback& cb) {
   NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mSlots->mChildNodes");
   cb.NoteXPCOMChild(mChildNodes);
 }
 
 void nsINode::nsSlots::Unlink() {
   if (mChildNodes) {
-    mChildNodes->DropReference();
+    mChildNodes->InvalidateCacheIfAvailable();
+    ImplCycleCollectionUnlink(mChildNodes);
   }
 }
 
 //----------------------------------------------------------------------
 
 #ifdef MOZILLA_INTERNAL_API
 nsINode::nsINode(already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo)
     : mNodeInfo(std::move(aNodeInfo)),