Assert that updateStyle and updateLayout are only called when it's safe to dispatch...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Nov 2017 03:48:11 +0000 (03:48 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Nov 2017 03:48:11 +0000 (03:48 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@224378 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/NoEventDispatchAssertion.h
Source/WebCore/svg/SVGSVGElement.cpp
Source/WebCore/svg/SVGSVGElement.h
Source/WebCore/svg/graphics/SVGImage.cpp

index 8c3f6bc..fb872b6 100644 (file)
@@ -1,3 +1,45 @@
+2017-11-01  Ryosuke Niwa  <rniwa@webkit.org>
+
+        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.
+
 2017-11-02  Alex Christensen  <achristensen@webkit.org>
 
         Fix Windows debug build after r224371
index 1903f7a..210b344 100644 (file)
@@ -72,7 +72,6 @@ ChildNodesLazySnapshot* ChildNodesLazySnapshot::latestSnapshot;
 
 #if !ASSERT_DISABLED
 unsigned NoEventDispatchAssertion::s_count = 0;
-unsigned NoEventDispatchAssertion::DisableAssertionsInScope::s_existingCount = 0;
 NoEventDispatchAssertion::EventAllowedScope* NoEventDispatchAssertion::EventAllowedScope::s_currentScope = nullptr;
 #endif
 
