Changeset 208378 in webkit

Timestamp:

Nov 3, 2016 11:21:06 PM (7 years ago)

Author:

Antti Koivisto

Message:

REGRESSION (r207717): DumpRenderTree crashed in com.apple.WebCore: WebCore::Style::Scope::flushPendingUpdate + 16
​https://bugs.webkit.org/show_bug.cgi?id=164397
<rdar://problem/29100135>

Reviewed by Ryosuke Niwa.

The problem here was that we were leaving stale pointers to Document::m_inDocumentShadowRoots set when
using fast-path document teardown.

(Patch and stories mostly by rniwa).

• dom/Document.cpp:

(WebCore::Document::~Document):
(WebCore::Document::didInsertInDocumentShadowRoot):
(WebCore::Document::didRemoveInDocumentShadowRoot):

Improve asserts.

• dom/Element.cpp:

(WebCore::Element::removeShadowRoot):

Remove the superfluous call to notifyChildNodeRemoved in Element::removeShadowRoot to
avoid invoking notifyChildNodeRemoved during a document teardown, which is incorrect. It's sufficient that
~ShadowRoot calls ContainerNode::removeDetachedChildren(), and in turn removeDetachedChildrenInContainer()
since the latter function tears down nodes via the deletion queue during a document destruction and use
notifyChildNodeRemoved() on nodes that outlive the shadow root.

• dom/ShadowRoot.cpp:

(WebCore::ShadowRoot::~ShadowRoot):

Take care to clean up inDocumentShadowRoots for fast-pathed destruction too.

(WebCore::ShadowRoot::insertedInto):
(WebCore::ShadowRoot::removedFrom):

Improve ShadowRoot's insertedInto and removedFrom so that they only try to add and remove itself from
m_inDocumentShadowRoots when the connected-ness changes.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (2 diffs)
• dom/Element.cpp (modified) (1 diff)
• dom/ShadowRoot.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-11-03  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r207717): DumpRenderTree crashed in com.apple.WebCore: WebCore::Style::Scope::flushPendingUpdate + 16
+        https://bugs.webkit.org/show_bug.cgi?id=164397
+        <rdar://problem/29100135>
+
+        Reviewed by Ryosuke Niwa.
+
+        The problem here was that we were leaving stale pointers to Document::m_inDocumentShadowRoots set when
+        using fast-path document teardown.
+
+        (Patch and stories mostly by rniwa).
+
+        * dom/Document.cpp:
+        (WebCore::Document::~Document):
+        (WebCore::Document::didInsertInDocumentShadowRoot):
+        (WebCore::Document::didRemoveInDocumentShadowRoot):
+
+            Improve asserts.
+
+        * dom/Element.cpp:
+        (WebCore::Element::removeShadowRoot):
+
+            Remove the superfluous call to notifyChildNodeRemoved in Element::removeShadowRoot to
+            avoid invoking notifyChildNodeRemoved during a document teardown, which is incorrect. It's sufficient that
+            ~ShadowRoot calls ContainerNode::removeDetachedChildren(), and in turn removeDetachedChildrenInContainer()
+            since the latter function tears down nodes via the deletion queue during a document destruction and use
+            notifyChildNodeRemoved() on nodes that outlive the shadow root.
+
+        * dom/ShadowRoot.cpp:
+        (WebCore::ShadowRoot::~ShadowRoot):
+
+            Take care to clean up inDocumentShadowRoots for fast-pathed destruction too.
+
+        (WebCore::ShadowRoot::insertedInto):
+        (WebCore::ShadowRoot::removedFrom):
+
+            Improve ShadowRoot's insertedInto and removedFrom so that they only try to add and remove itself from
+            m_inDocumentShadowRoots when the connected-ness changes.
+
 2016-11-03  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -592 +592 @@
     ASSERT(!m_parentTreeScope);
     ASSERT(!m_disabledFieldsetElementsCount);
+    ASSERT(m_inDocumentShadowRoots.isEmpty());
 
 #if ENABLE(DEVICE_ORIENTATION) && PLATFORM(IOS)
@@ -6981 +6982 @@
 {
     ASSERT(shadowRoot.inDocument());
+    ASSERT(!m_inDocumentShadowRoots.contains(&shadowRoot));
     m_inDocumentShadowRoots.add(&shadowRoot);
 }

trunk/Source/WebCore/dom/Element.cpp

@@ -1785 +1785 @@
     oldRoot->setHost(nullptr);
     oldRoot->setParentTreeScope(&document());
-
-    notifyChildNodeRemoved(*this, *oldRoot);
 }
 

trunk/Source/WebCore/dom/ShadowRoot.cpp

@@ -71 +71 @@
 ShadowRoot::~ShadowRoot()
 {
+    if (inDocument())
+        document().didRemoveInDocumentShadowRoot(*this);
+
     // We cannot let ContainerNode destructor call willBeDeletedFrom()
     // for this ShadowRoot instance because TreeScope destructor
@@ -85 +88 @@
 Node::InsertionNotificationRequest ShadowRoot::insertedInto(ContainerNode& insertionPoint)
 {
-    auto result = DocumentFragment::insertedInto(insertionPoint);
-    if (inDocument())
+    bool wasInDocument = inDocument();
+    DocumentFragment::insertedInto(insertionPoint);
+    if (insertionPoint.inDocument() && !wasInDocument)
         document().didInsertInDocumentShadowRoot(*this);
-    return result;
+    return InsertionDone;
 }
 
 void ShadowRoot::removedFrom(ContainerNode& insertionPoint)
 {
-    if (inDocument())
+    DocumentFragment::removedFrom(insertionPoint);
+    if (insertionPoint.inDocument() && !inDocument())
         document().didRemoveInDocumentShadowRoot(*this);
-    DocumentFragment::removedFrom(insertionPoint);
 }