We pull the NPObject prototype out of object tags from the OnPluginDestroy callback in nsJSNPRuntime, but it is created/injected in ObjectLoadingContent/DOMClassInfo. This makes things like delayed stop for plugins hard, since the object tag may wish to spawn a new plugin before the previous one is destroyed, and other such terribleness. It's also the reason our teardown ordering is difficult to fix in bug 843671 and bug 784131.

Comment on attachment 725219 [details] [diff] [review] Move removing the plugin prototype from objects to content You might want nsContentUtils::GetSafeJSContext.

Use nsContentUtils::GetSafeJSContext

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content bz - can you take a look at the js bits here and make sure they're sane? It's mostly just relocating code nsJSNPRuntime -> nsObjectLoadingContent

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content Josh - do you see any obvious ordering issues moving the proto teardown here?

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content I think you're better off with bsmedberg reviewing this, or alternatively jst.

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content r=me on the JS bits

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content I'm still a little worried about the JSAPI usage here (especially the safe JS context), but you're not actually changing that code, and the rest of this looks ok.

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content whoops

So this fixes a random race failure I found - pluginHost can destroy plugins out from under us with no notification from navigator.plugins.refresh(), so we don't know to tear down the protochain. This also allows us to cleanup the instance owner and channel sooner, rather than let them linger around with our defunct object tag.

Bonus, These patches randomly uncovered an event race that could hit this assertion: - Object tag sets up as plugin, queues instantiation event - Disable plugin - Event fires This just moves that assertion to below the bailout

Comment on attachment 726954 [details] [diff] [review] Notify owning content when we destroy plugins from under it This should arguably go away since plugins.refresh should probably not be doing this. But since I'm not 100% clear on what the desired behavior of plugins.refresh is, this will at least fix the problem at hand.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #12) > This should arguably go away since plugins.refresh should probably not be > doing this. But since I'm not 100% clear on what the desired behavior of > plugins.refresh is, this will at least fix the problem at hand. Bug 376746 and bug 418615 are open for the plugins.refresh weirdness

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content https://hg.mozilla.org/integration/mozilla-inbound/rev/3afecf060f06

Comment on attachment 726954 [details] [diff] [review] Notify owning content when we destroy plugins from under it https://hg.mozilla.org/integration/mozilla-inbound/rev/81b2e436001e

Comment on attachment 726955 [details] [diff] [review] Avoid bogus assertion when trying to spawn disabled plugin https://hg.mozilla.org/integration/mozilla-inbound/rev/3eea884c4435

https://hg.mozilla.org/mozilla-central/rev/3eea884c4435 https://hg.mozilla.org/mozilla-central/rev/81b2e436001e https://hg.mozilla.org/mozilla-central/rev/3afecf060f06

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content [Approval Request Comment] Bug caused by (feature/regressing bug #): N/A User impact if declined: Required change if we wish to uplift bug 843671 Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): Fairly low, changes ordering of some plugin code that was previously buggy, but introduces little new code. Smaller alternative patches would be possible for branches, but this would mean the ordering would change on branches then again on trunk. Regressions aren't likely to be seen until the beta audience regardless, given past experience with subtle plugin issues. String or UUID changes made by this patch: None

FWIW, this is slightly risky, but it's critically important for fixing tracking bug 843761. The kinds of risk are "unknown plugins behaving differently or maybe crashes".

(In reply to Benjamin Smedberg [:bsmedberg] from comment #19) > FWIW, this is slightly risky, but it's critically important for fixing > tracking bug 843761. The kinds of risk are "unknown plugins behaving > differently or maybe crashes". I don't think that's the bug you meant.

(In reply to :Ms2ger from comment #20) > (In reply to Benjamin Smedberg [:bsmedberg] from comment #19) > > FWIW, this is slightly risky, but it's critically important for fixing > > tracking bug 843761. The kinds of risk are "unknown plugins behaving > > differently or maybe crashes". > > I don't think that's the bug you meant. bug 843671

Comment on attachment 725567 [details] [diff] [review] Move removing the plugin prototype from objects to content Approving the uplift given this is low risk,baked for a couple of days and is needed in favor of bug 843671 which we want fixed in aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1d39eba9145 https://hg.mozilla.org/releases/mozilla-aurora/rev/6d8e9b790e81 https://hg.mozilla.org/releases/mozilla-aurora/rev/f7975b92291a

Apart from exploratory testing to find potential plug-ins regressions, is there anything specific QA can do to verify this is fixed?

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #24) > Apart from exploratory testing to find potential plug-ins regressions, is > there anything specific QA can do to verify this is fixed? This bug relates to cleaning up poorly factored code, to make it possible to fix bug 843671. There are unfortunately no symptoms to test for, only regressions.

We did extensive exploratory testing around plugins in Firefox 21.0b6 candidates overnight but were unable to find any regressions. Unfortunately this does not necessarily prove this has been fixed. As such I am not comfortable marking this verified and am instead marking [qa-].