@@ -143,24 +142,28 @@ ALWAYS_INLINE bool ContainerNode::removeNodeWithScriptAssertion(Node& childToRem
     if (childToRemove.parentNode() != this)
         return false;
 
-    WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
-    NoEventDispatchAssertion::InMainThread assertNoEventDispatch;
+    ChildChange change;
+    {
+        WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
+        NoEventDispatchAssertion::InMainThread assertNoEventDispatch;
 
-    document().nodeWillBeRemoved(childToRemove);
+        document().nodeWillBeRemoved(childToRemove);
 
-    ASSERT_WITH_SECURITY_IMPLICATION(childToRemove.parentNode() == this);
-    ASSERT(!childToRemove.isDocumentFragment());
+        ASSERT_WITH_SECURITY_IMPLICATION(childToRemove.parentNode() == this);
+        ASSERT(!childToRemove.isDocumentFragment());
 
-    RefPtr<Node> previousSibling = childToRemove.previousSibling();
-    RefPtr<Node> nextSibling = childToRemove.nextSibling();
-    removeBetween(previousSibling.get(), nextSibling.get(), childToRemove);
-    notifyChildNodeRemoved(*this, childToRemove);
+        RefPtr<Node> previousSibling = childToRemove.previousSibling();
+        RefPtr<Node> nextSibling = childToRemove.nextSibling();
+        removeBetween(previousSibling.get(), nextSibling.get(), childToRemove);
+        notifyChildNodeRemoved(*this, childToRemove);
 
-    ChildChange change;
-    change.type = is<Element>(childToRemove) ? ElementRemoved : (is<Text>(childToRemove) ? TextRemoved : NonContentsChildRemoved);
-    change.previousSiblingElement = (!previousSibling || is<Element>(*previousSibling)) ? downcast<Element>(previousSibling.get()) : ElementTraversal::previousSibling(*previousSibling);
-    change.nextSiblingElement = (!nextSibling || is<Element>(*nextSibling)) ? downcast<Element>(nextSibling.get()) : ElementTraversal::nextSibling(*nextSibling);
-    change.source = source;
+        change.type = is<Element>(childToRemove) ? ElementRemoved : (is<Text>(childToRemove) ? TextRemoved : NonContentsChildRemoved);
+        change.previousSiblingElement = (!previousSibling || is<Element>(*previousSibling)) ? downcast<Element>(previousSibling.get()) : ElementTraversal::previousSibling(*previousSibling);
+        change.nextSiblingElement = (!nextSibling || is<Element>(*nextSibling)) ? downcast<Element>(nextSibling.get()) : ElementTraversal::nextSibling(*nextSibling);
+        change.source = source;
+    }
+
+    // FIXME: Move childrenChanged into NoEventDispatchAssertion block.
     childrenChanged(change);
 
     return true;
index f19462e..f56393c 100644 (file)
@@ -1923,12 +1923,13 @@ bool Document::needsStyleRecalc() const
 
 bool Document::updateStyleIfNeeded()
 {
+    RefPtr<FrameView> frameView = view();
     {
         NoEventDispatchAssertion::InMainThread noEventDispatchAssertion;
         ASSERT(isMainThread());
-        ASSERT(!view() || !view()->isPainting());
+        ASSERT(!frameView || !frameView->isPainting());
 
-        if (!view() || view()->layoutContext().isInRenderTreeLayout())
+        if (!frameView || frameView->layoutContext().isInRenderTreeLayout())
             return false;
 
         styleScope().flushPendingUpdate();
@@ -1937,14 +1938,17 @@ bool Document::updateStyleIfNeeded()
             return false;
     }
 
+    // The early exit for needsStyleRecalc() is needed when updateWidgetPositions() is called in runOrScheduleAsynchronousTasks().
+    ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
+
     resolveStyle();
     return true;
 }
 
 void Document::updateLayout()
 {
-    ASSERT(LayoutDisallowedScope::isLayoutAllowed());
     ASSERT(isMainThread());
+    ASSERT(LayoutDisallowedScope::isLayoutAllowed());
 
     RefPtr<FrameView> frameView = view();
     if (frameView && frameView->layoutContext().isInRenderTreeLayout()) {
@@ -1952,6 +1956,8 @@ void Document::updateLayout()
         ASSERT_NOT_REACHED();
         return;
     }
+    ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
+
 
     RenderView::RepaintRegionAccumulator repaintRegionAccumulator(renderView());
 
index fd23caf..1e25ec3 100644 (file)
@@ -138,23 +138,21 @@ public:
 #endif
 
 #if !ASSERT_DISABLED
+    // FIXME: Remove this class once the sync layout inside SVGImage::draw is removed.
     class DisableAssertionsInScope {
     public:
         DisableAssertionsInScope()
         {
-            if (!isMainThread())
-                return;
-            s_existingCount = s_count;
-            s_count = 0;
+            ASSERT(isMainThread());
+            std::swap(s_count, m_originalCount);
         }
 
         ~DisableAssertionsInScope()
         {
-            s_count = s_existingCount;
-            s_existingCount = 0;
+            s_count = m_originalCount;
         }
     private:
-        WEBCORE_EXPORT static unsigned s_existingCount;
+        unsigned m_originalCount { 0 };
     };
 #else
     class DisableAssertionsInScope {
index 81add34..c5ae195 100644 (file)
@@ -323,26 +323,36 @@ void SVGSVGElement::forceRedraw()
 {
 }
 
-Ref<NodeList> SVGSVGElement::collectIntersectionOrEnclosureList(SVGRect& rect, SVGElement* referenceElement, bool (*checkFunction)(RefPtr<SVGElement>&&, SVGRect&))
+Ref<NodeList> SVGSVGElement::collectIntersectionOrEnclosureList(SVGRect& rect, SVGElement* referenceElement, bool (*checkFunction)(SVGElement&, SVGRect&))
 {
     Vector<Ref<Element>> elements;
     for (auto& element : descendantsOfType<SVGElement>(referenceElement ? *referenceElement : *this)) {
-        if (checkFunction(&element, rect))
+        if (checkFunction(element, rect))
             elements.append(element);
     }
     return StaticElementList::create(WTFMove(elements));
 }
 
+static bool checkIntersectionWithoutUpdatingLayout(SVGElement& element, SVGRect& rect)
+{
+    return RenderSVGModelObject::checkIntersection(element.renderer(), rect.propertyReference());
+}
+    
+static bool checkEnclosureWithoutUpdatingLayout(SVGElement& element, SVGRect& rect)
+{
+    return RenderSVGModelObject::checkEnclosure(element.renderer(), rect.propertyReference());
+}
+
 Ref<NodeList> SVGSVGElement::getIntersectionList(SVGRect& rect, SVGElement* referenceElement)
 {
     document().updateLayoutIgnorePendingStylesheets();
-    return collectIntersectionOrEnclosureList(rect, referenceElement, checkIntersection);
+    return collectIntersectionOrEnclosureList(rect, referenceElement, checkIntersectionWithoutUpdatingLayout);
 }
 
 Ref<NodeList> SVGSVGElement::getEnclosureList(SVGRect& rect, SVGElement* referenceElement)
 {
     document().updateLayoutIgnorePendingStylesheets();
-    return collectIntersectionOrEnclosureList(rect, referenceElement, checkEnclosure);
+    return collectIntersectionOrEnclosureList(rect, referenceElement, checkEnclosureWithoutUpdatingLayout);
 }
 
 bool SVGSVGElement::checkIntersection(RefPtr<SVGElement>&& element, SVGRect& rect)
@@ -350,7 +360,7 @@ bool SVGSVGElement::checkIntersection(RefPtr<SVGElement>&& element, SVGRect& rec
     if (!element)
         return false;
     element->document().updateLayoutIgnorePendingStylesheets();
-    return RenderSVGModelObject::checkIntersection(element->renderer(), rect.propertyReference());
+    return checkIntersectionWithoutUpdatingLayout(*element, rect);
 }
 
 bool SVGSVGElement::checkEnclosure(RefPtr<SVGElement>&& element, SVGRect& rect)
@@ -358,7 +368,7 @@ bool SVGSVGElement::checkEnclosure(RefPtr<SVGElement>&& element, SVGRect& rect)
     if (!element)
         return false;
     element->document().updateLayoutIgnorePendingStylesheets();
-    return RenderSVGModelObject::checkEnclosure(element->renderer(), rect.propertyReference());
+    return checkEnclosureWithoutUpdatingLayout(*element, rect);
 }
 
 void SVGSVGElement::deselectAll()
index eac309a..3288def 100644 (file)
@@ -153,7 +153,7 @@ private:
 
     Frame* frameForCurrentScale() const;
     void inheritViewAttributes(const SVGViewElement&);
-    Ref<NodeList> collectIntersectionOrEnclosureList(SVGRect&, SVGElement*, bool (*checkFunction)(RefPtr<SVGElement>&&, SVGRect&));
+    Ref<NodeList> collectIntersectionOrEnclosureList(SVGRect&, SVGElement*, bool (*checkFunction)(SVGElement&, SVGRect&));
 
     bool m_useCurrentView { false };
     SVGZoomAndPanType m_zoomAndPan { SVGZoomAndPanMagnify };
index e25d564..f376a3c 100644 (file)
@@ -43,6 +43,7 @@
 #include "JSDOMWindowBase.h"
 #include "LibWebRTCProvider.h"
 #include "MainFrame.h"
+#include "NoEventDispatchAssertion.h"
 #include "Page.h"
 #include "PageConfiguration.h"
 #include "RenderSVGRoot.h"
@@ -310,8 +311,11 @@ ImageDrawResult SVGImage::draw(GraphicsContext& context, const FloatRect& dstRec
 
     view->resize(containerSize());
 
-    if (view->needsLayout())
-        view->layoutContext().layout();
+    {
+        NoEventDispatchAssertion::DisableAssertionsInScope disabledScope;
+        if (view->needsLayout())
+            view->layoutContext().layout();
+    }
 
     view->paint(context, intersection(context.clipBounds(), enclosingIntRect(srcRect)));