Use DOM ordering for list counts

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Bucur]

The counters implementation is currently based on the render tree order. This generates problems in the case of regions and shadow DOM because the list items are not always rendered as children of their DOM parents.

[Andrei Bucur]

Created attachment 190269 [details]
Patch

[Antti Koivisto]

Comment on attachment 190269 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:3
> +        Move the list counting logic from the render tree to the DOM tree

The name is bit vague. Move how? "Use DOM ordering for list counts" or something?

> Source/WebCore/dom/NodeTraversal.h:60
> +// Pre-order traversal including the pseudo-elements.
> +Element* previousWithPseudo(const Element*, const Element* = 0);
> +Element* nextWithPseudo(const Element*, const Element* = 0);
> +Element* previousWithPseudoSkippingChildren(const Element*, const Element* = 0);
> +Element* nextWithPseudoSkippingChildren(const Element*, const Element* = 0);
> +
> +// Traversal helpers aware of pseudo-elements.
> +Element* nextElementSiblingWithPseudo(const Element*);
> +Element* previousElementSiblingWithPseudo(const Element*);
> +Element* firstElementChildWithPseudo(const Element*);
> +Element* lastElementChildWithPseudo(const Element*);

You can drop word "Element", it is already in the namespace name. Also WithPseudo sounds like you are excluding everything that is not pseudo. IncludingPseudo perhaps?

It would be nice to not pile everything to NodeTraversal.cpp/.h. However maybe that can be figure out separately.

[Elliott Sprehn]

Comment on attachment 190269 [details]
Patch

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

r-

This patch is not correct since it breaks Shadow DOM since it doesn't use NodeRenderingTraversal. You shouldn't need these tons of traversal methods added to ElementTraversal either. NodeRenderingTraversal already contains logic to handle pseudo elements, you want to add NodeRenderingTraversal::next and NodeRenderingTraversal::previous which use the existing nextSibling() and previousSibling() which are already pseudo element aware.

> Source/WebCore/dom/NodeTraversal.cpp:35
> +Element* nextElementSiblingWithPseudo(const Element* current)

These methods are not needed, we already have NodeRenderingTraversal::nextSibling and previousSibling which do the right thing.

> Source/WebCore/html/HTMLLIElement.cpp:101
> +            current = current->parentElement();

This doesn't seem right at all. You break Shadow DOM here, and EventPathWalker should have worked just fine (since it does this already).

> Source/WebCore/html/HTMLOListElement.cpp:109
> +    // This will iterate through the list items in DOM order.

These comments don't seem needed. If you want to explain the nextListItem method though, put this comment where that method is declared.

> Source/WebCore/rendering/RenderCounter.cpp:64
> +        previous = ElementTraversal::previousWithPseudo(previous);

Can we make the changes to RenderCounter in a separate patch?

> Source/WebCore/rendering/RenderCounter.cpp:88
> +    return self->parentElement();

Just remove the parentElement method entirely. As you noticed it's not needed anymore.

> Source/WebCore/rendering/RenderListItem.cpp:105
> +    for (Element* parent = listItemNode->parentElement(); parent; parent = parent->parentElement()) {

Again this needs to be NodeRenderingTraversal, not parentElement(), to be aware of what's going on inside Shadow DOM (which the render tree is implicitly aware of since it's composed already).

[WebKit Review Bot]

Comment on attachment 190269 [details]
Patch

Attachment 190269 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16672314

New failing tests:
editing/shadow/delete-list-in-shadow.html
fast/dom/shadow/shadow-and-list-elements.html
editing/shadow/pressing-enter-on-list.html

[Andrei Bucur]

