Bug 60194 – REGRESSION (r79167): Windowed plugins in Google Reader don't move when the article list is scrolled

• Component: Plug-ins
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

To reproduce:

1. View http://kottke.org/11/05/teach-me-how-to-dougie in Google Reader
2. Scroll the article list

The YouTube video stays in place while the article list scrolls underneath it. This happens in both WebKit1 and WebKit2. It does not happen on Mac.

[Adam Roben (:aroben)]

The bug does not occur in Safari 5.

[Adam Roben (:aroben)]

<rdar://problem/9383760>

[Adam Roben (:aroben)]

Reduction:

data:text/html,<div style="overflow:scroll;width:300px"><iframe width=500 height=314 src="http://www.youtube.com/embed/mYP4MgxDV2U" frameborder=0 allowfullscreen>

[Adam Roben (:aroben)]

Further reduction:


data:text/html,<div style="overflow:scroll;width:300px"><iframe width=500 height=314 src="http://www.youtube.com/embed/mYP4MgxDV2U">

[Adam Roben (:aroben)]

r79167 is starting to look suspicious: <http://trac.webkit.org/changeset/79167>

[Adam Roben (:aroben)]

Indeed: r79166 does not have the bug, r79167 does.

[Adam Roben (:aroben)]

Rolling out r79167 makes the bug go away.

[Adam Roben (:aroben)]

Here's the backtrace when the plugin is repositioned due to scrolling with r79167 rolled out:


