Bug 80020 – [Crash] Adding <content> into a ShadowRoot causes crash.

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

[Shinya Kawanaka]

Repro:

<div>DIV1</div><div id='div2'>DIV2</div><div>DIV3</div>
<script>
var div = document.getElementById('div2');
var root = new WebKitShadowRoot(div);
root.innerHTML = "<content></content>";
</script>

[Shinya Kawanaka]

Created attachment 130055 [details]
Patch

[Shinya Kawanaka]

Created attachment 130085 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 130085 [details]
Patch

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

Introducing two different paths for attaching seems like a bad idea. Can you help me understand why you need this?

> Source/WebCore/ChangeLog:8
> +        When layouting a shadow tree, we're currently assuming that the whole tree is re-created.

layouting -> laying out.

> Source/WebCore/ChangeLog:9
> +        However, when appending a child, subtree attaching occurs.

And why is that bad?

> Source/WebCore/dom/ShadowTree.cpp:41
>  ShadowTree::ShadowTree()

Also: why is this called a Tree? It's really a ShadowTreeStack, isn't it?

[Hajime Morrita]

(In reply to comment #3)
> 
> Also: why is this called a Tree? It's really a ShadowTreeStack, isn't it?
This is going to encapsulate whole Shadow subtree concept. "stack" is only part of that.
May be we should split stack (or list) to a separate object. I feel it a bit premature though.

[Hajime Morrita]

> > Also: why is this called a Tree? It's really a ShadowTreeStack, isn't it?
> This is going to encapsulate whole Shadow subtree concept. "stack" is only part of that.
> May be we should split stack (or list) to a separate object. I feel it a bit premature though.
My comment above wasn't clear enough.
Point here is that the ShadowTree class is no longer simple list, or stack.
It's now encapsulating whole shadow dom lifecycle.
That's why it's it7s renamed from ShadowRootList to ShadowTree.

One suggested name was ShadowForest.
Then I objected introducing another out-of-standard noun here.
I hope we have good name for that.
Maybe ElementShadow is sufficient.

[Hajime Morrita]

By the way, it looks attach and layout is mixed here. Which are you talking about?

[Shinya Kawanaka]

> > Source/WebCore/ChangeLog:9
> > +        However, when appending a child, subtree attaching occurs.
> 
> And why is that bad?

It's because recalculating distribution is performed in attaching. If only we attach only subtree, we cannot calculate distribution correctly.

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> > > Also: why is this called a Tree? It's really a ShadowTreeStack, isn't it?
> > This is going to encapsulate whole Shadow subtree concept. "stack" is only part of that.
> > May be we should split stack (or list) to a separate object. I feel it a bit premature though.
> My comment above wasn't clear enough.
> Point here is that the ShadowTree class is no longer simple list, or stack.
> It's now encapsulating whole shadow dom lifecycle.
> That's why it's it7s renamed from ShadowRootList to ShadowTree.
> 
> One suggested name was ShadowForest.
> Then I objected introducing another out-of-standard noun here.
> I hope we have good name for that.
> Maybe ElementShadow is sufficient.

Hmm... I had a mental stumble trying to understand the purpose of this class. I am sure I won't be the only one. I understand now what you mean. This is a Tree of Trees, and a dynamically maintained structure, according to the spec. It's the ReifiedTree, right?

I wonder if, from the software engineering perspective, we would be better off by making the data-holding structures remain only Stacks and Subtrees. Then, a reified tree is just an iterator that encapsulates the logic in which this tree is perceived when rendering. Does this make sense?

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (In reply to comment #5)
> > > > Also: why is this called a Tree? It's really a ShadowTreeStack, isn't it?
> > > This is going to encapsulate whole Shadow subtree concept. "stack" is only part of that.
> > > May be we should split stack (or list) to a separate object. I feel it a bit premature though.
> > My comment above wasn't clear enough.
> > Point here is that the ShadowTree class is no longer simple list, or stack.
> > It's now encapsulating whole shadow dom lifecycle.
> > That's why it's it7s renamed from ShadowRootList to ShadowTree.
> > 
> > One suggested name was ShadowForest.
> > Then I objected introducing another out-of-standard noun here.
> > I hope we have good name for that.
> > Maybe ElementShadow is sufficient.
> 
> Hmm... I had a mental stumble trying to understand the purpose of this class. I am sure I won't be the only one. I understand now what you mean. This is a Tree of Trees, and a dynamically maintained structure, according to the spec. It's the ReifiedTree, right?
> 
> I wonder if, from the software engineering perspective, we would be better off by making the data-holding structures remain only Stacks and Subtrees. Then, a reified tree is just an iterator that encapsulates the logic in which this tree is perceived when rendering. Does this make sense?

Hayato-san is implementing this iterator in bug 79197, it seems.

[Hajime Morrita]

(In reply to comment #8)

> 
> Hmm... I had a mental stumble trying to understand the purpose of this class. I am sure I won't be the only one. I understand now what you mean. This is a Tree of Trees, and a dynamically maintained structure, according to the spec. It's the ReifiedTree, right?
> 

I'm not sure. The list of trees isn't reified without the traverser. 
I can think it as a reified view of the whole shadow stuff.

> I wonder if, from the software engineering perspective, we would be better off by making the data-holding structures remain only Stacks and Subtrees. Then, a reified tree is just an iterator that encapsulates the logic in which this tree is perceived when rendering. Does this make sense?

In the engineering perspective, I agree that.
Especially because we have no such thing like ShadowTree in the standard.

On the other hand, this assumes that we have some consistent way to traverse
the whole shadow trees, which we don't have it yet in reality.

Without such abstractions, the tree traversal code around Node, Element, ContainerNode, etc.
will be really ugly and make folks who don't care shadow DOM uncomfortable.
So I thought that isolating such shadow-aware traversals into ShadowTree was a reasonable idea.

ReifiedTreeTraversal might help in certain level, but not all,
because many existing code relies recursion. which the traverser doesn't care about.
Also there are traversals in non-reified manner. 

Also, we maintain a data for node distribution which is owned by ShadowTree. 
We need to find a place for it.

Honestly I don't have good big picture of whole multiple shadow tree stuff yet.
Hopefully shinyak@ an hayato@ show their opinion here.

[Shinya Kawanaka]

I agree that ideally we should not have ShadowTree structure.
However, as morrita says, we have to have this kind of abstraction now.

Currently we perform node distribution in attach(). However, attach() is called not only constructing whole shadow tree but also constructing subtree (e.g. appendChild(), which causes this bug).

It would be good we can check shadow tree construction is necessary or not in appendChild(), however I'm hesitating to add such check from the view point of performance. I'm thinking we should be able to know context of attach() (e.g. the element is in shadow, we need recalculate shadow tree, etc.), but it needs a lot of refactoring.

I want to remove ShadowTree finally, however I don't think the day comes near future.

[Shinya Kawanaka]

Created attachment 130530 [details]
Patch

[Hayato Ito]

I agree that a class of ShadowTree has a lot of responsibilities, such as:

- Responsibility for maintaining a stack of ShadowRoots.
- Responsibility for attach()/detach() for all nodes in Shadow DOM subtrees.
- Responsibility for managing the result of node distribution algorithm, which ReifiedTreeTraversal uses.

It's not easy to separate these concerns so far since these are tightly related. It should be, but it requires some non-trivial refactoring. I guess ShadowTree is like a 'Greatest common divisor' name.
If we can separate these concerns nicely, we don't have difficulties to have a intuitive name.

(In reply to comment #11)
> I agree that ideally we should not have ShadowTree structure.
> However, as morrita says, we have to have this kind of abstraction now.
> 
> Currently we perform node distribution in attach(). However, attach() is called not only constructing whole shadow tree but also constructing subtree (e.g. appendChild(), which causes this bug).
> 
> It would be good we can check shadow tree construction is necessary or not in appendChild(), however I'm hesitating to add such check from the view point of performance. I'm thinking we should be able to know context of attach() (e.g. the element is in shadow, we need recalculate shadow tree, etc.), but it needs a lot of refactoring.
> 
> I want to remove ShadowTree finally, however I don't think the day comes near future.

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> 
> I want to remove ShadowTree finally, however I don't think the day comes near future.

Ok. Please file a bug on this.

[Dimitri Glazkov (Google)]

Comment on attachment 130530 [details]
Patch

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

I am not sure what this patch does. I am sorry. I tried to undestand, but I couldn't figure it out.

It seems to me that as soon as there's a <content> element inserted into document, we should immediately look at its treescope, reset distribution, and mark to reattach the shadow dom tree. Right?

> Source/WebCore/ChangeLog:8
> +        The problem is <content> try to select host children though it is not prepared.

What does "prepared" mean in this context? Also, the sentence is grammatically incorrect.

> Source/WebCore/ChangeLog:9
> +        Since populating host children for insertion points is performed just before

Perhaps you meant "distributing host children into insertion points"?

> Source/WebCore/ChangeLog:10
> +        attaching a shadow tree, we should recalculate whole shadow tree if <content> is

We should re-run distribution algorithm?

> Source/WebCore/ChangeLog:13
> +        However, element->appendChild() does not know the element is in a shadow tree or not.

"does not know the element" -> "is not aware whether the element"

> Source/WebCore/ChangeLog:14
> +        We have to ensure reattaching whole shadow tree here.

ensure reattaching -> ensure that we reattach

> Source/WebCore/ChangeLog:16
> +        So this patch ensures HTMLContentSelector exists only if it is prepared. If not,

Again, the word "prepared" is used here. What does it mean?

> Source/WebCore/ChangeLog:17
> +        we cannot select anything, but we have to enable a flag to reattach whole shadow tree.

we add a flag?

> Source/WebCore/ChangeLog:48
> +        When layouting a shadow tree, we're currently assuming that the whole tree is re-created.

layouting --> laying out

> Source/WebCore/ChangeLog:52
> +        do layout only when the flag is enabled.

I think this log entry is old :)

> Source/WebCore/dom/ShadowTree.h:87
> +    void populate();
> +    bool isPopulated();

are these used anywhere?

> Source/WebCore/html/shadow/HTMLContentSelector.h:138
> +    bool isSelecting() const { return m_isSelecting; }

Why do we need this?

> Source/WebCore/html/shadow/HTMLContentSelector.h:142
> +    void populateIfNesessary(Element* shadowHost);
> +    bool isPopulated() const { return m_isPopulated; }

let's keep the language closely aligned with selection: selectIfNecessary and hasSelected. It appears that hasSelected is closely related to hasCandidates: the former is always true when the latter is true.

[Shinya Kawanaka]

Sorry for late response..

Maybe I mistook updating ChangeLog in the previous patch.

I believe we all agree that the current implementation of shadow trees is far from ideal one. We have to make code clear. BTW, we have to fix the bug though.

After fixing this, let's do a lot of refactoring to make code sane.

[Shinya Kawanaka]

The name selectIfNecessary seems very confusing, since we have 'select' method already and the roles of 'select' and 'selectIfNecessary' are completely different.

I think populateIfNecessary() is better. It is taken from the spec.

BTW I agree that the current code is too messy to understand.

[Dimitri Glazkov (Google)]

(In reply to comment #17)
> The name selectIfNecessary seems very confusing, since we have 'select' method already and the roles of 'select' and 'selectIfNecessary' are completely different.
> 
> I think populateIfNecessary() is better. It is taken from the spec.
> 
> BTW I agree that the current code is too messy to understand.

Ok. There are still parts of the patch that I don't understand. Maybe Morrita-san could give it a look.

[Dimitri Glazkov (Google)]

(In reply to comment #16)
> Sorry for late response..
> 
> Maybe I mistook updating ChangeLog in the previous patch.
> 
> I believe we all agree that the current implementation of shadow trees is far from ideal one. We have to make code clear. BTW, we have to fix the bug though.
> 
> After fixing this, let's do a lot of refactoring to make code sane.

I am fine with fixing the bug, but you have a few things that are just sloppy code, like methods that aren't called anywhere.

[Shinya Kawanaka]

Created attachment 131271 [details]
Patch

[Shinya Kawanaka]

> I am fine with fixing the bug, but you have a few things that are just sloppy code, like methods that aren't called anywhere.

Ya, sorry, I didn't notice that there was sloppy code.

[Shinya Kawanaka]

Created attachment 131992 [details]
Patch

[Hajime Morrita]

Comment on attachment 131992 [details]
Patch

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

What makes this patch cryptic is that this changes both lifetime of the HTMLContentSelector and adding a re-entrant guard in distribution.
Ideally these could be done in separate patch. But I think this is OK for now to fix crash asap.

> Source/WebCore/ChangeLog:8
> +        The problem is <content> try to select host children though it is not prepared.

s/try/tries

> Source/WebCore/dom/ShadowTree.cpp:183
> +    ensureSelector()->willSelect();

It looks we no longer need ensurSelector(). that means we no longer need to allocate it in the heap.

> Source/WebCore/html/shadow/HTMLContentSelector.cpp:167
> +    ASSERT(m_phase == NotPopulatedHostChildren);

This looks too obvious.

> Source/WebCore/html/shadow/HTMLContentSelector.h:147
> +        NotPopulatedHostChildren,

Please consider to avoid negation.

> Source/WebCore/html/shadow/InsertionPoint.cpp:115
> +        selector->select(this, &m_selections);

Please consider early return. I think we can just return unless the selector is available.

[Shinya Kawanaka]

(In reply to comment #23)
> (From update of attachment 131992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131992&action=review
> 
> What makes this patch cryptic is that this changes both lifetime of the HTMLContentSelector and adding a re-entrant guard in distribution.
> Ideally these could be done in separate patch. But I think this is OK for now to fix crash asap.
> 
> > Source/WebCore/ChangeLog:8
> > +        The problem is <content> try to select host children though it is not prepared.
> 
> s/try/tries

Done.

> 
> > Source/WebCore/dom/ShadowTree.cpp:183
> > +    ensureSelector()->willSelect();
> 
> It looks we no longer need ensurSelector(). that means we no longer need to allocate it in the heap.

Done.

> 
> > Source/WebCore/html/shadow/HTMLContentSelector.cpp:167
> > +    ASSERT(m_phase == NotPopulatedHostChildren);
> 
> This looks too obvious.

Done.

> 
> > Source/WebCore/html/shadow/HTMLContentSelector.h:147
> > +        NotPopulatedHostChildren,
> 
> Please consider to avoid negation.

Done.

> 
> > Source/WebCore/html/shadow/InsertionPoint.cpp:115
> > +        selector->select(this, &m_selections);
> 
> Please consider early return. I think we can just return unless the selector is available.

Done.

[Shinya Kawanaka]

Created attachment 131997 [details]
Patch

[Hajime Morrita]

Comment on attachment 131997 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        So this patch ensures HTMLContentSelector exists only if it is prepared. If not,

It looks this is no longer true.

[Shinya Kawanaka]

(In reply to comment #26)
> (From update of attachment 131997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131997&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        So this patch ensures HTMLContentSelector exists only if it is prepared. If not,
> 
> It looks this is no longer true.

Done.

[Shinya Kawanaka]

Created attachment 132000 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 132000 [details]
Patch for landing

Clearing flags on attachment: 132000

Committed r110935: <http://trac.webkit.org/changeset/110935>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hajime Morrita]

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