Bug 106634

third_party/WebKit/Source/WebCore/dom/ComposedShadowTreeWalker.cpp(202) : static WebCore::Node *WebCore::ComposedShadowTreeWalker::traverseBackToYoungerShadowRoot(const WebCore::Node *, WebCore::ComposedShadowTreeWalker::TraversalDirection) 1 0x7fc5bf1e9d4a 2 0x7fc5bf1e96b1 3 0x7fc5bf1e82df 4 0x7fc5bf1e7eaf 5 0x7fc5bf9bfd08 6 0x7fc5bf9bd1ab 7 0x7fc5bf9b81cc 8 0x7fc5bf9ba70c 9 0x7fc5bf6875c8 10 0x7fc5bf6877ee 11 0x7fc5bf20ffd5 12 0x7fc5bf1ffe44 13 0x7fc5bf687918 14 0x7fc5c15897bf 15 0x7fc5bf6b36f4 16 0x7fc5bf68af71 17 0x7fc5bfb53f8c 18 0x7fc5bf6581e9 19 0x7fc5bf68bbac 20 0x7fc5bfb53f8c 21 0x7fc5bf6581e9 22 0x7fc5bf68bbac 23 0x7fc5bf68c314 24 0x7fc5bf2dadb1 25 0x7fc5bf2c70b9 26 0x7fc5bf2b61e9 27 0x7fc5bf4e905a 28 0x7fc5c185bac6 29 0x7fc5c185ad59 30 0x7fc5a812f282 31 0x7fc5a8138214 ASAN:SIGSEGV ================================================================= ==2619== ERROR: AddressSanitizer: SEGV on unknown address 0x0000bbadbeef (pc 0x7fc5bf1e9d8e sp 0x7fffe61a2b60 bp 0x7fffe61a2db0 T0) AddressSanitizer can not provide additional info. #0 0x7fc5bf1e9d8d in WebCore::ComposedShadowTreeWalker::traverseBackToYoungerShadowRoot(WebCore::Node const*, WebCore::ComposedShadowTreeWalker::TraversalDirection) third_party/WebKit/Source/WebCore/dom/ComposedShadowTreeWalker.cpp:202 #1 0x7fc5bf1e96b0 in WebCore::ComposedShadowTreeWalker::traverseSiblingInCurrentTree(WebCore::Node const*, WebCore::ComposedShadowTreeWalker::TraversalDirection) third_party/WebKit/Source/WebCore/dom/ComposedShadowTreeWalker.cpp:190 #2 0x7fc5bf1e82de in WebCore::ComposedShadowTreeWalker::traverseSiblingOrBackToInsertionPoint(WebCore::Node const*, WebCore::ComposedShadowTreeWalker::TraversalDirection) third_party/WebKit/Source/WebCore/dom/ComposedShadowTreeWalker.cpp:174 #3 0x7fc5bf1e7eae in WebCore::ComposedShadowTreeWalker::nextSibling() third_party/WebKit/Source/WebCore/dom/ComposedShadowTreeWalker.cpp:149 #4 0x7fc5bf9bfd07 in WebCore::NodeRenderingTraversal::nextSiblingSlow(WebCore::Node const*) third_party/WebKit/Source/WebCore/dom/NodeRenderingTraversal.cpp:64 #5 0x7fc5bf9bd1aa in WebCore::NodeRenderingTraversal::nextSibling(WebCore::Node const*) third_party/WebKit/Source/WebCore/dom/NodeRenderingTraversal.h:96 #6 0x7fc5bf9b81cb in WebCore::NodeRenderingContext::nextRenderer() const third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:89 #7 0x7fc5bf9ba70b in WebCore::NodeRenderingContext::createRendererForElementIfNeeded() third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:229 #8 0x7fc5bf6875c7 in WebCore::Element::createRendererIfNeeded() third_party/WebKit/Source/WebCore/dom/Element.cpp:1161 #9 0x7fc5bf6877ed in WebCore::Element::attach() third_party/WebKit/Source/WebCore/dom/Element.cpp:1170 #10 0x7fc5bf20ffd4 in WebCore::ContainerNode::attachChildren() third_party/WebKit/Source/WebCore/dom/ContainerNode.h:206 #11 0x7fc5bf1ffe43 in WebCore::ContainerNode::attach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:786 #12 0x7fc5bf687917 in WebCore::Element::attach() third_party/WebKit/Source/WebCore/dom/Element.cpp:1184 #13 0x7fc5c15897be in WebCore::InsertionPoint::attach() third_party/WebKit/Source/WebCore/html/shadow/InsertionPoint.cpp:63 #14 0x7fc5bf6b36f3 in WebCore::Node::reattach() third_party/WebKit/Source/WebCore/dom/Node.h:883 #15 0x7fc5bf68af70 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/Element.cpp:1295 #16 0x7fc5bfb53f8b in WebCore::ShadowRoot::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/ShadowRoot.cpp:217 #17 0x7fc5bf6581e8 in WebCore::ElementShadow::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/ElementShadow.cpp:165 #18 0x7fc5bf68bbab in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/Element.cpp:1336 #19 0x7fc5bfb53f8b in WebCore::ShadowRoot::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/ShadowRoot.cpp:217 #20 0x7fc5bf6581e8 in WebCore::ElementShadow::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/ElementShadow.cpp:165 #21 0x7fc5bf68bbab in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/Element.cpp:1336 #22 0x7fc5bf68c313 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/Element.cpp:1360 #23 0x7fc5bf2dadb0 in WebCore::Document::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/Document.cpp:1824 #24 0x7fc5bf2c70b8 in WebCore::Document::updateStyleIfNeeded() third_party/WebKit/Source/WebCore/dom/Document.cpp:1867 #25 0x7fc5bf2b61e8 in WebCore::Document::styleRecalcTimerFired(WebCore::Timer<WebCore::Document>*) third_party/WebKit/Source/WebCore/dom/Document.cpp:1757 #26 0x7fc5bf4e9059 in WebCore::Timer<WebCore::Document>::fired() third_party/WebKit/Source/WebCore/platform/Timer.h:106 #27 0x7fc5c185bac5 in WebCore::ThreadTimers::sharedTimerFiredInternal() third_party/WebKit/Source/WebCore/platform/ThreadTimers.cpp:116 #28 0x7fc5c185ad58 in WebCore::ThreadTimers::sharedTimerFired() third_party/WebKit/Source/WebCore/platform/ThreadTimers.cpp:93 #29 0x7fc5a812f281 in webkit_glue::WebKitPlatformSupportImpl::DoTimeout() ./webkit/glue/webkitplatformsupport_impl.h:163 #30 0x7fc5a8138213 in base::internal::RunnableAdapter<void (webkit_glue::WebKitPlatformSupportImpl::*)()>::Run(webkit_glue::WebKitPlatformSupportImpl*) ./base/bind_internal.h:134 #31 0x7fc5a8137e07 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void (webkit_glue::WebKitPlatformSupportImpl::*)()>, void (webkit_glue::WebKitPlatformSupportImpl*)>::MakeItSo(base::internal::RunnableAdapter<void (webkit_glue::WebKitPlatformSupportImpl::*)()>, webkit_glue::WebKitPlatformSupportImpl*) ./base/bind_internal.h:871 #32 0x7fc5a8137af1 in base::internal::Invoker<1, base::internal::BindState<base::internal::RunnableAdapter<void (webkit_glue::WebKitPlatformSupportImpl::*)()>, void (webkit_glue::WebKitPlatformSupportImpl*), void (base::internal::UnretainedWrapper<webkit_glue::WebKitPlatformSupportImpl>)>, void (webkit_glue::WebKitPlatformSupportImpl*)>::Run(base::internal::BindStateBase*) ./base/bind_internal.h:1173 #33 0x7fc5afc2c302 in base::Callback<void ()>::Run() const ./base/callback.h:396 #34 0x7fc5b02d6027 in base::Timer::RunScheduledTask() base/timer.cc:181 #35 0x7fc5b02d6a12 in base::BaseTimerTaskInternal::Run() base/timer.cc:46 #36 0x7fc5b02d9873 in base::internal::RunnableAdapter<void (base::BaseTimerTaskInternal::*)()>::Run(base::BaseTimerTaskInternal*) ./base/bind_internal.h:134 #37 0x7fc5b02d9467 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void (base::BaseTimerTaskInternal::*)()>, void (base::BaseTimerTaskInternal*)>::MakeItSo(base::internal::RunnableAdapter<void (base::BaseTimerTaskInternal::*)()>, base::BaseTimerTaskInternal*) ./base/bind_internal.h:871 #38 0x7fc5b02d912d in base::internal::Invoker<1, base::internal::BindState<base::internal::RunnableAdapter<void (base::BaseTimerTaskInternal::*)()>, void (base::BaseTimerTaskInternal*), void (base::internal::OwnedWrapper<base::BaseTimerTaskInternal>)>, void (base::BaseTimerTaskInternal*)>::Run(base::internal::BindStateBase*) ./base/bind_internal.h:1173 #39 0x7fc5afc2c302 in base::Callback<void ()>::Run() const ./base/callback.h:396 #40 0x7fc5afe5da51 in MessageLoop::RunTask(base::PendingTask const&) base/message_loop.cc:473 #41 0x7fc5afe5f788 in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop.cc:485 #42 0x7fc5afe5fe89 in MessageLoop::DoWork() base/message_loop.cc:668 #43 0x7fc5afb2963b in base::MessagePumpGlib::HandleDispatch() base/message_pump_glib.cc:268 #44 0x7fc5afb2b64e in (anonymous namespace)::WorkSourceDispatch(_GSource*, int (*)(void*), void*) base/message_pump_glib.cc:105 #45 0x7fc5a14d5d52 in g_main_dispatch /build/buildd/glib2.0-2.32.3/./glib/gmain.c:2539 Stats: 15M malloced (92M for red zones) by 92632 calls Stats: 1M realloced by 1516 calls Stats: 13M freed by 75446 calls Stats: 3M really freed by 20477 calls Stats: 89M (22818 full pages) mmaped in 178 calls mmaps by size class: 10:71029; 11:1275; 12:896; 13:192; 14:160; 15:80; 16:24; 17:4; 18:18; 19:1; mallocs by size class: 10:89745; 11:1433; 12:895; 13:187; 14:236; 15:96; 16:18; 17:4; 18:17; 19:1; frees by size class: 10:73184; 11:1303; 12:451; 13:169; 14:221; 15:87; 16:10; 17:3; 18:17; 19:1; rfrees by size class: 10:19994; 11:313; 12:25; 13:28; 14:95; 15:16; 16:1; 18:5; Stats: malloc large: 136 small slow: 3178