>	WebKit.dll!WebKit::NetscapePlugin::scheduleWindowedGeometryUpdate()  Line 174	C++
 	WebKit.dll!WebKit::NetscapePlugin::platformGeometryDidChange()  Line 149	C++
 	WebKit.dll!WebKit::NetscapePlugin::geometryDidChange(const WebCore::IntRect & frameRect={...}, const WebCore::IntRect & clipRect={...})  Line 549	C++
 	WebKit.dll!WebKit::PluginView::viewGeometryDidChange()  Line 640 + 0x2e bytes	C++
 	WebKit.dll!WebKit::PluginView::frameRectsChanged()  Line 547	C++
 	WebKit.dll!WebCore::ScrollView::frameRectsChanged()  Line 841 + 0x21 bytes	C++
 	WebKit.dll!WebCore::ScrollView::setFrameRect(const WebCore::IntRect & newRect={...})  Line 831 + 0xf bytes	C++
 	WebKit.dll!WebCore::FrameView::setFrameRect(const WebCore::IntRect & newRect={...})  Line 374	C++
 	WebKit.dll!WebCore::RenderWidget::setWidgetGeometry(const WebCore::IntRect & frame={...}, const WebCore::IntSize & boundsSize={...})  Line 177 + 0x21 bytes	C++
 	WebKit.dll!WebCore::RenderWidget::updateWidgetPosition()  Line 347 + 0x1c bytes	C++
 	WebKit.dll!WebCore::RenderView::updateWidgetPositions()  Line 618 + 0x13 bytes	C++
 	WebKit.dll!WebCore::RenderLayer::scrollTo(int x=6, int y=0)  Line 1368	C++
 	WebKit.dll!WebCore::RenderLayer::setScrollOffset(const WebCore::IntPoint & offset={...})  Line 1656	C++
 	WebKit.dll!WebCore::ScrollableArea::setScrollOffsetFromAnimation(const WebCore::IntPoint & offset={...})  Line 140 + 0x13 bytes	C++
 	WebKit.dll!WebCore::ScrollAnimator::notityPositionChanged()  Line 130	C++
 	WebKit.dll!WebCore::ScrollAnimator::scrollToOffsetWithoutAnimation(const WebCore::FloatPoint & offset={...})  Line 80 + 0xf bytes	C++
 	WebKit.dll!WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(const WebCore::FloatPoint & offset={...})  Line 104 + 0x1e bytes	C++
 	WebKit.dll!WebCore::ScrollableArea::scrollToXOffsetWithoutAnimation(float x=6.0272727)  Line 118	C++
 	WebKit.dll!WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(WebCore::ScrollbarOrientation orientation=HorizontalScrollbar, float offset=6.0272727)  Line 111	C++
 	WebKit.dll!WebCore::Scrollbar::moveThumb(int pos=124, bool draggingDocument=false)  Line 302	C++
 	WebKit.dll!WebCore::Scrollbar::mouseMoved(const WebCore::PlatformMouseEvent & evt={...})  Line 340	C++
 	WebKit.dll!WebCore::EventHandler::handleMouseMoveEvent(const WebCore::PlatformMouseEvent & mouseEvent={...}, WebCore::HitTestResult * hoveredNode=0x0034f240)  Line 1574 + 0x19 bytes	C++
 	WebKit.dll!WebCore::EventHandler::mouseMoved(const WebCore::PlatformMouseEvent & event={...})  Line 1524 + 0x10 bytes	C++
 	WebKit.dll!WebKit::handleMouseEvent(const WebKit::WebMouseEvent & mouseEvent={...}, WebCore::Page * page=0x00abef00)  Line 1011 + 0x13 bytes	C++
 	WebKit.dll!WebKit::WebPage::mouseEvent(const WebKit::WebMouseEvent & mouseEvent={...})  Line 1037 + 0x15 bytes	C++
 	WebKit.dll!CoreIPC::callMemberFunction<WebKit::WebPage,void (__thiscall WebKit::WebPage::*)(WebKit::WebMouseEvent const &),WebKit::WebMouseEvent>(const CoreIPC::Arguments1<WebKit::WebMouseEvent> & args={...}, WebKit::WebPage * object=0x00aefd70, void (const WebKit::WebMouseEvent &)* function=0x53298e36)  Line 19 + 0xf bytes	C++
 	WebKit.dll!CoreIPC::handleMessage<Messages::WebPage::MouseEvent,WebKit::WebPage,void (__thiscall WebKit::WebPage::*)(WebKit::WebMouseEvent const &)>(CoreIPC::ArgumentDecoder * argumentDecoder=0x053cf0f8, WebKit::WebPage * object=0x00aefd70, void (const WebKit::WebMouseEvent &)* function=0x53298e36)  Line 265 + 0x15 bytes	C++
 	WebKit.dll!WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection * __formal=0x00a5aca8, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x053cf0f8)  Line 88 + 0x23 bytes	C++
 	WebKit.dll!WebKit::WebPage::didReceiveMessage(CoreIPC::Connection * connection=0x00a5aca8, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x053cf0f8)  Line 1974	C++
 	WebKit.dll!WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection * connection=0x00a5aca8, CoreIPC::MessageID messageID={...}, CoreIPC::ArgumentDecoder * arguments=0x053cf0f8)  Line 644	C++
 	WebKit.dll!CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::ArgumentDecoder> & message={...})  Line 690 + 0x30 bytes	C++
 	WebKit.dll!CoreIPC::Connection::dispatchMessages()  Line 714 + 0x15 bytes	C++
 	WebKit.dll!MemberFunctionWorkItem0<CoreIPC::Connection>::execute()  Line 76 + 0x10 bytes	C++
 	WebKit.dll!RunLoop::performWork()  Line 63 + 0x1a bytes	C++
 	WebKit.dll!RunLoop::wndProc(HWND__ * hWnd=0x000f01f2, unsigned int message=1025, unsigned int wParam=10789288, long lParam=0)  Line 62	C++
 	WebKit.dll!RunLoop::RunLoopWndProc(HWND__ * hWnd=0x000f01f2, unsigned int message=1025, unsigned int wParam=10789288, long lParam=0)  Line 44 + 0x18 bytes	C++
 	user32.dll!_InternalCallWinProc@20()  + 0x23 bytes	
 	user32.dll!_UserCallWinProcCheckWow@32()  + 0xb7 bytes	
 	user32.dll!_DispatchMessageWorker@8()  + 0xed bytes	
 	user32.dll!_DispatchMessageW@4()  + 0xf bytes	
 	WebKit.dll!RunLoop::run()  Line 78 + 0xc bytes	C++
 	WebKit.dll!WebKit::WebProcessMain(const WebKit::CommandLine & commandLine={...})  Line 82	C++
 	WebKit.dll!WebKitMain(const WebKit::CommandLine & commandLine={...})  Line 48 + 0x9 bytes	C++
 	WebKit.dll!WebKitMain(HINSTANCE__ * hInstance=0x01160000, HINSTANCE__ * hPrevInstance=0x00000000, wchar_t * lpstrCmdLine=0x0077463a, int nCmdShow=10)  Line 172 + 0x9 bytes	C++
 	WebKit2WebProcess.exe!wWinMain(HINSTANCE__ * hInstance=0x01160000, HINSTANCE__ * hPrevInstance=0x00000000, wchar_t * lpstrCmdLine=0x0077463a, int nCmdShow=10)  Line 66 + 0x18 bytes	C++
 	WebKit2WebProcess.exe!__tmainCRTStartup()  Line 589 + 0x1c bytes	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes

