Reduce the size of Node::deref by eliminating an explicit parentNode check
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Apr 2019 02:03:14 +0000 (02:03 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Apr 2019 02:03:14 +0000 (02:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195776

Reviewed by Darin Adler.

Address post-commit review comments.

* dom/Document.cpp:
(WebCore::Document::removedLastRef):
* dom/Node.cpp:
(WebCore::Node::~Node):
(WebCore::Node::removedLastRef):
* dom/Node.h:
(WebCore::Node::deref):
(WebCore::Node::setParentNode):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243686 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/Node.h

index abaee66..ea3fa6c 100644 (file)
@@ -1,3 +1,21 @@
+2019-03-31  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reduce the size of Node::deref by eliminating an explicit parentNode check
+        https://bugs.webkit.org/show_bug.cgi?id=195776
+
+        Reviewed by Darin Adler.
+
+        Address post-commit review comments.
+
+        * dom/Document.cpp:
+        (WebCore::Document::removedLastRef):
+        * dom/Node.cpp:
+        (WebCore::Node::~Node):
+        (WebCore::Node::removedLastRef):
+        * dom/Node.h:
+        (WebCore::Node::deref):
+        (WebCore::Node::setParentNode):
+
 2019-03-31  Sam Weinig  <weinig@apple.com>
 
         Remove more i386 specific configurations
index efeb3f1..bd16ed2 100644 (file)
@@ -668,7 +668,7 @@ void Document::removedLastRef()
     ASSERT(!m_deletionHasBegun);
     if (m_referencingNodeCount) {
         // Node::removedLastRef doesn't set refCount() to zero because it's not observable.
-        // But we need to remember that our refCount reached zero in subsequent calls to decrementReferencingNodeCount()
+        // But we need to remember that our refCount reached zero in subsequent calls to decrementReferencingNodeCount().
         m_refCountAndParentBit = 0;
 
         // If removing a child removes the last node reference, we don't want the scope to be destroyed
index 0035a8a..f8cb211 100644 (file)
@@ -332,8 +332,6 @@ Node::Node(Document& document, ConstructionType type)
 Node::~Node()
 {
     ASSERT(isMainThread());
-    // We set m_refCount to 2 before calling delete to avoid double destruction through use of Ref<T>/RefPtr<T>.
-    // This is a security mitigation in case of programmer errorm (caught by a debug assertion).
     ASSERT(m_refCountAndParentBit == s_refCountIncrement);
     ASSERT(m_deletionHasBegun);
     ASSERT(!m_adoptionIsRequired);
@@ -2514,8 +2512,6 @@ bool Node::willRespondToMouseWheelEvents()
 // delete a Node at each deref call site.
 void Node::removedLastRef()
 {
-    // This avoids double destruction even when there is a programming error to use Ref<T> / RefPtr<T> on this node.
-    // There are debug assertions in Node::ref() / Node::deref() to catch such a programming error.
     ASSERT(m_refCountAndParentBit == s_refCountIncrement);
 
     // An explicit check for Document here is better than a virtual function since it is
index 28f0abc..fccdd7d 100644 (file)
@@ -618,7 +618,7 @@ protected:
     Node(Document&, ConstructionType);
 
     static constexpr uint32_t s_refCountIncrement = 2;
-    static constexpr uint32_t s_refCountMask = ~static_cast<uint32_t>(0x1);
+    static constexpr uint32_t s_refCountMask = ~static_cast<uint32_t>(1);
 
     virtual void addSubresourceAttributeURLs(ListHashSet<URL>&) const { }
 
@@ -707,15 +707,17 @@ ALWAYS_INLINE void Node::deref()
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
     ASSERT(!m_adoptionIsRequired);
-    auto tempRefCount = m_refCountAndParentBit - s_refCountIncrement;
-    if (!tempRefCount) {
+    auto updatedRefCount = m_refCountAndParentBit - s_refCountIncrement;
+    if (!updatedRefCount) {
+        // Don't update m_refCountAndParentBit to avoid double destruction through use of Ref<T>/RefPtr<T>.
+        // (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
 #ifndef NDEBUG
         m_inRemovedLastRefFunction = true;
 #endif
         removedLastRef();
         return;
     }
-    m_refCountAndParentBit = tempRefCount;
+    m_refCountAndParentBit = updatedRefCount;
 }
 
 ALWAYS_INLINE bool Node::hasOneRef() const
@@ -741,8 +743,7 @@ inline void Node::setParentNode(ContainerNode* parent)
 {
     ASSERT(isMainThread());
     m_parentNode = parent;
-    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
-    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;
+    m_refCountAndParentBit = (m_refCountAndParentBit & s_refCountMask) | !!parent;
 }
 
 inline ContainerNode* Node::parentNode() const