Bug 110352

Created attachment 190269 [details] Patch

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.

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

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

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

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

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

Created attachment 191972 [details] Patch

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.

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

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

Created attachment 194600 [details] Patch

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.

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

Created attachment 195064 [details] Patch for landing

Comment on attachment 195064 [details] Patch for landing Clearing flags on attachment: 195064 Committed r146879: <http://trac.webkit.org/changeset/146879>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 113312

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

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.

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

Created attachment 197021 [details] Patch for landing

Comment on attachment 197021 [details] Patch for landing Clearing flags on attachment: 197021 Committed r148026: <http://trac.webkit.org/changeset/148026>

All reviewed patches have been landed. Closing bug.

Reopened after 113517 rollback.

Comment on attachment 197021 [details] Patch for landing Re-land after http://trac.webkit.org/changeset/148754 .

Comment on attachment 197021 [details] Patch for landing Clearing flags on attachment: 197021 Committed r148863: <http://trac.webkit.org/changeset/148863>

All reviewed patches have been landed. Closing bug.