Unreviewed, rolling out r243037.
authorryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Mar 2019 17:05:54 +0000 (17:05 +0000)
committerryanhaddad@apple.com <ryanhaddad@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Mar 2019 17:05:54 +0000 (17:05 +0000)
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

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

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

index e7a1366..5ac6a2a 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         Change some logging levels
index 5fa2c77..2c317a0 100644 (file)
@@ -669,10 +669,6 @@ 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()
-        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.
         incrementReferencingNodeCount();
@@ -722,6 +718,7 @@ void Document::removedLastRef()
         m_inRemovedLastRefFunction = false;
         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;
     }
 }
index 3a3553e..f1dbd48 100644 (file)
@@ -376,7 +376,7 @@ public:
 #if !ASSERT_DISABLED
             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;
         }
     }
index e9ff1b3..7729179 100644 (file)
@@ -316,7 +316,8 @@ void Node::trackForDebugging()
 }
 
 Node::Node(Document& document, ConstructionType type)
-    : m_nodeFlags(type)
+    : m_refCount(1)
+    , m_nodeFlags(type)
     , m_treeScope(&document)
 {
     ASSERT(isMainThread());
@@ -331,9 +332,9 @@ 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>.
+    // 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);
 
@@ -2522,10 +2523,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
     // faster for non-Document nodes, and because the call to removedLastRef that is inlined
     // at all deref call sites is smaller if it's a non-virtual function.
@@ -2537,6 +2534,7 @@ void Node::removedLastRef()
 #ifndef NDEBUG
     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;
 }
 
index b144925..2369ea2 100644 (file)
@@ -500,7 +500,7 @@ public:
     void ref();
     void deref();
     bool hasOneRef() const;
-    unsigned refCount() const;
+    int refCount() const;
 
 #ifndef NDEBUG
     bool m_deletionHasBegun { false };
@@ -618,9 +618,6 @@ protected:
     };
     Node(Document&, ConstructionType);
 
-    static constexpr uint32_t s_refCountIncrement = 2;
-    static constexpr uint32_t s_refCountMask = ~0x1;
-
     virtual void addSubresourceAttributeURLs(ListHashSet<URL>&) const { }
 
     bool hasRareData() const { return getFlag(HasRareDataFlag); }
@@ -667,7 +664,7 @@ private:
     static void moveTreeToNewScope(Node&, TreeScope& oldScope, TreeScope& newScope);
     void moveNodeToNewDocument(Document& oldDocument, Document& newDocument);
 
-    uint32_t m_refCountAndParentBit { s_refCountIncrement };
+    int m_refCount;
     mutable uint32_t m_nodeFlags;
 
     ContainerNode* m_parentNode { nullptr };
@@ -698,37 +695,34 @@ ALWAYS_INLINE void Node::ref()
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
     ASSERT(!m_adoptionIsRequired);
-    m_refCountAndParentBit += s_refCountIncrement;
+    ++m_refCount;
 }
 
 ALWAYS_INLINE void Node::deref()
 {
     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;
 }
 
 ALWAYS_INLINE bool Node::hasOneRef() const
 {
     ASSERT(!m_deletionHasBegun);
     ASSERT(!m_inRemovedLastRefFunction);
-    return refCount() == 1;
+    return m_refCount == 1;
 }
 
-ALWAYS_INLINE unsigned Node::refCount() const
+ALWAYS_INLINE int Node::refCount() const
 {
-    return m_refCountAndParentBit / s_refCountIncrement;
+    return m_refCount;
 }
 
 // Used in Node::addSubresourceAttributeURLs() and in addSubresourceStyleURLs()
@@ -742,8 +736,6 @@ inline void Node::setParentNode(ContainerNode* parent)
 {
     ASSERT(isMainThread());
     m_parentNode = parent;
-    auto refCountWithoutParentBit = m_refCountAndParentBit & s_refCountMask;
-    m_refCountAndParentBit = refCountWithoutParentBit | !!parent;
 }
 
 inline ContainerNode* Node::parentNode() const