Bug 61428

Summary: REGRESSION (WebKit2): Crash in Flash on USA Today photo gallery
Product: WebKit Reporter: Jon Honeycutt <jhoneycutt>
Component: Plug-insAssignee: Jon Honeycutt <jhoneycutt>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, aroben, webkit.review.bot
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Windows 7   
URL: http://mediagallery.usatoday.com/Day-in-celebrities/May-17,-2011/G56,A9164
Attachments:
Description Flags
Patch aroben: review+

Description Jon Honeycutt 2011-05-25 01:53:28 PDT
Wait until the ad stops playing; the WebProcess crashes.

<rdar://problem/9457006>
Comment 1 Jon Honeycutt 2011-05-25 02:16:03 PDT
Created attachment 94757 [details]
Patch
Comment 2 WebKit Review Bot 2011-05-25 02:17:22 PDT
Attachment 94757 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:110:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Adam Roben (:aroben) 2011-05-25 05:03:07 PDT
Comment on attachment 94757 [details]
Patch

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

So great to have a test for this!

> LayoutTests/platform/win/plugins/call-javascript-that-destroys-plugin.html:7
> +            layoutTestController.waitUntilDone();

I like to put the waitUntilDone call inside the PluginTest object. That way both waitUntilDone and notifyDone are taken care of by the plugin.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1137
> +    // Flash may ask us to evaluate JavaScript that removes the plug-in from the page,
> +    // but it expects the plug-in to still be alive when the call completes. To prevent
> +    // a crash, if the plug-in has only one remaining reference, call deref()
> +    // asynchronously.

This comment feels a little too Flash-specific, even though Flash is the only plugin we know of that does this.

I think it would be good to mention that we crash because we end up returning to plugin code higher up on the stack but the plugin has already been unloaded. (If that is in fact the reason.)

> Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:55
> +static PluginTest::Register<CallJSThatDestroysPlugin> callJavaScriptThatDestroysPlugin("call-javascript-that-destroys-plugin");

I like to just call this variable "registrar".

> Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:85
> +NPError CallJSThatDestroysPlugin::NPP_DestroyStream(NPStream*, NPReason)

Is there a particular reason why you chose to start the test in NPP_DestroyStream, as opposed to somewhere like NPP_New? It might be worth mentioning.

> Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:93
> +    wndClass.hInstance = GetModuleHandle(0);

This will register the class for WebKit2WebProcess.exe, not for npTestNetscapePlugin.dll. But it doesn't matter very much.

>> Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:110
>> +}
> 
> Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]

You should fix this.
Comment 4 Jon Honeycutt 2011-05-25 13:21:30 PDT
(In reply to comment #3)
> (From update of attachment 94757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94757&action=review
> 
> So great to have a test for this!
> 
> > LayoutTests/platform/win/plugins/call-javascript-that-destroys-plugin.html:7
> > +            layoutTestController.waitUntilDone();
> 
> I like to put the waitUntilDone call inside the PluginTest object. That way both waitUntilDone and notifyDone are taken care of by the plugin.

OK, moved it into the PluginTest.

> 
> > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1137
> > +    // Flash may ask us to evaluate JavaScript that removes the plug-in from the page,
> > +    // but it expects the plug-in to still be alive when the call completes. To prevent
> > +    // a crash, if the plug-in has only one remaining reference, call deref()
> > +    // asynchronously.
> 
> This comment feels a little too Flash-specific, even though Flash is the only plugin we know of that does this.
> 
> I think it would be good to mention that we crash because we end up returning to plugin code higher up on the stack but the plugin has already been unloaded. (If that is in fact the reason.)

I don't think we've unloaded the plug-in by this time. I updated the comment to:

    // A plug-in may ask us to evaluate JavaScript that removes the plug-in from the
    // page, but expect the object to still be alive when the call completes. Flash,
    // for example, may crash if the plug-in is destroyed and we return to code for
    // the destroyed object higher on the stack. To prevent this, if the plug-in has
    // only one remaining reference, call deref() asynchronously.

> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:55
> > +static PluginTest::Register<CallJSThatDestroysPlugin> callJavaScriptThatDestroysPlugin("call-javascript-that-destroys-plugin");
> 
> I like to just call this variable "registrar".

Fixed.

> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:85
> > +NPError CallJSThatDestroysPlugin::NPP_DestroyStream(NPStream*, NPReason)
> 
> Is there a particular reason why you chose to start the test in NPP_DestroyStream, as opposed to somewhere like NPP_New? It might be worth mentioning.

Nope, I changed it to start in NPP_New.

> 
> > Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:93
> > +    wndClass.hInstance = GetModuleHandle(0);
> 
> This will register the class for WebKit2WebProcess.exe, not for npTestNetscapePlugin.dll. But it doesn't matter very much.

I left this as-is.

> 
> >> Tools/DumpRenderTree/TestNetscapePlugIn/win/CallJSThatDestroysPlugin.cpp:110
> >> +}
> > 
> > Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
> 
> You should fix this.

Fixed.
Comment 5 Jon Honeycutt 2011-05-25 14:58:28 PDT
Landed in http://trac.webkit.org/changeset/87324.