It turned out that distribution become inconsistent in the current implementation if reprojection is used.

Created attachment 182915 [details] WIP

Currently distribution validity is not correct. This is because we introduced content-reprojection. When we modify the children of host or ShadowRoot, we have to invalidate distribution for some of elements having ElementShadow in ShadowDOM. Also, when we ensure distribution, we have to check parent TreeScope's distribution has been finished. If not, we have to do it.

Created attachment 182916 [details] Patch

Comment on attachment 182916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182916&action=review > Source/WebCore/ChangeLog:3 > + Distribution becomes inconsistent state with content/shadow reprojection Nit: Distribution state becomes inconsistent with … > Source/WebCore/ChangeLog:8 > + When distribution nested ShadowDOM is resolved before resolving ShadowDOM, distribution could become inconsistent state. It is not 100% clear what you mean here. Do you mean that nested Shadow DOM is done first, and because of this, distribution state can become inconsistent? Or do you mean that if nested Shadow DOM distribution happens to be done first, distribution state can become inconsistent? > Source/WebCore/ChangeLog:23 > + element of SR4 does not have InsertionPoint in direct children. This seems to confuse describing the current buggy implementation ("we’re invalidating SR1’s distribution only") with what should be happening ("we don’t invalidate SR4’s distribution because …"). That part about SR4 is not a critical point related to this bug, right? I think this comment could be friendlier if it explained why being a child of a host is special, ie because only the children of hosts are distributed. Also: What about fallback content being reprojected? Is there an edge case there? > Source/WebCore/dom/ElementShadow.cpp:125 > + ASSERT(youngestShadowRoot()); This assertion should go in ensureDistributionFromAncestor, not here. > LayoutTests/fast/dom/shadow/distribution-crash.html:11 > + testRunner.waitUntilDone(); Consider using the helpers in resources/js-test-pre.js etc. > LayoutTests/fast/dom/shadow/distribution-crash.html:18 > +shadowRoot2.innerHTML = '<details><shadow id="shadow">something 2</shadow></details>'; Is using details necessary? Because unless you realize that details uses Shadow DOM, the point of the test is a bit mysterious. > LayoutTests/fast/dom/shadow/distribution-crash.html:22 > + shadow.removeChild(shadow.firstChild); Maybe shadow.firstChild.remove() is more succinct? > LayoutTests/fast/dom/shadow/nested-reprojection-inconsistent.html:12 > +shadowRoot1.innerHTML = '<div id="div1"></div>'; You could simplify this by not using an ID and just retrieving it with shadowRoot1.firstChild. Similarly with other elements below. > LayoutTests/fast/dom/shadow/nested-reprojection-inconsistent.html:32 > +</script> successfullyParsed?

