Bug 148782 – innerHTML should always add a mutation record for removing all children

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

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

[Radar WebKit Bug Importer]

<rdar://problem/22571962>

[Ryosuke Niwa]

This bug was found by the newly added test for mutation observer: LayoutTests/http/tests/w3c/dom/nodes/MutationObserver-inner-outer.html

[Ryosuke Niwa]

Created attachment 269036 [details]
Fixes the bug

[Ryosuke Niwa]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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

[Ryosuke Niwa]

Created attachment 269233 [details]
Fix tests and address Darin's comments

[Antti Koivisto]

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.

[Darin Adler]

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

[Darin Adler]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 269257 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 269257 [details]
Patch for landing

Clearing flags on attachment: 269257

Committed r195263: <http://trac.webkit.org/changeset/195263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.