Changeset 221139 in webkit

Timestamp:

Aug 24, 2017, 9:37:53 AM (9 years ago)

Author:

Darin Adler

Message:

REGRESSION (r220052): ASSERTION FAILED: !frame().isMainFrame()

!needsStyleRecalcOrLayout() in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()

​https://bugs.webkit.org/show_bug.cgi?id=175270

Reviewed by Simon Fraser.

Source/WebCore:

• dom/Document.cpp:

(WebCore::Document::Document): Initialize m_styleRecalcTimer with a lamdba so it can work
with a function that returns a bool and ignore the return value.
(WebCore::Document::updateStyleIfNeeded): Added a boolean return value indicating if the
function did any work or not.

• dom/Document.h: Updated for above change.

• page/FrameView.cpp:

(WebCore::appendRenderedChildren): Added helper that will later replace the
FrameView::renderedChildFrameViews function and is used below.
(WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive): Instead of always doing two
passes of style and layout update do up to four passes, but stop as soon as a pass does
no work. This is slightly more efficient in cases where no layout and style update is
needed, and works correctly when a third pass is needed, which is what happens in the
test that was failing. We can eventually improve this further, but this resolves the
immediate problem we are seeing in the test.

LayoutTests:

• platform/mac-wk2/TestExpectations: Re-enable the disabled test.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac-wk2/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (3 diffs)
• Source/WebCore/dom/Document.h (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-08-22  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r220052): ASSERTION FAILED: !frame().isMainFrame() || !needsStyleRecalcOrLayout()  in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()
+        https://bugs.webkit.org/show_bug.cgi?id=175270
+
+        Reviewed by Simon Fraser.
+
+        * platform/mac-wk2/TestExpectations: Re-enable the disabled test.
+
 2017-08-24  Ms2ger  <Ms2ger@igalia.com>
 

trunk/LayoutTests/platform/mac-wk2/TestExpectations

@@ -753 +753 @@
 webkit.org/b/173946 [ Debug ] media/modern-media-controls/fullscreen-support/fullscreen-support-press.html [ Pass Failure ]
 
-webkit.org/b/175270 [ Debug ] plugins/crash-restoring-plugin-page-from-page-cache.html [ Skip ]
-
 # CSS Regions are being phased out.
 tiled-drawing/scrolling/non-fast-region/wheel-handler-in-region.html [ Skip ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-08-22  Darin Adler  <darin@apple.com>
+
+        REGRESSION (r220052): ASSERTION FAILED: !frame().isMainFrame() || !needsStyleRecalcOrLayout()  in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()
+        https://bugs.webkit.org/show_bug.cgi?id=175270
+
+        Reviewed by Simon Fraser.
+
+        * dom/Document.cpp:
+        (WebCore::Document::Document): Initialize m_styleRecalcTimer with a lamdba so it can work
+        with a function that returns a bool and ignore the return value.
+        (WebCore::Document::updateStyleIfNeeded): Added a boolean return value indicating if the
+        function did any work or not.
+        * dom/Document.h: Updated for above change.
+
+        * page/FrameView.cpp:
+        (WebCore::appendRenderedChildren): Added helper that will later replace the
+        FrameView::renderedChildFrameViews function and is used below.
+        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive): Instead of always doing two
+        passes of style and layout update do up to four passes, but stop as soon as a pass does
+        no work. This is slightly more efficient in cases where no layout and style update is
+        needed, and works correctly when a third pass is needed, which is what happens in the
+        test that was failing. We can eventually improve this further, but this resolves the
+        immediate problem we are seeing in the test.
+
 2017-08-24  Don Olmstead  <don.olmstead@sony.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -461 +461 @@
     , m_visitedLinkState(std::make_unique<VisitedLinkState>(*this))
     , m_markers(std::make_unique<DocumentMarkerController>(*this))
-    , m_styleRecalcTimer(*this, &Document::updateStyleIfNeeded)
+    , m_styleRecalcTimer([this] { updateStyleIfNeeded(); })
     , m_updateFocusAppearanceTimer(*this, &Document::updateFocusAppearanceTimerFired)
     , m_documentCreationTime(MonotonicTime::now())
@@ -1888 +1888 @@
 }
 
