Changeset 201471 in webkit


Ignore:
Timestamp:
May 27, 2016 3:31:43 PM (8 years ago)
Author:
rniwa@webkit.org
Message:

Crash in TreeScope::focusedElement
​https://bugs.webkit.org/show_bug.cgi?id=158108

Reviewed by Enrica Casucci.

Source/WebCore:

The bug was caused by a flawed sequence of steps we took to remove an element. When an element is removed,
willRemoveChild and willRemoveChildren fire blur events on removed focused element and its ancestors and
unload event on any removed iframes. However, it was possible to focus an element on which we had fired blur
during an unload event, leaving m_focusedElement point to an element that's not in the document anymore.

Changing the order doesn't help because that would make it possible to insert the removed iframes back into
the document inside a event listener of the blur event, which was specifically fixed by r127534 four years ago.

Instead, fix the bug by not firing blur and change events on removed nodes. New behavior matches Firefox and HTML5
specification: ​https://html.spec.whatwg.org/multipage/interaction.html#focus-fixup-rule-one

Test: fast/shadow-dom/shadow-root-active-element-crash.html

  • dom/ContainerNode.cpp:

(WebCore::willRemoveChild): Made this function static local since it didn't need to have access to any private
member variables. Call Document::nodeWillBeRemoved after disconnecting iframes since unload event handler could
allocate new Ranges just like mutation events.
(WebCore::willRemoveChildren): Ditto.
(WebCore::ContainerNode::removeChild): Removed the calls to removeFullScreenElementOfSubtree and
removeFocusedNodeOfSubtree as they're now called in Document::nodeWillBeRemoved.
(WebCore::ContainerNode::removeChildren): Ditto.

  • dom/ContainerNode.h:
  • dom/Document.cpp:

(WebCore::Document::removeFocusedNodeOfSubtree): Don't dispatch blur and change events when a node is removed.
(WebCore::Document::setFocusedElement): Added FocusRemovalEventsMode as the third argument. Avoid dispatching blur
and change events when FocusRemovalEventsMode::Dispatch is set.
(WebCore::Document::nodeChildrenWillBeRemoved): Added calls to removeFullScreenElementOfSubtree and
removeFocusedNodeOfSubtree. Also assert that no events are fired within this function. If we ever fire an event here,
"unloaded" iframes can be inserted back into a document before ContainerNode::removeChild actually removes them.
(WebCore::Document::nodeWillBeRemoved): Ditto.

  • dom/Document.h:
  • dom/TreeScope.cpp:

(WebCore::TreeScope::focusedElement): Added a release assertion to make sure the focused element is in the document
of the tree scope, and added an explicit type check just in case.

LayoutTests:

Added a regression test for accessing shadowRoot.activeElement after re-focusing an element
inside DOMNodeRemovedFromDocument event and unload events.

This patch also restores the expected result of fast/events/onblur-remove.html to that of when
the test was in r15720 and updated in r19014. The expected result was changed in r85495 as it was
converted to a eventSender test.

  • fast/dom/Range/range-created-during-remove-children-expected.txt:
  • fast/dom/Range/range-created-during-remove-children.html: Update the test to use unload event

of an iframe since we no longer fire blur event when removing a focused element.

  • fast/dom/adopt-node-prevented-expected.txt:
  • fast/dom/adopt-node-prevented.html: Ditto.
  • fast/dom/remove-body-during-body-replacement2.html: Ditto. Use DOMNodeRemoved instead.
  • fast/events/nested-event-remove-node-crash.html: Ditto. Use DOMNodeRemovedFromDocument instead.
  • fast/events/onblur-remove-expected.txt:
  • fast/events/onblur-remove.html: See above.
  • fast/shadow-dom/shadow-root-active-element-crash-expected.txt: Added.
  • fast/shadow-dom/shadow-root-active-element-crash.html: Added.
