Removing <ul>, <li> inside shadow DOM triggers assertion in updateListMarkerNumbers

[Dominic Cooney]

ASSERTION FAILED: listNode && listNode->renderer()
/Volumes/HD2/chromium/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderListItem.cpp(431) : void WebCore::RenderListItem::updateListMarkerNumbers()

[Hajime Morrita]

Tree reforming pushing down this to 59802

[Hajime Morrita]

Hi Dominic,
Could you provide any repro for this?
I'm trying to reproduce this, but I have had none.

Thanks in advance,

[Dominic Cooney]

I will work on a repro.

[Dominic Cooney]

Created attachment 118291 [details]
Repro layout test

[Shinya Kawanaka]

I was investigating this.
UL seems destroyed twice...?

[Shinya Kawanaka]

(In reply to comment #5)
> I was investigating this.
> UL seems destroyed twice...?

RenderObjectChildList::destroyLeftoverChildren() destroys shadow objects, and it removes a renderer. The renderer of UL is deleted here, then LI is being deleted after that.
I'm not sure setting renderer 0 in the function is necessary.

[Shinya Kawanaka]

Created attachment 120166 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #7)
> Created an attachment (id=120166) [details]
> Patch

I'm not sure this is the right way, but this patch fixes the issue.

[Ryosuke Niwa]

Comment on attachment 120166 [details]
Patch

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

> Source/WebCore/rendering/RenderObjectChildList.cpp:62
> -            if (firstChild()->node())
> -                firstChild()->node()->setRenderer(0);
> +            Node* node = firstChild()->node();
>              firstChild()->destroy();
> +            if (node)
> +                node->setRenderer(0);

This doesn't look like a right fix.

[Ryosuke Niwa]

This is renderer related so should belong "Layout and Rendering".

[Dimitri Glazkov (Google)]

Comment on attachment 120166 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This patch postpones removing renderers just after children are destroyed.

Can you explain a bit more as to why this is the right solution? Why does this behave differently in a shadow DOM subtree?

[Shinya Kawanaka]

(In reply to comment #11)
> (From update of attachment 120166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120166&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This patch postpones removing renderers just after children are destroyed.
> 
> Can you explain a bit more as to why this is the right solution? Why does this behave differently in a shadow DOM subtree?

Actually that code was written when I was a (very) newbie of rendering tree...
Let me consider more right solution (or confirm this is the right solution) later.

[Hajime Morrita]

Created attachment 128949 [details]
Patch

[Hajime Morrita]

Dimitri, could you take a look?
It looks making code consistent just fixes the problem.

[Ryosuke Niwa]

Comment on attachment 128949 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:971
> +        for (Node* child = firstChild(); child; child = child->nextSibling()) {
> +            if (child->attached())
> +                child->detach();
> +        }

Can we call ContainerNode::detach() here instead?

[Shinya Kawanaka]

Comment on attachment 128949 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:971
>> +        }
> 
> Can we call ContainerNode::detach() here instead?

I don't think so, because ContainerNode::detach() will call Node::detach().
Having ContainerNode::detachChildren() might be useful, because we have a lot of similar code.

> Source/WebCore/dom/Element.cpp:973
> +        shadowRootList()->detach();

Now that shadowRootList() has been renamed to shadowTree().

[Ryosuke Niwa]

Comment on attachment 128949 [details]
Patch

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

Okay, I think I understand what's happening on my second look.

>>> Source/WebCore/dom/Element.cpp:971
>>> +        }
>> 
>> Can we call ContainerNode::detach() here instead?
> 
> I don't think so, because ContainerNode::detach() will call Node::detach().
> Having ContainerNode::detachChildren() might be useful, because we have a lot of similar code.

Oh I see what you're saying. Yeah, adding ContainerNode::detachChildren seems like a valuable refactoring of its own. I wonder it should be inlined or not.

> LayoutTests/fast/dom/shadow/shadow-ul-li.html:8
> +        return;  // needs ShadowRoot

Can we output some message about needing shadowRoot? Presumably, we should be able to modify test to be able to run without DRT once shadow DOM API becomes available to DOM?

[Hajime Morrita]

(In reply to comment #17)
> > LayoutTests/fast/dom/shadow/shadow-ul-li.html:8
> > +        return;  // needs ShadowRoot
> 
> Can we output some message about needing shadowRoot? Presumably, we should be able to modify test to be able to run without DRT once shadow DOM API becomes available to DOM?
Oops. This has been there when it was an attached repro. I'll remove it and add Skipped lines.

(In reply to comment #16)
> (From update of attachment 128949 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128949&action=review
> > Source/WebCore/dom/Element.cpp:973
> > +        shadowRootList()->detach();
> 
> Now that shadowRootList() has been renamed to shadowTree().
God catch! I'll followup shortly.

[Julien Chaffraix]

It looks like the change was landed in http://trac.webkit.org/changeset/108980. Because the ChangeLog contained the wrong bug number, the final patch ended up in bug 79630. Morrita, could you update the ChangeLog bug URL and close this bug if I haven't missed anything?

[Hajime Morrita]

Good catch! Fixed http://trac.webkit.org/changeset/109037.
Thank you for pointing it out.