Page::setPageScaleFactor should layout the FrameView if it needs layout regardless of scroll position

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Fady Samuel]

Trivial change, I need to come up with a simple test for it though.

[Fady Samuel]

Created attachment 112147 [details]
Patch

[Darin Adler]

Comment on attachment 112147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112147&action=review

> Source/WebCore/ChangeLog:3
> +        Page::setPageScaleFactor should layout the FrameView if it needs layout regardless of scroll position

Why?

[Fady Samuel]

The change is trivial. I discovered the issue while implementing/testing the Viewport meta tag for Chromium. I don't yet have a reduction. I'll try to have something ready by tomorrow.

[Darin Adler]

Please supply the reason for the change.

[Fady Samuel]

(In reply to comment #3)
> The change is trivial. I discovered the issue while implementing/testing the Viewport meta tag for Chromium. I don't yet have a reduction. I'll try to have something ready by tomorrow.

In a nutshell, I'm seeing cases where FrameView::paintContents has an ASSERT(!needsLayout()) that's failing while testing setPageScaleFactor in conjugation with enabling fixed layout. This small change eliminates the problem. I'm not sure if this is the right solution. I logged the bug anyway to remind myself to investigate this further.

[Darin Adler]

(In reply to comment #5)
> I'm seeing cases where FrameView::paintContents has an ASSERT(!needsLayout()) that's failing while testing setPageScaleFactor in conjugation with enabling fixed layout. This small change eliminates the problem. I'm not sure if this is the right solution. I logged the bug anyway to remind myself to investigate this further.

Aha! Given your description, I am sure it is not the right solution. The code that depends on layout should be the code that triggers layout. We should not change this function because we want to rely on its side effect.

[Darin Adler]

If you show me the backtrace of the needsLayout assertion I can help you figure out what code should be responsible for doing the layout.

[Fady Samuel]

(In reply to comment #7)
> If you show me the backtrace of the needsLayout assertion I can help you figure out what code should be responsible for doing the layout.

Oddly enough, I can't repro this anymore. If I can't repro again for another day or two, I'll mark this bug as invalid and close it.

[Fady Samuel]

(In reply to comment #8)
> (In reply to comment #7)
> > If you show me the backtrace of the needsLayout assertion I can help you figure out what code should be responsible for doing the layout.
> 
> Oddly enough, I can't repro this anymore. If I can't repro again for another day or two, I'll mark this bug as invalid and close it.

Ah ha! Finally, repo'ing again!


#0  0x00007ffff3b7e9c8 in WebCore::FrameView::paintContents (this=0x7fffd5fa2000, p=0x7fffdd6171d0, rect=...) at third_party/WebKit/Source/WebCore/page/FrameView.cpp:2732
#1  0x00007ffff3729118 in WebCore::ScrollView::paint (this=0x7fffd5fa2000, context=0x7fffdd6171d0, rect=...) at third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:1046
#2  0x00007ffff3390f93 in WebKit::WebFrameImpl::paintWithContext (this=0x7fffde3301c0, gc=..., rect=...) at third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1977
#3  0x00007ffff33910a3 in WebKit::WebFrameImpl::paint (this=0x7fffde3301c0, canvas=0x7fffcda2fe00, rect=...) at third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1989
#4  0x00007ffff33b5ffc in WebKit::WebViewImpl::paint (this=0x7fffd8554c80, canvas=0x7fffcda2fe00, rect=...) at third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1150
#5  0x00007ffff4b97de1 in RenderWidget::PaintRect (this=0x7fffd53b4400, rect=..., canvas_origin=..., canvas=0x7fffcda2fe00) at content/renderer/render_widget.cc:584
#6  0x00007ffff4b99d1b in RenderWidget::DoDeferredUpdate (this=0x7fffd53b4400) at content/renderer/render_widget.cc:822
#7  0x00007ffff4b98b6f in RenderWidget::DoDeferredUpdateAndSendInputAck (this=0x7fffd53b4400) at content/renderer/render_widget.cc:695
#8  0x00007ffff4b98b3f in RenderWidget::InvalidationCallback (this=0x7fffd53b4400) at content/renderer/render_widget.cc:691
#9  0x00007ffff4b9e7c7 in void DispatchToMethod<RenderWidget, void (RenderWidget::*)()>(RenderWidget*, void (RenderWidget::*)(), Tuple0 const&) ()
#10 0x00007ffff4b9e70a in RunnableMethod<RenderWidget, void (RenderWidget::*)(), Tuple0>::Run() ()
#11 0x00007ffff2648ae7 in base::subtle::TaskClosureAdapter::Run (this=0x7fffd8ad9360) at base/task.cc:71
#12 0x00007ffff2601b30 in base::internal::Invoker1<false, base::internal::InvokerStorage1<void (base::subtle::TaskClosureAdapter::*)(), base::subtle::TaskClosureAdapter*>, void (base::subtle::TaskClosureAdapter::*)()>::DoInvoke (
    base=0x7fffd5235120) at ./base/bind_internal.h:596
#13 0x00007ffff1ca26bf in base::Callback<void()>::Run(void) const (this=0x7fffdd618560) at ./base/callback.h:269
#14 0x00007ffff25fe9ae in MessageLoop::RunTask (this=0x7fffdd618ba0, pending_task=...) at base/message_loop.cc:495
#15 0x00007ffff25feaed in MessageLoop::DeferOrRunPendingTask (this=0x7fffdd618ba0, pending_task=...) at base/message_loop.cc:509
#16 0x00007ffff25ff303 in MessageLoop::DoWork (this=0x7fffdd618ba0) at base/message_loop.cc:699
#17 0x00007ffff26071bc in base::MessagePumpDefault::Run (this=0x7fffe46532a0, delegate=0x7fffdd618ba0) at base/message_pump_default.cc:23
#18 0x00007ffff25fe633 in MessageLoop::RunInternal (this=0x7fffdd618ba0) at base/message_loop.cc:453
#19 0x00007ffff25fe4e6 in MessageLoop::RunHandler (this=0x7fffdd618ba0) at base/message_loop.cc:426
#20 0x00007ffff25fde9f in MessageLoop::Run (this=0x7fffdd618ba0) at base/message_loop.cc:341
#21 0x00007ffff264b6a6 in base::Thread::Run (this=0x7fffe4626c80, message_loop=0x7fffdd618ba0) at base/threading/thread.cc:129
#22 0x00007ffff264b839 in base::Thread::ThreadMain (this=0x7fffe4626c80) at base/threading/thread.cc:167
#23 0x00007ffff264a3cb in base::(anonymous namespace)::ThreadFunc (params=0x7fffdd9e5060) at base/threading/platform_thread_posix.cc:54
#24 0x00007fffec32c9ca in start_thread (arg=<value optimized out>) at pthread_create.c:300
#25 0x00007fffe9b1a70d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112

[Darin Adler]

(In reply to comment #9)
> #1  0x00007ffff3729118 in WebCore::ScrollView::paint (this=0x7fffd5fa2000, context=0x7fffdd6171d0, rect=...) at third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:1046
> #2  0x00007ffff3390f93 in WebKit::WebFrameImpl::paintWithContext (this=0x7fffde3301c0, gc=..., rect=...) at third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1977
> #3  0x00007ffff33910a3 in WebKit::WebFrameImpl::paint (this=0x7fffde3301c0, canvas=0x7fffcda2fe00, rect=...) at third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1989
> #4  0x00007ffff33b5ffc in WebKit::WebViewImpl::paint (this=0x7fffd8554c80, canvas=0x7fffcda2fe00, rect=...) at third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:1150
> #5  0x00007ffff4b97de1 in RenderWidget::PaintRect (this=0x7fffd53b4400, rect=..., canvas_origin=..., canvas=0x7fffcda2fe00) at content/renderer/render_widget.cc:584
> #6  0x00007ffff4b99d1b in RenderWidget::DoDeferredUpdate (this=0x7fffd53b4400) at content/renderer/render_widget.cc:822

The bug is here somewhere in this Chromium platform code.

There’s a deferred paint. But between the time the paint was deferred and now, some change may have occurred that triggered the need for layout.

To accommodate that case one of these functions has to do a layout. Maybe RenderWidget::DoDeferredUpdate or maybe one of the five functions it calls.

[Fady Samuel]

Can be (more appropriately) fixed on the Chromium-side of the world.