Changeset 195263 in webkit


Ignore:
Timestamp:
Jan 19, 2016 12:39:44 AM (8 years ago)
Author:
rniwa@webkit.org
Message:

innerHTML should always add a mutation record for removing all children
​https://bugs.webkit.org/show_bug.cgi?id=148782
<rdar://problem/22571962>

Reviewed by Antti Koivisto.

LayoutTests/imported/w3c:

Rebaseline a test now that all test test cases are passing.

  • web-platform-tests/dom/nodes/MutationObserver-inner-outer-expected.txt:

Source/WebCore:

Fixed the bug by disabling WebKit's optimization to avoid the node replacement when the behavior
is observable to scripts by either:

  • Author scripts has a reference to the node
  • MutationObserver can be observing this subtree
  • Mutation events can be observing this subtree

Note that no caller of this function exposes fragment to author scripts so it couldn't be referenced.
It also means that we don't need to check DOMNodeInsertedIntoDocument since it doesn't bubble up
(it's only relevant if the text node in fragment has its event listener but that's impossible).

Test: fast/dom/innerHTML-single-text-node.html

  • dom/ChildListMutationScope.h:

(WebCore::ChildListMutationScope::canObserve): Added.

  • editing/markup.cpp:

(WebCore::hasMutationEventListeners): Added.
(WebCore::replaceChildrenWithFragment):

LayoutTests:

Add a more comprehensive test for replacing a single text node with innerHTML's setter to ensure
WebKit's optimization to avoid replacing the node should not be observable by scripts in any way.

  • fast/dom/innerHTML-single-text-node-expected.txt: Added.
  • fast/dom/innerHTML-single-text-node.html: Added.
