Bug 109777 – ShadowRoot needs guardRef() and guardDeref()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

After http://trac.webkit.org/changeset/137524, ShadowRoot has similar lifecycle characteristics to one of Document's:
It needs to live even after refcount goes zero. For this purpose, ShadowRoot needs guardRef and guardDeref() and
Scope change code needs to take care of that.

[Hajime Morrita]

Created attachment 188264 [details]
Patch

[Build Bot]

Comment on attachment 188264 [details]
Patch

Attachment 188264 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16559342

[Hajime Morrita]

Created attachment 188268 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188268 [details]
Patch

Attachment 188268 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16542470

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-describedby-on-input.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
http/tests/appcache/access-via-redirect.php
animations/3d/change-transform-in-end-event.html
accessibility/aria-controls-with-tabs.html
http/tests/appcache/destroyed-frame.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
accessibility/adjacent-continuations-cause-assertion-failure.html

[Hajime Morrita]

Created attachment 188281 [details]
For re-executing the test

[WebKit Review Bot]

Comment on attachment 188281 [details]
For re-executing the test

Attachment 188281 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16546559

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-describedby-on-input.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
animations/animation-controller-drt-api.html
animations/3d/change-transform-in-end-event.html
accessibility/aria-controls-with-tabs.html
canvas/philip/tests/2d.canvas.readonly.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/access-via-redirect.php
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
accessibility/adjacent-continuations-cause-assertion-failure.html

[WebKit Review Bot]

Comment on attachment 188281 [details]
For re-executing the test

Attachment 188281 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16581236

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-describedby-on-input.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
http/tests/appcache/access-via-redirect.php
animations/3d/change-transform-in-end-event.html
accessibility/aria-controls-with-tabs.html
http/tests/appcache/destroyed-frame.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
accessibility/adjacent-continuations-cause-assertion-failure.html

[Hajime Morrita]

Created attachment 188477 [details]
Fixed crashes

[WebKit Review Bot]

Comment on attachment 188477 [details]
Fixed crashes

Attachment 188477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16570710

New failing tests:
accessibility/aria-checkbox-checked.html
animations/3d/matrix-transform-type-animation.html
http/tests/appcache/cyrillic-uri.html
accessibility/anchor-linked-anonymous-block-crash.html
animations/animation-add-events-in-handler.html
http/tests/appcache/deferred-events-delete-while-raising.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-describedby-on-input.html
http/tests/appcache/crash-when-navigating-away-then-back.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
accessibility/accessibility-object-detached.html
http/tests/appcache/abort-cache-ondownloading-manifest-404.html
http/tests/appcache/credential-url.html
http/tests/appcache/access-via-redirect.php
animations/3d/change-transform-in-end-event.html
accessibility/aria-controls-with-tabs.html
http/tests/appcache/destroyed-frame.html
animations/animation-controller-drt-api.html
animations/3d/transform-perspective.html
accessibility/accessibility-node-reparent.html
http/tests/appcache/deferred-events.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
accessibility/adjacent-continuations-cause-assertion-failure.html

[Build Bot]

Comment on attachment 188477 [details]
Fixed crashes

Attachment 188477 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16559831

New failing tests:
accessibility/anchor-linked-anonymous-block-crash.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
http/tests/cache/loaded-from-cache-after-reload-within-iframe.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
http/tests/cache/cached-main-resource.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Hajime Morrita]

Created attachment 189015 [details]
Patch

[EFL EWS Bot]

Comment on attachment 189015 [details]
Patch

Attachment 189015 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16628422

[Elliott Sprehn]

Comment on attachment 189015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189015&action=review

Overall this looks okay, I don't like the special method for updating the scope. Just add a check in setTreeScope and call that.

What's the long term plan for all this btw? Is haraken going to fix our if ancestor then child GC behavior? The guard refs appear to just be a hack around that.

> Source/WebCore/ChangeLog:23
> +        - Introduced setTreeScopeOfThisScope() which set Node::m_treeScope without chaning scope's m_guardRefCount.

changing

> Source/WebCore/dom/Document.h:1582
> +    scope->guardRef();

if (scope != this) scope->guardRef();

> Source/WebCore/dom/Document.h:1587
> +inline void Node::setTreeScopeOfThisScope(TreeScope* scope)

This method is confusing.

