Attachment 357033 Details for Bug 192461 – Cleanup

[patch] Cleanup

bug-192461-20181210210210.patch (text/plain), 13.37 KB, created by Ryosuke Niwa on 2018-12-10 21:02:11 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-12-10 21:02:11 PST

Size: 13.37 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 239063)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,60 @@
>+2018-12-07  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        Remove children of ShadowRoot before deleting it
>+        https://bugs.webkit.org/show_bug.cgi?id=192461
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch changes the order in which ShadowRoot and its descendant nodes are destructed such that
>+        descendant nodes are now removed from ShadowRoot and destructed before ShadowRoot itself is deleted.
>+        With this change, it's safe to use Ref / RefPtr on ShadowRoot or Document inside removedFromAncestor.
>+
>+        Before this patch, the code that runs as a part of removedFromAncestor on any of those descendant nodes
>+        had to carefully avoid invoking ref() or deref() on ShadowRoot via Ref and RefPtr because that would
>+        result in ref'ing an object that's already being destructed. See r235956 for an example.
>+
>+        To do this, we introduce ShadowRoot::removedLastRef() much like Document::removedLastRef(), which is
>+        invoked in Node::removedLastRef(). ShadowRoot::removedLastRef() first invokes removeDetachedChildren on
>+        descendent nodes and then do "delete this". During the invocation of removeDetachedChildren, it is safe
>+        to ref & deref this ShadowRoot as long as they're paired because this object had just lost the last ref
>+        count but is yet to be destructed.
>+
>+        However, the last such deref() call inside removeDetachedChildren() would bring down m_refCount to 0
>+        once more and would reentrantly invoke removedLastRef(). We avoid this problem by manually incrementing
>+        and decrementing m_refCount before and after calling removeDetachedChildren(). This patch brings this
>+        fix to Document::removedLastRef() and introduces Document::isInTeardown() to fix custom elements code
>+        which replied on refCount() being 0 during removeDetachedChildren() to avoid some work.
>+
>+        This patch also removes m_inRemovedLastRefFunction and the related assertions as it is now totally sound
>+        to invoke deref() inside ShadowRoot::removedLastRef() and m_deletionHasBegun provides a better assertions
>+        for preventing ref'ing an object that is already running or has already ran its destructor.
>+
>+        Finally, this patch reverts r235956 to demonstrate the new capabilty to ref & deref a ShadowRoot during
>+        a tree teardown.
>+
>+        * dom/CustomElementReactionQueue.cpp:
>+        (WebCore::CustomElementReactionQueue::enqueueConnectedCallbackIfNeeded):
>+        (WebCore::CustomElementReactionQueue::enqueueDisconnectedCallbackIfNeeded):
>+        (WebCore::CustomElementReactionQueue::enqueueAdoptedCallbackIfNeeded):
>+        (WebCore::CustomElementReactionQueue::enqueueAttributeChangedCallbackIfNeeded):
>+        * dom/Document.cpp:
>+        (WebCore::Document::removedLastRef):
>+        * dom/Document.h:
>+        (WebCore::Document::isInTeardown const):
>+        * dom/Node.cpp:
>+        (WebCore::Node::removedLastRef):
>+        * dom/Node.h:
>+        (WebCore::adopted):
>+        (WebCore::Node::ref):
>+        (WebCore::Node::deref):
>+        (WebCore::Node::hasOneRef const):
>+        * dom/ShadowRoot.cpp:
>+        (WebCore::ShadowRoot::~ShadowRoot):
>+        (WebCore::ShadowRoot::removedLastRef):
>+        * dom/ShadowRoot.h:
>+        * html/FormAssociatedElement.cpp:
>+        (WebCore::FormAssociatedElement::formOwnerRemovedFromTree):
>+
> 2018-12-10  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Use WeakPtr to refer to VTTCue in VTTCueBox
>Index: Source/WebCore/dom/CustomElementReactionQueue.cpp
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.cpp	(revision 239063)
>+++ Source/WebCore/dom/CustomElementReactionQueue.cpp	(working copy)
>@@ -151,7 +151,7 @@ void CustomElementReactionQueue::enqueue
> {
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>-    ASSERT(element.document().refCount() > 0);
>+    ASSERT(!element.document().isInTeardown());
>     auto& queue = ensureCurrentQueue(element);
>     if (queue.m_interface->hasConnectedCallback())
>         queue.m_items.append({CustomElementReactionQueueItem::Type::Connected});
>@@ -161,7 +161,7 @@ void CustomElementReactionQueue::enqueue
> {
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>-    if (element.document().refCount() <= 0)
>+    if (element.document().isInTeardown())
>         return; // Don't enqueue disconnectedCallback if the entire document is getting destructed.
>     auto& queue = ensureCurrentQueue(element);
>     if (queue.m_interface->hasDisconnectedCallback())
>@@ -172,7 +172,7 @@ void CustomElementReactionQueue::enqueue
> {
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>-    ASSERT(element.document().refCount() > 0);
>+    ASSERT(!element.document().isInTeardown());
>     auto& queue = ensureCurrentQueue(element);
>     if (queue.m_interface->hasAdoptedCallback())
>         queue.m_items.append({oldDocument, newDocument});
>@@ -182,7 +182,7 @@ void CustomElementReactionQueue::enqueue
> {
>     ASSERT(CustomElementReactionDisallowedScope::isReactionAllowed());
>     ASSERT(element.isDefinedCustomElement());
>-    ASSERT(element.document().refCount() > 0);
>+    ASSERT(!element.document().isInTeardown());
>     auto& queue = ensureCurrentQueue(element);
>     if (queue.m_interface->observesAttribute(attributeName.localName()))
>         queue.m_items.append({attributeName, oldValue, newValue});
>Index: Source/WebCore/dom/Document.cpp
>===================================================================
>--- Source/WebCore/dom/Document.cpp	(revision 239063)
>+++ Source/WebCore/dom/Document.cpp	(working copy)
>@@ -672,10 +672,18 @@ Document::~Document()
> void Document::removedLastRef()
> {
>     ASSERT(!m_deletionHasBegun);
>+    ASSERT(m_refCount <= 0);
>+
>     if (m_referencingNodeCount) {
>+        ASSERT(!m_isInTeardown);
>+        // When HTML parser, etc... is holding onto a node of this document,
>+        // it could ref() and deref() this document again.
>+        SetForScope<bool> inTeardown(m_isInTeardown, true);
>+
>         // 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();
>+        ++m_refCount; // Prevent a reentrant call of this function via deref().
> 
>         RELEASE_ASSERT(!hasLivingRenderTree());
>         // We must make sure not to be retaining any of our children through
>@@ -712,14 +720,15 @@ void Document::removedLastRef()
>         
>         commonTeardown();
> 
>-#ifndef NDEBUG
>-        // We need to do this right now since selfOnlyDeref() can delete this.
>-        m_inRemovedLastRefFunction = false;
>-#endif
>+        --m_refCount; // Prevent a reentrant call of this function via deref().
>+
>+        // It's possible to temporarily ref this Document in the above code
>+        // but no heap object should be holding onto this Document at this point.
>+        RELEASE_ASSERT(m_refCount <= 0);
>+
>         decrementReferencingNodeCount();
>     } else {
> #ifndef NDEBUG
>-        m_inRemovedLastRefFunction = false;
>         m_deletionHasBegun = true;
> #endif
>         delete this;
>Index: Source/WebCore/dom/Document.h
>===================================================================
>--- Source/WebCore/dom/Document.h	(revision 239063)
>+++ Source/WebCore/dom/Document.h	(working copy)
>@@ -388,6 +388,8 @@ public:
> 
>     unsigned referencingNodeCount() const { return m_referencingNodeCount; }
> 
>+    bool isInTeardown() const { return m_isInTeardown; }
>+
>     void removedLastRef();
> 
>     DocumentIdentifier identifier() const { return m_identifier; }
>@@ -1938,6 +1940,7 @@ private:
> 
>     unsigned m_listenerTypes { 0 };
>     unsigned m_referencingNodeCount { 0 };
>+    bool m_isInTeardown { false };
>     int m_loadEventDelayCount { 0 };
>     unsigned m_lastStyleUpdateSizeForTesting { 0 };
> 
>Index: Source/WebCore/dom/Node.cpp
>===================================================================
>--- Source/WebCore/dom/Node.cpp	(revision 239063)
>+++ Source/WebCore/dom/Node.cpp	(working copy)
>@@ -2529,11 +2529,16 @@ void Node::removedLastRef()
>     // 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.
>-    if (is<Document>(*this)) {
>+    if (UNLIKELY(is<Document>(*this))) {
>         downcast<Document>(*this).removedLastRef();
>         return;
>     }
> 
>+    if (UNLIKELY(is<ShadowRoot>(*this))) {
>+        downcast<ShadowRoot>(*this).removedLastRef();
>+        return;
>+    }
>+
> #ifndef NDEBUG
>     m_deletionHasBegun = true;
> #endif
>Index: Source/WebCore/dom/Node.h
>===================================================================
>--- Source/WebCore/dom/Node.h	(revision 239063)
>+++ Source/WebCore/dom/Node.h	(working copy)
>@@ -82,6 +82,7 @@ class Node : public EventTarget {
>     WTF_MAKE_ISO_ALLOCATED(Node);
> 
>     friend class Document;
>+    friend class ShadowRoot;
>     friend class TreeScope;
> public:
>     enum NodeType {
>@@ -507,7 +508,6 @@ public:
> 
> #ifndef NDEBUG
>     bool m_deletionHasBegun { false };
>-    bool m_inRemovedLastRefFunction { false };
>     bool m_adoptionIsRequired { true };
> #endif
> 
>@@ -687,7 +687,6 @@ inline void adopted(Node* node)
>     if (!node)
>         return;
>     ASSERT(!node->m_deletionHasBegun);
>-    ASSERT(!node->m_inRemovedLastRefFunction);
>     node->m_adoptionIsRequired = false;
> }
> #endif
>@@ -696,7 +695,6 @@ ALWAYS_INLINE void Node::ref()
> {
>     ASSERT(isMainThread());
>     ASSERT(!m_deletionHasBegun);
>-    ASSERT(!m_inRemovedLastRefFunction);
>     ASSERT(!m_adoptionIsRequired);
>     ++m_refCount;
> }
>@@ -706,20 +704,14 @@ ALWAYS_INLINE void Node::deref()
>     ASSERT(isMainThread());
>     ASSERT(m_refCount >= 0);
>     ASSERT(!m_deletionHasBegun);
>-    ASSERT(!m_inRemovedLastRefFunction);
>     ASSERT(!m_adoptionIsRequired);
>-    if (--m_refCount <= 0 && !parentNode()) {
>-#ifndef NDEBUG
>-        m_inRemovedLastRefFunction = true;
>-#endif
>+    if (--m_refCount <= 0 && !parentNode())
>         removedLastRef();
>-    }
> }
> 
> ALWAYS_INLINE bool Node::hasOneRef() const
> {
>     ASSERT(!m_deletionHasBegun);
>-    ASSERT(!m_inRemovedLastRefFunction);
>     return m_refCount == 1;
> }
> 
>Index: Source/WebCore/dom/ShadowRoot.cpp
>===================================================================
>--- Source/WebCore/dom/ShadowRoot.cpp	(revision 239063)
>+++ Source/WebCore/dom/ShadowRoot.cpp	(working copy)
>@@ -72,7 +72,6 @@ ShadowRoot::ShadowRoot(Document& documen
> {
> }
> 
>-
> ShadowRoot::~ShadowRoot()
> {
>     if (isConnected())
>@@ -86,14 +85,28 @@ ShadowRoot::~ShadowRoot()
>     // clears Node::m_treeScope thus ContainerNode is no longer able
>     // to access it Document reference after that.
>     willBeDeletedFrom(document());
>+}
>+
>+void ShadowRoot::removedLastRef()
>+{
>+    ASSERT(!m_deletionHasBegun);
>+    ASSERT(m_refCount <= 0);
> 
>     ASSERT(!m_hasBegunDeletingDetachedChildren);
>     m_hasBegunDeletingDetachedChildren = true;
> 
>-    // We must remove all of our children first before the TreeScope destructor
>-    // runs so we don't go through Node::setTreeScopeRecursively for each child with a
>-    // destructed tree scope in each descendant.
>+    ++m_refCount; // Prevent a reentrant call of this function via deref() during removeDetachedChildren().
>     removeDetachedChildren();
>+    --m_refCount;
>+
>+    // It's possible for removeDetachedChildren() to temporarily ref this ShadowRoot
>+    // but no heap object should be holding onto this ShadowRoot at this point.
>+    RELEASE_ASSERT(m_refCount <= 0);
>+
>+#ifndef NDEBUG
>+    m_deletionHasBegun = true;
>+#endif
>+    delete this;
> }
> 
> Node::InsertedIntoAncestorResult ShadowRoot::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree)
>Index: Source/WebCore/dom/ShadowRoot.h
>===================================================================
>--- Source/WebCore/dom/ShadowRoot.h	(revision 239063)
>+++ Source/WebCore/dom/ShadowRoot.h	(working copy)
>@@ -54,6 +54,8 @@ public:
> 
>     using TreeScope::rootNode;
> 
>+    void removedLastRef();
>+    
>     Style::Scope& styleScope();
>     StyleSheetList& styleSheets();
> 
>Index: Source/WebCore/html/FormAssociatedElement.cpp
>===================================================================
>--- Source/WebCore/html/FormAssociatedElement.cpp	(revision 239063)
>+++ Source/WebCore/html/FormAssociatedElement.cpp	(working copy)
>@@ -123,8 +123,8 @@ void FormAssociatedElement::formOwnerRem
> {
>     ASSERT(m_form);
>     // Can't use RefPtr here beacuse this function might be called inside ~ShadowRoot via addChildNodesToDeletionQueue. See webkit.org/b/189493.
>-    Node* rootNode = &asHTMLElement();
>-    for (auto* ancestor = asHTMLElement().parentNode(); ancestor; ancestor = ancestor->parentNode()) {
>+    RefPtr<Node> rootNode = &asHTMLElement();
>+    for (auto ancestor = makeRefPtr(asHTMLElement().parentNode()); ancestor; ancestor = ancestor->parentNode()) {
>         if (ancestor == m_form) {
>             // Form is our ancestor so we don't need to reset our owner, we also no longer
>             // need an id observer since we are no longer connected.

Actions: View | Formatted Diff | Diff

Attachments on bug 192461: 356718 | 356742 | 357033 | 357036 | 359073 | 359089