Location:
trunk
Files:
2 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r195248 r195263  
     12016-01-19  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        innerHTML should always add a mutation record for removing all children
     4        https://bugs.webkit.org/show_bug.cgi?id=148782
     5        <rdar://problem/22571962>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Add a more comprehensive test for replacing a single text node with innerHTML's setter to ensure
     10        WebKit's optimization to avoid replacing the node should not be observable by scripts in any way.
     11
     12        * fast/dom/innerHTML-single-text-node-expected.txt: Added.
     13        * fast/dom/innerHTML-single-text-node.html: Added.
     14
    1152016-01-18  Ryosuke Niwa  <rniwa@webkit.org>
    216

  • trunk/LayoutTests/fast/dom/HTMLElement/set-inner-outer-optimization.html

    r120792 r195263  
    7575
    7676    runTest('text', 'innerHTML', '', 'modified');
    77     runTest('text', 'innerHTML', 'different text', 'modified, with same first child');
    78     runTest('text', 'innerHTML', 'text', 'not modified');
     77    runTest('text', 'innerHTML', 'different text', 'modified');
     78    runTest('text', 'innerHTML', 'text', 'modified');
    7979    runTest('text', 'innerHTML', '<a></a>', 'modified');
    8080    runTest('text', 'innerHTML', '<a></a><b></b>', 'modified');

  • trunk/LayoutTests/fast/dom/adopt-node-crash.html

    r120792 r195263  
    1111
    1212var mutationHandler = function() {
     13    document.body.removeEventListener('DOMSubtreeModified', mutationHandler, true);
    1314    document.getElementById('result').innerHTML = "DOMSubtreeModified fired";
    14     document.body.removeEventListener('DOMSubtreeModified', mutationHandler, true);
    1515    document.body.appendChild(nodeToAdopt);
    1616};

  • trunk/LayoutTests/fast/events/crash-on-mutate-during-drop.html

    r120792 r195263  
    22<head>
    33<script>
    4 function foo() {
     4function onDOMNodeInserted(event) {
     5    document.removeEventListener("DOMNodeInserted", onDOMNodeInserted, true);
    56    if (event.type == "DOMNodeInserted" && event.target.nodeType == 3)
    67        document.body.innerHTML = "PASSED";
    … …  
    1314    window.testRunner.dumpAsText();
    1415
    15     document.addEventListener("DOMNodeInserted", function() { foo() }, true);
     16    document.addEventListener("DOMNodeInserted", onDOMNodeInserted, true);
    1617
    1718    // Select the element 'dragSource'.

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r195248 r195263  
     12016-01-19  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        innerHTML should always add a mutation record for removing all children
     4        https://bugs.webkit.org/show_bug.cgi?id=148782
     5        <rdar://problem/22571962>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Rebaseline a test now that all test test cases are passing.
     10
     11        * web-platform-tests/dom/nodes/MutationObserver-inner-outer-expected.txt:
     12
    1132016-01-18  Ryosuke Niwa  <rniwa@webkit.org>
    214

  • trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/MutationObserver-inner-outer-expected.txt

    r189471 r195263  
    22
    33
    4 FAIL innerHTML mutation assert_equals: mutation records must match expected 2 but got 1
     4PASS innerHTML mutation
    55PASS innerHTML with 2 children mutation
    66PASS outerHTML mutation

  • trunk/LayoutTests/platform/mac/fast/events/updateLayoutForHitTest-expected.txt

    r177774 r195263  
    1515          text run at (13,6) width 50: " Project"
    1616        RenderText {#text} at (0,0) size 0x0
    17 selection start: position 0 of child 0 {#text} of child 1 {SPAN} of child 3 {DIV} of body
     17selection start: position 0 of child 1 {SPAN} of child 3 {DIV} of body
    1818selection end:   position 0 of child 1 {#text} of child 3 {DIV} of child 3 {DIV} of body

  • trunk/Source/WebCore/ChangeLog

    r195248 r195263  
     12016-01-19  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        innerHTML should always add a mutation record for removing all children
     4        https://bugs.webkit.org/show_bug.cgi?id=148782
     5        <rdar://problem/22571962>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Fixed the bug by disabling WebKit's optimization to avoid the node replacement when the behavior
     10        is observable to scripts by either:
     11         - Author scripts has a reference to the node
     12         - MutationObserver can be observing this subtree
     13         - Mutation events can be observing this subtree
     14
     15        Note that no caller of this function exposes fragment to author scripts so it couldn't be referenced.
     16        It also means that we don't need to check DOMNodeInsertedIntoDocument since it doesn't bubble up
     17        (it's only relevant if the text node in fragment has its event listener but that's impossible).
     18
     19        Test: fast/dom/innerHTML-single-text-node.html
     20
     21        * dom/ChildListMutationScope.h:
     22        (WebCore::ChildListMutationScope::canObserve): Added.
     23
     24        * editing/markup.cpp:
     25        (WebCore::hasMutationEventListeners): Added.
     26        (WebCore::replaceChildrenWithFragment):
     27
    1282016-01-18  Ryosuke Niwa  <rniwa@webkit.org>
    229

  • trunk/Source/WebCore/dom/ChildListMutationScope.h

    r193408 r195263  
    8383    }
    8484
     85    bool canObserve() const { return m_accumulator; }
     86
    8587    void childAdded(Node& child)
    8688    {

  • trunk/Source/WebCore/editing/markup.cpp

    r194819 r195263  
    990990}
    991991
     992static inline bool hasMutationEventListeners(const Document& document)
     993{
     994    return document.hasListenerType(Document::DOMSUBTREEMODIFIED_LISTENER)
     995        || document.hasListenerType(Document::DOMNODEINSERTED_LISTENER)
     996        || document.hasListenerType(Document::DOMNODEREMOVED_LISTENER)
     997        || document.hasListenerType(Document::DOMNODEREMOVEDFROMDOCUMENT_LISTENER)
     998        || document.hasListenerType(Document::DOMCHARACTERDATAMODIFIED_LISTENER);
     999}
     1000
     1001// We can use setData instead of replacing Text node as long as script can't observe the difference.
     1002static inline bool canUseSetDataOptimization(const Text& containerChild, const ChildListMutationScope& mutationScope)
     1003{
     1004    bool authorScriptMayHaveReference = containerChild.refCount();
     1005    return !authorScriptMayHaveReference && !mutationScope.canObserve() && !hasMutationEventListeners(containerChild.document());
     1006}
     1007
    9921008void replaceChildrenWithFragment(ContainerNode& container, Ref<DocumentFragment>&& fragment, ExceptionCode& ec)
    9931009{
    … …  
    10001016    }
    10011017
    1002     if (hasOneTextChild(containerNode) && hasOneTextChild(fragment)) {
    1003         downcast<Text>(*containerNode->firstChild()).setData(downcast<Text>(*fragment->firstChild()).data(), ec);
    1004         return;
    1005     }
    1006 
    1007     if (hasOneChild(containerNode)) {
    1008         containerNode->replaceChild(WTFMove(fragment), *containerNode->firstChild(), ec);
     1018    auto* containerChild = containerNode->firstChild();
     1019    if (containerChild && !containerChild->nextSibling()) {
     1020        if (is<Text>(*containerChild) && hasOneTextChild(fragment) && canUseSetDataOptimization(downcast<Text>(*containerChild), mutation)) {
     1021            ASSERT(!fragment->firstChild()->refCount());
     1022            downcast<Text>(*containerChild).setData(downcast<Text>(*fragment->firstChild()).data(), ec);
     1023            return;
     1024        }
     1025
     1026        containerNode->replaceChild(WTFMove(fragment), *containerChild, ec);
    10091027        return;
    10101028    }
Note: See TracChangeset for help on using the changeset viewer.