Simplify ContainerNode::removeChildren
Created attachment 195598 [details] Patch
Comment on attachment 195598 [details] Patch Attachment 195598 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17317222 New failing tests: fast/dom/MutationObserver/added-out-of-order.html fast/dom/containerNode.html
Created attachment 195616 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 195598 [details] Patch Attachment 195598 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17216991 New failing tests: fast/dom/MutationObserver/added-out-of-order.html fast/dom/containerNode.html
Created attachment 195625 [details] Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 195598 [details] Patch Attachment 195598 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17345174
This isn't going to work because of https://trac.webkit.org/changeset/74101 Every other browser I tested has an infinite loop in this case, but if we want to prevent it we can't avoid the second iteration over all the children. I feel like we should just put the infinite loop back in though, since clearly no one depends on this busted behavior, and our current behavior means the newly added children don't get removal events which violates correctness of mutation events.
Comment on attachment 195598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195598&action=review > Source/WebCore/dom/ContainerNode.cpp:596 > + child->notifyMutationObserversNodeWillDetach(); I'm wondering if it would make sense to move these two calls further down, so that they're only run if the child isn't interfered with by mutation event handlers.
(In reply to comment #8) > (From update of attachment 195598 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195598&action=review > > > Source/WebCore/dom/ContainerNode.cpp:596 > > + child->notifyMutationObserversNodeWillDetach(); > > I'm wondering if it would make sense to move these two calls further down, so that they're only run if the child isn't interfered with by mutation event handlers. Yeah, I did that in my local version already. The posted patch also manually detaches even though removeBetween does it for you which I fixed too.
Created attachment 196028 [details] Patch
Comment on attachment 196028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196028&action=review > Source/WebCore/ChangeLog:12 > + Simplify ContainerNode::removeChildren by merging the loops and removing > + willRemoveChildren. This removes two traversals of the children, avoids > + refing and derefing all the children once, avoids allocating a second > + NodeVector of children, and means we detach() in the same order as > + normal removal. This observably changes behavior of mutation events. You should explain that in your changelog, and ideally test the change. > LayoutTests/fast/dom/containerNode.html:-2 > -<!DOCTYPE html> > -<html> Missing a ChangeLOg for this. Why are you removing this? --update-changelogs passed to upload will add one for you. :)
I modified innerHTML-setter.html to add 100000 divs as siblings and test just container.innerHTML = "" removal cost and it's 3% improvement there on my MBP Retina. That matches the improvement on the original test so I don't think there's a reason for a new test.
Created attachment 196422 [details] Patch
Comment on attachment 196422 [details] Patch This makes sense to me.
Comment on attachment 196422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196422&action=review > Source/WebCore/dom/ContainerNode.cpp:604 > + mutation.willRemoveChild(child.get()); > + child->notifyMutationObserversNodeWillDetach(); > + removeBetween(0, child->nextSibling(), child.get()); Do we end up firing events here?
(In reply to comment #15) > (From update of attachment 196422 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196422&action=review > > > Source/WebCore/dom/ContainerNode.cpp:604 > > + mutation.willRemoveChild(child.get()); > > + child->notifyMutationObserversNodeWillDetach(); > > + removeBetween(0, child->nextSibling(), child.get()); > > Do we end up firing events here? No, events only fire in the lines before the if (child != m_firstChild) check, that's why the code is written that way.
Created attachment 196426 [details] Patch
Comment on attachment 196426 [details] Patch Attachment 196426 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17513022 New failing tests: fast/dom/MutationObserver/observe-childList.html fast/dom/MutationObserver/added-out-of-order.html fast/dom/MutationObserver/document-fragment-insertion.html
Created attachment 196440 [details] Archive of layout-test-results from gce-cr-linux-01 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Created attachment 196859 [details] Patch
*** Bug 113433 has been marked as a duplicate of this bug. ***
Comment on attachment 196859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196859&action=review Some risky changes to observable behavior, but still seems worth doing. > Source/WebCore/ChangeLog:37 > + (Document): Please remove this bogus line from the change log.
Created attachment 196872 [details] Patch for landing
Created attachment 196873 [details] Patch for landing with reviewer
Comment on attachment 196873 [details] Patch for landing with reviewer Rejecting attachment 196873 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 196873, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ren-event.html patching file LayoutTests/fast/dom/containerNode-expected.txt rm 'LayoutTests/fast/dom/containerNode-expected.txt' patching file LayoutTests/fast/dom/containerNode.html rm 'LayoutTests/fast/dom/containerNode.html' patching file LayoutTests/fast/events/mutation-during-innerHTML-expected.txt patching file LayoutTests/fast/events/mutation-during-innerHTML.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-commit-queue.appspot.com/results/17452048
Created attachment 196876 [details] Correct patch for landing
Created attachment 196878 [details] Correct patch for landing 2
Comment on attachment 196878 [details] Correct patch for landing 2 Clearing flags on attachment: 196878 Committed r147942: <http://trac.webkit.org/changeset/147942>
All reviewed patches have been landed. Closing bug.
Comment on attachment 196878 [details] Correct patch for landing 2 View in context: https://bugs.webkit.org/attachment.cgi?id=196878&action=review > Source/WebCore/dom/ContainerNode.cpp:605 > + // document()->nodeWillbeRemoved() may modify the children list so we may need to retry. Where could document()->nodeWillbeRemoved modify the child list? I can't find anywhere this can happen.
(In reply to comment #30) > Where could document()->nodeWillbeRemoved modify the child list? I can't find anywhere this can happen. I suspect that FrameSelection::setSelection, called by FrameSelection::respondToNodeModification , called in turn by FrameSelection::nodeWillBeRemoved, could run some arbitrary JavaScript.
(In reply to comment #31) > (In reply to comment #30) > > Where could document()->nodeWillbeRemoved modify the child list? I can't find anywhere this can happen. > > I suspect that FrameSelection::setSelection, called by FrameSelection::respondToNodeModification , called in turn by FrameSelection::nodeWillBeRemoved, could run some arbitrary JavaScript. That's always done async by queueing a selection change event. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/WebCore/editing/FrameSelection.cpp&l=343 Looking further I'm almost certain this is not possible since Document::nodeChildrenWillBeRemoved has a loops over bare ptrs for nextSibling to update selection, event handlers and dragging, and other bare ptr loops inside Range updates and others. Those would all be security bugs if JS execution was possible. I don't think the comment, or the second check is needed.
Can we add some sort of assertion for that?
(In reply to comment #32) > (In reply to comment #31) > > (In reply to comment #30) > > > Where could document()->nodeWillbeRemoved modify the child list? I can't find anywhere this can happen. > > > > I suspect that FrameSelection::setSelection, called by FrameSelection::respondToNodeModification , called in turn by FrameSelection::nodeWillBeRemoved, could run some arbitrary JavaScript. > > That's always done async by queueing a selection change event. Sure, I wasn’t thinking of the selection change event. I was thinking about all the other things that function does. Did you audit them all? I hope you’re right that none of them can do arbitrary DOM manipulation or send events. For example, I noticed that FrameSelection::setSelection calls selectFrameElementInParentIfFullySelected and it calls setFocusedFrame, which definitely dispatches events, but in this case that won’t happen because we are reducing the selection, not increasing it. I don’t think Editor::respondToChangedSelection does anything that could change the DOM, but it does a lot of things. I believe we have machinery for asserting that nothing changes the DOM or triggers JavaScript, and we might want to use it here. Then at least we’d learn at runtime when running tests or doing ad hoc debug build testing if we made a mistake.
Wait… setSelection changes focus so it can synchronously fire blur.
(In reply to comment #35) > Wait… setSelection changes focus so it can synchronously fire blur. We pass a flag so that can never happen. It looks like there may be a case inside selectFrameElementInParentIfFullySelected, but I can't figure out a repro yet. It would require having a selection that covers an entire iframe contents, but is _not_ a range selection (ex. caret) so we'd go through setFocusedFrame and fire those focus/blur events. Nothing I do seems to be able to trigger that though but it looks possible. Perhaps someone else can figure out how to write a repro. I see leviw@ wrote a test for selectFrameElementInParentIfFullySelected a few weeks ago.
(In reply to comment #30) > (From update of attachment 196878 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196878&action=review > > > Source/WebCore/dom/ContainerNode.cpp:605 > > + // document()->nodeWillbeRemoved() may modify the children list so we may need to retry. > > Where could document()->nodeWillbeRemoved modify the child list? I can't find anywhere this can happen. We actually have a test case when this happens in fast/dom/HTMLObjectElement/beforeload-set-text-crash.xhtml . I've attached a relevant stack trace at the end of my post. The guardedDispatchBeforeLoadEvent seems to be not so guarded ( https://bugs.webkit.org/show_bug.cgi?id=71264 ). I imagined this is not the only situation when this could happen so I decided to prevent a future disaster by guarding the removal function. I think this hasn't popped up as an issue until now is because the layout tests don't move nodes around the DOM so nodeChildrenWillBeRemoved() loops seemed safe. I'm really not familiar with the plugins/objects code so if you want me to replace the if () continue; with an ASSERT I'm happy to do so. But we'd have to fix this plugin stuff first. Thoughts? #0 0x000000010309df89 in WebCore::ContainerNode::removeChildren() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/ContainerNode.cpp:606 #1 0x000000010416457e in WebCore::Node::setTextContent(WTF::String const&, int&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Node.cpp:1709 #2 0x0000000103d21a99 in WebCore::setJSNodeTextContent(JSC::ExecState*, JSC::JSObject*, JSC::JSValue) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/DerivedSources/WebCore/JSNode.cpp:444 #3 0x0000000103d25ad9 in bool JSC::lookupPut<WebCore::JSNode>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSNode*, bool) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/JavaScriptCore.framework/PrivateHeaders/Lookup.h:373 #4 0x0000000103d25568 in void JSC::lookupPut<WebCore::JSNode, WebCore::JSDOMWrapper>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSNode*, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/JavaScriptCore.framework/PrivateHeaders/Lookup.h:389 #5 0x0000000103d20ce1 in WebCore::JSNode::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/DerivedSources/WebCore/JSNode.cpp:404 #6 0x0000000103b87b3c in void JSC::lookupPut<WebCore::JSElement, WebCore::JSNode>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSElement*, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/JavaScriptCore.framework/PrivateHeaders/Lookup.h:390 #7 0x0000000103b7b971 in WebCore::JSElement::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/DerivedSources/WebCore/JSElement.cpp:1225 #8 0x0000000103c1292c in void JSC::lookupPut<WebCore::JSHTMLElement, WebCore::JSElement>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSHTMLElement*, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/JavaScriptCore.framework/PrivateHeaders/Lookup.h:390 #9 0x0000000103c0f761 in WebCore::JSHTMLElement::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/DerivedSources/WebCore/JSHTMLElement.cpp:445 #10 0x0000000103c7ef6c in void JSC::lookupPut<WebCore::JSHTMLObjectElement, WebCore::JSHTMLElement>(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::HashTable const*, WebCore::JSHTMLObjectElement*, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/JavaScriptCore.framework/PrivateHeaders/Lookup.h:390 #11 0x0000000103c7bbcc in WebCore::JSHTMLObjectElement::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/WebKitBuild/b113517/Debug/DerivedSources/WebCore/JSHTMLObjectElement.cpp:404 #12 0x0000000101e8dd79 in JSC::JSValue::put(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) at /Volumes/HDD/NonPerforce/WebKit/Source/JavaScriptCore/runtime/JSCJSValueInlines.h:678 #13 0x0000000101fa846b in cti_op_put_by_id_generic at /Volumes/HDD/NonPerforce/WebKit/Source/JavaScriptCore/jit/JITStubs.cpp:1417 #14 0x0000000101fb5230 in JSC::hasIndexedProperties(unsigned char) at /Volumes/HDD/NonPerforce/WebKit/Source/JavaScriptCore/runtime/IndexingType.h:101 #15 0x0000000101f70f54 in JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::JSGlobalData*) at /Volumes/HDD/NonPerforce/WebKit/Source/JavaScriptCore/jit/JITCode.h:135 #16 0x0000000101f6dcb8 in JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) at /Volumes/HDD/NonPerforce/WebKit/Source/JavaScriptCore/interpreter/Interpreter.cpp:1111 #17 0x0000000101d73efc in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) at /Volumes/HDD/NonPerforce/WebKit/Source/JavaScriptCore/runtime/CallData.cpp:40 #18 0x0000000103a4b442 in WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/bindings/js/JSMainThreadExecState.h:56 #19 0x0000000103ba4570 in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/bindings/js/JSEventListener.cpp:129 #20 0x00000001034cf9e2 in WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow>&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/EventTarget.cpp:257 #21 0x00000001034cf5d6 in WebCore::EventTarget::fireEventListeners(WebCore::Event*) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/EventTarget.cpp:203 #22 0x0000000104167742 in WebCore::Node::handleLocalEvents(WebCore::Event*) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Node.cpp:2328 #23 0x000000010349cee1 in WebCore::EventContext::handleLocalEvents(WebCore::Event*) const at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/EventContext.cpp:58 #24 0x000000010349e902 in WebCore::EventDispatcher::dispatchEventAtCapturing(WebCore::WindowEventContext&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/EventDispatcher.cpp:157 #25 0x000000010349e1e9 in WebCore::EventDispatcher::dispatch() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/EventDispatcher.cpp:124 #26 0x000000010349ff8b in WebCore::EventDispatchMediator::dispatchEvent(WebCore::EventDispatcher*) const at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/EventDispatchMediator.cpp:54 #27 0x000000010349d6fc in WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WTF::PassRefPtr<WebCore::EventDispatchMediator>) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/EventDispatcher.cpp:56 #28 0x00000001041678bb in WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Node.cpp:2349 #29 0x00000001041682a7 in WebCore::Node::dispatchBeforeLoadEvent(WTF::String const&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Node.cpp:2428 #30 0x00000001037c525b in WebCore::HTMLPlugInElement::guardedDispatchBeforeLoadEvent(WTF::String const&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/html/HTMLPlugInElement.cpp:146 #31 0x00000001037bb1f7 in WebCore::HTMLObjectElement::updateWidget(WebCore::PluginCreationOption) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/html/HTMLObjectElement.cpp:317 #32 0x00000001037c6f81 in WebCore::HTMLPlugInImageElement::updateWidgetIfNecessary() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/html/HTMLPlugInImageElement.cpp:239 #33 0x00000001037c6e38 in WebCore::HTMLPlugInImageElement::updateWidgetCallback(WebCore::Node*, unsigned int) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/html/HTMLPlugInImageElement.cpp:296 #34 0x000000010309e87d in WebCore::ContainerNode::dispatchPostAttachCallbacks() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/ContainerNode.cpp:779 #35 0x000000010309e728 in WebCore::ContainerNode::resumePostAttachCallbacks() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/ContainerNode.cpp:742 #36 0x000000010333dad8 in WebCore::PostAttachCallbackDisabler::~PostAttachCallbackDisabler() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/ContainerNode.h:356 #37 0x00000001032f7685 in WebCore::PostAttachCallbackDisabler::~PostAttachCallbackDisabler() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/ContainerNode.h:355 #38 0x00000001032e4306 in WebCore::Document::recalcStyle(WebCore::Node::StyleChange) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Document.cpp:1893 #39 0x00000001032e031b in WebCore::Document::updateStyleIfNeeded() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Document.cpp:1913 #40 0x00000001035bf12a in WebCore::clearRenderViewSelection(WebCore::Position const&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/editing/FrameSelection.cpp:364 #41 0x00000001035bf69a in WebCore::FrameSelection::respondToNodeModification(WebCore::Node*, bool, bool, bool, bool) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/editing/FrameSelection.cpp:436 #42 0x00000001035bf2db in WebCore::FrameSelection::nodeWillBeRemoved(WebCore::Node*) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/editing/FrameSelection.cpp:389 #43 0x00000001032ebbca in WebCore::Document::nodeWillBeRemoved(WebCore::Node*) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Document.cpp:3585 #44 0x000000010309dfba in WebCore::ContainerNode::removeChildren() at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/ContainerNode.cpp:606 #45 0x000000010416457e in WebCore::Node::setTextContent(WTF::String const&, int&) at /Volumes/HDD/NonPerforce/WebKit/Source/WebCore/dom/Node.cpp:1709
Re-opened since this is blocked by bug 114521
*** This bug has been marked as a duplicate of bug 114677 ***