Bug 72440

Tree reforming pushing down this to 59802

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

I will work on a repro.

Created attachment 118291 [details] Repro layout test

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

(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.

Created attachment 120166 [details] Patch

(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.

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.

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

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?

(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.

Created attachment 128949 [details] Patch

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

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?

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().

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?

(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.

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?

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