Bug 46711 - platform/win/plugins/plugin-delayed-destroy.html fails in WebKit2
Summary: platform/win/plugins/plugin-delayed-destroy.html fails in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, LayoutTestFailure, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-09-28 03:52 PDT by Adam Roben (:aroben)
Modified: 2010-11-03 10:33 PDT (History)
2 users (show)

See Also:


Attachments
Add a plugin test that evaluates JS after removing the plugin element from the document (21.06 KB, patch)
2010-11-03 10:18 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-09-28 03:52:52 PDT
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.
Comment 1 Adam Roben (:aroben) 2010-09-28 04:29:28 PDT
<rdar://problem/8485903>
Comment 2 Adam Roben (:aroben) 2010-10-25 08:59:24 PDT
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
Comment 3 Adam Roben (:aroben) 2010-10-25 10:16:03 PDT
(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.
Comment 4 Adam Roben (:aroben) 2010-10-25 11:24:41 PDT
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.)
Comment 5 Adam Roben (:aroben) 2010-10-25 11:26:51 PDT
(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.
Comment 6 Adam Roben (:aroben) 2010-11-03 06:39:00 PDT
(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!
Comment 7 Adam Roben (:aroben) 2010-11-03 09:17:02 PDT
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.
Comment 8 Adam Roben (:aroben) 2010-11-03 10:18:19 PDT
Created attachment 72836 [details]
Add a plugin test that evaluates JS after removing the plugin element from the document
Comment 9 Anders Carlsson 2010-11-03 10:19:31 PDT
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.
Comment 10 Adam Roben (:aroben) 2010-11-03 10:33:58 PDT
Committed r71249: <http://trac.webkit.org/changeset/71249>