> Source/WebCore/dom/DocumentFragment.cpp:41
> +    ASSERT(document);

Is there a reason for this change?

> Source/WebCore/dom/TreeScope.cpp:105
> +    m_rootNode->setTreeScopeOfThisScope(noDocumentInstance());

Clever.

> Source/WebCore/dom/TreeScope.cpp:119
> +    rootNode()->m_inRemovedLastRefFunction = false;

This seems like a bad place to do this since the value depends on if you call SuperClass::dispose() before or after in your override.

> Source/WebCore/dom/TreeScope.h:107
> +    // Nodes belonging to this document hold guard references -

not a document. Fix the comment.

> Source/WebCore/editing/RemoveNodeCommand.cpp:29
> +#include "Document.h"

This seems like it's unrelated.

[Hajime Morrita]

Comment on attachment 189015 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189015&action=review

>> Source/WebCore/ChangeLog:23
>> +        - Introduced setTreeScopeOfThisScope() which set Node::m_treeScope without chaning scope's m_guardRefCount.
> 
> changing

Fixed.

>> Source/WebCore/dom/Document.h:1582
>> +    scope->guardRef();
> 
> if (scope != this) scope->guardRef();

The condition is rejected by the caller side. 
Anyway, I'll do this ref/unref on caller side to avoid confusion.

>> Source/WebCore/dom/Document.h:1587
>> +inline void Node::setTreeScopeOfThisScope(TreeScope* scope)
> 
> This method is confusing.

Maybe. Will remove.

>> Source/WebCore/dom/DocumentFragment.cpp:41
>> +    ASSERT(document);
> 
> Is there a reason for this change?

Yep. Now ShadowRoot, a subclass of DocumentFragment, passes null document to DocumentFragment constructor.
And the ShadowRoot change is to align what Document is doing.

>> Source/WebCore/dom/TreeScope.cpp:119
>> +    rootNode()->m_inRemovedLastRefFunction = false;
> 
> This seems like a bad place to do this since the value depends on if you call SuperClass::dispose() before or after in your override.

Right. Will move to caller side.

>> Source/WebCore/dom/TreeScope.h:107
>> +    // Nodes belonging to this document hold guard references -
> 
> not a document. Fix the comment.

Woa good catch! Will do.

>> Source/WebCore/editing/RemoveNodeCommand.cpp:29
>> +#include "Document.h"
> 
> This seems like it's unrelated.

Well, I inlined moved some function to Document.h.
The linker told that this file uses it somewhere.

[Hajime Morrita]

Created attachment 189268 [details]
Patch

[EFL EWS Bot]

Comment on attachment 189268 [details]
Patch

Attachment 189268 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16652212

[Hajime Morrita]

Created attachment 189442 [details]
Fixing EFL build failure

[Elliott Sprehn]

Comment on attachment 189442 [details]
Fixing EFL build failure

View in context: https://bugs.webkit.org/attachment.cgi?id=189442&action=review

Couple nits, but otherwise looks good.

> Source/WebCore/dom/TreeScope.h:134
> +    virtual void dispose() { }

I'd make this private.

> Source/WebCore/dom/TreeScopeAdopter.h:48
> +    void setNewScopeTo(Node*) const;

Lets rename this updateTreeScope(Node*)

[Hajime Morrita]

Created attachment 189712 [details]
Patch

[Hajime Morrita]

Created attachment 189910 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189910 [details]
Patch for landing

Clearing flags on attachment: 189910

Committed r143840: <http://trac.webkit.org/changeset/143840>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vsevolod Vlasov]

This patch caused assertion failures in multiple layout tests,

fullscreen/full-screen-enabled-prefixed.html
fast/dom/HTMLMeterElement/meter-element-repaint-on-update-value.html
fast/forms/month/month-stepup-stepdown.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fullscreen%2Ffull-screen-enabled-prefixed.html%2Cfast%2Fdom%2FHTMLMeterElement%2Fmeter-element-repaint-on-update-value.html%2Cfast%2Fforms%2Fmonth%2Fmonth-stepup-stepdown.html

