Changeset 243686 in webkit

Timestamp:

Mar 31, 2019, 7:03:14 PM (7 years ago)

Author:

rniwa@webkit.org

Message:

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (1 diff)
• dom/Node.cpp (modified) (2 diffs)
• dom/Node.h (modified) (4 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -669 +669 @@
     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;
 

trunk/Source/WebCore/dom/Node.cpp

@@ -333 +333 @@
 {
     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);
@@ -2515 +2513 @@
 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);
 

trunk/Source/WebCore/dom/Node.h

@@ -619 +619 @@
 
     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 { }
@@ -708 +708 @@
     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;
@@ -716 +718 @@
         return;
     }
-    m_refCountAndParentBit = tempRefCount;
+    m_refCountAndParentBit = updatedRefCount;
 }
 
@@ -742 +744 @@
     ASSERT(isMainThread());
     m_parentNode = parent;
-    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
-    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;
+    m_refCountAndParentBit = (m_refCountAndParentBit & s_refCountMask) | !!parent;
 }