Bug 86035

Summary: Crash in FrameView::windowClipRectForFrameOwner after r116371
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Plug-insAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed blind fix. Added a NULL-check. none

Description Julien Chaffraix 2012-05-09 16:23:20 PDT 
Example stack-trace:

0x67258b37	 [chrome.dll]	 - frameview.cpp:2573	WebCore::FrameView::windowClipRectForFrameOwner(WebCore::HTMLFrameOwnerElement const *,bool)
0x67258ae1	 [chrome.dll]	 - frameview.cpp:2564	WebCore::FrameView::windowClipRect(bool)
0x67258bde	 [chrome.dll]	 - frameview.cpp:2587	WebCore::FrameView::windowClipRectForFrameOwner(WebCore::HTMLFrameOwnerElement const *,bool)
0x67cde64f	 [chrome.dll]	 - webplugincontainerimpl.cpp:718	WebKit::WebPluginContainerImpl::windowClipRect()
0x67cdee3f	 [chrome.dll]	 - webplugincontainerimpl.cpp:697	WebKit::WebPluginContainerImpl::calculateGeometry(WebCore::IntRect const &,WebCore::IntRect &,WebCore::IntRect &,WTF::Vector<WebCore::IntRect,0> &)
0x67cdef4e	 [chrome.dll]	 - webplugincontainerimpl.cpp:326	WebKit::WebPluginContainerImpl::reportGeometry()
0x67cde32c	 [chrome.dll]	 - webplugincontainerimpl.cpp:239	WebKit::WebPluginContainerImpl::setParent(WebCore::ScrollView *)
0x66c64205	 [chrome.dll]	 - scrollview.cpp:74	WebCore::ScrollView::addChild(WTF::PassRefPtr<WebCore::Widget>)
0x671dd73f	 [chrome.dll]	 - renderwidget.cpp:91	WebCore::moveWidgetToParentSoon
0x671ddddb	 [chrome.dll]	 - renderwidget.cpp:219	WebCore::RenderWidget::setWidget(WTF::PassRefPtr<WebCore::Widget>)
0x671d673a	 [chrome.dll]	 - renderpart.cpp:55	WebCore::RenderPart::setWidget(WTF::PassRefPtr<WebCore::Widget>)
0x675a56f2	 [chrome.dll]	 - subframeloader.cpp:383	WebCore::SubframeLoader::loadPlugin(WebCore::HTMLPlugInImageElement *,WebCore::KURL const &,WTF::String const &,WTF::Vector<WTF::String,0> const &,WTF::Vector<WTF::String,0> const &,bool)
0x675a4918	 [chrome.dll]	 - subframeloader.cpp:131	WebCore::SubframeLoader::requestPlugin(WebCore::HTMLPlugInImageElement *,WebCore::KURL const &,WTF::String const &,WTF::Vector<WTF::String,0> const &,WTF::Vector<WTF::String,0> const &,bool)
0x675a4a43	 [chrome.dll]	 - subframeloader.cpp:151	WebCore::SubframeLoader::requestObject(WebCore::HTMLPlugInImageElement *,WTF::String const &,WTF::AtomicString const &,WTF::String const &,WTF::Vector<WTF::String,0> const &,WTF::Vector<WTF::String,0> const &)
0x675efa5f	 [chrome.dll]	 - htmlembedelement.cpp:177	WebCore::HTMLEmbedElement::updateWidget(WebCore::PluginCreationOption)
0x67258913	 [chrome.dll]	 - frameview.cpp:2290	WebCore::FrameView::updateWidget(WebCore::RenderEmbeddedObject *)
0x66b599f7	 [chrome.dll]	 - frameview.cpp:2323	WebCore::FrameView::updateWidgets()
0x66b56390	 [chrome.dll]	 - frameview.cpp:2376	WebCore::FrameView::performPostLayoutTasks()
0x66b396e2	 [chrome.dll]	 - frameview.cpp:1166	WebCore::FrameView::layout(bool)
0x67258732	 [chrome.dll]	 - frameview.cpp:2029	WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView> *)
0x66e8815b	 [chrome.dll]	 - timer.h:100	WebCore::Timer<WebCore::CachedImage>::fired()
0x66b68a42	 [chrome.dll]	 - threadtimers.cpp:115	WebCore::ThreadTimers::sharedTimerFiredInternal()
0x66b688fc	 [chrome.dll]	 - threadtimers.cpp:93	WebCore::ThreadTimers::sharedTimerFired()
0x66a0ebde	 [chrome.dll]	 - timer.cc:179	base::Timer::RunScheduledTask()
0x66a0eb49	 [chrome.dll]	 - timer.cc:44	base::BaseTimerTaskInternal::Run()
0x669c772d	 [chrome.dll]	 - message_loop.cc:458	MessageLoop::RunTask(base::PendingTask const &)
0x669c6c44	 [chrome.dll]	 - message_loop.cc:685	MessageLoop::DoDelayedWork(base::TimeTicks *)
0x669d81cc	 [chrome.dll]	 - message_pump_default.cc:32	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x669c5fa6	 [chrome.dll]	 - message_loop.cc:390	MessageLoop::RunHandler()
0x669c5f54	 [chrome.dll]	 - message_loop.cc:300	MessageLoop::Run()
0x66a48893	 [chrome.dll]	 - renderer_main.cc:271	RendererMain(content::MainFunctionParams const &)
0x669c242b	 [chrome.dll]	 - content_main_runner.cc:292	`anonymous namespace'::RunNamedProcessTypeMain(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,content::MainFunctionParams const &,content::ContentMainDelegate *)
0x669c23b0	 [chrome.dll]	 - content_main_runner.cc:550	`anonymous namespace'::ContentMainRunnerImpl::Run()
0x669b44d7	 [chrome.dll]	 - content_main.cc:35	content::ContentMain(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *,content::ContentMainDelegate *)
0x669b4462	 [chrome.dll]	 - chrome_main.cc:28	ChromeMain
0x00ee7fa1	 [chrome.exe]	 - client_util.cc:423	MainDllLoader::Launch(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *)
0x00ee72a4	 [chrome.exe]	 - chrome_exe_main_win.cc:31	RunChrome(HINSTANCE__ *)
0x00ee730f	 [chrome.exe]	 - chrome_exe_main_win.cc:47	wWinMain
0x00f07788	 [chrome.exe]	 - crt0.c:263	__tmainCRTStartup
0x75723676	 [kernel32.dll]	 + 0x00013676]	BaseThreadInitThunk
0x77d19f41	 [ntdll.dll]	 + 0x00039f41]	__RtlUserThreadStart
0x77d19f14	 [ntdll.dll]	 + 0x00039f14]	_RtlUserThreadStart

