Bug 47009 - WebKit2 needs to call NPP_SetWindow when destroying a plugin
Summary: WebKit2 needs to call NPP_SetWindow when destroying a plugin
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Keywords: InRadar
: 55780 (view as bug list)
Depends on:
Reported: 2010-10-01 13:14 PDT by Adam Roben (:aroben)
Modified: 2011-04-08 09:11 PDT (History)
6 users (show)

See Also:

Pass NPP_SetWindow a null window handle during plugin destruction on non-Mac platforms (24.60 KB, patch)
2011-04-07 14:27 PDT, Adam Roben (:aroben)
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-10-01 13:14:00 PDT
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!)
Comment 1 Adam Roben (:aroben) 2010-10-01 13:14:31 PDT
Comment 2 Anders Carlsson 2010-10-21 14:55:42 PDT
This might only be needed on Windows, but we should at least see what the behavior is in other mac browsers.
Comment 3 Adam Roben (:aroben) 2011-04-06 11:35:26 PDT
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.
Comment 4 Anders Carlsson 2011-04-06 11:50:18 PDT
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.
Comment 5 Adam Roben (:aroben) 2011-04-06 11:50:59 PDT
How lucky that we already have a test, then. :-)
Comment 6 Adam Roben (:aroben) 2011-04-06 11:51:11 PDT
*** Bug 55780 has been marked as a duplicate of this bug. ***
Comment 7 Adam Roben (:aroben) 2011-04-07 08:48:47 PDT
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).
Comment 8 Adam Roben (:aroben) 2011-04-07 10:23:56 PDT
Testing the fix on Mac is proving a little difficult, since NPWindow::window is always 0 on Mac.
Comment 9 Adam Roben (:aroben) 2011-04-07 10:24:23 PDT
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.
Comment 10 Adam Roben (:aroben) 2011-04-07 13:25:01 PDT
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).
Comment 11 Adam Roben (:aroben) 2011-04-07 14:27:46 PDT
Created attachment 88697 [details]
Pass NPP_SetWindow a null window handle during plugin destruction on non-Mac platforms
Comment 12 Anders Carlsson 2011-04-07 14:29:34 PDT
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! :)
Comment 13 Adam Roben (:aroben) 2011-04-08 08:58:51 PDT
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.)
Comment 14 WebKit Review Bot 2011-04-08 09:06:14 PDT
http://trac.webkit.org/changeset/83300 might have broken Qt Linux Release minimal
Comment 15 Adam Roben (:aroben) 2011-04-08 09:10:55 PDT
Committed r83300: <http://trac.webkit.org/changeset/83300>
Comment 16 Adam Roben (:aroben) 2011-04-08 09:11:23 PDT
Qt build fix

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