(In reply to comment #3)
> (From update of attachment 190269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190269&action=review
> 
> r-
> 
> This patch is not correct since it breaks Shadow DOM since it doesn't use NodeRenderingTraversal. You shouldn't need these tons of traversal methods added to ElementTraversal either. NodeRenderingTraversal already contains logic to handle pseudo elements, you want to add NodeRenderingTraversal::next and NodeRenderingTraversal::previous which use the existing nextSibling() and previousSibling() which are already pseudo element aware.
> 
> > Source/WebCore/dom/NodeTraversal.cpp:35
> > +Element* nextElementSiblingWithPseudo(const Element* current)
> 
> These methods are not needed, we already have NodeRenderingTraversal::nextSibling and previousSibling which do the right thing.
> 
> > Source/WebCore/html/HTMLLIElement.cpp:101
> > +            current = current->parentElement();
> 
> This doesn't seem right at all. You break Shadow DOM here, and EventPathWalker should have worked just fine (since it does this already).
> 
> > Source/WebCore/html/HTMLOListElement.cpp:109
> > +    // This will iterate through the list items in DOM order.
> 
> These comments don't seem needed. If you want to explain the nextListItem method though, put this comment where that method is declared.
> 
> > Source/WebCore/rendering/RenderCounter.cpp:64
> > +        previous = ElementTraversal::previousWithPseudo(previous);
> 
> Can we make the changes to RenderCounter in a separate patch?
> 
> > Source/WebCore/rendering/RenderCounter.cpp:88
> > +    return self->parentElement();
> 
> Just remove the parentElement method entirely. As you noticed it's not needed anymore.
> 
> > Source/WebCore/rendering/RenderListItem.cpp:105
> > +    for (Element* parent = listItemNode->parentElement(); parent; parent = parent->parentElement()) {
> 
> Again this needs to be NodeRenderingTraversal, not parentElement(), to be aware of what's going on inside Shadow DOM (which the render tree is implicitly aware of since it's composed already).

I'm sorry in advance if I talk rubbish, I'm not familiar with shadow DOM code or the terminology from the spec.

NodeRenderingTraversal doesn't seem to provide a way to NOT cross both lower-boundary and upper-boundary encapsulation into the shadow tree. This is what I understand it's the expected behavior after reading this thread:
http://lists.w3.org/Archives/Public/public-webapps/2013JanMar/0484.html

For document nodes, we want to do the numbering based on the non-shadow tree ancestor chain if I understand correctly. EventPathWalker seems to do node distribution and visiting inside the reprojection. To me this doesn't sound in agreement with the mailing list thread.

As an example:
If we have a projected ancestor chain like this:
ol1->ShadowRoot->ol2->InesrtionPoint->li
We want the li element to be numbered by ol1, not ol2. If We use EventPathWalker, we take into account ol2 as well, right?

[Elliott Sprehn]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 190269 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190269&action=review
> > 
> > r-
> > 
> ...
> NodeRenderingTraversal doesn't seem to provide a way to NOT cross both lower-boundary and upper-boundary encapsulation into the shadow tree. This is what I understand it's the expected behavior after reading this thread:
> http://lists.w3.org/Archives/Public/public-webapps/2013JanMar/0484.html
> 
> For document nodes, we want to do the numbering based on the non-shadow tree ancestor chain if I understand correctly. EventPathWalker seems to do node distribution and visiting inside the reprojection. To me this doesn't sound in agreement with the mailing list thread.
> 
> As an example:
> If we have a projected ancestor chain like this:
> ol1->ShadowRoot->ol2->InesrtionPoint->li
> We want the li element to be numbered by ol1, not ol2. If We use EventPathWalker, we take into account ol2 as well, right?

Ah good point! Seems I forgot about that discussion to use the original DOM; let me review this again today. :)

[Andrei Bucur]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 190269 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=190269&action=review
> > > 
> > > r-
> > > 
> > ...
> > NodeRenderingTraversal doesn't seem to provide a way to NOT cross both lower-boundary and upper-boundary encapsulation into the shadow tree. This is what I understand it's the expected behavior after reading this thread:
> > http://lists.w3.org/Archives/Public/public-webapps/2013JanMar/0484.html
> > 
> > For document nodes, we want to do the numbering based on the non-shadow tree ancestor chain if I understand correctly. EventPathWalker seems to do node distribution and visiting inside the reprojection. To me this doesn't sound in agreement with the mailing list thread.
> > 
> > As an example:
> > If we have a projected ancestor chain like this:
> > ol1->ShadowRoot->ol2->InesrtionPoint->li
> > We want the li element to be numbered by ol1, not ol2. If We use EventPathWalker, we take into account ol2 as well, right?
> 
> Ah good point! Seems I forgot about that discussion to use the original DOM; let me review this again today. :)

The crashers happen because the ShadowRoot nodes are not Elements. If the first child of a ShadowRoot is an <li> it will try to select it as a counting root. Unfortunately the patch assumes it's possible to walk only on Elements.
I need to rewrite the crawling logic to be Node based.

