Bug 46711 – platform/win/plugins/plugin-delayed-destroy.html fails in WebKit2

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

[Adam Roben (:aroben)]

platform/win/plugins/plugin-delayed-destroy.html fails in WebKit2. Here's the diff:

--- /tmp/layout-test-results/platform/win/plugins/plugin-delayed-destroy-expected.txt	2010-09-27 19:40:33.968625000 -0400
+++ /tmp/layout-test-results/platform/win/plugins/plugin-delayed-destroy-actual.txt	2010-09-27 19:40:33.968625000 -0400
@@ -1,3 +1,3 @@
-The should be printed before destroy.
 Plug-in destroyed.
+The should be printed before destroy.

[Adam Roben (:aroben)]

<rdar://problem/8485903>

[Adam Roben (:aroben)]

This test was added in r54614 [1], which fixed a crash with Flash. The one known page that crash occurred on has changed a lot since then. But it looks like the test itself would have suffered from the same crash that Flash did. It looks like WebKit2 does not crash on this test. I'll try undoing the fix in WebKit1 and see if the test does crash. If it does, I think we should just remove the logging from the test and say that it succeeds if it doesn't crash.

1. http://trac.webkit.org/changeset/54614

[Adam Roben (:aroben)]

(In reply to comment #2)
> I'll try undoing the fix in WebKit1 and see if the test does crash. If it does, I think we should just remove the logging from the test and say that it succeeds if it doesn't crash.

Indeed, WebKit1 still crashes if the fix is removed. I'll modify the test not to depend on when NPP_Destroy is called.

[Adam Roben (:aroben)]

It looks like WebKit2 is not susceptible to the original crash because JSNPObject::callMethod keeps the PluginView alive via NPRuntimeObjectMap::PluginProtector. The equivalent code in WebKit1, JSC::callRuntimeMethod, only keeps the plugin's Instance alive, but not its PluginView. (WebKit1 ends up keeping the PluginView alive via the code added in r54614.)

[Adam Roben (:aroben)]

(In reply to comment #3)
> (In reply to comment #2)
> > I'll try undoing the fix in WebKit1 and see if the test does crash. If it does, I think we should just remove the logging from the test and say that it succeeds if it doesn't crash.
> 
> Indeed, WebKit1 still crashes if the fix is removed.

Actually, WebKit1 only asserts if the fix is removed.

[Adam Roben (:aroben)]

(In reply to comment #2)
> But it looks like the test itself would have suffered from the same crash that Flash did.

This appears to be incorrect. The test that was added did not crash, it merely logged differently. It's too bad we don't have a test that would have crashed!

[Adam Roben (:aroben)]

I've come up with a new test to replace plugin-delayed-destroy.html that actually crashes when the call to PluginView::keepAlive that was added in r54614 is removed. The new test passes in WebKit2. I'll replace plugin-delayed-destroy.html with this new test.

[Adam Roben (:aroben)]

Created attachment 72836 [details]
Add a plugin test that evaluates JS after removing the plugin element from the document

[Anders Carlsson]

Comment on attachment 72836 [details]
Add a plugin test that evaluates JS after removing the plugin element from the document

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

> LayoutTests/ChangeLog:5
> +        Need a short description and bug URL (OOPS!)

Please remove this.

[Adam Roben (:aroben)]

Committed r71249: <http://trac.webkit.org/changeset/71249>