Relayout if FrameView::updateViewSize() set needsLayout

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

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Dirk Mueller <mueller@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *           (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
@@ skipping 143 matching lines @@
     , m_scrollbarsSuppressed(false)
     , m_inUpdateScrollbars(false)
     , m_frameTimingRequestsDirty(true)
     , m_viewportIntersectionValid(false)
     , m_hiddenForThrottling(false)
     , m_crossOriginForThrottling(false)
     , m_subtreeThrottled(false)
     , m_isUpdatingAllLifecyclePhases(false)
     , m_scrollAnchor(this)
     , m_needsScrollbarsUpdate(false)
+    , m_suppressAdjustViewSize(false)
 {
     ASSERT(m_frame);
     init();
 }
 
 FrameView* FrameView::create(LocalFrame* frame)
 {
     FrameView* view = new FrameView(frame);
     view->show();
     return view;
@@ skipping 350 matching lines @@
         return;
 
     updateScrollableAreaSet();
 
     page->chromeClient().contentsSizeChanged(m_frame.get(), size);
     frame().loader().restoreScrollPositionAndViewState();
 }
 
 void FrameView::adjustViewSize()
 {
+    if (m_suppressAdjustViewSize)
+        return;
+
     LayoutViewItem layoutViewItem = this->layoutViewItem();
     if (layoutViewItem.isNull())
         return;
 
     ASSERT(m_frame->view() == this);
 
     const IntRect rect = layoutViewItem.documentRect();
     const IntSize& size = rect.size();
 
     const IntPoint origin(-rect.x(), -rect.y());
@@ skipping 488 matching lines @@
         }
 
         TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("blink.debug.layout"), "LayoutTree",
             this, TracedLayoutObject::create(*layoutView(), false));
 
         performLayout(inSubtreeLayout);
 
         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;
 
     // FIXME: Could find the common ancestor layer of all dirty subtrees and mark from there. crbug.com/462719
     layoutViewItem().enclosingLayer()->updateLayerPositionsAfterLayout();
 
     TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("blink.debug.layout"), "LayoutTree",
         this, TracedLayoutObject::create(*layoutView(), true));
 
     layoutViewItem().compositor()->didLayout();
@@ skipping 3061 matching lines @@
     return m_subtreeThrottled || (m_hiddenForThrottling && m_crossOriginForThrottling);
 }
 
 LayoutBox& FrameView::boxForScrollControlPaintInvalidation() const
 {
     ASSERT(!layoutViewItem().isNull());
     return *layoutView();
 }
 
 } // namespace blink