Changeset 224378 in webkit


Ignore:
Timestamp:
Nov 2, 2017 8:48:11 PM (7 years ago)
Author:
rniwa@webkit.org
Message:

Assert that updateStyle and updateLayout are only called when it's safe to dispatch events
​https://bugs.webkit.org/show_bug.cgi?id=179157
<rdar://problem/35144778>

Reviewed by Zalan Bujtas.

Added assertions to Document::updateStyleIfNeeded and Document::updateLayout that these functions are
only called when NoEventDispatchAssertion::isEventAllowedInMainThread() is true with two exceptions:

  1. Inside SVGImage::draw which triggers a layout on a separate document.
  2. While doing a nested layout for a frame flattening.

No new tests since there should be no behavioral changes.

  • dom/ContainerNode.cpp:

(NoEventDispatchAssertion::DisableAssertionsInScope::s_existingCount): Deleted. This is now an instance
variable of DisableAssertionsInScope.
(ContainerNode::removeNodeWithScriptAssertion): Moved childrenChanged out of the scope since it could
invoke respondToChangedSelection via HTMLTextAreaElement::childrenChanged.

  • dom/Document.cpp:

(WebCore::Document::updateStyleIfNeeded): Added the assertion. Allow updateWidgetPositions() to call
this function but exit early when checking needsStyleRecalc().
(WebCore::Document::updateLayout): Added the assertion.

  • dom/NoEventDispatchAssertion.h:

(WebCore::NoEventDispatchAssertion::DisableAssertionsInScope::DisableAssertionsInScope): Made this class
store the original value of s_count as an instance variable to support re-entrancy.
(WebCore::NoEventDispatchAssertion::DisableAssertionsInScope::~DisableAssertionsInScope): Ditto.

  • page/LayoutContext.cpp:

(WebCore::LayoutContext::runOrScheduleAsynchronousTasks): Temporarily disable the assertion. This is safe
since SVGImage has its own document.

  • svg/SVGSVGElement.cpp:

(WebCore::checkIntersectionWithoutUpdatingLayout): Extracted out of SVGSVGElement::checkIntersection.
(WebCore::checkEnclosureWithoutUpdatingLayout): Extracted out of SVGSVGElement::checkEnclosure.
(WebCore::SVGSVGElement::getIntersectionList): Use checkIntersectionWithoutUpdatingLayout to avoid
calling updateLayoutIgnorePendingStylesheets while iterating over elements.
(WebCore::SVGSVGElement::getEnclosureList): Ditto.
(WebCore::SVGSVGElement::checkIntersection):
(WebCore::SVGSVGElement::checkEnclosure):

  • svg/graphics/SVGImage.cpp:

(WebCore::SVGImage::draw): Temporarily disable the assertion. This is safe as SVGImage has its own page.

