- Revision
- 224378
- Author
- [email protected]
- Date
- 2017-11-02 20:48:11 -0700 (Thu, 02 Nov 2017)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (224377 => 224378)
--- trunk/Source/WebCore/ChangeLog 2017-11-03 03:44:43 UTC (rev 224377)
+++ trunk/Source/WebCore/ChangeLog 2017-11-03 03:48:11 UTC (rev 224378)
@@ -1,3 +1,45 @@
+2017-11-01 Ryosuke Niwa <[email protected]>
+
+ 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 <[email protected]>
Fix Windows debug build after r224371
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (224377 => 224378)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2017-11-03 03:44:43 UTC (rev 224377)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2017-11-03 03:48:11 UTC (rev 224378)
@@ -72,7 +72,6 @@
#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 @@
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;
Modified: trunk/Source/WebCore/dom/Document.cpp (224377 => 224378)
--- trunk/Source/WebCore/dom/Document.cpp 2017-11-03 03:44:43 UTC (rev 224377)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-11-03 03:48:11 UTC (rev 224378)
@@ -1923,12 +1923,13 @@
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,6 +1938,9 @@
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;
}
@@ -1943,8 +1947,8 @@
void Document::updateLayout()
{
+ ASSERT(isMainThread());
ASSERT(LayoutDisallowedScope::isLayoutAllowed());
- ASSERT(isMainThread());
RefPtr<FrameView> frameView = view();
if (frameView && frameView->layoutContext().isInRenderTreeLayout()) {
@@ -1952,7 +1956,9 @@
ASSERT_NOT_REACHED();
return;
}
+ ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
+
RenderView::RepaintRegionAccumulator repaintRegionAccumulator(renderView());
if (HTMLFrameOwnerElement* owner = ownerElement())
Modified: trunk/Source/WebCore/dom/NoEventDispatchAssertion.h (224377 => 224378)
--- trunk/Source/WebCore/dom/NoEventDispatchAssertion.h 2017-11-03 03:44:43 UTC (rev 224377)
+++ trunk/Source/WebCore/dom/NoEventDispatchAssertion.h 2017-11-03 03:48:11 UTC (rev 224378)
@@ -138,23 +138,21 @@
#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 {
Modified: trunk/Source/WebCore/svg/SVGSVGElement.cpp (224377 => 224378)
--- trunk/Source/WebCore/svg/SVGSVGElement.cpp 2017-11-03 03:44:43 UTC (rev 224377)
+++ trunk/Source/WebCore/svg/SVGSVGElement.cpp 2017-11-03 03:48:11 UTC (rev 224378)
@@ -323,26 +323,36 @@
{
}
-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 @@
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 @@
if (!element)
return false;
element->document().updateLayoutIgnorePendingStylesheets();
- return RenderSVGModelObject::checkEnclosure(element->renderer(), rect.propertyReference());
+ return checkEnclosureWithoutUpdatingLayout(*element, rect);
}
void SVGSVGElement::deselectAll()
Modified: trunk/Source/WebCore/svg/SVGSVGElement.h (224377 => 224378)
--- trunk/Source/WebCore/svg/SVGSVGElement.h 2017-11-03 03:44:43 UTC (rev 224377)
+++ trunk/Source/WebCore/svg/SVGSVGElement.h 2017-11-03 03:48:11 UTC (rev 224378)
@@ -153,7 +153,7 @@
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 };
Modified: trunk/Source/WebCore/svg/graphics/SVGImage.cpp (224377 => 224378)
--- trunk/Source/WebCore/svg/graphics/SVGImage.cpp 2017-11-03 03:44:43 UTC (rev 224377)
+++ trunk/Source/WebCore/svg/graphics/SVGImage.cpp 2017-11-03 03:48:11 UTC (rev 224378)
@@ -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 @@
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)));
