Bug 106634

Summary: Distribution state becomes inconsistent with content/shadow reprojection
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: ojan.autocc, webcomponents-bugzilla, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 97279    
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Patch none

Description Shinya Kawanaka 2013-01-10 23:39:02 PST
Repro:

<script>if (window.testRunner) testRunner.waitUntilDone(); var nodes = Array();
var shadows = [document.createElement('div').webkitCreateShadowRoot()];
var text = Array();
function boom() {
try { nodes[2].setAttribute('class', 'c2'); } catch(e) {}
try { nodes[3] = document.createElement('abbr'); } catch(e) {}
try { document.documentElement.appendChild(nodes[3]); } catch (e) {}
try { nodes[8] = document.createElement('th'); } catch(e) {}
try { shadows[3] = nodes[5].webkitCreateShadowRoot(); } catch (e) {}
try { shadows[4] = nodes[3].webkitCreateShadowRoot(); } catch (e) {}
try { nodes[19] = document.createElement('details'); } catch(e) {}
try { nodes[19].setAttribute('class', 'c12'); } catch(e) {}
try { shadows[7] = nodes[3].webkitCreateShadowRoot(); } catch (e) {}
try { shadows[19] = nodes[31].webkitCreateShadowRoot(); } catch (e) {}
try { nodes[98] = document.createElement('shadow'); } catch(e) {}
try { nodes[19].appendChild(nodes[98]); } catch(e) {}
try { text[32] = document.createTextNode('-979400190'); } catch(e) {}
try { nodes[98].appendChild(text[32]); } catch(e) {}
try { text[38] = document.createTextNode('1052734043'); } catch(e) {}
try { shadows[7].appendChild(nodes[19]); } catch(e) {}
try { shadows[4].appendChild(text[38]); } catch(e) {}
setTimeout('try { nodes[8].appendChild(text[32]); } catch(e) {}', 95);
setTimeout('clear();', 700);
}
window.onload = boom;
</script>
Comment 1 Shinya Kawanaka 2013-01-10 23:39:28 PST
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
Comment 2 Shinya Kawanaka 2013-01-14 23:43:30 PST
It turned out that distribution become inconsistent in the current implementation if reprojection is used.
Comment 3 Shinya Kawanaka 2013-01-15 20:44:47 PST
Created attachment 182915 [details]
WIP
Comment 4 Shinya Kawanaka 2013-01-15 20:48:27 PST
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.
Comment 5 Shinya Kawanaka 2013-01-15 21:19:30 PST
Created attachment 182916 [details]
Patch
Comment 6 Dominic Cooney 2013-01-16 07:48:24 PST
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 7 Shinya Kawanaka 2013-01-16 22:13:10 PST
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.
Comment 8 Shinya Kawanaka 2013-01-16 22:15:50 PST
Created attachment 183121 [details]
Patch
Comment 9 Hajime Morrita 2013-01-20 18:45:53 PST
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".
Comment 10 Shinya Kawanaka 2013-01-20 22:00:08 PST
Created attachment 183710 [details]
Patch
Comment 11 Shinya Kawanaka 2013-01-20 22:00:48 PST
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 12 WebKit Review Bot 2013-01-20 23:01:04 PST
Comment on attachment 183710 [details]
Patch

Clearing flags on attachment: 183710

Committed r140299: <http://trac.webkit.org/changeset/140299>
Comment 13 WebKit Review Bot 2013-01-20 23:01:09 PST
All reviewed patches have been landed.  Closing bug.