Location:
trunk/Source/WebCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r224374 r224378  
     12017-11-01  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Assert that updateStyle and updateLayout are only called when it's safe to dispatch events
     4        https://bugs.webkit.org/show_bug.cgi?id=179157
     5        <rdar://problem/35144778>
     6
     7        Reviewed by Zalan Bujtas.
     8
     9        Added assertions to Document::updateStyleIfNeeded and Document::updateLayout that these functions are
     10        only called when NoEventDispatchAssertion::isEventAllowedInMainThread() is true with two exceptions:
     11        1. Inside SVGImage::draw which triggers a layout on a separate document.
     12        2. While doing a nested layout for a frame flattening.
     13
     14        No new tests since there should be no behavioral changes.
     15
     16        * dom/ContainerNode.cpp:
     17        (NoEventDispatchAssertion::DisableAssertionsInScope::s_existingCount): Deleted. This is now an instance
     18        variable of DisableAssertionsInScope.
     19        (ContainerNode::removeNodeWithScriptAssertion): Moved childrenChanged out of the scope since it could
     20        invoke respondToChangedSelection via HTMLTextAreaElement::childrenChanged.
     21        * dom/Document.cpp:
     22        (WebCore::Document::updateStyleIfNeeded): Added the assertion. Allow updateWidgetPositions() to call
     23        this function but exit early when checking needsStyleRecalc().
     24        (WebCore::Document::updateLayout): Added the assertion.
     25        * dom/NoEventDispatchAssertion.h:
     26        (WebCore::NoEventDispatchAssertion::DisableAssertionsInScope::DisableAssertionsInScope): Made this class
     27        store the original value of s_count as an instance variable to support re-entrancy.
     28        (WebCore::NoEventDispatchAssertion::DisableAssertionsInScope::~DisableAssertionsInScope): Ditto.
     29        * page/LayoutContext.cpp:
     30        (WebCore::LayoutContext::runOrScheduleAsynchronousTasks): Temporarily disable the assertion. This is safe
     31        since SVGImage has its own document.
     32        * svg/SVGSVGElement.cpp:
     33        (WebCore::checkIntersectionWithoutUpdatingLayout): Extracted out of SVGSVGElement::checkIntersection.
     34        (WebCore::checkEnclosureWithoutUpdatingLayout): Extracted out of SVGSVGElement::checkEnclosure.
     35        (WebCore::SVGSVGElement::getIntersectionList): Use checkIntersectionWithoutUpdatingLayout to avoid
     36        calling updateLayoutIgnorePendingStylesheets while iterating over elements.
     37        (WebCore::SVGSVGElement::getEnclosureList): Ditto.
     38        (WebCore::SVGSVGElement::checkIntersection):
     39        (WebCore::SVGSVGElement::checkEnclosure):
     40        * svg/graphics/SVGImage.cpp:
     41        (WebCore::SVGImage::draw): Temporarily disable the assertion. This is safe as SVGImage has its own page.
     42
    1432017-11-02  Alex Christensen  <achristensen@webkit.org>
    244

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

    r224356 r224378  
    7373#if !ASSERT_DISABLED
    7474unsigned NoEventDispatchAssertion::s_count = 0;
    75 unsigned NoEventDispatchAssertion::DisableAssertionsInScope::s_existingCount = 0;
    7675NoEventDispatchAssertion::EventAllowedScope* NoEventDispatchAssertion::EventAllowedScope::s_currentScope = nullptr;
    7776#endif
    … …  
    144143        return false;
    145144
    146     WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
    147     NoEventDispatchAssertion::InMainThread assertNoEventDispatch;
    148 
    149     document().nodeWillBeRemoved(childToRemove);
    150 
    151     ASSERT_WITH_SECURITY_IMPLICATION(childToRemove.parentNode() == this);
    152     ASSERT(!childToRemove.isDocumentFragment());
    153 
    154     RefPtr<Node> previousSibling = childToRemove.previousSibling();
    155     RefPtr<Node> nextSibling = childToRemove.nextSibling();
    156     removeBetween(previousSibling.get(), nextSibling.get(), childToRemove);
    157     notifyChildNodeRemoved(*this, childToRemove);
    158 
    159145    ChildChange change;
    160     change.type = is<Element>(childToRemove) ? ElementRemoved : (is<Text>(childToRemove) ? TextRemoved : NonContentsChildRemoved);
    161     change.previousSiblingElement = (!previousSibling || is<Element>(*previousSibling)) ? downcast<Element>(previousSibling.get()) : ElementTraversal::previousSibling(*previousSibling);
    162     change.nextSiblingElement = (!nextSibling || is<Element>(*nextSibling)) ? downcast<Element>(nextSibling.get()) : ElementTraversal::nextSibling(*nextSibling);
    163     change.source = source;
     146    {
     147        WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
     148        NoEventDispatchAssertion::InMainThread assertNoEventDispatch;
     149
     150        document().nodeWillBeRemoved(childToRemove);
     151
     152        ASSERT_WITH_SECURITY_IMPLICATION(childToRemove.parentNode() == this);
     153        ASSERT(!childToRemove.isDocumentFragment());
     154
     155        RefPtr<Node> previousSibling = childToRemove.previousSibling();
     156        RefPtr<Node> nextSibling = childToRemove.nextSibling();
     157        removeBetween(previousSibling.get(), nextSibling.get(), childToRemove);
     158        notifyChildNodeRemoved(*this, childToRemove);
     159
     160        change.type = is<Element>(childToRemove) ? ElementRemoved : (is<Text>(childToRemove) ? TextRemoved : NonContentsChildRemoved);
     161        change.previousSiblingElement = (!previousSibling || is<Element>(*previousSibling)) ? downcast<Element>(previousSibling.get()) : ElementTraversal::previousSibling(*previousSibling);
     162        change.nextSiblingElement = (!nextSibling || is<Element>(*nextSibling)) ? downcast<Element>(nextSibling.get()) : ElementTraversal::nextSibling(*nextSibling);
     163        change.source = source;
     164    }
     165
     166    // FIXME: Move childrenChanged into NoEventDispatchAssertion block.
    164167    childrenChanged(change);
    165168

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

    r224356 r224378  
    19241924bool Document::updateStyleIfNeeded()
    19251925{
     1926    RefPtr<FrameView> frameView = view();
    19261927    {
    19271928        NoEventDispatchAssertion::InMainThread noEventDispatchAssertion;
    19281929        ASSERT(isMainThread());
    1929         ASSERT(!view() || !view()->isPainting());
    1930 
    1931         if (!view() || view()->layoutContext().isInRenderTreeLayout())
     1930        ASSERT(!frameView || !frameView->isPainting());
     1931
     1932        if (!frameView || frameView->layoutContext().isInRenderTreeLayout())
    19321933            return false;
    19331934
    … …  
    19381939    }
    19391940
     1941    // The early exit for needsStyleRecalc() is needed when updateWidgetPositions() is called in runOrScheduleAsynchronousTasks().
     1942    ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
     1943
    19401944    resolveStyle();
    19411945    return true;
    … …  
    19441948void Document::updateLayout()
    19451949{
     1950    ASSERT(isMainThread());
    19461951    ASSERT(LayoutDisallowedScope::isLayoutAllowed());
    1947     ASSERT(isMainThread());
    19481952
    19491953    RefPtr<FrameView> frameView = view();
    … …  
    19531957        return;
    19541958    }
     1959    ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
     1960
    19551961
    19561962    RenderView::RepaintRegionAccumulator repaintRegionAccumulator(renderView());

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

    r224356 r224378  
    139139
    140140#if !ASSERT_DISABLED
     141    // FIXME: Remove this class once the sync layout inside SVGImage::draw is removed.
    141142    class DisableAssertionsInScope {
    142143    public:
    143144        DisableAssertionsInScope()
    144145        {
    145             if (!isMainThread())
    146                 return;
    147             s_existingCount = s_count;
    148             s_count = 0;
     146            ASSERT(isMainThread());
     147            std::swap(s_count, m_originalCount);
    149148        }
    150149
    151150        ~DisableAssertionsInScope()
    152151        {
    153             s_count = s_existingCount;
    154             s_existingCount = 0;
     152            s_count = m_originalCount;
    155153        }
    156154    private:
    157         WEBCORE_EXPORT static unsigned s_existingCount;
     155        unsigned m_originalCount { 0 };
    158156    };
    159157#else

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

    r223947 r224378  
    324324}
    325325
    326 Ref<NodeList> SVGSVGElement::collectIntersectionOrEnclosureList(SVGRect& rect, SVGElement* referenceElement, bool (*checkFunction)(RefPtr<SVGElement>&&, SVGRect&))
     326Ref<NodeList> SVGSVGElement::collectIntersectionOrEnclosureList(SVGRect& rect, SVGElement* referenceElement, bool (*checkFunction)(SVGElement&, SVGRect&))
    327327{
    328328    Vector<Ref<Element>> elements;
    329329    for (auto& element : descendantsOfType<SVGElement>(referenceElement ? *referenceElement : *this)) {
    330         if (checkFunction(&element, rect))
     330        if (checkFunction(element, rect))
    331331            elements.append(element);
    332332    }
    … …  
    334334}
    335335
     336static bool checkIntersectionWithoutUpdatingLayout(SVGElement& element, SVGRect& rect)
     337{
     338    return RenderSVGModelObject::checkIntersection(element.renderer(), rect.propertyReference());
     339}
     340   
     341static bool checkEnclosureWithoutUpdatingLayout(SVGElement& element, SVGRect& rect)
     342{
     343    return RenderSVGModelObject::checkEnclosure(element.renderer(), rect.propertyReference());
     344}
     345
    336346Ref<NodeList> SVGSVGElement::getIntersectionList(SVGRect& rect, SVGElement* referenceElement)
    337347{
    338348    document().updateLayoutIgnorePendingStylesheets();
    339     return collectIntersectionOrEnclosureList(rect, referenceElement, checkIntersection);
     349    return collectIntersectionOrEnclosureList(rect, referenceElement, checkIntersectionWithoutUpdatingLayout);
    340350}
    341351
    … …  
    343353{
    344354    document().updateLayoutIgnorePendingStylesheets();
    345     return collectIntersectionOrEnclosureList(rect, referenceElement, checkEnclosure);
     355    return collectIntersectionOrEnclosureList(rect, referenceElement, checkEnclosureWithoutUpdatingLayout);
    346356}
    347357
    … …  
    351361        return false;
    352362    element->document().updateLayoutIgnorePendingStylesheets();
    353     return RenderSVGModelObject::checkIntersection(element->renderer(), rect.propertyReference());
     363    return checkIntersectionWithoutUpdatingLayout(*element, rect);
    354364}
    355365
    … …  
    359369        return false;
    360370    element->document().updateLayoutIgnorePendingStylesheets();
    361     return RenderSVGModelObject::checkEnclosure(element->renderer(), rect.propertyReference());
     371    return checkEnclosureWithoutUpdatingLayout(*element, rect);
    362372}
    363373

  • trunk/Source/WebCore/svg/SVGSVGElement.h

    r223947 r224378  
    154154    Frame* frameForCurrentScale() const;
    155155    void inheritViewAttributes(const SVGViewElement&);
    156     Ref<NodeList> collectIntersectionOrEnclosureList(SVGRect&, SVGElement*, bool (*checkFunction)(RefPtr<SVGElement>&&, SVGRect&));
     156    Ref<NodeList> collectIntersectionOrEnclosureList(SVGRect&, SVGElement*, bool (*checkFunction)(SVGElement&, SVGRect&));
    157157
    158158    bool m_useCurrentView { false };

  • trunk/Source/WebCore/svg/graphics/SVGImage.cpp

    r224150 r224378  
    4444#include "LibWebRTCProvider.h"
    4545#include "MainFrame.h"
     46#include "NoEventDispatchAssertion.h"
    4647#include "Page.h"
    4748#include "PageConfiguration.h"
    … …  
    311312    view->resize(containerSize());
    312313
    313     if (view->needsLayout())
    314         view->layoutContext().layout();
     314    {
     315        NoEventDispatchAssertion::DisableAssertionsInScope disabledScope;
     316        if (view->needsLayout())
     317            view->layoutContext().layout();
     318    }
    315319
    316320    view->paint(context, intersection(context.clipBounds(), enclosingIntRect(srcRect)));
Note: See TracChangeset for help on using the changeset viewer.