[Adam Roben (:aroben)]

Presumably the bug is that we're not calling frameRectsChanged from ScrollView::setFrameRect anymore during scrolling.

[Adam Roben (:aroben)]

Moving the frameRectsChanged call from setBoundsSize to setFrameRect seems to fix the bug. Now I need to write a test.

[Adam Roben (:aroben)]

Created attachment 92656 [details]
Tell ScrollView's children their frameRects have changed when its own frameRect changes

[Adam Roben (:aroben)]

Comment on attachment 92656 [details]
Tell ScrollView's children their frameRects have changed when its own frameRect changes

I'm a little worried that I may have broken some things with this patch. I'm going to do some more testing on Monday to try to find out.

[Adam Roben (:aroben)]

Comment on attachment 92656 [details]
Tell ScrollView's children their frameRects have changed when its own frameRect changes

Gonna do this a liiiiiittle differently.

[Adam Roben (:aroben)]

I think I might be able to make a non-pixel test, too, by dumping the position of the plugin HWND.

[Adam Roben (:aroben)]

I talked to James Robinson and came up with a plan. Here are some snippets of our conversation in IRC:

<aroben> jamesr: including the call to frameRectsChanged
<aroben> jamesr: this was done on the assumption that scrollbars didn't need to be respositioned when the ScrollView is repositioned
<aroben> jamesr: (I guess that means that scrollbars are positioned relative to the ScrollView, and move along with it automatically)
<jamesr> they are, yeah
<aroben> jamesr: that change caused https://bugs.webkit.org/show_bug.cgi?id=60194 because frameRectsChanged did more than just reposition scrollbars
<aroben> jamesr: so I'm trying to figure out how to fix it
<jamesr> my vague recollection is that setFrameRects did a whole lot and it wasn't clear to me what it should or shouldn't be doing
<aroben> jamesr: I could move the positionScrollbarLayers call out of frameRectsChanged
<jamesr> the call is there to try to catch adding/removing scrollbars, i think
<jamesr> aroben: the other difficulty for you is that scrollbar layers are only created on the mac port when overlay scrollbars are in use, and last i checked DumpRenderTree did not use overlay scrollbars on any platform
<aroben> jamesr: frameRectsChanged is called in four places: 1) updateScrollbars (when scrollbars are added/removed), 2) scrollContents, 3) setBoundsSize, 4) recursively from frameRectsChanged
<jamesr> yeah, you should only need the positionScrollbarLayers() call in (1) and (3)
<aroben> jamesr: ok, so maybe I should call positionScrollbarLayers() explicitly in those two places and move it out of frameRectsChanged
<aroben> jamesr: and then move the frameRectsChanged call back from setBoundsSize to setFrameRect
<jamesr> aroben: that sounds like it should work!

[Adam Roben (:aroben)]

I have a new (dumpAsText) test that should show this bug. We put a plugin inside an iframe inside a scrolled overflow area and dump the plugin window's rect. But for some reason it's giving me the same behavior both before and after fixing this bug!

[Adam Roben (:aroben)]

I now have a working test. Time to fix the bug!

[Adam Roben (:aroben)]

Created attachment 93470 [details]
Tell ScrollView's child Widgets that their frame rects have changed when its own frame rect changes

[Adam Roben (:aroben)]

Committed r86442: <http://trac.webkit.org/changeset/86442>

[Ademar Reis]

Revision r86442 cherry-picked into qtwebkit-2.2 with commit 457a0f0 <http://gitorious.org/webkit/qtwebkit/commit/457a0f0>