Bug 77686 - [Chromium] WebCore::toV8Context crashes if DomWindow::frame() returns null
Summary: [Chromium] WebCore::toV8Context crashes if DomWindow::frame() returns null
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-02 14:55 PST by Dmitry Lomov
Modified: 2012-02-03 17:45 PST (History)
4 users (show)

See Also:


Attachments
Fix (1.26 KB, patch)
2012-02-02 15:09 PST, Dmitry Lomov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Typo fixed. (1.25 KB, patch)
2012-02-02 15:44 PST, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2012-02-02 14:55:09 PST
http://crbug.com/112110
Comment 1 Dmitry Lomov 2012-02-02 14:58:36 PST
The break is introduced in http://trac.webkit.org/changeset/99311
Comment 2 Dmitry Lomov 2012-02-02 15:09:28 PST
Created attachment 125195 [details]
Fix
Comment 3 Nate Chapin 2012-02-02 15:10:56 PST
Interesting. In what case is this happening?
Comment 4 Adam Barth 2012-02-02 15:27:35 PST
Comment on attachment 125195 [details]
Fix

DOMWindow::frame() is null after the Frame has been destroyed.
Comment 5 WebKit Review Bot 2012-02-02 15:28:09 PST
Comment on attachment 125195 [details]
Fix

Attachment 125195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11394945
Comment 6 Dmitry Lomov 2012-02-02 15:44:29 PST
Created attachment 125203 [details]
Typo fixed.
Comment 7 Adam Barth 2012-02-03 14:33:46 PST
Comment on attachment 125203 [details]
Typo fixed.

Can we write a test using the test plugin?
Comment 8 Nate Chapin 2012-02-03 14:34:48 PST
Comment on attachment 125203 [details]
Typo fixed.

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

> Source/WebCore/bindings/v8/V8Helpers.cpp:45
> +    if (!domWindow || !domWindow->frame() || domWindow != domWindow->frame()->domWindow())

Do we have any idea when this happens? In theory, we shouldn't find ourselves in this state.

Is there any hope of a test?
Comment 9 Dmitry Lomov 2012-02-03 14:36:30 PST
(In reply to comment #8)
> (From update of attachment 125203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125203&action=review
> 
> > Source/WebCore/bindings/v8/V8Helpers.cpp:45
> > +    if (!domWindow || !domWindow->frame() || domWindow != domWindow->frame()->domWindow())
> 
> Do we have any idea when this happens? In theory, we shouldn't find ourselves in this state.

Why? What guarantees it?

> 
> Is there any hope of a test?
Comment 10 Dmitry Lomov 2012-02-03 14:36:57 PST
(In reply to comment #7)
> (From update of attachment 125203 [details])
> Can we write a test using the test plugin?

Maybe - do we have any examples? Thanks
Comment 11 Adam Barth 2012-02-03 14:37:02 PST
The DOMWindow's frame pointer is null after the Frame associated with the DOMWindow has been destroyed (typically by being removed from its document).  I'm not sure how this function can get called if the frame no longer exists.
Comment 12 Adam Barth 2012-02-03 14:37:55 PST
Take a look in LayoutTests/plugins for a bunch of tests that use the test plugin.
Comment 13 Dmitry Lomov 2012-02-03 14:44:42 PST
(In reply to comment #11)
> The DOMWindow's frame pointer is null after the Frame associated with the DOMWindow has been destroyed (typically by being removed from its document).  I'm not sure how this function can get called if the frame no longer exists.

Looks like NPObject is still alive and receives messages: 
Here is the call stack:
0012f26c 0207b583 chrome_1c30000!WebCore::Frame::domWindow+0x3
0012f278 0207b7f4 chrome_1c30000!WebCore::toV8Context+0x15
0012f2c4 02ccdfc2 chrome_1c30000!_NPN_GetProperty+0x3a
0012f2f0 02ccf1cd chrome_1c30000!NPObjectStub::OnGetProperty+0x6c
0012f3f4 02ccf73b chrome_1c30000!IPC::SyncMessageSchema<Tuple1<NPIdentifier_Param>,Tuple2<NPVariant_Param &,bool &> >::DispatchWithSendParams<NPObjectStub,NPObjectStub,void (__thiscall NPObjectStub::*)(NPIdentifier_Param const &,NPVariant_Param *,bool *)>+0x87
0012f42c 02ccfb87 chrome_1c30000!NPObjectMsg_GetProperty::Dispatch<NPObjectStub,NPObjectStub,void (__thiscall NPObjectStub::*)(NPIdentifier_Param const &,NPVariant_Param *,bool *)>+0x45
0012f5ac 01f2c969 chrome_1c30000!NPObjectStub::OnMessageReceived+0x1f1
0012f5bc 02cc8ddb chrome_1c30000!MessageRouter::RouteMessage+0x30
0012f5d4 01c75c80 chrome_1c30000!NPChannelBase::OnMessageReceived+0xd2
0012f5e0 01c6eae6 chrome_1c30000!base::internal::Invoker<2,base::internal::BindState<base::internal::RunnableAdapter<void (__thiscall media::FFmpegAudioDecoder::*)(base::Callback<void __cdecl(scoped_refptr<media::Buffer>)> const &)>,void __cdecl(media::FFmpegAudioDecoder *,base::Callback<void __cdecl(scoped_refptr<media::Buffer>)> const &),void __cdecl(media::FFmpegAudioDecoder *,base::Callback<void __cdecl(scoped_refptr<media::Buffer>)>)>,void __cdecl(media::FFmpegAudioDecoder *,base::Callback<void __cdecl(scoped_refptr<media::Buffer>)> const &)>::Run+0x16
0012f6b8 01c6e7a1 chrome_1c30000!MessageLoop::RunTask+0x203
0012f708 01c75b70 chrome_1c30000!MessageLoop::DoWork+0x22c
0012f7cc 01c6e47c chrome_1c30000!base::MessagePumpDefault::Run+0x122
Comment 14 Adam Barth 2012-02-03 16:44:52 PST
That looks like a message from the plugin to the page.  Maybe this is a shutdown race whereby the plugin is getting torn down but it doesn't know it yet.
Comment 15 Adam Barth 2012-02-03 16:46:17 PST
That seems consistent with this crash being caused by <http://trac.webkit.org/changeset/99311>.  That might make it hard to test in DRT, which is single process.
Comment 16 Adam Barth 2012-02-03 16:49:33 PST
Comment on attachment 125203 [details]
Typo fixed.

Ok.  I don't think this is testable in DumpRenderTree.  We likely need a separate plug-in process around to test this shutdown race.

Jochen is working on getting the layout tests running in content_shell, which will provide a nice framework for testing these kinds of bugs.
Comment 17 WebKit Review Bot 2012-02-03 17:45:30 PST
Comment on attachment 125203 [details]
Typo fixed.

Clearing flags on attachment: 125203

Committed r106722: <http://trac.webkit.org/changeset/106722>
Comment 18 WebKit Review Bot 2012-02-03 17:45:35 PST
All reviewed patches have been landed.  Closing bug.