Changeset 224131 in webkit

Timestamp:

Oct 27, 2017 2:35:14 PM (7 years ago)

Author:

Ryan Haddad

Message:

Unreviewed, rolling out r224011.

xsl LayoutTests hit an assertion added with this change since
r223999 was rolled out.

Reverted changeset:

"Assert that no script is executed during style recalc"
​https://bugs.webkit.org/show_bug.cgi?id=178845
​https://trac.webkit.org/changeset/224011

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (5 diffs)
• dom/Element.cpp (modified) (2 diffs)
• dom/EventDispatcher.cpp (modified) (2 diffs)
• svg/SVGUseElement.cpp (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-10-27  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r224011.
+
+        xsl LayoutTests hit an assertion added with this change since
+        r223999 was rolled out.
+
+        Reverted changeset:
+
+        "Assert that no script is executed during style recalc"
+        https://bugs.webkit.org/show_bug.cgi?id=178845
+        https://trac.webkit.org/changeset/224011
+
 2017-10-27  Antoine Quint  <graouts@apple.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -1790 +1790 @@
     // hits a null-dereference due to security code always assuming the document has a SecurityOrigin.
 
-    {
-        NoEventDispatchAssertion noEventDispatchAssertion;
-        styleScope().flushPendingUpdate();
-        frameView.willRecalcStyle();
-    }
+    styleScope().flushPendingUpdate();
+
+    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)
@@ -1853 +1849 @@
         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?
     }
 
@@ -1873 +1856 @@
         implicitClose();
     }
+    
+    ++m_styleRecalcCount;
 
     InspectorInstrumentation::didRecalculateStyle(cookie);
@@ -1882 +1867 @@
         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.
 }
 
@@ -1921 +1918 @@
 bool Document::updateStyleIfNeeded()
 {
-    {
-        NoEventDispatchAssertion noEventDispatchAssertion;
-        ASSERT(isMainThread());
-        ASSERT(!view() || !view()->isPainting());
-
-        if (!view() || view()->isInRenderTreeLayout())
-            return false;
-
-        styleScope().flushPendingUpdate();
-
-        if (!needsStyleRecalc())
-            return false;
-    }
+    ASSERT(isMainThread());
+    ASSERT(!view() || !view()->isPainting());
+
+    if (!view() || view()->isInRenderTreeLayout())
+        return false;
+
+    styleScope().flushPendingUpdate();
+
+    if (!needsStyleRecalc())
+        return false;
 
     resolveStyle();

trunk/Source/WebCore/dom/Element.cpp

@@ -342 +342 @@
 {
     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;
@@ -354 +349 @@
 {
     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.

trunk/Source/WebCore/dom/EventDispatcher.cpp

@@ -131 +131 @@
 bool EventDispatcher::dispatchEvent(Node& node, Event& event)
 {
-    ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventDispatchAllowedInSubtree(node));
+    ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
     Ref<Node> protectedNode(node);
     RefPtr<FrameView> view = node.document().view();
@@ -149 +149 @@
     if (!event.target())
         return true;
+
+    ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
 
     InputElementClickState clickHandlingState;

trunk/Source/WebCore/svg/SVGUseElement.cpp

@@ -217 +217 @@
 {
     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();
@@ -245 +244 @@
     }
 
-    {
-        // 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();
-    }
-
+    cloneTarget(ensureUserAgentShadowRoot(), *target);
+    expandUseElementsInShadowTree();
+    expandSymbolElementsInShadowTree();
     transferEventListenersToShadowTree();
+
+    updateRelativeLengthsInformation();
 
     // When we invalidate the other shadow trees, it's important that we don't
@@ -437 +431 @@
 {
     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());
@@ -471 +463 @@
 
         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);
 
@@ -507 +496 @@
 
         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);
 
@@ -520 +507 @@
 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 = descendant.correspondingElement()->eventTargetData())