Relayout if FrameView::updateViewSize() set needsLayout

Relayout if FrameView::updateViewSize() set needsLayout

BUG=590856
R=chrishtr@chromium.org

Committed: https://crrev.com/9eba18f3e99ba3ac91bc0beab8d52cd9f38f3d42
Cr-Commit-Position: refs/heads/master@{#396962}

[Xianzhu, 2016-05-31 19:33:28]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, eae@chromium.org

[Xianzhu, 2016-05-31 19:33:29]

Hopefully this is the main reason of the crash (the 5th crash on m51 for now).
Would like to land it today into m51 before the stable cut.

Don't know how to test it yet. Will try to create a test but might not succeed
before the branch cut.

[eae, 2016-05-31 19:49:09]

eae@chromium.org changed reviewers:
+ szager@chromium.org

[eae, 2016-05-31 19:49:11]

+szager

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

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1049: TemporaryChange<bool>
suppressAdjustViewSize(m_suppressAdjustViewSize, true);
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]

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1049: TemporaryChange<bool>
suppressAdjustViewSize(m_suppressAdjustViewSize, true);
On 2016/05/31 19:58:15, chrishtr wrote:
> WebViewImpl::layoutUpdated will do this layout already, no? Do you have a case
> where that doesn't happen?

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]

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1049: TemporaryChange<bool>
suppressAdjustViewSize(m_suppressAdjustViewSize, true);
On 2016/05/31 at 20:22:54, Xianzhu wrote:
> On 2016/05/31 19:58:15, chrishtr wrote:
> > WebViewImpl::layoutUpdated will do this layout already, no? Do you have a
case
> > where that doesn't happen?
> 
> 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()).

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...

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1050: layout();
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]

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1049: TemporaryChange<bool>
suppressAdjustViewSize(m_suppressAdjustViewSize, true);
On 2016/05/31 20:31:38, chrishtr wrote:
> On 2016/05/31 at 20:22:54, Xianzhu wrote:
> > On 2016/05/31 19:58:15, chrishtr wrote:
> > > WebViewImpl::layoutUpdated will do this layout already, no? Do you have a
> case
> > > where that doesn't happen?
> > 
> > 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()).
> 
> 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...

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

https://codereview.chromium.org/2022013004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:1050: layout();
On 2016/05/31 20:31:38, chrishtr wrote:
> Does it make more sense to push this down into adjustViewSize() ? There are
> other call sites that might
> need the same behavior.

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.

[chrishtr, 2016-05-31 21:08:24]

lgtm

[Xianzhu, 2016-05-31 21:26:14]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-05-31 21:26:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2022013004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2022013004/20001

[commit-bot: I haz the power, 2016-05-31 22:59:16]

Description was changed from

==========
Relayout if FrameView::updateViewSize() set needsLayout

BUG=590856
==========

to

==========
Relayout if FrameView::updateViewSize() set needsLayout

BUG=590856
R=chrishtr@chromium.org

Committed: https://crrev.com/9eba18f3e99ba3ac91bc0beab8d52cd9f38f3d42
Cr-Commit-Position: refs/heads/master@{#396962}
==========

[commit-bot: I haz the power, 2016-05-31 22:59:17]

Patchset 2 (id:??) landed as
https://crrev.com/9eba18f3e99ba3ac91bc0beab8d52cd9f38f3d42
Cr-Commit-Position: refs/heads/master@{#396962}

[szager1, 2016-06-01 07:48:57]

Sorry for the late drive-by...

https://codereview.chromium.org/2022013004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2022013004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1055: if (!inSubtreeLayout &&
!document->printing())
Can you make adjustViewSizeAndLayout return a bool indicating whether it
actually ran layout, and add an early return here?  Like this:

if (!inSubtreeLayout && !document->printing() && adjustViewSizeAndLayout()
  return;

Without this change, the rest of the code in layout() will redundantly run
twice.

[esprehn, 2016-06-06 03:26:20]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-06-06 03:26:21]

This needs a test, you should be able to easily unit test this.

[dglazkov, 2016-06-06 03:58:17]

On 2016/06/06 at 03:26:21, esprehn wrote:
> This needs a test, you should be able to easily unit test this.

... also, could y'all file a bug to come up with a proper solution here? I am
totally fine with time-critical short-term fixes, but we need to be careful not
to get those piled up into more technical debt.

[Xianzhu, 2016-06-06 04:21:38]

On 2016/06/06 03:26:21, esprehn wrote:
> This needs a test, you should be able to easily unit test this.

Tried but hadn't succeeded before the branch cut. Still not sure why needsLayout
is left after adjustViewSize(). Debugged with a suspected case and found that we
do double-layout when scrollbar existence changes. Just noticed more details
about double-layout in crbug.com/512914.

Anyway adding a test for this is still in my task list. Will try to reduce from
crbug.com/611290.

[Xianzhu, 2016-06-06 04:24:43]

On 2016/06/06 03:58:17, dglazkov wrote:
> On 2016/06/06 at 03:26:21, esprehn wrote:
> > This needs a test, you should be able to easily unit test this.
> 
> ... also, could y'all file a bug to come up with a proper solution here? I am
> totally fine with time-critical short-term fixes, but we need to be careful
not
> to get those piled up into more technical debt.

Hopefully such issues will disappear when rootLayerScrolls launches.

[eae, 2016-06-06 17:24:15]

LGTM

[dglazkov, 2016-06-06 18:36:11]

On 2016/06/06 at 04:24:43, wangxianzhu wrote:
> On 2016/06/06 03:58:17, dglazkov wrote:
> > On 2016/06/06 at 03:26:21, esprehn wrote:
> > > This needs a test, you should be able to easily unit test this.
> > 
> > ... also, could y'all file a bug to come up with a proper solution here? I
am
> > totally fine with time-critical short-term fixes, but we need to be careful
not
> > to get those piled up into more technical debt.
> 
> Hopefully such issues will disappear when rootLayerScrolls launches.

Great! In the past, I've found it useful to add comments to that effect to the
code that's added as a short-term fix. This both discourages spurious additional
callsites from being added, and helps the code archeologists -- just in case :)