-void Document::updateStyleIfNeeded()
+bool Document::updateStyleIfNeeded()
 {
     ASSERT(isMainThread());
@@ -1894 +1894 @@
 
     if (!view() || view()->isInRenderTreeLayout())
-        return;
+        return false;
 
     styleScope().flushPendingUpdate();
 
     if (!needsStyleRecalc())
-        return;
+        return false;
 
     resolveStyle();
+    return true;
 }
 

trunk/Source/WebCore/dom/Document.h

@@ -545 +545 @@
     enum class ResolveStyleType { Normal, Rebuild };
     void resolveStyle(ResolveStyleType = ResolveStyleType::Normal);
-    WEBCORE_EXPORT void updateStyleIfNeeded();
+    WEBCORE_EXPORT bool updateStyleIfNeeded();
     bool needsStyleRecalc() const;
     unsigned lastStyleUpdateSizeForTesting() const { return m_lastStyleUpdateSizeForTesting; }

trunk/Source/WebCore/page/FrameView.cpp

@@ -4576 +4576 @@
 }
 
+static void appendRenderedChildren(FrameView& view, Deque<Ref<FrameView>, 16>& deque)
+{
+    for (Frame* frame = view.frame().tree().firstRenderedChild(); frame; frame = frame->tree().nextRenderedSibling()) {
+        if (frame->view())
+            deque.append(*frame->view());
+    }
+}
+
+// FIXME: Change the one remaining caller of this to use appendRenderedChildren above instead,
+// and then remove FrameViewList and renderedChildFrameViews.
 FrameView::FrameViewList FrameView::renderedChildFrameViews() const
 {
@@ -4589 +4599 @@
 void FrameView::updateLayoutAndStyleIfNeededRecursive()
 {
-    // We have to crawl our entire tree looking for any FrameViews that need
-    // layout and make sure they are up to date.
-    // Mac actually tests for intersection with the dirty region and tries not to
-    // update layout for frames that are outside the dirty region.  Not only does this seem
-    // pointless (since those frames will have set a zero timer to layout anyway), but
-    // it is also incorrect, since if two frames overlap, the first could be excluded from the dirty
-    // region but then become included later by the second frame adding rects to the dirty region
-    // when it lays out.
+    // The number "4" here is empirically determined as the number of passes needed to
+    // make sure we don't need more style recalculation or layout; we have not seen a
+    // case where we need more than 3 passes, so 4 is a bit more than needed in any
+    // normal case. But it's not clear that anything prevents an indefinite number of
+    // passes from being needed. So it would be better to instead have a firm guarantee
+    // of the number of times this needs to be done, or come up with a way to do this
+    // in one pass without a loop.
+    const unsigned maxUpdatePasses = 4;
 
     AnimationUpdateBlock animationUpdateBlock(&frame().animation());
 
-    frame().document()->updateStyleIfNeeded();
-
-    if (needsLayout())
-        layout();
-
-    // Grab a copy of the child views, as the list may be mutated by the following updateLayoutAndStyleIfNeededRecursive
-    // calls, as they can potentially re-enter a layout of the parent frame view.
-    for (auto& frameView : renderedChildFrameViews())
-        frameView->updateLayoutAndStyleIfNeededRecursive();
-
-    // A child frame may have dirtied us during its layout.
-    frame().document()->updateStyleIfNeeded();
-    if (needsLayout())
-        layout();
-
+    auto updateOnce = [this] {
+        auto updateOneFrame = [] (FrameView& view) {
+            bool didWork = view.frame().document()->updateStyleIfNeeded();
+            if (view.needsLayout()) {
+                view.layout();
+                didWork = true;
+            }
+            return didWork;
+        };
+
+        bool didWork = false;
+
+        // Use a copy of the child frame list because it can change while updating.
+        Deque<Ref<FrameView>, 16> views;
+        views.append(*this);
+        while (!views.isEmpty()) {
+            auto view = views.takeFirst();
+            if (updateOneFrame(view.get()))
+                didWork = true;
+            appendRenderedChildren(view.get(), views);
+        }
+
+        return didWork;
+    };
+
+    for (unsigned i = 0; i < maxUpdatePasses; ++i) {
+        if (!updateOnce())
+            break;
+    }
+
+    // FIXME: Unclear why it's appropriate to skip this assertion for non-main frames.
+    // The need for this may be obsolete and a leftover from when this fucntion was
+    // implemented by recursively calling itself.
     ASSERT(!frame().isMainFrame() || !needsStyleRecalcOrLayout());
 }