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.
Created attachment 188264 [details] Patch
Comment on attachment 188264 [details] Patch Attachment 188264 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16559342
Created attachment 188268 [details] Patch
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
Created attachment 188281 [details] For re-executing the test
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
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
Created attachment 188477 [details] Fixed crashes
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
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
Created attachment 189015 [details] Patch
Comment on attachment 189015 [details] Patch Attachment 189015 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16628422
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.
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.
Created attachment 189268 [details] Patch
Comment on attachment 189268 [details] Patch Attachment 189268 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16652212
Created attachment 189442 [details] Fixing EFL build failure
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*)
Created attachment 189712 [details] Patch
Created attachment 189910 [details] Patch for landing
Comment on attachment 189910 [details] Patch for landing Clearing flags on attachment: 189910 Committed r143840: <http://trac.webkit.org/changeset/143840>
All reviewed patches have been landed. Closing bug.
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
Filed https://bugs.webkit.org/show_bug.cgi?id=110749
Rolled out in r143940.
(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.
Created attachment 191385 [details] Patch for landing
Comment on attachment 191385 [details] Patch for landing The crash was due to Bug 108713. I included one line fix for that as well.
(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?
(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();
Comment on attachment 191385 [details] Patch for landing Clearing flags on attachment: 191385 Committed r144735: <http://trac.webkit.org/changeset/144735>
*** Bug 108710 has been marked as a duplicate of this bug. ***
*** Bug 108713 has been marked as a duplicate of this bug. ***