Closed Bug 851378 Opened 8 years ago Closed 8 years ago

Move tearing down plugin prototype from NPRuntime to content

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox21 --- fixed
firefox22 --- fixed

People

(Reporter: johns, Assigned: johns)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

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
Attachment #725219 - Attachment is obsolete: true
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
Attachment #725567 - Flags: review?(bzbarsky)
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?
Attachment #725567 - Flags: review?(joshmoz)
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.
Attachment #725567 - Flags: review?(joshmoz) → review?(benjamin)
Comment on attachment 725567 [details] [diff] [review]
Move removing the plugin prototype from objects to content

r=me on the JS bits
Attachment #725567 - Flags: review?(bzbarsky) → review+
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.
Attachment #725567 - Flags: review?(bzbarsky)
Attachment #725567 - Flags: review?(benjamin)
Attachment #725567 - Flags: review+
Comment on attachment 725567 [details] [diff] [review]
Move removing the plugin prototype from objects to content

whoops
Attachment #725567 - Flags: review?(bzbarsky)
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.
Attachment #726954 - Flags: review?(benjamin)
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
Attachment #726955 - Flags: review?(benjamin)
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.
Attachment #726954 - Flags: review?(benjamin) → review+
Attachment #726955 - Flags: review?(benjamin) → review+
(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
Attachment #725567 - Flags: checkin+
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
Attachment #726954 - Flags: checkin+
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
Attachment #726955 - Flags: checkin+
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
Attachment #725567 - Flags: approval-mozilla-aurora?
Attachment #726954 - Flags: approval-mozilla-aurora?
Attachment #726955 - Flags: approval-mozilla-aurora?
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.
Attachment #725567 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #726954 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #726955 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 859099
Apart from exploratory testing to find potential plug-ins regressions, is there anything specific QA can do to verify this is fixed?
Keywords: verifyme
(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-].
Keywords: verifyme
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.