70632 2011-10-21 12:46:04 -0700 Page::setPageScaleFactor should layout the FrameView if it needs layout regardless of scroll position 2011-11-30 16:23:00 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 70559 1 fsamuel fsamuel ap aroben bdakin darin rjkroege simon.fraser oldest_to_newest 488569 0 fsamuel 2011-10-21 12:46:04 -0700 Trivial change, I need to come up with a simple test for it though. 489167 1 112147 fsamuel 2011-10-23 22:14:26 -0700 Created attachment 112147 Patch 489169 2 112147 darin 2011-10-23 22:16:33 -0700 Comment on attachment 112147 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? 489171 3 fsamuel 2011-10-23 22:16:51 -0700 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. 489172 4 darin 2011-10-23 22:17:22 -0700 Please supply the reason for the change. 489173 5 fsamuel 2011-10-23 22:20:19 -0700 (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. 489176 6 darin 2011-10-23 22:26:49 -0700 (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. 489177 7 darin 2011-10-23 22:27:14 -0700 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. 490218 8 fsamuel 2011-10-25 12:15:05 -0700 (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. 490253 9 fsamuel 2011-10-25 12:45:39 -0700 (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 490348 10 darin 2011-10-25 14:13:13 -0700 (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. 512187 11 fsamuel 2011-11-30 15:36:10 -0800 Can be (more appropriately) fixed on the Chromium-side of the world. 112147 2011-10-23 22:14:26 -0700 2011-11-30 16:23:00 -0800 Patch bug-70632-20111024011424.patch text/plain 1515 fsamuel U3VidmVyc2lvbiBSZXZpc2lvbjogOTgyMDYKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBmZjZmM2NkNDJlYjJjZTNl M2RkOTQyNWI3ZjQxZWE4NWQ0YTE1MjQxLi5iMWEyMjJlODQ1MTM1NzhhNDNlMWNkZDM2MTdjNzY5 YWU5YzMxNTVkIDEwMDc1NQotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTEtMTAtMjMgIEZhZHkg U2FtdWVsICA8ZnNhbXVlbEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUGFnZTo6c2V0UGFnZVNj YWxlRmFjdG9yIHNob3VsZCBsYXlvdXQgdGhlIEZyYW1lVmlldyBpZiBpdCBuZWVkcyBsYXlvdXQg cmVnYXJkbGVzcyBvZiBzY3JvbGwgcG9zaXRpb24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTcwNjMyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgKiBwYWdlL1BhZ2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 UGFnZTo6c2V0UGFnZVNjYWxlRmFjdG9yKToKKwogMjAxMS0xMC0yMyAgTWFyayBIYWhuZW5iZXJn ICA8bWhhaG5lbmJlcmdAYXBwbGUuY29tPgogCiAgICAgICAgIEFkZCBkZWxldGVQcm9wZXJ0eSB0 byB0aGUgTWV0aG9kVGFibGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2UuY3BwCmluZGV4IDljNWIyNmQ3YzU2MWI3YWYy ZGYzODk5ZTA2MzhmN2EzNTMzN2QyMmEuLjg0YmQ1OWMzNTMwNzk5ZTU5ZGY4YzA2YTZiOTg3ZjJj ZTVlN2U2ZTAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvcGFnZS9QYWdlLmNwcApAQCAtNjM4LDExICs2MzgsMTAgQEAgdm9pZCBQ YWdlOjpzZXRQYWdlU2NhbGVGYWN0b3IoZmxvYXQgc2NhbGUsIGNvbnN0IExheW91dFBvaW50JiBv cmlnaW4pCiAjZW5kaWYKIAogICAgIGlmIChGcmFtZVZpZXcqIHZpZXcgPSBkb2N1bWVudC0+dmll dygpKSB7Ci0gICAgICAgIGlmICh2aWV3LT5zY3JvbGxQb3NpdGlvbigpICE9IG9yaWdpbikgewot ICAgICAgICAgIGlmIChkb2N1bWVudC0+cmVuZGVyZXIoKSAmJiBkb2N1bWVudC0+cmVuZGVyZXIo KS0+bmVlZHNMYXlvdXQoKSAmJiB2aWV3LT5kaWRGaXJzdExheW91dCgpKQotICAgICAgICAgICAg ICB2aWV3LT5sYXlvdXQoKTsKKyAgICAgICAgaWYgKGRvY3VtZW50LT5yZW5kZXJlcigpICYmIGRv Y3VtZW50LT5yZW5kZXJlcigpLT5uZWVkc0xheW91dCgpICYmIHZpZXctPmRpZEZpcnN0TGF5b3V0 KCkpCisgICAgICAgICAgICB2aWV3LT5sYXlvdXQoKTsKKyAgICAgICAgaWYgKHZpZXctPnNjcm9s bFBvc2l0aW9uKCkgIT0gb3JpZ2luKQogICAgICAgICAgIHZpZXctPnNldFNjcm9sbFBvc2l0aW9u KG9yaWdpbik7Ci0gICAgICAgIH0KICAgICB9CiB9CiAK