Bug 143675 – Regression: Scrolling on popsci.com spends too much time in FrameView::viewportsContentsChanged()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Dumez]

Scrolling on popsci.com spends too much time in FrameView::viewportsContentsChanged().

Radar: <rdar://problem/20507791>

[Chris Dumez]

Created attachment 250666 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 250666 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2142
>          if (auto* renderView = frame->contentRenderer())
> -            renderView->resumePausedImageAnimationsIfNeeded();
> +            renderView->resumePausedImageAnimationsIfNeeded(visibleRect);

There's a behavior change here (and you don't have any tests that noticed it). Previously you were computing windowClipRect() for each subframe. Now you're just using the main frame's windowClipRect for those subframes, which seems wrong.

[Chris Dumez]

Created attachment 250683 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 250683 [details]
Patch

Clearing flags on attachment: 250683

Committed r182773: <http://trac.webkit.org/changeset/182773>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This appears to have broken Windows pretty thoroughly: <https://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/r182776%20(65689)/results.html>.

[Chris Dumez]

(In reply to comment #6)
> This appears to have broken Windows pretty thoroughly:
> <https://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/
> r182776%20(65689)/results.html>.

Looking now.

[Chris Dumez]

(In reply to comment #7)
> (In reply to comment #6)
> > This appears to have broken Windows pretty thoroughly:
> > <https://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/
> > r182776%20(65689)/results.html>.
> 
> Looking now.

Looks like the failures started later? This is when my patch landed on this bot:
https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/65687

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > This appears to have broken Windows pretty thoroughly:
> > > <https://build.webkit.org/results/Apple%20Win%207%20Debug%20(Tests)/
> > > r182776%20(65689)/results.html>.
> > 
> > Looking now.
> 
> Looks like the failures started later? This is when my patch landed on this
> bot:
> https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/
> builds/65687

Never mind, it does seem related to my change. Most crashes don't have a backtrace but I found one:
0018eee8 05ff4290 064f14f7 0018ef44 00000001 WTF!WTFCrash+0x21
0018ef4c 05ff4952 0018ef58 00000001 0018ef9c WebKit!WebCore::FrameView::windowClipRect+0x40
0018ef90 05ff490f 0ebc3ea8 0018efbc 05ffa4ff WebKit!WebCore::FrameView::resumeVisibleImageAnimationsIncludingSubframes+0x32
0018ef9c 05ffa4ff 0ebc3ea8 0060aa90 006060c0 WebKit!WebCore::FrameView::viewportContentsChanged+0xf
0018efbc 05ff56bf 0ebc3ea8 0018efd4 06911367 WebKit!WebCore::FrameView::performPostLayoutTasks+0x12f
0018efc8 06911367 0d18bc60 0018efe4 067e0259 WebKit!WebCore::FrameView::postLayoutTimerFired+0xf
0018efd4 067e0259 0ebc3ea8 0d18bc60 0018effc WebKit!std::_Pmf_wrap<void (__thiscall WebCore::WebSocket::*)(void),void,WebCore::WebSocket>::operator()+0x17
0018efe4 067e00f5 00000000 00000000 0d18bc60 WebKit!std::_Bind<1,void,std::_Pmf_wrap<void (__thiscall WebCore::XMLHttpRequest::*)(void),void,WebCore::XMLHttpRequest>,WebCore::XMLHttpRequest *>::_Do_call<,0>+0x39
0018effc 067e0116 0d18bc60 0018f014 067e0a72 WebKit!std::_Bind<1,void,std::_Pmf_wrap<void (__thiscall WebCore::XMLHttpRequest::*)(void),void,WebCore::XMLHttpRequest>,WebCore::XMLHttpRequest *>::operator()<>+0x25
0018f008 067e0a72 0d18bc58 0018f020 05cd5f78 WebKit!std::_Callable_obj<std::_Bind<1,void,std::_Pmf_wrap<void (__thiscall WebCore::XMLHttpRequest::*)(void),void,WebCore::XMLHttpRequest>,WebCore::XMLHttpRequest *>,0>::_ApplyX<void>+0x16
0018f014 05cd5f78 0ebc4050 0018f02c 05cd6f32 WebKit!std::_Func_impl<std::_Callable_obj<std::_Bind<1,void,std::_Pmf_wrap<void (__thiscall WebCore::XMLHttpRequest::*)(void),void,WebCore::XMLHttpRequest>,WebCore::XMLHttpRequest *>,0>,std::allocator<std::_Func_class<void> >,void>::_Do_call+0x12
0018f020 05cd6f32 0ebc4018 0018f05c 066d341c WebKit!std::_Func_class<void>::operator()+0x28
0018f02c 066d341c d3d79865 41d54b72 00000000 WebKit!WebCore::Timer::fired+0x12
0018f05c 066d32f6 0018f06c 06ad16bf 0018f098 WebKit!WebCore::ThreadTimers::sharedTimerFiredInternal+0x11c
0018f064 06ad16bf 0018f098 76f862fa 003d074e WebKit!WebCore::ThreadTimers::sharedTimerFired+0x16
0018f06c 76f862fa 003d074e 0000c126 00000000 WebKit!WebCore::TimerWindowWndProc+0x7f

[Chris Dumez]

The assertion being hit is likely this one, even though the line number does not exactly match:
ASSERT(frame().view() == this);

I am not sure yet how my patch could impact this invariant. Also, the crashes appear to be flaky on the Windows bot.

[Chris Dumez]

(In reply to comment #10)
> The assertion being hit is likely this one, even though the line number does
> not exactly match:
> ASSERT(frame().view() == this);
> 
> I am not sure yet how my patch could impact this invariant. Also, the
> crashes appear to be flaky on the Windows bot.

Windows bot is now in good shape after https://bugs.webkit.org/show_bug.cgi?id=143723.