[Andrei Bucur]

Created attachment 191972 [details]
Patch

[Elliott Sprehn]

Comment on attachment 191972 [details]
Patch

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

This should use Element* pretty much everywhere. There's no reason to be skipping the Text nodes manually everywhere. :)

> Source/WebCore/dom/NodeTraversal.cpp:35
> +Node* nextSiblingIncludingPseudo(const Node* current)

This is more complicated than it needs to be. Just add Node::pseudoAwareNextSibling() and Node::pseudoAwarePreviousSibling(). In fact these methods used to exist and then I removed them when I didnt' need them. Just add them back. https://trac.webkit.org/changeset/136744/trunk/Source/WebCore/dom/Node.cpp

> Source/WebCore/dom/NodeTraversal.cpp:52
> +Node* previousSiblingIncludingPseudo(const Node* current)

All these methods should be in ElementTraversal and should return Element*.

> Source/WebCore/dom/NodeTraversal.cpp:69
> +Node* firstChildIncludingPseudo(const Node* current)

These methods would be much clearer if they were split by type.

if (current->isElement()) { ... return ...; } ... return;

> Source/WebCore/dom/NodeTraversal.h:82
> +// Pre-order traversal including the pseudo-elements.

ElementTraversal and return Element*. I don't know why you need to see Text nodes in any of these traversals.

> Source/WebCore/rendering/RenderCounter.cpp:75
> +        previous = NodeTraversal::previousSiblingIncludingPseudo(previous);

ElementTraversal removes the need for all this.

