Bug 60194 - REGRESSION (r79167): Windowed plugins in Google Reader don't move when the article list is scrolled
Summary: REGRESSION (r79167): Windowed plugins in Google Reader don't move when the ar...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL: data:text/html,<div style="overflow:s...
Keywords: InRadar, PlatformOnly, Regression
Depends on: 60776
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-04 11:53 PDT by Adam Roben (:aroben)
Modified: 2011-05-17 11:09 PDT (History)
5 users (show)

See Also:


Attachments
Tell ScrollView's children their frameRects have changed when its own frameRect changes (21.52 KB, patch)
2011-05-06 16:15 PDT, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
Tell ScrollView's child Widgets that their frame rects have changed when its own frame rect changes (11.96 KB, patch)
2011-05-13 09:46 PDT, Adam Roben (:aroben)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-05-04 11:53:19 PDT
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.
Comment 1 Adam Roben (:aroben) 2011-05-04 11:57:01 PDT
The bug does not occur in Safari 5.
Comment 2 Adam Roben (:aroben) 2011-05-04 11:57:29 PDT
<rdar://problem/9383760>
Comment 3 Adam Roben (:aroben) 2011-05-04 12:02:48 PDT
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>
Comment 4 Adam Roben (:aroben) 2011-05-04 13:39:13 PDT
Further reduction:


data:text/html,<div style="overflow:scroll;width:300px"><iframe width=500 height=314 src="http://www.youtube.com/embed/mYP4MgxDV2U">
Comment 5 Adam Roben (:aroben) 2011-05-05 10:23:41 PDT
r79167 is starting to look suspicious: <http://trac.webkit.org/changeset/79167>
Comment 6 Adam Roben (:aroben) 2011-05-05 11:14:01 PDT
Indeed: r79166 does not have the bug, r79167 does.
Comment 7 Adam Roben (:aroben) 2011-05-06 12:07:02 PDT
Rolling out r79167 makes the bug go away.
Comment 8 Adam Roben (:aroben) 2011-05-06 12:07:38 PDT
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
Comment 9 Adam Roben (:aroben) 2011-05-06 12:08:43 PDT
Presumably the bug is that we're not calling frameRectsChanged from ScrollView::setFrameRect anymore during scrolling.
Comment 10 Adam Roben (:aroben) 2011-05-06 12:20:27 PDT
Moving the frameRectsChanged call from setBoundsSize to setFrameRect seems to fix the bug. Now I need to write a test.
Comment 11 Adam Roben (:aroben) 2011-05-06 16:15:46 PDT
Created attachment 92656 [details]
Tell ScrollView's children their frameRects have changed when its own frameRect changes
Comment 12 Adam Roben (:aroben) 2011-05-06 16:39:57 PDT
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.
Comment 13 Adam Roben (:aroben) 2011-05-12 11:37:53 PDT
Comment on attachment 92656 [details]
Tell ScrollView's children their frameRects have changed when its own frameRect changes

Gonna do this a liiiiiittle differently.
Comment 14 Adam Roben (:aroben) 2011-05-12 11:42:36 PDT
I think I might be able to make a non-pixel test, too, by dumping the position of the plugin HWND.
Comment 15 Adam Roben (:aroben) 2011-05-12 12:07:47 PDT
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!
Comment 16 Adam Roben (:aroben) 2011-05-12 15:02:37 PDT
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!
Comment 17 Adam Roben (:aroben) 2011-05-13 07:09:34 PDT
I now have a working test. Time to fix the bug!
Comment 18 Adam Roben (:aroben) 2011-05-13 09:46:56 PDT
Created attachment 93470 [details]
Tell ScrollView's child Widgets that their frame rects have changed when its own frame rect changes
Comment 19 Adam Roben (:aroben) 2011-05-13 10:26:45 PDT
Committed r86442: <http://trac.webkit.org/changeset/86442>
Comment 20 Ademar Reis 2011-05-17 11:09:23 PDT
Revision r86442 cherry-picked into qtwebkit-2.2 with commit 457a0f0 <http://gitorious.org/webkit/qtwebkit/commit/457a0f0>