crash log for DumpRenderTree (pid 2101):
STDOUT: <empty>
STDERR: ASSERTION FAILED: node->treeScope() == m_oldScope
STDERR: ../../third_party/WebKit/Source/WebCore/dom/TreeScopeAdopter.cpp(109) : void WebCore::TreeScopeAdopter::updateTreeScope(WebCore::Node *) const
STDERR: 1   0x74e2a2d WebCore::TreeScopeAdopter::updateTreeScope(WebCore::Node*) const
STDERR: 2   0x74e252a WebCore::TreeScopeAdopter::moveTreeToNewScope(WebCore::Node*) const
STDERR: 3   0x74e2644 WebCore::TreeScopeAdopter::moveTreeToNewScope(WebCore::Node*) const
STDERR: 4   0x74dd7e1 WebCore::TreeScopeAdopter::execute() const
STDERR: 5   0x74dc1f0 WebCore::TreeScope::adoptIfNeeded(WebCore::Node*)
STDERR: 6   0x727c569 WebCore::Private::NodeRemovalDispatcher<WebCore::Node, WebCore::ContainerNode, true>::dispatch(WebCore::Node*, WebCore::ContainerNode*)
STDERR: 7   0x727c4b4 void WebCore::Private::addChildNodesToDeletionQueue<WebCore::Node, WebCore::ContainerNode>(WebCore::Node*&, WebCore::Node*&, WebCore::ContainerNode*)
STDERR: 8   0x7278939 void WebCore::removeDetachedChildrenInContainer<WebCore::Node, WebCore::ContainerNode>(WebCore::ContainerNode*)
STDERR: 9   0x7272514 WebCore::ContainerNode::removeDetachedChildren()
STDERR: 10  0x74bdc6b WebCore::ShadowRoot::dispose()
STDERR: 11  0x74bdcc1 non-virtual thunk to WebCore::ShadowRoot::dispose()
STDERR: 12  0x7431179 WebCore::TreeScope::removedLastRefToScope()
STDERR: 13  0x74299fe WebCore::Node::removedLastRef()
STDERR: 14  0x6529bc4 WebCore::TreeShared<WebCore::Node>::deref()
STDERR: 15  0x7ba472d void WTF::derefIfNotNull<WebCore::ShadowRoot>(WebCore::ShadowRoot*)
STDERR: 16  0x7ba46bd WTF::RefPtr<WebCore::ShadowRoot>::~RefPtr()
STDERR: 17  0x7ba466b WTF::RefPtr<WebCore::ShadowRoot>::~RefPtr()
STDERR: 18  0x73804a4 WebCore::ElementShadow::removeAllShadowRoots()
STDERR: 19  0x73b858e WebCore::ElementShadow::~ElementShadow()
STDERR: 20  0x73b84cb WebCore::ElementShadow::~ElementShadow()
STDERR: 21  0x73b846d void WTF::deleteOwnedPtr<WebCore::ElementShadow>(WebCore::ElementShadow*)
STDERR: 22  0x73beda9 WTF::OwnPtr<WebCore::ElementShadow>::clear()
STDERR: 23  0x73bed45 WTF::OwnPtr<WebCore::ElementShadow>::operator=(nullptr_t)
STDERR: 24  0x7394f12 WebCore::ElementRareData::clearShadow()
STDERR: 25  0x7383409 WebCore::Element::~Element()
STDERR: 26  0x74c9652 WebCore::StyledElement::~StyledElement()
STDERR: 27  0x77e1e4b WebCore::HTMLElement::~HTMLElement()
STDERR: 28  0x765e4ab WebCore::LabelableElement::~LabelableElement()
STDERR: 29  0x76139ab WebCore::HTMLProgressElement::~HTMLProgressElement()
STDERR: 30  0x761395b WebCore::HTMLProgressElement::~HTMLProgressElement()
STDERR: 31  0x76138fe WebCore::HTMLProgressElement::~HTMLProgressElement()
STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef
STDERR:  [0x00000295287f]
STDERR:  [0x00000295281b]
STDERR:  [0x0000029524ab]
STDERR:  [0x000096f1059b]
STDERR:  [0x0000ffffffff]
STDERR:  [0x0000074e252a]
STDERR:  [0x0000074e2644]
STDERR:  [0x0000074dd7e1]
STDERR:  [0x0000074dc1f0]
STDERR:  [0x00000727c569]
STDERR:  [0x00000727c4b4]
STDERR:  [0x000007278939]
STDERR:  [0x000007272514]
STDERR:  [0x0000074bdc6b]
STDERR:  [0x0000074bdcc1]
STDERR:  [0x000007431179]
STDERR:  [0x0000074299fe]
STDERR:  [0x000006529bc4]
STDERR:  [0x000007ba472d]
STDERR:  [0x000007ba46bd]
STDERR:  [0x000007ba466b]
STDERR:  [0x0000073804a4]
STDERR:  [0x0000073b858e]
STDERR:  [0x0000073b84cb]
STDERR:  [0x0000073b846d]
STDERR:  [0x0000073beda9]
STDERR:  [0x0000073bed45]
STDERR:  [0x000007394f12]
STDERR:  [0x000007383409]
STDERR:  [0x0000074c9652]
STDERR:  [0x0000077e1e4b]
STDERR:  [0x00000765e4ab]
STDERR:  [0x0000076139ab]
STDERR:  [0x00000761395b]
STDERR:  [0x0000076138fe]
STDERR:  [0x000007429a20]
STDERR:  [0x000006529bc4]
STDERR:  [0x000007f29481]
STDERR:  [0x000008c57478]
STDERR:  [0x000007f6f857]
STDERR:  [0x0000030392a6]
STDERR:  [0x000003036802]
STDERR:  [0x000003051a1b]
STDERR:  [0x000003051229]
STDERR:  [0x000002f5a9ab]
STDERR:  [0x00000305213b]
STDERR:  [0x00000305201c]
STDERR:  [0x0000033c5fa6]
STDERR:  [0x0000033d0f70]
STDERR:  [0x000002f1d6ad]
STDERR:  [0x000002f0d8da]
STDERR:  [0x000002f0d7a9]
STDERR:  [0x000002eafdbb]
STDERR:  [0x000008ce3087]
STDERR:  [0x000008ce2408]
STDERR:  [0x000008c872f2]
STDERR:  [0x000008c86ff9]
STDERR:  [0x000008c8a535]
STDERR:  [0x00000962f9bb]
STDERR:  [0x000009485c06]
STDERR:  [0x00000945d00f]
STDERR:  [0x000006629bb5]
STDERR: ax: bbadbeef, bx: 7c214c01, cx: ab63af32, dx: ab63af32
STDERR: di: 9b30552, si: 9b304f6, bp: c0056878, sp: c0056840, ss: 23, flags: 10282
STDERR: ip: 74e2a37, cs: 1b, ds: 23, es: 23, fs: 0, gs: f

