Changeset 243075 in webkit

Timestamp:

Mar 18, 2019 10:05:54 AM (5 years ago)

Author:

Ryan Haddad

Message:

Unreviewed, rolling out r243037.

Broke the Windows build

Reverted changeset:

"Reduce the size of Node::deref by eliminating an explicit
parentNode check"
​https://bugs.webkit.org/show_bug.cgi?id=195776
​https://trac.webkit.org/changeset/243037

Location:

trunk/Source/WebCore

Files:

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

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-03-18  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r243037.
+
+        Broke the Windows build
+
+        Reverted changeset:
+
+        "Reduce the size of Node::deref by eliminating an explicit
+        parentNode check"
+        https://bugs.webkit.org/show_bug.cgi?id=195776
+        https://trac.webkit.org/changeset/243037
+
 2019-03-18  Eric Carlson  <eric.carlson@apple.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -670 +670 @@
     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()
-        m_refCountAndParentBit = 0;
-
         // If removing a child removes the last node reference, we don't want the scope to be destroyed
         // until after removeDetachedChildren returns, so we protect ourselves.
@@ -723 +719 @@
         m_deletionHasBegun = true;
 #endif
+        m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
         delete this;
     }

trunk/Source/WebCore/dom/Document.h

@@ -377 +377 @@
             m_deletionHasBegun = true;
 #endif
-            m_refCountAndParentBit = s_refCountIncrement; // 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.)
+            m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
             delete this;
         }

trunk/Source/WebCore/dom/Node.cpp

@@ -317 +317 @@
 
 Node::Node(Document& document, ConstructionType type)
-    : m_nodeFlags(type)
+    : m_refCount(1)
+    , m_nodeFlags(type)
     , m_treeScope(&document)
 {
@@ -332 +333 @@
 {
     ASSERT(isMainThread());
-    // We set m_refCount to 2 before calling delete to avoid double destruction through use of Ref<T>/RefPtr<T>.
+    // We set m_refCount to 1 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_refCount == 1);
     ASSERT(m_deletionHasBegun);
     ASSERT(!m_adoptionIsRequired);
@@ -2523 +2524 @@
 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
     // faster for non-Document nodes, and because the call to removedLastRef that is inlined
@@ -2538 +2535 @@
     m_deletionHasBegun = true;
 #endif
+    m_refCount = 1; // Avoid double destruction through use of RefPtr<T>. (This is a security mitigation in case of programmer error. It will ASSERT in debug builds.)
     delete this;
 }

trunk/Source/WebCore/dom/Node.h

@@ -501 +501 @@
     void deref();
     bool hasOneRef() const;
-    unsigned refCount() const;
+    int refCount() const;
 
 #ifndef NDEBUG
@@ -619 +619 @@
     Node(Document&, ConstructionType);
 
-    static constexpr uint32_t s_refCountIncrement = 2;
-    static constexpr uint32_t s_refCountMask = ~0x1;
-
     virtual void addSubresourceAttributeURLs(ListHashSet<URL>&) const { }
 
@@ -668 +665 @@
     void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
 
-    uint32_t m_refCountAndParentBit { s_refCountIncrement };
+    int m_refCount;
     mutable uint32_t m_nodeFlags;
 
@@ -699 +696 @@
     ASSERT(!m_inRemovedLastRefFunction);
     ASSERT(!m_adoptionIsRequired);
-    m_refCountAndParentBit += s_refCountIncrement;
+    ++m_refCount;
 }
 
@@ -705 +702 @@
 {
     ASSERT(isMainThread());
-    ASSERT(refCount());
+    ASSERT(m_refCount >= 0);
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
     ASSERT(!m_adoptionIsRequired);
-    auto tempRefCount = m_refCountAndParentBit - s_refCountIncrement;
-    if (!tempRefCount) {
+    if (--m_refCount <= 0 && !parentNode()) {
 #ifndef NDEBUG
         m_inRemovedLastRefFunction = true;
 #endif
         removedLastRef();
-        return;
     }
-    m_refCountAndParentBit = tempRefCount;
 }
 
@@ -724 +718 @@
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
-    return refCount() == 1;
-}
-
-ALWAYS_INLINE unsigned Node::refCount() const
-{
-    return m_refCountAndParentBit / s_refCountIncrement;
+    return m_refCount == 1;
+}
+
+ALWAYS_INLINE int Node::refCount() const
+{
+    return m_refCount;
 }
 
@@ -743 +737 @@
     ASSERT(isMainThread());
     m_parentNode = parent;
-    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
-    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;
 }