Merge r221423 - REGRESSION (r220052): ASSERTION FAILED: !frame().isMa…

…inFrame() || !needsStyleRecalcOrLayout() in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() https://bugs.webkit.org/show_bug.cgi?id=175270 Reviewed by Simon Fraser and Antti Koivisto. 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 25 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 additional passes are 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.
WebKit · Sep 2, 2017 · 5564095 · 5564095
1 parent 0e1ce60
commit 5564095
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 24 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,12 @@
+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 and Antti Koivisto.
+
+        * platform/mac-wk2/TestExpectations: Re-enable the disabled test.
+
 2017-08-30  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Previous elements with lang= can affect fonts selected for subsequent elements

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,27 @@
+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 and Antti Koivisto.
+
+        * 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 25 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 additional passes are 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-30  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Previous elements with lang= can affect fonts selected for subsequent elements

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
@@ -460,7 +460,7 @@ Document::Document(Frame* frame, const URL& url, unsigned documentClasses, unsig
     , m_extensionStyleSheets(std::make_unique<ExtensionStyleSheets>(*this))
     , 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())
     , m_scriptRunner(std::make_unique<ScriptRunner>(*this))
@@ -1887,20 +1887,21 @@ bool Document::needsStyleRecalc() const
     return false;
 }
 
-void Document::updateStyleIfNeeded()
+bool Document::updateStyleIfNeeded()
 {
     ASSERT(isMainThread());
     ASSERT(!view() || !view()->isPainting());
 
     if (!view() || view()->isInRenderTreeLayout())
-        return;
+        return false;
 
     styleScope().flushPendingUpdate();
 
     if (!needsStyleRecalc())
-        return;
+        return false;
 
     resolveStyle();
+    return true;
 }
 
 void Document::updateLayout()

diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
@@ -543,7 +543,7 @@ class Document
 
     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; }
 

diff --git a/Source/WebCore/page/FrameView.cpp b/Source/WebCore/page/FrameView.cpp
@@ -4574,6 +4574,16 @@ void FrameView::paintOverhangAreas(GraphicsContext& context, const IntRect& hori
     ScrollView::paintOverhangAreas(context, horizontalOverhangArea, verticalOverhangArea, dirtyRect);
 }
 
+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
 {
     FrameViewList childViews;
@@ -4587,32 +4597,52 @@ FrameView::FrameViewList FrameView::renderedChildFrameViews() const
 
 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.
+    // Style updating, render tree, creation, and layout needs to be done multiple times
+    // for more than one reason. But one reason is that when an <object> element determines
+    // what it needs to load a subframe, a second pass is needed. That requires update
+    // passes equal to the number of levels of DOM nesting. That is why this number is large.
+    // There are test cases where we have roughly 10 levels of DOM nesting, so this needs to
+    // be greater than that. We have a limit to avoid the possibility of an infinite loop.
+    // Typical calls will run the loop 2 times (once to do work, once to detect no further work
+    // is needed).
+    // FIXME: We should find an approach that does not require a loop at all.
+    const unsigned maxUpdatePasses = 25;
 
     AnimationUpdateBlock animationUpdateBlock(&frame().animation());
 
-    frame().document()->updateStyleIfNeeded();
+    auto updateOnce = [this] {
+        auto updateOneFrame = [] (FrameView& view) {
+            bool didWork = view.frame().document()->updateStyleIfNeeded();
+            if (view.needsLayout()) {
+                view.layout();
+                didWork = true;
+            }
+            return didWork;
+        };
 
-    if (needsLayout())
-        layout();
+        bool didWork = false;
 
-    // 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();
+        // 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);
+        }
 
-    // A child frame may have dirtied us during its layout.
-    frame().document()->updateStyleIfNeeded();
-    if (needsLayout())
-        layout();
+        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());
 }