[Vsevolod Vlasov]

Filed https://bugs.webkit.org/show_bug.cgi?id=110749

[Dimitri Glazkov (Google)]

Rolled out in r143940.

[Ryosuke Niwa]

(In reply to comment #25)
> Rolled out in r143940.

I don't think rolling out the patch makes much sense. We're only seeing failures of an assertion added in this patch.

All these svn churns are ridiculously annoying.

[Hajime Morrita]

Created attachment 191385 [details]
Patch for landing

[Hajime Morrita]

Comment on attachment 191385 [details]
Patch for landing

The crash was due to Bug 108713. I included one line fix for that as well.

[Elliott Sprehn]

(In reply to comment #28)
> (From update of attachment 191385 [details])
> The crash was due to Bug 108713. I included one line fix for that as well.

What was the one line fix?

[Hajime Morrita]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 191385 [details] [details])
> > The crash was due to Bug 108713. I included one line fix for that as well.
> 
> What was the one line fix?

One copied from https://bugs.webkit.org/attachment.cgi?id=188241&action=review

--- a/Source/WebCore/dom/Element.cpp
+++ b/Source/WebCore/dom/Element.cpp
@@ -2753,6 +2753,7 @@ PassRefPtr<Attr> Element::ensureAttr(const QualifiedName& name)
     RefPtr<Attr> attrNode = findAttrNodeInList(attrNodeList, name);
     if (!attrNode) {
         attrNode = Attr::create(this, name);
+        treeScope()->adoptIfNeeded(attrNode.get());
         attrNodeList->append(attrNode);
     }
     return attrNode.release();

[WebKit Review Bot]

Comment on attachment 191385 [details]
Patch for landing

Clearing flags on attachment: 191385

Committed r144735: <http://trac.webkit.org/changeset/144735>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 108710 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 108713 has been marked as a duplicate of this bug. ***