> Source/WebCore/rendering/RenderListItem.cpp:105
> +    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {

parentElement(), this should all be element related.

[Andrei Bucur]

Comment on attachment 191972 [details]
Patch

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

>> Source/WebCore/dom/NodeTraversal.cpp:35
>> +Node* nextSiblingIncludingPseudo(const Node* current)
> 
> This is more complicated than it needs to be. Just add Node::pseudoAwareNextSibling() and Node::pseudoAwarePreviousSibling(). In fact these methods used to exist and then I removed them when I didnt' need them. Just add them back. https://trac.webkit.org/changeset/136744/trunk/Source/WebCore/dom/Node.cpp

Oh, nice! I didn't want to touch Node, but if they were there before... :)

>> Source/WebCore/dom/NodeTraversal.cpp:52
>> +Node* previousSiblingIncludingPseudo(const Node* current)
> 
> All these methods should be in ElementTraversal and should return Element*.

Yea, this was the case of my initial patch but it was crashing because the ShadowRoot is not an element. I'll go back to Element and see what I can do about the ShadowRoot.

>> Source/WebCore/dom/NodeTraversal.cpp:69
>> +Node* firstChildIncludingPseudo(const Node* current)
> 
> These methods would be much clearer if they were split by type.
> 
> if (current->isElement()) { ... return ...; } ... return;

Sounds good.

>> Source/WebCore/dom/NodeTraversal.h:82
>> +// Pre-order traversal including the pseudo-elements.
> 
> ElementTraversal and return Element*. I don't know why you need to see Text nodes in any of these traversals.

Yea, I didn't like this either but it was the easiest way to not monkey patch the code for ShadowRoot.

>> Source/WebCore/rendering/RenderCounter.cpp:75
>> +        previous = NodeTraversal::previousSiblingIncludingPseudo(previous);
> 
> ElementTraversal removes the need for all this.

Agree.

>> Source/WebCore/rendering/RenderListItem.cpp:105
>> +    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
> 
> parentElement(), this should all be element related.

Unfortunately the shadow root is not an element. I need to monkey patch the case if I use parentElement().

[Elliott Sprehn]

(In reply to comment #10)
> (From update of attachment 191972 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191972&action=review
> 
>...
> 
> Yea, this was the case of my initial patch but it was crashing because the ShadowRoot is not an element. I'll go back to Element and see what I can do about the ShadowRoot.

parentElement should return 0 if you would have crossed through a ShadowRoot which is what you want if we're saying counters are encapsulated inside a tree scope.

[Andrei Bucur]

Created attachment 194600 [details]
Patch

[Elliott Sprehn]

Comment on attachment 194600 [details]
Patch

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

You don't actually handle ShadowRoot hosts properly, and ShadowRoot will never pass the isList test. Please fix that comment and file a follow up bug and mention it in the ChangeLog before landing.

> Source/WebCore/ChangeLog:25
> +        In later patches I'll add tests that should cover the regions and shadow DOM use cases.

File a bug about needing these tests and add a link here.

> Source/WebCore/rendering/RenderCounter.cpp:83
> +    Element* self = toElement(object->node());

nit: Don't need the extra local.

> Source/WebCore/rendering/RenderListItem.cpp:106
> +    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {

This loop doesn't do what you think it does for ShadowRoot. ShadowRoot is a Node but it's parentNode() is also always null and it'll never match isList since that's about tags. You'd need to check if parent->isShadowRoot() and isList(toShadowRoot(parent)->host()). I'd add a FIXME here. The comment makes it sound like this handles ShadowRoot when it doesn't.

[Andrei Bucur]

(In reply to comment #13)
> (From update of attachment 194600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194600&action=review
> 
> You don't actually handle ShadowRoot hosts properly, and ShadowRoot will never pass the isList test. Please fix that comment and file a follow up bug and mention it in the ChangeLog before landing.
> 
> > Source/WebCore/ChangeLog:25
> > +        In later patches I'll add tests that should cover the regions and shadow DOM use cases.
> 
> File a bug about needing these tests and add a link here.
> 
> > Source/WebCore/rendering/RenderCounter.cpp:83
> > +    Element* self = toElement(object->node());
> 
> nit: Don't need the extra local.
> 
> > Source/WebCore/rendering/RenderListItem.cpp:106
> > +    for (Node* parent = listItemNode->parentNode(); parent; parent = parent->parentNode()) {
> 
> This loop doesn't do what you think it does for ShadowRoot. ShadowRoot is a Node but it's parentNode() is also always null and it'll never match isList since that's about tags. You'd need to check if parent->isShadowRoot() and isList(toShadowRoot(parent)->host()). I'd add a FIXME here. The comment makes it sound like this handles ShadowRoot when it doesn't.

I'm a bit confused. Didn't we decide list counting doesn't cross shadow root boundaries? Why do I need to inspect the host element? The case I'm talking about in the comment is this one:
<HostElement>
<ShadowRoot>
<li/>
</ShadowRoot>
</HostElement>

The <li> element will use the ShadowRoot as a counter scope (the <li> is NOT projected, it's just a child of the shadow root). It will not go up to the HostElement. The comment tries to explain why I can't use parentElement instead of parentNode. parentElement will return 0 for the <li>. parentNode will return the shadow root. isList will indeed return false all the time for the shadow root but it may very well be the first ancestor node (as in the example above) of a <li> so it will become the counter scope for it.

[Andrei Bucur]

Created attachment 195064 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 195064 [details]
Patch for landing

Clearing flags on attachment: 195064

Committed r146879: <http://trac.webkit.org/changeset/146879>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 113312

[Ryosuke Niwa]

This patch caused multiple crashes on Mac as well:
e.g. http://build.webkit.org/results/Apple%20Lion%20Debug%20WK2%20(Tests)/r146879%20(8380)/results.html

CRASHING TEST: fast/css/pseudo-required-optional-006.html

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010f17f96e WebCore::RenderListItem::updateListMarkerNumbers() + 94 (RenderListItem.cpp:474)
1   com.apple.WebCore             	0x000000010f17faa8 WebCore::RenderListItem::willBeRemovedFromTree() + 40 (RenderListItem.cpp:93)
2   com.apple.WebCore             	0x000000010f1bc4f0 WebCore::RenderObjectChildList::removeChildNode(WebCore::RenderObject*, WebCore::RenderObject*, bool) + 416 (RenderObjectChildList.cpp:93)
3   com.apple.WebCore             	0x000000010f1a9394 WebCore::RenderObject::removeChild(WebCore::RenderObject*) + 164 (RenderObject.cpp:355)
4   com.apple.WebCore             	0x000000010eff09e0 WebCore::RenderBlock::removeChild(WebCore::RenderObject*) + 1040 (RenderBlock.cpp:1234)
5   com.apple.WebCore             	0x000000010f0f8c26 WebCore::RenderObject::remove() + 70 (RenderObject.h:965)
6   com.apple.WebCore             	0x000000010f1b80b7 WebCore::RenderObject::willBeDestroyed() + 327 (RenderObject.cpp:2421)
7   com.apple.WebCore             	0x000000010f176e92 WebCore::RenderLayerModelObject::willBeDestroyed() + 162 (RenderLayerModelObject.cpp:91)
8   com.apple.WebCore             	0x000000010f09a6f9 WebCore::RenderBoxModelObject::willBeDestroyed() + 153 (RenderBoxModelObject.cpp:340)
9   com.apple.WebCore             	0x000000010f073c2d WebCore::RenderBox::willBeDestroyed() + 61 (RenderBox.cpp:179)
10  com.apple.WebCore             	0x000000010efec4d2 WebCore::RenderBlock::willBeDestroyed() + 578 (RenderBlock.cpp:304)
11  com.apple.WebCore             	0x000000010f17f8da WebCore::RenderListItem::willBeDestroyed() + 90 (RenderListItem.cpp:79)
12  com.apple.WebCore             	0x000000010f1b884d WebCore::RenderObject::destroy() + 29 (RenderObject.cpp:2570)
13  com.apple.WebCore             	0x000000010f1b8827 WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() + 247 (RenderObject.cpp:2565)
14  com.apple.WebCore             	0x000000010ef02875 WebCore::Node::detach() + 149 (Node.cpp:1118)
15  com.apple.WebCore             	0x000000010de5252b WebCore::ContainerNode::detach() + 43 (ContainerNode.cpp:834)
16  com.apple.WebCore             	0x000000010e2a9730 WebCore::Element::detach() + 288 (Element.cpp:1353)
17  com.apple.WebCore             	0x000000010de51c95 WebCore::ContainerNode::removeChildren() + 741 (ContainerNode.cpp:640)
18  com.apple.WebCore             	0x000000010ee58ee6 WebCore::replaceChildrenWithFragment(WebCore::ContainerNode*, WTF::PassRefPtr<WebCore::DocumentFragment>, int&) + 422 (markup.cpp:1110)
19  com.apple.WebCore             	0x000000010e56e0bc WebCore::HTMLElement::setInnerHTML(WTF::String const&, int&) + 156 (HTMLElement.cpp:356)
20  com.apple.WebCore             	0x000000010ea1f3aa WebCore::setJSHTMLElementInnerHTML(JSC::ExecState*, JSC::JSObject*, JSC::JSValue) + 106 (JSHTMLElement.cpp:544)
21  com.apple.WebCore             	0x000000010ea21349 bool JSC::lookupPut<WebCore::JSHTMLElement>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSHTMLElement*, bool) + 249 (Lookup.h:373)
22  com.apple.WebCore             	0x000000010ea20de8 void JSC::lookupPut<WebCore::JSHTMLElement, WebCore::JSElement>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSHTMLElement*, JSC::PutPropertySlot&) + 120 (Lookup.h:389)
23  com.apple.WebCore             	0x000000010ea1e033 WebCore::JSHTMLElement::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) + 387 (JSHTMLElement.cpp:446)
24  com.apple.WebCore             	0x000000010ea8b42c void JSC::lookupPut<WebCore::JSHTMLOListElement, WebCore::JSHTMLElement>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSHTMLOListElement*, JSC::PutPropertySlot&) + 172 (Lookup.h:391)
25  com.apple.WebCore             	0x000000010ea8a163 WebCore::JSHTMLOListElement::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) + 387 (JSHTMLOListElement.cpp:177)
26  com.apple.JavaScriptCore      	0x000000010ccf0159 JSC::JSValue::put(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) + 185 (JSCJSValueInlines.h:679)
27  com.apple.JavaScriptCore      	0x000000010cee5930 llint_slow_path_put_by_id + 416 (LLIntSlowPaths.cpp:983)
28  com.apple.JavaScriptCore      	0x000000010ceeee58 llint_op_put_by_id + 155
29  com.apple.JavaScriptCore      	0x000000010cdcc044 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::JSGlobalData*) + 84 (JITCode.h:135)
30  com.apple.JavaScriptCore      	0x000000010cdc92bf JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1519 (Interpreter.cpp:1059)
31  com.apple.JavaScriptCore      	0x000000010cbd95f2 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 306 (CallData.cpp:40)
32  com.apple.WebCore             	0x000000010e864b72 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 146 (JSMainThreadExecState.h:56)
33  com.apple.WebCore             	0x000000010e9b60f6 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 1238 (JSEventListener.cpp:129)
34  com.apple.WebCore             	0x000000010e303e22 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&) + 498 (EventTarget.cpp:258)
35  com.apple.WebCore             	0x000000010e303a3c WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 380 (EventTarget.cpp:203)
36  com.apple.WebCore             	0x000000010e243b20 WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>) + 272 (DOMWindow.cpp:1714)
37  com.apple.WebCore             	0x000000010e24abf8 WebCore::DOMWindow::dispatchLoadEvent() + 296 (DOMWindow.cpp:1688)
38  com.apple.WebCore             	0x000000010e0922cf WebCore::Document::dispatchWindowLoadEvent() + 143 (Document.cpp:3731)
39  com.apple.WebCore             	0x000000010e08fce2 WebCore::Document::implicitClose() + 466 (Document.cpp:2485)
40  com.apple.WebCore             	0x000000010e3d46db WebCore::FrameLoader::checkCallImplicitClose() + 155 (FrameLoader.cpp:838)
41  com.apple.WebCore             	0x000000010e3d4386 WebCore::FrameLoader::checkCompleted() + 358 (FrameLoader.cpp:782)
42  com.apple.WebCore             	0x000000010e3d2fe3 WebCore::FrameLoader::finishedParsing() + 179 (FrameLoader.cpp:715)
43  com.apple.WebCore             	0x000000010e09ac59 WebCore::Document::finishedParsing() + 521 (Document.cpp:4513)
44  com.apple.WebCore             	0x000000010e53ed3c WebCore::HTMLConstructionSite::finishedParsing() + 28 (HTMLConstructionSite.cpp:342)
45  com.apple.WebCore             	0x000000010e63520b WebCore::HTMLTreeBuilder::finished() + 139 (HTMLTreeBuilder.cpp:2923)
46  com.apple.WebCore             	0x000000010e55ecfc WebCore::HTMLDocumentParser::end() + 220 (HTMLDocumentParser.cpp:756)
47  com.apple.WebCore             	0x000000010e55d6e0 WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd() + 288 (HTMLDocumentParser.cpp:767)
48  com.apple.WebCore             	0x000000010e55d4c6 WebCore::HTMLDocumentParser::prepareToStopParsing() + 294 (HTMLDocumentParser.cpp:211)
49  com.apple.WebCore             	0x000000010e55ed53 WebCore::HTMLDocumentParser::attemptToEnd() + 67 (HTMLDocumentParser.cpp:779)
50  com.apple.WebCore             	0x000000010e55eda8 WebCore::HTMLDocumentParser::finish() + 72 (HTMLDocumentParser.cpp:828)
51  com.apple.WebCore             	0x000000010e12d4b8 WebCore::DocumentWriter::end() + 392 (DocumentWriter.cpp:249)
52  com.apple.WebCore             	0x000000010e0f0cae WebCore::DocumentLoader::finishedLoading(double) + 398 (DocumentLoader.cpp:403)
53  com.apple.WebCore             	0x000000010e0f0a8c WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource*) + 316 (DocumentLoader.cpp:342)
54  com.apple.WebCore             	0x000000010dd9dd9d WebCore::CachedResource::checkNotify() + 109 (CachedResource.cpp:379)
55  com.apple.WebCore             	0x000000010dd9de05 WebCore::CachedResource::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool) + 69 (CachedResource.cpp:389)
56  com.apple.WebCore             	0x000000010dd979f1 WebCore::CachedRawResource::data(WTF::PassRefPtr<WebCore::ResourceBuffer>, bool) + 673 (CachedRawResource.cpp:72)
57  com.apple.WebCore             	0x000000010f523cff WebCore::SubresourceLoader::didFinishLoading(double) + 527 (SubresourceLoader.cpp:290)
58  com.apple.WebCore             	0x000000010f305f35 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 53 (ResourceLoader.cpp:501)
59  com.apple.WebCore             	0x000000010f83d36a -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 186 (WebCoreResourceHandleAsDelegate.mm:232)
60  com.apple.Foundation          	0x00007fff8a04063e ___NSURLConnectionDidFinishLoading_block_invoke_1 + 122
61  com.apple.Foundation          	0x00007fff8a0405be _NSURLConnectionDidFinishLoading + 81
62  com.apple.CFNetwork           	0x00007fff9128c4e6 URLConnectionClient::_clientDidFinishLoading(URLConnectionClient::ClientConnectionEventQueue*) + 296
63  com.apple.CFNetwork           	0x00007fff9133c8fe URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload(XConnectionEventInfo<XClientEvent, XClientEventParams>*, long) + 862
64  com.apple.CFNetwork           	0x00007fff91267231 URLConnectionClient::processEvents() + 185
65  com.apple.CFNetwork           	0x00007fff912670d6 MultiplexerSource::perform() + 212
66  com.apple.CoreFoundation      	0x00007fff8c4314f1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
67  com.apple.CoreFoundation      	0x00007fff8c430d5d __CFRunLoopDoSources0 + 253
68  com.apple.CoreFoundation      	0x00007fff8c457b49 __CFRunLoopRun + 905
69  com.apple.CoreFoundation      	0x00007fff8c457486 CFRunLoopRunSpecific + 230
70  com.apple.HIToolbox           	0x00007fff8d7ae2bf RunCurrentEventLoopInMode + 277
71  com.apple.HIToolbox           	0x00007fff8d7b556d ReceiveNextEventCommon + 355
72  com.apple.HIToolbox           	0x00007fff8d7b53fa BlockUntilNextEventMatchingListInMode + 62
73  com.apple.AppKit              	0x00007fff8b2d0779 _DPSNextEvent + 659
74  com.apple.AppKit              	0x00007fff8b2d007d -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 135
75  com.apple.AppKit              	0x00007fff8b2cc9b9 -[NSApplication run] + 470
76  com.apple.WebCore             	0x000000010f33ac99 WebCore::RunLoop::run() + 105 (RunLoopMac.mm:44)
77  com.apple.WebKit2             	0x000000010b947af5 int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebContentProcessMainDelegate>(int, char**) + 917 (ChildProcessEntryPoint.h:98)
78  com.apple.WebKit2             	0x000000010b94774b WebContentProcessMain + 27 (WebContentProcessMain.mm:179)
79  com.apple.WebProcess          	0x000000010b6d5c94 _ZN6WebKitL13BootstrapMainEiPPc + 308
80  com.apple.WebProcess          	0x000000010b6d5b52 main + 34
81  com.apple.WebProcess          	0x000000010b6d5b24 start + 52

