- Revision
- 221423
- Author
- [email protected]
- Date
- 2017-08-31 09:55:50 -0700 (Thu, 31 Aug 2017)
Log 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 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.
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (221422 => 221423)
--- trunk/LayoutTests/ChangeLog 2017-08-31 16:38:16 UTC (rev 221422)
+++ trunk/LayoutTests/ChangeLog 2017-08-31 16:55:50 UTC (rev 221423)
@@ -1,3 +1,12 @@
+2017-08-22 Darin Adler <[email protected]>
+
+ 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-31 Ms2ger <[email protected]>
[GTK] Mark some tests as passing.
Modified: trunk/LayoutTests/platform/mac-wk2/TestExpectations (221422 => 221423)
--- trunk/LayoutTests/platform/mac-wk2/TestExpectations 2017-08-31 16:38:16 UTC (rev 221422)
+++ trunk/LayoutTests/platform/mac-wk2/TestExpectations 2017-08-31 16:55:50 UTC (rev 221423)
@@ -754,8 +754,6 @@
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 ]
Modified: trunk/Source/WebCore/ChangeLog (221422 => 221423)
--- trunk/Source/WebCore/ChangeLog 2017-08-31 16:38:16 UTC (rev 221422)
+++ trunk/Source/WebCore/ChangeLog 2017-08-31 16:55:50 UTC (rev 221423)
@@ -1,3 +1,27 @@
+2017-08-22 Darin Adler <[email protected]>
+
+ 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-22 Filip Pizlo <[email protected]>
Strings need to be in some kind of gigacage
Modified: trunk/Source/WebCore/dom/Document.cpp (221422 => 221423)
--- trunk/Source/WebCore/dom/Document.cpp 2017-08-31 16:38:16 UTC (rev 221422)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-08-31 16:55:50 UTC (rev 221423)
@@ -460,7 +460,7 @@
, 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 @@
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()
Modified: trunk/Source/WebCore/dom/Document.h (221422 => 221423)
--- trunk/Source/WebCore/dom/Document.h 2017-08-31 16:38:16 UTC (rev 221422)
+++ trunk/Source/WebCore/dom/Document.h 2017-08-31 16:55:50 UTC (rev 221423)
@@ -544,7 +544,7 @@
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; }
Modified: trunk/Source/WebCore/page/FrameView.cpp (221422 => 221423)
--- trunk/Source/WebCore/page/FrameView.cpp 2017-08-31 16:38:16 UTC (rev 221422)
+++ trunk/Source/WebCore/page/FrameView.cpp 2017-08-31 16:55:50 UTC (rev 221423)
@@ -4575,6 +4575,16 @@
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;
@@ -4588,32 +4598,52 @@
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());
}
