e.g. <div id=t>old text</div> <script>t.innerHTML = "new text"</script> should always add a mutation record for removing #"old text" for interoperability since it uses "replace all" in DOM: http://www.w3.org/TR/dom/#concept-node-replace-all
<rdar://problem/22571962>
This bug was found by the newly added test for mutation observer: LayoutTests/http/tests/w3c/dom/nodes/MutationObserver-inner-outer.html
Created attachment 269036 [details] Fixes the bug
Comment on attachment 269036 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=269036&action=review This patch disables optimization added by http://trac.webkit.org/r21861 when it can be observable. > LayoutTests/fast/dom/innerHTML-single-text-node.html:59 > +// FIXME: Add a test for DOMNodeRemovedFromDocument event. Unfortunately, I couldn't create a test case for DOMNodeRemovedFromDocument because it requires attaching an event listener on the old text node and that seems to retain the text node. I tried calling gc(), executing code inside a function, eval, etc... all but none of that worked.
Comment on attachment 269036 [details] Fixes the bug Attachment 269036 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/693150 New failing tests: fast/events/updateLayoutForHitTest.html fast/events/crash-on-mutate-during-drop.html fast/dom/adopt-node-crash.html fast/dom/HTMLElement/set-inner-outer-optimization.html
Created attachment 269038 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269036 [details] Fixes the bug Attachment 269036 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/693153 New failing tests: fast/events/updateLayoutForHitTest.html fast/dom/HTMLElement/set-inner-outer-optimization.html fast/dom/adopt-node-crash.html
Created attachment 269039 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 269036 [details] Fixes the bug Attachment 269036 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/693145 New failing tests: fast/events/updateLayoutForHitTest.html fast/images/destroyed-image-load-event.html fast/events/crash-on-mutate-during-drop.html fast/dom/adopt-node-crash.html fast/dom/HTMLElement/set-inner-outer-optimization.html
Created attachment 269040 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 269036 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=269036&action=review I won’t review until after the tests pass. > Source/WebCore/dom/ChildListMutationScope.h:85 > + bool canObserve() { return m_accumulator; } Should be a const member function. > Source/WebCore/editing/markup.cpp:992 > +static inline bool hasMutationEventListeners(Document& document) const Document& maybe? Not sure if it’s work using const in a case like this. > Source/WebCore/editing/markup.cpp:1015 > + && !containerChild->refCount() Wow, this seems a bit fragile! Isn’t there some way this child could have a single reference for some random reason other than being exposed to the DOM? Is it really OK to have different behavior in that case? I guess this just makes the optimization fragile, which is OK I suppose. I think this whole sequence of code needs a comment. It’s unclear why these checks are needed. Some comment to say “we can do this optimization as long as it’s not detectable by script”. I might even consider putting all the checks into an inline helper function so we can comment on each one, explaining why that particular check is needed. Then the name of the function can take on some of the burden of explaining what’s going on, reducing the need for comment a bit. Something like this: // We can use setData instead of actually replacing the child as long as script can’t tell we did that. static inline bool canUseSetDataOptimization(...) { // If there are any references to the old node, we need to make sure that node is removed. // We’ll assume any reference count on the node at all comes from observable script. if (containerChild->refCount()) return false; ... } Then the code change will be simpler, more like this: if (containerChild->isTextNode() && hasOneTextChild(fragment) && canUseSetDataOptimization(downcast<Text>(*containerChild), mutation))
Created attachment 269233 [details] Fix tests and address Darin's comments
Comment on attachment 269233 [details] Fix tests and address Darin's comments View in context: https://bugs.webkit.org/attachment.cgi?id=269233&action=review > Source/WebCore/editing/markup.cpp:1022 > + downcast<Text>(*containerChild).setData(downcast<Text>(*fragment->firstChild()).data(), ec); It is weird setData takes an exception code and ignores it. It is not in IDL so appears to be entirely internal.
(In reply to comment #13) > It is weird setData takes an exception code and ignores it. It is not in IDL > so appears to be entirely internal. It is in IDL. From CharacterData.idl: [TreatNullAs=NullString, SetterRaisesException] attribute DOMString data; But it seems that it never raises an exception and so we could take out SetterRaisesException.
Comment on attachment 269233 [details] Fix tests and address Darin's comments View in context: https://bugs.webkit.org/attachment.cgi?id=269233&action=review > Source/WebCore/editing/markup.cpp:1002 > +static inline bool canUseSetDataOptimization(const Node& containerChild, const ChildListMutationScope& mutationScope) I think the argument type should be const Text& since the caller knows it’s a Text, not an arbitrary Node.
(In reply to comment #14) > (In reply to comment #13) > > It is weird setData takes an exception code and ignores it. It is not in IDL > > so appears to be entirely internal. > > It is in IDL. From CharacterData.idl: > > [TreatNullAs=NullString, SetterRaisesException] attribute DOMString data; > > But it seems that it never raises an exception and so we could take out > SetterRaisesException. Let's make that change in a separate patch.
Created attachment 269257 [details] Patch for landing
Comment on attachment 269257 [details] Patch for landing Clearing flags on attachment: 269257 Committed r195263: <http://trac.webkit.org/changeset/195263>
All reviewed patches have been landed. Closing bug.