[Andrei Bucur]

Thanks for the stack trace! I've managed already to find the issue and want to clarify something.
In ContainerNode, when removing a child, we first detach it and then remove if from the tree. When removing all the children the process is reversed. We first remove them from the tree and then detach them.
This crash appears because the list item renderer wants to find the counter node when it is removed from the tree so it can notify the other list items they need to be updated. This doesn't work any more when the list item node is removed through removeChildren because its node is not in the DOM any more :/.
Is there a good reason why we have to remove the DOM nodes first and then the renderers? The order of operations seems wrong to me.

[Elliott Sprehn]

(In reply to comment #20)
> ...
> Is there a good reason why we have to remove the DOM nodes first and then the renderers? The order of operations seems wrong to me.

The reason is in the comment:

            // Detach the nodes only after properly removed from the tree because
            // a. detaching requires a proper DOM tree (for counters and quotes for
            // example) and during the previous loop the next sibling still points to
            // the node being removed while the node being removed does not point back
            // and does not point to the same parent as its next sibling.
            // b. destroying Renderers of standalone nodes is sometimes faster.

there are cases where this could be important, but I think a saner approach than what's inside ::removeChildren() right now would be to detach() yourself, remove all your children, then reattach() yourself. We'd be destroying and recreating one more renderer, but we'd be doing something much saner in the process. The only thing to watch out for would be that innerHTML doesn't get slower.

[Andrei Bucur]

Created attachment 197021 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 197021 [details]
Patch for landing

Clearing flags on attachment: 197021

Committed r148026: <http://trac.webkit.org/changeset/148026>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andrei Bucur]

Reopened  after 113517 rollback.

[Andrei Bucur]

Comment on attachment 197021 [details]
Patch for landing

Re-land after http://trac.webkit.org/changeset/148754 .

[WebKit Commit Bot]

Comment on attachment 197021 [details]
Patch for landing

Clearing flags on attachment: 197021

Committed r148863: <http://trac.webkit.org/changeset/148863>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.