Looking at the error and the code, it seems like it is possible for windowClipRect to get a NULL parentView. I don't have a reproduction case unfortunately.
Comment 1 Julien Chaffraix 2012-05-09 16:41:32 PDT 
Created attachment 141050 [details]
Proposed blind fix. Added a NULL-check.
Comment 2 Tony Chang 2012-05-09 17:04:31 PDT 
I'm not a good reviewer for this.  Maybe Adam is?
Comment 3 Adam Barth 2012-05-09 17:29:00 PDT 
Have you tried using the beforeload event on the <object> element to screw around with the DOM below FrameView::updateWidget ?
Comment 4 Eric Seidel (no email) 2012-05-09 17:29:16 PDT 
Comment on attachment 141050 [details]
Proposed blind fix. Added a NULL-check.

Why is something calling windowClipRect w/o this being a child FrameView?
Comment 5 Eric Seidel (no email) 2012-05-09 17:30:18 PDT 
Is WebKit::WebPluginContainerImpl::setParent calling this before actually setting the parent?  Is somethign else executing to remove this widget from the hierarchy before this executes?
Comment 6 Julien Chaffraix 2012-05-10 07:56:57 PDT 
Comment on attachment 141050 [details]
Proposed blind fix. Added a NULL-check.

(In reply to comment #3)
> Have you tried using the beforeload event on the <object> element to screw around with the DOM below FrameView::updateWidget ?

I haven't as I didn't know this code enough to create a test on the spot. Let me regroup and come back to you with hopefully a test case!

> Is WebKit::WebPluginContainerImpl::setParent calling this before actually setting the parent?  Is somethign else executing to remove this widget from the hierarchy before this executes?

It's difficult for me to answer any question without a reproduction (I don't hide it's a blind fix based on the stack-trace). My understanding is that it's possible for |parentView| to be NULL (as mentioned in Document.h). The code prior to r116371 was doing NULL checking the enclosingLayer() which may explain how they go away with that.
Comment 7 Julien Chaffraix 2012-05-14 12:18:01 PDT 
(In reply to comment #6)
> (From update of attachment 141050 [details])
> (In reply to comment #3)
> > Have you tried using the beforeload event on the <object> element to screw around with the DOM below FrameView::updateWidget ?
> 
> I haven't as I didn't know this code enough to create a test on the spot. Let me regroup and come back to you with hopefully a test case!

OK, unfortunately that didn't help (tried to kill one of our parent FrameView) nor did the Chromium crash reports so far.

> > Is WebKit::WebPluginContainerImpl::setParent calling this before actually setting the parent? 

Looking at the code in WebPluginContainerImpl, that's not the case:

Widget::setParent(view);
if (view)
    reportGeometry();

(the crash is below reportGeometry)

> Is somethign else executing to remove this widget from the hierarchy before this executes?

I wonder about that. This is a big crasher on Youtube but it's very flaky (no video seems to reproduce the issue more than a couple of times).

This may as well indicate a race condition in the Chromium code or just in WebKit.
Comment 8 Dave Hyatt 2012-05-14 14:41:54 PDT 
Comment on attachment 141050 [details]
Proposed blind fix. Added a NULL-check.

r=me
Comment 9 Julien Chaffraix 2012-05-14 14:51:30 PDT 
Comment on attachment 141050 [details]
Proposed blind fix. Added a NULL-check.

Landed in http://trac.webkit.org/changeset/117005.