Comment on attachment 182916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182916&action=review >> Source/WebCore/ChangeLog:3 >> + Distribution becomes inconsistent state with content/shadow reprojection > > Nit: > > Distribution state becomes inconsistent with … Done >> Source/WebCore/ChangeLog:8 >> + When distribution nested ShadowDOM is resolved before resolving ShadowDOM, distribution could become inconsistent state. > > It is not 100% clear what you mean here. Do you mean that nested Shadow DOM is done first, and because of this, distribution state can become inconsistent? Or do you mean that if nested Shadow DOM distribution happens to be done first, distribution state can become inconsistent? The latter. Update the ChangeLog. >> Source/WebCore/ChangeLog:23 >> + element of SR4 does not have InsertionPoint in direct children. > > This seems to confuse describing the current buggy implementation ("we’re invalidating SR1’s distribution only") with what should be happening ("we don’t invalidate SR4’s distribution because …"). That part about SR4 is not a critical point related to this bug, right? > > I think this comment could be friendlier if it explained why being a child of a host is special, ie because only the children of hosts are distributed. > > Also: What about fallback content being reprojected? Is there an edge case there? > What about fallback content being reprojected? Is there an edge case there? When distributing a fallback element, we have to check grand parent node has ElementShadow. However, in reprojecting, basically we should be able to handle it as the normal distributed elements. >> Source/WebCore/dom/ElementShadow.cpp:125 >> + ASSERT(youngestShadowRoot()); > > This assertion should go in ensureDistributionFromAncestor, not here. I don't think this ASSERT is necessary if it's in ensureDistributionFromAncestor(). Removed. >> LayoutTests/fast/dom/shadow/distribution-crash.html:11 >> + testRunner.waitUntilDone(); > > Consider using the helpers in resources/js-test-pre.js etc. Done >> LayoutTests/fast/dom/shadow/distribution-crash.html:18 >> +shadowRoot2.innerHTML = '<details><shadow id="shadow">something 2</shadow></details>'; > > Is using details necessary? Because unless you realize that details uses Shadow DOM, the point of the test is a bit mysterious. It just came from the fuzzer. I've replaced it with complicated div. >> LayoutTests/fast/dom/shadow/distribution-crash.html:22 >> + shadow.removeChild(shadow.firstChild); > > Maybe shadow.firstChild.remove() is more succinct? OK. >> LayoutTests/fast/dom/shadow/nested-reprojection-inconsistent.html:12 >> +shadowRoot1.innerHTML = '<div id="div1"></div>'; > > You could simplify this by not using an ID and just retrieving it with shadowRoot1.firstChild. Similarly with other elements below. I think it is easy to understand if the variable name is corresponding to id, though... >> LayoutTests/fast/dom/shadow/nested-reprojection-inconsistent.html:32 >> +</script> > > successfullyParsed? finishJSTest() will automatically make successfullyParsed true if no error was found, I think.

