- Revision
- 224011
- Author
- [email protected]
- Date
- 2017-10-26 01:46:02 -0700 (Thu, 26 Oct 2017)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (224010 => 224011)
--- trunk/Source/WebCore/ChangeLog 2017-10-26 07:10:05 UTC (rev 224010)
+++ trunk/Source/WebCore/ChangeLog 2017-10-26 08:46:02 UTC (rev 224011)
@@ -1,3 +1,46 @@
+2017-10-26 Ryosuke Niwa <[email protected]>
+
+ 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):
+
2017-10-25 Keith Miller <[email protected]>
Move platform Cocoa sources to unified sources
Modified: trunk/Source/WebCore/dom/Document.cpp (224010 => 224011)
--- trunk/Source/WebCore/dom/Document.cpp 2017-10-26 07:10:05 UTC (rev 224010)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-10-26 08:46:02 UTC (rev 224011)
@@ -1776,18 +1776,22 @@
// re-attaching our containing iframe, which when asked HTMLFrameElementBase::isURLAllowed
// hits a null-dereference due to security code always assuming the document has a SecurityOrigin.
- styleScope().flushPendingUpdate();
+ {
+ NoEventDispatchAssertion noEventDispatchAssertion;
+ styleScope().flushPendingUpdate();
+ frameView.willRecalcStyle();
+ }
- frameView.willRecalcStyle();
-
InspectorInstrumentationCookie cookie = InspectorInstrumentation::willRecalculateStyle(*this);
- m_inStyleRecalc = true;
bool updatedCompositingLayers = false;
{
Style::PostResolutionCallbackDisabler disabler(*this);
WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
+ NoEventDispatchAssertion noEventDispatchAssertion;
+ m_inStyleRecalc = true;
+
if (m_pendingStyleRecalcShouldForce)
type = ResolveStyleType::Rebuild;
@@ -1835,6 +1839,19 @@
if (m_renderView->needsLayout())
frameView.scheduleRelayout();
+
+ // Usually this is handled by post-layout.
+ if (!frameView.needsLayout())
+ frameView.frame().selection().scheduleAppearanceUpdateAfterStyleChange();
+
+ // As a result of the style recalculation, the currently hovered element might have been
+ // detached (for example, by setting display:none in the :hover style), schedule another mouseMove event
+ // to check if any other elements ended up under the mouse pointer due to re-layout.
+ if (m_hoveredElement && !m_hoveredElement->renderer())
+ frameView.frame().mainFrame().eventHandler().dispatchFakeMouseMoveEventSoon();
+
+ ++m_styleRecalcCount;
+ // FIXME: Assert ASSERT(!needsStyleRecalc()) here. Do we still have some cases where it's not true?
}
// If we wanted to call implicitClose() during recalcStyle, do so now that we're finished.
@@ -1842,8 +1859,6 @@
m_closeAfterStyleRecalc = false;
implicitClose();
}
-
- ++m_styleRecalcCount;
InspectorInstrumentation::didRecalculateStyle(cookie);
@@ -1853,20 +1868,8 @@
if (updatedCompositingLayers && !frameView.needsLayout())
frameView.viewportContentsChanged();
- // Usually this is handled by post-layout.
- if (!frameView.needsLayout())
- frameView.frame().selection().scheduleAppearanceUpdateAfterStyleChange();
-
- // As a result of the style recalculation, the currently hovered element might have been
- // detached (for example, by setting display:none in the :hover style), schedule another mouseMove event
- // to check if any other elements ended up under the mouse pointer due to re-layout.
- if (m_hoveredElement && !m_hoveredElement->renderer())
- frameView.frame().mainFrame().eventHandler().dispatchFakeMouseMoveEventSoon();
-
if (m_gotoAnchorNeededAfterStylesheetsLoad && !styleScope().hasPendingSheets())
frameView.scrollToFragment(m_url);
-
- // FIXME: Ideally we would ASSERT(!needsStyleRecalc()) here but we have some cases where it is not true.
}
void Document::updateTextRenderer(Text& text, unsigned offsetOfReplacedText, unsigned lengthOfReplacedText)
@@ -1904,16 +1907,19 @@
bool Document::updateStyleIfNeeded()
{
- ASSERT(isMainThread());
- ASSERT(!view() || !view()->isPainting());
+ {
+ NoEventDispatchAssertion noEventDispatchAssertion;
+ ASSERT(isMainThread());
+ ASSERT(!view() || !view()->isPainting());
- if (!view() || view()->isInRenderTreeLayout())
- return false;
+ if (!view() || view()->isInRenderTreeLayout())
+ return false;
- styleScope().flushPendingUpdate();
+ styleScope().flushPendingUpdate();
- if (!needsStyleRecalc())
- return false;
+ if (!needsStyleRecalc())
+ return false;
+ }
resolveStyle();
return true;
Modified: trunk/Source/WebCore/dom/Element.cpp (224010 => 224011)
--- trunk/Source/WebCore/dom/Element.cpp 2017-10-26 07:10:05 UTC (rev 224010)
+++ trunk/Source/WebCore/dom/Element.cpp 2017-10-26 08:46:02 UTC (rev 224011)
@@ -341,6 +341,11 @@
Ref<Element> Element::cloneElementWithChildren(Document& targetDocument)
{
Ref<Element> clone = cloneElementWithoutChildren(targetDocument);
+
+ // It's safe to dispatch events on the cloned node since author scripts have no access to it yet.
+ // This is needed for SVGUseElement::cloneTarget.
+ NoEventDispatchAssertion::EventAllowedScope allowedScope(clone.get());
+
cloneChildNodes(clone);
return clone;
}
@@ -348,6 +353,11 @@
Ref<Element> Element::cloneElementWithoutChildren(Document& targetDocument)
{
Ref<Element> clone = cloneElementWithoutAttributesAndChildren(targetDocument);
+
+ // It's safe to dispatch events on the cloned node since author scripts have no access to it yet.
+ // This is needed for SVGUseElement::cloneTarget.
+ NoEventDispatchAssertion::EventAllowedScope allowedScope(clone.get());
+
// This will catch HTML elements in the wrong namespace that are not correctly copied.
// This is a sanity check as HTML overloads some of the DOM methods.
ASSERT(isHTMLElement() == clone->isHTMLElement());
Modified: trunk/Source/WebCore/dom/EventDispatcher.cpp (224010 => 224011)
--- trunk/Source/WebCore/dom/EventDispatcher.cpp 2017-10-26 07:10:05 UTC (rev 224010)
+++ trunk/Source/WebCore/dom/EventDispatcher.cpp 2017-10-26 08:46:02 UTC (rev 224011)
@@ -130,7 +130,7 @@
bool EventDispatcher::dispatchEvent(Node& node, Event& event)
{
- ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
+ ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventDispatchAllowedInSubtree(node));
Ref<Node> protectedNode(node);
RefPtr<FrameView> view = node.document().view();
EventPath eventPath(node, event);
@@ -149,8 +149,6 @@
if (!event.target())
return true;
- ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
-
InputElementClickState clickHandlingState;
if (is<HTMLInputElement>(node))
downcast<HTMLInputElement>(node).willDispatchEvent(event, clickHandlingState);
Modified: trunk/Source/WebCore/svg/SVGUseElement.cpp (224010 => 224011)
--- trunk/Source/WebCore/svg/SVGUseElement.cpp 2017-10-26 07:10:05 UTC (rev 224010)
+++ trunk/Source/WebCore/svg/SVGUseElement.cpp 2017-10-26 08:46:02 UTC (rev 224011)
@@ -216,6 +216,7 @@
void SVGUseElement::clearShadowTree()
{
if (auto root = userAgentShadowRoot()) {
+ // Safe because SVG use element's shadow tree is never used to fire synchronous events during layout or DOM mutations.
NoEventDispatchAssertion::EventAllowedScope scope(*root);
root->removeChildren();
}
@@ -243,13 +244,18 @@
return;
}
- cloneTarget(ensureUserAgentShadowRoot(), *target);
- expandUseElementsInShadowTree();
- expandSymbolElementsInShadowTree();
+ {
+ // Safe because the cloned shadow tree has never been exposed to author scripts.
+ auto& shadowRoot = ensureUserAgentShadowRoot();
+ NoEventDispatchAssertion::EventAllowedScope scope(shadowRoot);
+ cloneTarget(shadowRoot, *target);
+ expandUseElementsInShadowTree();
+ expandSymbolElementsInShadowTree();
+ updateRelativeLengthsInformation();
+ }
+
transferEventListenersToShadowTree();
- updateRelativeLengthsInformation();
-
// When we invalidate the other shadow trees, it's important that we don't
// follow any cycles and invalidate ourselves. To avoid that, we temporarily
// set m_shadowTreeNeedsUpdate to true so invalidateShadowTree will
@@ -430,6 +436,8 @@
void SVGUseElement::cloneTarget(ContainerNode& container, SVGElement& target) const
{
Ref<SVGElement> targetClone = static_cast<SVGElement&>(target.cloneElementWithChildren(document()).get());
+ // Safe because the newy cloned nodes in the shadow tree has not been exposed to author scripts yet.
+ NoEventDispatchAssertion::EventAllowedScope scope(targetClone);
associateClonesWithOriginals(targetClone.get(), target);
removeDisallowedElementsFromSubtree(targetClone.get());
removeSymbolElementsFromSubtree(targetClone.get());
@@ -462,6 +470,9 @@
// 'use' element except for x, y, width, height and xlink:href are transferred to the generated 'g' element.
auto replacementClone = SVGGElement::create(document());
+ // 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.
+ NoEventDispatchAssertion::EventAllowedScope scope(replacementClone);
+
cloneDataAndChildren(replacementClone.get(), originalClone);
replacementClone->removeAttribute(SVGNames::xAttr);
@@ -495,6 +506,8 @@
// 'svg' element will use values of 100% for these attributes.
auto replacementClone = SVGSVGElement::create(document());
+ // Safe because the newly created SVG element and the newly created shadow tree has not been exposed to author scripts yet.
+ NoEventDispatchAssertion::EventAllowedScope scope(replacementClone);
cloneDataAndChildren(replacementClone.get(), originalClone);
originalClone.parentNode()->replaceChild(replacementClone, originalClone);
@@ -506,6 +519,7 @@
void SVGUseElement::transferEventListenersToShadowTree() const
{
+ // FIXME: Don't directly add event listeners on each descendant. Copy event listeners on the use element instead.
for (auto& descendant : descendantsOfType<SVGElement>(*userAgentShadowRoot())) {
if (EventTargetData* data = ""
data->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(&descendant);