Location:
trunk
Files:
2 added
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r201468 r201471  
     12016-05-26  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Crash in TreeScope::focusedElement
     4        https://bugs.webkit.org/show_bug.cgi?id=158108
     5
     6        Reviewed by Enrica Casucci.
     7
     8        Added a regression test for accessing shadowRoot.activeElement after re-focusing an element
     9        inside DOMNodeRemovedFromDocument event and unload events.
     10
     11        This patch also restores the expected result of fast/events/onblur-remove.html to that of when
     12        the test was in r15720 and updated in r19014. The expected result was changed in r85495 as it was
     13        converted to a eventSender test.
     14
     15        * fast/dom/Range/range-created-during-remove-children-expected.txt:
     16        * fast/dom/Range/range-created-during-remove-children.html: Update the test to use unload event
     17        of an iframe since we no longer fire blur event when removing a focused element.
     18        * fast/dom/adopt-node-prevented-expected.txt:
     19        * fast/dom/adopt-node-prevented.html: Ditto.
     20        * fast/dom/remove-body-during-body-replacement2.html: Ditto. Use DOMNodeRemoved instead.
     21        * fast/events/nested-event-remove-node-crash.html: Ditto. Use DOMNodeRemovedFromDocument instead.
     22        * fast/events/onblur-remove-expected.txt:
     23        * fast/events/onblur-remove.html: See above.
     24        * fast/shadow-dom/shadow-root-active-element-crash-expected.txt: Added.
     25        * fast/shadow-dom/shadow-root-active-element-crash.html: Added.
     26
    1272016-05-27  Brent Fulgham  <bfulgham@apple.com>
    228

  • trunk/LayoutTests/fast/dom/Range/range-created-during-remove-children-expected.txt

    r158739 r201471  
    1 PASS ranges["blur"].startContainer is sample
    2 PASS ranges["blur"].endContainer is sample
    3 PASS ranges["blur"].startOffset is 0
    4 PASS ranges["blur"].endOffset is 0
     1PASS ranges["unload"].startContainer is sample
     2PASS ranges["unload"].endContainer is sample
     3PASS ranges["unload"].startOffset is 0
     4PASS ranges["unload"].endOffset is 0
    55PASS ranges["DOMNodeRemovedFromDocument"].startContainer is sample
    66PASS ranges["DOMNodeRemovedFromDocument"].endContainer is sample

  • trunk/LayoutTests/fast/dom/Range/range-created-during-remove-children.html

    r158739 r201471  
    11<div id="container">
    22<p id="description"></p>
    3 <div id="sample"><span contenteditable="true">foobar</span></div>
     3<div id="sample"><span contenteditable="true">foobar<iframe id="target"></iframe></span></div>
    44</div>
    55<div id="console"></div>
    … …  
    1717}
    1818
    19 document.addEventListener('blur', eventHandler, true);
     19document.querySelector('iframe').contentWindow.addEventListener('unload', eventHandler, true);
    2020document.addEventListener('DOMNodeRemovedFromDocument', eventHandler, true);
    2121
    … …  
    2323sample.innerHTML = '';
    2424
    25 shouldBe('ranges["blur"].startContainer', 'sample');
    26 shouldBe('ranges["blur"].endContainer', 'sample');
    27 shouldBe('ranges["blur"].startOffset', '0');
    28 shouldBe('ranges["blur"].endOffset', '0');
     25shouldBe('ranges["unload"].startContainer', 'sample');
     26shouldBe('ranges["unload"].endContainer', 'sample');
     27shouldBe('ranges["unload"].startOffset', '0');
     28shouldBe('ranges["unload"].endOffset', '0');
    2929shouldBe('ranges["DOMNodeRemovedFromDocument"].startContainer', 'sample');
    3030shouldBe('ranges["DOMNodeRemovedFromDocument"].endContainer', 'sample');

  • trunk/LayoutTests/fast/dom/adopt-node-prevented-expected.txt

    r129469 r201471  
    99PASS target.ownerDocument.location is document.location
    1010
     11

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

    r155265 r201471  
    66<body>
    77  <div id="newParent"></div>
    8   <a href="#" id="target"></a>
     8  <iframe href="#" id="target"></iframe>
    99<script>
    1010description("Test that adoptNode fails safely if prevented by a DOM mutation.");
    … …  
    1313    newParent = document.getElementById("newParent");
    1414    target = document.getElementById("target");
    15     target.addEventListener("blur", function () { newParent.appendChild(target); }, false);
     15    target.contentWindow.addEventListener("unload", function () { newParent.appendChild(target); }, false);
    1616    target.focus();
    1717    var anotherDocument = document.implementation.createDocument("", "", null);

  • trunk/LayoutTests/fast/dom/remove-body-during-body-replacement2.html

    r120792 r201471  
    1818
    1919    setTimeout(function () {
    20         document.addEventListener('DOMFocusOut', function () { crash(); }, true);
     20        document.addEventListener('DOMNodeRemoved', function () { crash(); }, true);
    2121        document.addEventListener('DOMSubtreeModified', function () { /* noop */ }, false);
    2222        document.designMode = "on";

  • trunk/LayoutTests/fast/events/nested-event-remove-node-crash.html

    r120792 r201471  
    3535    4. the onblur event handler will send off an XHR who's handler will remove the node
    3636*/
    37     document.getElementById("theSelect").focus();
     37    var select = document.getElementById("theSelect");
     38    select.addEventListener('DOMNodeRemovedFromDocument', function () { sendXHR();GC(); });
     39    select.focus();
    3840    sendXHR();
    3941   

  • trunk/LayoutTests/fast/events/onblur-remove-expected.txt

    r85495 r201471  
    1 This tests that elements shouldn't emit any onblur events when they are being removed from the document.
    2 Note, this test is expected to fail as of 04/25/2011. See bug #59379.
     1This tests that elements shouldn't emit any onblur events when they are being removed from the document.
    32
    43On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
    54
    65
    7 FAIL Onblur handler called.
     6PASS blurEventCount is 0
    87
    98TEST COMPLETE

  • trunk/LayoutTests/fast/events/onblur-remove.html

    r155267 r201471  
    66            testRunner.waitUntilDone();
    77
    8         var numBlurs = 0;
     8        var blurEventCount = 0;
    99
    1010        window.onload = function() { document.getElementById("input").focus(); }
    … …  
    1515            f.innerHTML = '';
    1616
    17             if (numBlurs)
    18                 testFailed('Onblur handler called.');
    19             else
    20                 testPassed('Onblur handler not called.');
     17            shouldBe("blurEventCount", "0");
    2118
    2219            debug('<br /><span class="pass">TEST COMPLETE</span>');
    … …  
    2926    <p id="description"></p>
    3027    <form id='f'>
    31       <input id="input" onblur="numBlurs++" onfocus="setTimeout('finish()', 0)">
     28      <input id="input" onblur="blurEventCount++" onfocus="setTimeout('finish()', 0)">
    3229    </form>
    3330    <div id="console"></div>
    3431    <script>
    35         description("This tests that elements shouldn't emit any onblur events when they are being removed from the document. <br>" +
    36                     "Note, this test is expected to fail as of 04/25/2011. See bug #59379.");
     32        description("This tests that elements shouldn't emit any onblur events when they are being removed from the document.");
    3733    </script>
    3834  </body>

  • trunk/Source/WebCore/ChangeLog

    r201468 r201471  
     12016-05-26  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Crash in TreeScope::focusedElement
     4        https://bugs.webkit.org/show_bug.cgi?id=158108
     5
     6        Reviewed by Enrica Casucci.
     7
     8        The bug was caused by a flawed sequence of steps we took to remove an element. When an element is removed,
     9        willRemoveChild and willRemoveChildren fire blur events on removed focused element and its ancestors and
     10        unload event on any removed iframes. However, it was possible to focus an element on which we had fired blur
     11        during an unload event, leaving m_focusedElement point to an element that's not in the document anymore.
     12
     13        Changing the order doesn't help because that would make it possible to insert the removed iframes back into
     14        the document inside a event listener of the blur event, which was specifically fixed by r127534 four years ago.
     15
     16        Instead, fix the bug by not firing blur and change events on removed nodes. New behavior matches Firefox and HTML5
     17        specification: https://html.spec.whatwg.org/multipage/interaction.html#focus-fixup-rule-one
     18
     19        Test: fast/shadow-dom/shadow-root-active-element-crash.html
     20
     21        * dom/ContainerNode.cpp:
     22        (WebCore::willRemoveChild): Made this function static local since it didn't need to have access to any private
     23        member variables. Call Document::nodeWillBeRemoved after disconnecting iframes since unload event handler could
     24        allocate new Ranges just like mutation events.
     25        (WebCore::willRemoveChildren): Ditto.
     26        (WebCore::ContainerNode::removeChild): Removed the calls to removeFullScreenElementOfSubtree and
     27        removeFocusedNodeOfSubtree as they're now called in Document::nodeWillBeRemoved.
     28        (WebCore::ContainerNode::removeChildren): Ditto.
     29        * dom/ContainerNode.h:
     30        * dom/Document.cpp:
     31        (WebCore::Document::removeFocusedNodeOfSubtree): Don't dispatch blur and change events when a node is removed.
     32        (WebCore::Document::setFocusedElement): Added FocusRemovalEventsMode as the third argument. Avoid dispatching blur
     33        and change events when FocusRemovalEventsMode::Dispatch is set.
     34        (WebCore::Document::nodeChildrenWillBeRemoved): Added calls to removeFullScreenElementOfSubtree and
     35        removeFocusedNodeOfSubtree. Also assert that no events are fired within this function. If we ever fire an event here,
     36        "unloaded" iframes can be inserted back into a document before ContainerNode::removeChild actually removes them.
     37        (WebCore::Document::nodeWillBeRemoved): Ditto.
     38        * dom/Document.h:
     39        * dom/TreeScope.cpp:
     40        (WebCore::TreeScope::focusedElement): Added a release assertion to make sure the focused element is in the document
     41        of the tree scope, and added an explicit type check just in case.
     42
    1432016-05-27  Brent Fulgham  <bfulgham@apple.com>
    244

  • trunk/Source/WebCore/dom/ContainerNode.cpp

    r201416 r201471  
    457457}
    458458
    459 void ContainerNode::willRemoveChild(Node& child)
     459static void willRemoveChild(ContainerNode& container, Node& child)
    460460{
    461461    ASSERT(child.parentNode());
    … …  
    465465    dispatchChildRemovalEvents(child);
    466466
    467     if (child.parentNode() != this)
     467    if (child.parentNode() != &container)
    468468        return;
    469469
    470     child.document().nodeWillBeRemoved(child); // e.g. mutation event listener can create a new range.
    471470    if (is<ContainerNode>(child))
    472471        disconnectSubframesIfNeeded(downcast<ContainerNode>(child), RootAndDescendants);
     472
     473    if (child.parentNode() != &container)
     474        return;
     475
     476    child.document().nodeWillBeRemoved(child); // e.g. mutation event listener can create a new range.
    473477}
    474478
    … …  
    487491    }
    488492
     493    disconnectSubframesIfNeeded(container, DescendantsOnly);
     494
    489495    container.document().nodeChildrenWillBeRemoved(container);
    490496
    491     disconnectSubframesIfNeeded(container, DescendantsOnly);
    492497}
    493498
    … …  
    514519
    515520    Ref<Node> child(oldChild);
    516 
    517     document().removeFocusedNodeOfSubtree(child.ptr());
    518 
    519 #if ENABLE(FULLSCREEN_API)
    520     document().removeFullScreenElementOfSubtree(&child.get());
    521 #endif
    522521
    523522    // Events fired when blurring currently focused node might have moved this
    … …  
    528527    }
    529528
    530     willRemoveChild(child);
     529    willRemoveChild(*this, child);
    531530
    532531    // Mutation events might have moved this child into a different parent.
    … …  
    611610    // The container node can be removed from event handlers.
    612611    Ref<ContainerNode> protectedThis(*this);
    613 
    614     // exclude this node when looking for removed focusedNode since only children will be removed
    615     document().removeFocusedNodeOfSubtree(this, true);
    616 
    617 #if ENABLE(FULLSCREEN_API)
    618     document().removeFullScreenElementOfSubtree(this, true);
    619 #endif
    620612
    621613    // Do any prep work needed before actually starting to detach

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

    r200696 r201471  
    131131    bool isContainerNode() const = delete;
    132132
    133     void willRemoveChild(Node& child);
    134 
    135133    Node* m_firstChild { nullptr };
    136134    Node* m_lastChild { nullptr };

  • trunk/Source/WebCore/dom/Document.cpp

    r201441 r201471  
    37013701   
    37023702    if (nodeInSubtree)
    3703         setFocusedElement(nullptr);
     3703        setFocusedElement(nullptr, FocusDirectionNone, FocusRemovalEventsMode::DoNotDispatch);
    37043704}
    37053705
    … …  
    37393739#endif
    37403740
    3741 bool Document::setFocusedElement(Element* element, FocusDirection direction)
     3741bool Document::setFocusedElement(Element* element, FocusDirection direction, FocusRemovalEventsMode eventsMode)
    37423742{
    37433743    RefPtr<Element> newFocusedElement = element;
    … …  
    37623762        oldFocusedElement->setFocus(false);
    37633763
    3764         // Dispatch a change event for form control elements that have been edited.
    3765         if (is<HTMLFormControlElement>(*oldFocusedElement)) {
    3766             HTMLFormControlElement& formControlElement = downcast<HTMLFormControlElement>(*oldFocusedElement);
    3767             if (formControlElement.wasChangedSinceLastFormControlChangeEvent())
    3768                 formControlElement.dispatchFormControlChangeEvent();
    3769         }
    3770 
    3771         // Dispatch the blur event and let the node do any other blur related activities (important for text fields)
    3772         oldFocusedElement->dispatchBlurEvent(newFocusedElement.copyRef());
    3773 
    3774         if (m_focusedElement) {
    3775             // handler shifted focus
    3776             focusChangeBlocked = true;
    3777             newFocusedElement = nullptr;
    3778         }
    3779        
    3780         oldFocusedElement->dispatchFocusOutEvent(eventNames().focusoutEvent, newFocusedElement.copyRef()); // DOM level 3 name for the bubbling blur event.
    3781         // FIXME: We should remove firing DOMFocusOutEvent event when we are sure no content depends
    3782         // on it, probably when <rdar://problem/8503958> is resolved.
    3783         oldFocusedElement->dispatchFocusOutEvent(eventNames().DOMFocusOutEvent, newFocusedElement.copyRef()); // DOM level 2 name for compatibility.
    3784 
    3785         if (m_focusedElement) {
    3786             // handler shifted focus
    3787             focusChangeBlocked = true;
    3788             newFocusedElement = nullptr;
    3789         }
    3790            
     3764        if (eventsMode == FocusRemovalEventsMode::Dispatch) {
     3765            // Dispatch a change event for form control elements that have been edited.
     3766            if (is<HTMLFormControlElement>(*oldFocusedElement)) {
     3767                HTMLFormControlElement& formControlElement = downcast<HTMLFormControlElement>(*oldFocusedElement);
     3768                if (formControlElement.wasChangedSinceLastFormControlChangeEvent())
     3769                    formControlElement.dispatchFormControlChangeEvent();
     3770            }
     3771
     3772            // Dispatch the blur event and let the node do any other blur related activities (important for text fields)
     3773            oldFocusedElement->dispatchBlurEvent(newFocusedElement.copyRef());
     3774
     3775            if (m_focusedElement) {
     3776                // handler shifted focus
     3777                focusChangeBlocked = true;
     3778                newFocusedElement = nullptr;
     3779            }
     3780
     3781            oldFocusedElement->dispatchFocusOutEvent(eventNames().focusoutEvent, newFocusedElement.copyRef()); // DOM level 3 name for the bubbling blur event.
     3782            // FIXME: We should remove firing DOMFocusOutEvent event when we are sure no content depends
     3783            // on it, probably when <rdar://problem/8503958> is resolved.
     3784            oldFocusedElement->dispatchFocusOutEvent(eventNames().DOMFocusOutEvent, newFocusedElement.copyRef()); // DOM level 2 name for compatibility.
     3785
     3786            if (m_focusedElement) {
     3787                // handler shifted focus
     3788                focusChangeBlocked = true;
     3789                newFocusedElement = nullptr;
     3790            }
     3791        } else
     3792            ASSERT(!m_focusedElement);
     3793
    37913794        if (oldFocusedElement->isRootEditableElement())
    37923795            frame()->editor().didEndEditing();
    … …  
    39683971void Document::nodeChildrenWillBeRemoved(ContainerNode& container)
    39693972{
     3973    NoEventDispatchAssertion assertNoEventDispatch;
     3974
     3975    removeFocusedNodeOfSubtree(&container, true /* amongChildrenOnly */);
     3976
     3977#if ENABLE(FULLSCREEN_API)
     3978    removeFullScreenElementOfSubtree(&container, true /* amongChildrenOnly */);
     3979#endif
     3980
    39703981    for (auto* range : m_ranges)
    39713982        range->nodeChildrenWillBeRemoved(container);
    … …  
    39924003void Document::nodeWillBeRemoved(Node& n)
    39934004{
     4005    NoEventDispatchAssertion assertNoEventDispatch;
     4006
     4007    removeFocusedNodeOfSubtree(&n);
     4008
     4009#if ENABLE(FULLSCREEN_API)
     4010    removeFullScreenElementOfSubtree(&n);
     4011#endif
     4012
    39944013    for (auto* it : m_nodeIterators)
    39954014        it->nodeWillBeRemoved(n);

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

    r201104 r201471  
    723723    void setSelectedStylesheetSet(const String&);
    724724
    725     WEBCORE_EXPORT bool setFocusedElement(Element*, FocusDirection = FocusDirectionNone);
     725    enum class FocusRemovalEventsMode { Dispatch, DoNotDispatch };
     726    WEBCORE_EXPORT bool setFocusedElement(Element*, FocusDirection = FocusDirectionNone,
     727        FocusRemovalEventsMode = FocusRemovalEventsMode::Dispatch);
    726728    Element* focusedElement() const { return m_focusedElement.get(); }
    727729    UserActionElementSet& userActionElements()  { return m_userActionElements; }

  • trunk/Source/WebCore/dom/TreeScope.cpp

    r196223 r201471  
    312312        return nullptr;
    313313    TreeScope* treeScope = &element->treeScope();
     314    RELEASE_ASSERT(&document == &treeScope->documentScope());
    314315    while (treeScope != this && treeScope != &document) {
    315         element = downcast<ShadowRoot>(treeScope->rootNode()).host();
     316        auto& rootNode = treeScope->rootNode();
     317        if (is<ShadowRoot>(rootNode))
     318            element = downcast<ShadowRoot>(rootNode).host();
     319        else
     320            return nullptr;
    316321        treeScope = &element->treeScope();
    317322    }
Note: See TracChangeset for help on using the changeset viewer.