Created attachment 183121 [details] Patch

Comment on attachment 183121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183121&action=review > Source/WebCore/html/shadow/ContentDistributor.cpp:349 > +void ContentDistributor::ensureDistributionFromAncestor(Node* source) It looks we can now simply call this ensureDistribution() since the "ensure" should be always drom the ancestor. > Source/WebCore/html/shadow/ContentDistributor.cpp:365 > + elementShadows[i - 1]->ensureDistribution(); It looks we no longer need ElementShadow::ensureDistribution(). No other place uses it. Is that right? > Source/WebCore/html/shadow/ContentDistributor.cpp:388 > + } Could you clarify or elaborate what is happening here? > Source/WebCore/html/shadow/ContentDistributor.cpp:390 > + } I guess this new code should be orthogonal to node's attached() state. > Source/WebCore/html/shadow/ContentDistributor.h:137 > + static void ensureDistributionFromAncestor(Node* source); Nit: could be container node. Or, we have an overload for shadow root. > Source/WebCore/html/shadow/ContentDistributor.h:149 > + bool distributionIsValid() const { return m_validity == Valid; } Nit: It's fine to just call this isValid(). This is "distributor".

Created attachment 183710 [details] Patch

Comment on attachment 183121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183121&action=review >> Source/WebCore/html/shadow/ContentDistributor.cpp:349 >> +void ContentDistributor::ensureDistributionFromAncestor(Node* source) > > It looks we can now simply call this ensureDistribution() since the "ensure" should be always drom the ancestor. Done. >> Source/WebCore/html/shadow/ContentDistributor.cpp:365 >> + elementShadows[i - 1]->ensureDistribution(); > > It looks we no longer need ElementShadow::ensureDistribution(). No other place uses it. Is that right? Right. Done. >> Source/WebCore/html/shadow/ContentDistributor.cpp:388 >> + } > > Could you clarify or elaborate what is happening here? yes. >> Source/WebCore/html/shadow/ContentDistributor.cpp:390 >> + } > > I guess this new code should be orthogonal to node's attached() state. Right. And I think this should be moved in invalidate(). not here. >> Source/WebCore/html/shadow/ContentDistributor.h:137 >> + static void ensureDistributionFromAncestor(Node* source); > > Nit: could be container node. Or, we have an overload for shadow root. Hmm.. I think we only need a method taking a ShadowRoot. >> Source/WebCore/html/shadow/ContentDistributor.h:149 >> + bool distributionIsValid() const { return m_validity == Valid; } > > Nit: It's fine to just call this isValid(). This is "distributor". Done

Comment on attachment 183710 [details] Patch Clearing flags on attachment: 183710 Committed r140299: <http://trac.webkit.org/changeset/140299>

All reviewed patches have been landed. Closing bug.