Relayout if FrameView::updateViewSize() set needsLayout

third_party/WebKit/Source/core/frame/FrameView.cpp

Index: third_party/WebKit/Source/core/frame/FrameView.cpp
diff --git a/third_party/WebKit/Source/core/frame/FrameView.cpp b/third_party/WebKit/Source/core/frame/FrameView.cpp
index f881a6042a874b94b9ee6295408a6c25b0d0dcb8..a2c059e2e82c11a9a6e99961a5865a063e474960 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -161,6 +161,7 @@ FrameView::FrameView(LocalFrame* frame)
     , m_isUpdatingAllLifecyclePhases(false)
     , m_scrollAnchor(this)
     , m_needsScrollbarsUpdate(false)
+    , m_suppressAdjustViewSize(false)
 {
     ASSERT(m_frame);
     init();
@@ -531,6 +532,9 @@ void FrameView::setContentsSize(const IntSize& size)
 
 void FrameView::adjustViewSize()
 {
+    if (m_suppressAdjustViewSize)
+        return;
+
     LayoutViewItem layoutViewItem = this->layoutViewItem();
     if (layoutViewItem.isNull())
         return;
@@ -1039,8 +1043,13 @@ void FrameView::layout()
         ASSERT(m_layoutSubtreeRootList.isEmpty());
     } // Reset m_layoutSchedulingEnabled to its previous value.
 
-    if (!inSubtreeLayout && !document->printing())
+    if (!inSubtreeLayout && !document->printing()) {
         adjustViewSize();
+        if (needsLayout()) {
+            TemporaryChange<bool> suppressAdjustViewSize(m_suppressAdjustViewSize, true);

[chrishtr, 2016/05/31 19:58:15]

WebViewImpl::layoutUpdated will do this layout already, no? Do you have a case
where that doesn't happen?

[Xianzhu, 2016/05/31 20:22:54]

I'm using the case in crbug.com/611290. On m51, it crashes at the
RELEASE_ASSERT(!needsLayout()) just after the layout() call in
FrameView::updateStyleAndLayoutIfNeededRecursiveInternal().

On m53, the page doesn't crash in release build, because the RELEASE_ASSERT
doesn't trigger for document lifecycle updates. However, if I add
RELEASE_ASSERT(!needsLayout()) at the end of FrameView::layout(), it still
crashes:

#2 0x00000227001d blink::FrameView::layout()
#3 0x0000024afda9 blink::LayoutPart::updateWidgetGeometry()
#4 0x000002270c37 blink::FrameView::updateWidgetGeometries()
#5 0x00000226e88f blink::FrameView::performPostLayoutTasks()
#6 0x00000226f1a2 blink::FrameView::scheduleOrPerformPostLayoutTasks()
#7 0x00000226fe12 blink::FrameView::layout()
#8 0x000001e839e4 blink::Document::updateStyleAndLayout()
#9 0x000001ea52eb blink::Element::clientWidth()

This means that WebViewImpl::layoutUpdated() didn't update layout for this
FrameView (perhaps because it's not a local root).

Also layoutUpdated() is too late. With DCHECK enabled, the needsLayout flag set
by updateViewSize() also causes assertion failure after updateViewSize() and
layoutUpdated() (e.g. in
CompositingReasonFinder::requiresCompositingForScrollableFrame()).

[chrishtr, 2016/05/31 20:31:38]

Ok then it appears we need a more general version of what WebViewImpl is doing.
The layout there is
for resizes, which may agree with your proposed approach...

[Xianzhu, 2016/05/31 20:54:27]

I would like to investigate this but the change would not be suitable to be
merged in m51. Will file a bug for this.

+            layout();

[chrishtr, 2016/05/31 20:31:38]

Does it make more sense to push this down into adjustViewSize() ? There are
other call sites that might
need the same behavior.

[Xianzhu, 2016/05/31 20:54:27]

This applies to the call site in FrameView::forceLayoutForPagination(), but not
in recalcOverflowAfterStyleChange() which should leave the needsLayout flag to
the later layout.

Added adjustViewSizeAndLayout() for the cases needing a clean layout.

+        }
+    }
 
     m_frameTimingRequestsDirty = true;