[webkit-reviews] review granted: [Bug 61428] REGRESSION (WebKit2): Crash in Flash on USA Today photo gallery : [Attachment 94757] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 25 05:03:07 PDT 2011


Adam Roben (:aroben) <aroben at apple.com> has granted Jon Honeycutt
<jhoneycutt at apple.com>'s request for review:
Bug 61428: REGRESSION (WebKit2): Crash in Flash on USA Today photo gallery
https://bugs.webkit.org/show_bug.cgi?id=61428

Attachment 94757: Patch
https://bugs.webkit.org/attachment.cgi?id=94757&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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.


More information about the webkit-reviews mailing list