Changeset 224159 in webkit


Ignore:
Timestamp:
Oct 29, 2017 3:15:55 AM (6 years ago)
Author:
rniwa@webkit.org
Message:

Assert that no script is executed during style recalc
​https://bugs.webkit.org/show_bug.cgi?id=178845
<rdar://problem/35106129>

Reviewed by Antti Koivisto.

This patch adds NoEventDispatchAssertion to Document::updateStyle and Document::updateStyleIfNeeded
to make sure we don't start mutating DOM in the middle of a style update.

Added NoEventDispatchAssertion::EventAllowedScope for various places in SVGUseElement to update its
shadow tree since that happens while updating the style.

No new tests since there should be no behavioral change.

  • dom/Document.cpp:

(WebCore::Document::resolveStyle): Added NoEventDispatchAssertion while flushing pending stylesheets
and calling FrameView::willRecalcStyle, and while the style tree solver is in works. Also moved in
the code to update the selection and schedule to dispatch a fake mouse event into the same scope.
Also increment m_styleRecalcCount in the same code since post resolution callbacks could run author
scripts which in turn trigger another (recursive) style recalc.
(WebCore::Document::updateStyleIfNeeded): Put everything but the call to resolveStyle in a scope with
NoEventDispatchAssertion.

  • dom/Element.cpp:

(WebCore::Element::cloneElementWithChildren): Added NoEventDispatchAssertion::EventAllowedScope to the
newly cloned element for SVG use element's shadow tree.
(WebCore::Element::cloneElementWithoutChildren): Ditto.

  • dom/EventDispatcher.cpp:

(WebCore::EventDispatcher::dispatchEvent): Make the assertion more precise to workaround the fact SVG
use elements update its shadow tree in the middle of style updates. Also removed a redundant assertion
since the result of NoEventDispatchAssertion::isEventDispatchAllowedInSubtree cannot chance without
pushing or popoing the stack frame.

  • svg/SVGUseElement.cpp:

