Bug 47009 – WebKit2 needs to call NPP_SetWindow when destroying a plugin

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

[Adam Roben (:aroben)]

NPP_SetWindow is supposed to be called when a plugin is destroyed. WebKit1 does this but WebKit2 does not!

(And we should have a test for it!)

[Adam Roben (:aroben)]

<rdar://problem/8503832>

[Anders Carlsson]

This might only be needed on Windows, but we should at least see what the behavior is in other mac browsers.

[Adam Roben (:aroben)]

This is causing bug 55780.

It looks like Firefox doesn't do this. See bug 55780 comment 13.

Fixing this bug would probably be best for compatibility with WebKit1. But it would make us less like Firefox.

[Anders Carlsson]

The NPAPI documentation on https://developer.mozilla.org/en/NPP_SetWindow states that 

If the window handle is set to null, the window is destroyed. In this case, the plug-in must not perform any additional graphics operations on the window and should free any associated resources.

so I think we should do this.

[Adam Roben (:aroben)]

How lucky that we already have a test, then. :-)

[Adam Roben (:aroben)]

*** Bug 55780 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

Here's the WebKit1 code: <http://trac.webkit.org/browser/trunk/Source/WebCore/plugins/PluginView.cpp?rev=79988#L361>

It looks like WebKit1 explicitly does not call NPP_SetWindow when destroying a plugin on Mac. This seems to match the code used in Apple's WebKit1 Mac port (in WebNetscapePluginView).

[Adam Roben (:aroben)]

Testing the fix on Mac is proving a little difficult, since NPWindow::window is always 0 on Mac.

[Adam Roben (:aroben)]

I guess I could tell the plugin "you're about to be destroyed", and it could assert that it gets no NPP_SetWindow calls from that point forward on Mac.

[Adam Roben (:aroben)]

OK, I now have a test that works. Except it doesn't work in 64-bit WebKit1 on SnowLeopard. The test involves logging messages during NPP_Destroy, and 64-bit WebKit1 on SnowLeopard doesn't allow that (due to bugs in WebKitPluginHost).

[Adam Roben (:aroben)]

Created attachment 88697 [details]
Pass NPP_SetWindow a null window handle during plugin destruction on non-Mac platforms

[Anders Carlsson]

Comment on attachment 88697 [details]
Pass NPP_SetWindow a null window handle during plugin destruction on non-Mac platforms

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

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:93
> +void pluginLog(NPP instance, const char* format, ...)

It would be nice to make this a member function of PluginTest.

> Tools/DumpRenderTree/TestNetscapePlugIn/PluginTest.cpp:172
> +void PluginTest::log(const char* format, ...)

Oh, never mind! :)

[Adam Roben (:aroben)]

It's too bad that Qt, GTK, and Mac EWS were not able to apply this patch due to bug 53625. I guess I'll just have to watch the bots. (I did test locally on Mac.)

[WebKit Review Bot]

http://trac.webkit.org/changeset/83300 might have broken Qt Linux Release minimal

[Adam Roben (:aroben)]

Committed r83300: <http://trac.webkit.org/changeset/83300>

[Adam Roben (:aroben)]

Qt build fix

Committed r83302: <http://trac.webkit.org/changeset/83302>