Bug 61428 – REGRESSION (WebKit2): Crash in Flash on USA Today photo gallery

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Jon Honeycutt]

Wait until the ad stops playing; the WebProcess crashes.

<rdar://problem/9457006>

[Jon Honeycutt]

Created attachment 94757 [details]
Patch

[WebKit Review Bot]

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.

[Adam Roben (:aroben)]

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.

[Jon Honeycutt]

(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.

[Jon Honeycutt]

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