(WebCore::SVGUseElement::clearShadowTree):
(WebCore::SVGUseElement::updateShadowTree): Added NoEventDispatchAssertion to the user-agent shadow root
of a SVG use element. Since this is a newly created shadow tree which hasn't been exposed to author
scripts, it's safe to mutate them during the style recalc even though it's not the best design.
(WebCore::SVGUseElement::cloneTarget const): Ditto.
(WebCore::SVGUseElement::expandUseElementsInShadowTree const): Ditto.
(WebCore::SVGUseElement::expandSymbolElementsInShadowTree const): Ditto.
(WebCore::SVGUseElement::transferEventListenersToShadowTree const):

Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r224158 r224159  
     12017-10-29  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Assert that no script is executed during style recalc
     4        https://bugs.webkit.org/show_bug.cgi?id=178845
     5        <rdar://problem/35106129>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        This patch adds NoEventDispatchAssertion to Document::updateStyle and Document::updateStyleIfNeeded
     10        to make sure we don't start mutating DOM in the middle of a style update.
     11
     12        Added NoEventDispatchAssertion::EventAllowedScope for various places in SVGUseElement to update its
     13        shadow tree since that happens while updating the style.
     14
     15        No new tests since there should be no behavioral change.
     16
     17        * dom/Document.cpp:
     18        (WebCore::Document::resolveStyle): Added NoEventDispatchAssertion while flushing pending stylesheets
     19        and calling FrameView::willRecalcStyle, and while the style tree solver is in works. Also moved in
     20        the code to update the selection and schedule to dispatch a fake mouse event into the same scope.
     21        Also increment m_styleRecalcCount in the same code since post resolution callbacks could run author
     22        scripts which in turn trigger another (recursive) style recalc.
     23        (WebCore::Document::updateStyleIfNeeded): Put everything but the call to resolveStyle in a scope with
     24        NoEventDispatchAssertion.
     25        * dom/Element.cpp:
     26        (WebCore::Element::cloneElementWithChildren): Added NoEventDispatchAssertion::EventAllowedScope to the
     27        newly cloned element for SVG use element's shadow tree.
     28        (WebCore::Element::cloneElementWithoutChildren): Ditto.
     29        * dom/EventDispatcher.cpp:
     30        (WebCore::EventDispatcher::dispatchEvent): Make the assertion more precise to workaround the fact SVG
     31        use elements update its shadow tree in the middle of style updates. Also removed a redundant assertion
     32        since the result of NoEventDispatchAssertion::isEventDispatchAllowedInSubtree cannot chance without
     33        pushing or popoing the stack frame.
     34        * svg/SVGUseElement.cpp:
     35        (WebCore::SVGUseElement::clearShadowTree):
     36        (WebCore::SVGUseElement::updateShadowTree): Added NoEventDispatchAssertion to the user-agent shadow root
     37        of a SVG use element. Since this is a newly created shadow tree which hasn't been exposed to author
     38        scripts, it's safe to mutate them during the style recalc even though it's not the best design.
     39        (WebCore::SVGUseElement::cloneTarget const): Ditto.
     40        (WebCore::SVGUseElement::expandUseElementsInShadowTree const): Ditto.
     41        (WebCore::SVGUseElement::expandSymbolElementsInShadowTree const): Ditto.
     42        (WebCore::SVGUseElement::transferEventListenersToShadowTree const):
     43
    1442017-10-28  Dean Jackson  <dino@apple.com>
    245

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

    r224150 r224159  
    17931793    // hits a null-dereference due to security code always assuming the document has a SecurityOrigin.
    17941794
    1795     styleScope().flushPendingUpdate();
    1796 
    1797     frameView.willRecalcStyle();
     1795    {
     1796        NoEventDispatchAssertion noEventDispatchAssertion;
     1797        styleScope().flushPendingUpdate();
     1798        frameView.willRecalcStyle();
     1799    }
    17981800
    17991801    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willRecalculateStyle(*this);
    18001802
    1801     m_inStyleRecalc = true;
    18021803    bool updatedCompositingLayers = false;
    18031804    {
    18041805        Style::PostResolutionCallbackDisabler disabler(*this);
    18051806        WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
     1807        NoEventDispatchAssertion noEventDispatchAssertion;
     1808
     1809        m_inStyleRecalc = true;
    18061810
    18071811        if (m_pendingStyleRecalcShouldForce)
    … …  
    18521856        if (m_renderView->needsLayout())
    18531857            frameView.layoutContext().scheduleLayout();
     1858
     1859        // Usually this is handled by post-layout.
     1860        if (!frameView.needsLayout())
     1861            frameView.frame().selection().scheduleAppearanceUpdateAfterStyleChange();
     1862
     1863        // As a result of the style recalculation, the currently hovered element might have been
     1864        // detached (for example, by setting display:none in the :hover style), schedule another mouseMove event
     1865        // to check if any other elements ended up under the mouse pointer due to re-layout.
     1866        if (m_hoveredElement && !m_hoveredElement->renderer())
     1867            frameView.frame().mainFrame().eventHandler().dispatchFakeMouseMoveEventSoon();
     1868
     1869        ++m_styleRecalcCount;
     1870        // FIXME: Assert ASSERT(!needsStyleRecalc()) here. Do we still have some cases where it's not true?
    18541871    }
    18551872
    … …  
    18591876        implicitClose();
    18601877    }
    1861    
    1862     ++m_styleRecalcCount;
    18631878
    18641879    InspectorInstrumentation::didRecalculateStyle(cookie);
    … …  
    18701885        frameView.viewportContentsChanged();
    18711886
    1872     // Usually this is handled by post-layout.
    1873     if (!frameView.needsLayout())
    1874         frameView.frame().selection().scheduleAppearanceUpdateAfterStyleChange();
    1875 
    1876     // As a result of the style recalculation, the currently hovered element might have been
    1877     // detached (for example, by setting display:none in the :hover style), schedule another mouseMove event
    1878     // to check if any other elements ended up under the mouse pointer due to re-layout.
    1879     if (m_hoveredElement && !m_hoveredElement->renderer())
    1880         frameView.frame().mainFrame().eventHandler().dispatchFakeMouseMoveEventSoon();
    1881 
    18821887    if (m_gotoAnchorNeededAfterStylesheetsLoad && !styleScope().hasPendingSheets())
    18831888        frameView.scrollToFragment(m_url);
    1884 
    1885     // FIXME: Ideally we would ASSERT(!needsStyleRecalc()) here but we have some cases where it is not true.
    18861889}
    18871890
    … …  
    19211924bool Document::updateStyleIfNeeded()
    19221925{
    1923     ASSERT(isMainThread());
    1924     ASSERT(!view() || !view()->isPainting());
    1925 
    1926     if (!view() || view()->layoutContext().isInRenderTreeLayout())
    1927         return false;
    1928 
    1929     styleScope().flushPendingUpdate();
    1930 
    1931     if (!needsStyleRecalc())
    1932         return false;
     1926    {
     1927        NoEventDispatchAssertion noEventDispatchAssertion;
     1928        ASSERT(isMainThread());
     1929        ASSERT(!view() || !view()->isPainting());
     1930
     1931        if (!view() || view()->layoutContext().isInRenderTreeLayout())
     1932            return false;
     1933
     1934        styleScope().flushPendingUpdate();
     1935
     1936        if (!needsStyleRecalc())
     1937            return false;
     1938    }
    19331939
    19341940    resolveStyle();

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

    r224150 r224159  
    342342{
    343343    Ref<Element> clone = cloneElementWithoutChildren(targetDocument);
     344
     345    // It's safe to dispatch events on the cloned node since author scripts have no access to it yet.
     346    // This is needed for SVGUseElement::cloneTarget.
     347    NoEventDispatchAssertion::EventAllowedScope allowedScope(clone.get());
     348
    344349    cloneChildNodes(clone);
    345350    return clone;
    … …  
    349354{
    350355    Ref<Element> clone = cloneElementWithoutAttributesAndChildren(targetDocument);
     356
     357    // It's safe to dispatch events on the cloned node since author scripts have no access to it yet.
     358    // This is needed for SVGUseElement::cloneTarget.
     359    NoEventDispatchAssertion::EventAllowedScope allowedScope(clone.get());
     360
    351361    // This will catch HTML elements in the wrong namespace that are not correctly copied.
    352362    // This is a sanity check as HTML overloads some of the DOM methods.

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

    r224131 r224159  
    131131bool EventDispatcher::dispatchEvent(Node& node, Event& event)
    132132{
    133     ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
     133    ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventDispatchAllowedInSubtree(node));
    134134    Ref<Node> protectedNode(node);
    135135    RefPtr<FrameView> view = node.document().view();
    … …  
    149149    if (!event.target())
    150150        return true;
    151 
    152     ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
    153151
    154152    InputElementClickState clickHandlingState;

  • trunk/Source/WebCore/svg/SVGUseElement.cpp

    r224131 r224159  
    217217{
    218218    if (auto root = userAgentShadowRoot()) {
     219        // Safe because SVG use element's shadow tree is never used to fire synchronous events during layout or DOM mutations.
    219220        NoEventDispatchAssertion::EventAllowedScope scope(*root);
    220221        root->removeChildren();
    … …  
    244245    }
    245246
    246     cloneTarget(ensureUserAgentShadowRoot(), *target);
    247     expandUseElementsInShadowTree();
    248     expandSymbolElementsInShadowTree();
     247    {
     248        // Safe because the cloned shadow tree has never been exposed to author scripts.
     249        auto& shadowRoot = ensureUserAgentShadowRoot();
     250        NoEventDispatchAssertion::EventAllowedScope scope(shadowRoot);
     251        cloneTarget(shadowRoot, *target);
     252        expandUseElementsInShadowTree();
     253        expandSymbolElementsInShadowTree();
     254        updateRelativeLengthsInformation();
     255    }
     256
    249257    transferEventListenersToShadowTree();
    250 
    251     updateRelativeLengthsInformation();
    252258
    253259    // When we invalidate the other shadow trees, it's important that we don't
    … …  
    431437{
    432438    Ref<SVGElement> targetClone = static_cast<SVGElement&>(target.cloneElementWithChildren(document()).get());
     439    // Safe because the newy cloned nodes in the shadow tree has not been exposed to author scripts yet.
     440    NoEventDispatchAssertion::EventAllowedScope scope(targetClone);
    433441    associateClonesWithOriginals(targetClone.get(), target);
    434442    removeDisallowedElementsFromSubtree(targetClone.get());
    … …  
    463471
    464472        auto replacementClone = SVGGElement::create(document());
     473        // Safe because the use element's shadow tree is not exposed to author scripts, and we don't fire synchronous events during layout & DOM layout.
     474        NoEventDispatchAssertion::EventAllowedScope scope(replacementClone);
     475
    465476        cloneDataAndChildren(replacementClone.get(), originalClone);
    466477
    … …  
    496507
    497508        auto replacementClone = SVGSVGElement::create(document());
     509        // Safe because the newly created SVG element and the newly created shadow tree has not been exposed to author scripts yet.
     510        NoEventDispatchAssertion::EventAllowedScope scope(replacementClone);
    498511        cloneDataAndChildren(replacementClone.get(), originalClone);
    499512
    … …  
    507520void SVGUseElement::transferEventListenersToShadowTree() const
    508521{
     522    // FIXME: Don't directly add event listeners on each descendant. Copy event listeners on the use element instead.
    509523    for (auto& descendant : descendantsOfType<SVGElement>(*userAgentShadowRoot())) {
    510524        if (EventTargetData* data = descendant.correspondingElement()->eventTargetData())
Note: See TracChangeset for help on using the changeset viewer.