418615 - nsPluginHost's handling of navigator.plugins.refresh() is broken

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Post-bug 1156, it shouldn't be necessary anymore to reframe anything in order to reload a plugin (and why did that code reframe everything instead of just the object element!?)

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

That is, it should just ask the element to attempt to reinstantiate somehow.

Comment 2

[Benjamin Smedberg]

This is true, but unfortunately most pages don't expect plugins to disappear and reappear out from under them, so I don't think we should attempt to fix it.

Comment 3

[John Schoenick [:johns]]

Actually, the current pluginhost code does pull plugins out from under pages, then triggers a reframe to re-instatiate them. This only coincidentally-works now as the object element waits for a frame before spawning the plugin, so the HasNewFrame callback + no running plugin makes it believe it is still in instantiation :-/

We should probably pull all this code out and tweak navigator.plugins.refresh() behavior to always refresh affected pages or something.

Comment 4

[Benjamin Smedberg]

Are you talking about navigator.plugins.refresh(true) or (false)? AIUI, "true" should reload the affected pages (although see bug 376746). And "false" shouldn't affect existing pages at all, it should only affect new plugin instantiations.

Comment 5

[John Schoenick [:johns]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Are you talking about navigator.plugins.refresh(true) or (false)? AIUI,
> "true" should reload the affected pages (although see bug 376746). And
> "false" shouldn't affect existing pages at all, it should only affect new
> plugin instantiations.

Reading through this code it looks like refresh(true) reloads the current page regardless of plugins changing, and tries to reframe all other pages with changed plugins -- which does not refresh them, but does (incidentally) restart their plugin instances. (false) appears to kill (but not restart?) all plugin instances. In-process plugin instances are not unloaded and I don't see how refresh() would ever upgrade them.

Either way, the reframe code needs to be ripped out and this all probably needs to be sanity-checked.

Comment 6

[John Schoenick [:johns]]

While investigating bug 859099 I found numerous ways in which navigator.plugins.refresh causes issues. It basically bypasses all of nsObjectLoadingContent's re-entrance protection by tearing down plugins from underneath it. When called with (true) it kills every plugin instance, instead of just updated instances, and refreshes the current page for some reason.

Calling it inside plugin instantiate breaks things, as well as within plugin destroy. It also kills a list of plugins, but doesn't check that no other plugins were spawned while doing so (which may have happened due to re-entry).

I looked at this to make sure there's no weak pointers involved or any other potentially dangerous corruption, but we can definitely hit a few null derefs and cause nsObjectLoadingContent to leak plugin instances, which will cause it to abort in its destructor (bug 859099)

Comment 8

[Benjamin Smedberg]

https://tbpl.mozilla.org/?tree=Try&rev=6387618aa9b4 test failures in

* test_nice_plugin_name.js
* test_persist_in_prefs.js
* test_plugins.js

Comment 9

[Georg Fritzsche [:gfritzsche]]

Those depend on ReloadPlugins() finding new (test-)plugin libraries and sorting them before the previous ones due to newer mtime.

Comment 10

[Benjamin Smedberg]

Attachment: Neuter parts of navigator.plugins.reload, rev. 1

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 740928 [details] [diff] [review]
Neuter parts of navigator.plugins.reload, rev. 1

sr=me

Comment 12

[John Schoenick [:johns]]

Comment on attachment 740928 [details] [diff] [review]
Neuter parts of navigator.plugins.reload, rev. 1

Review of attachment 740928 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginHost.cpp
@@ -443,1 @@
>    // shutdown plugins and kill the list if there are no running plugins

If plugins are still running, do these old versions ever get removed from the list?

@@ -2389,5 @@
>  
>    // Reload instances if needed
>    if (aPluginTag->IsActive()) {
>      return NS_OK;
>    }

This can go too

@@ -3760,5 @@
>  }
>  
>  void 
> -nsPluginHost::DestroyRunningInstances(nsTArray<nsCOMPtr<nsIDocument> >* aReloadDocs,
> -                                      nsPluginTag* aPluginTag)

Is this called anywhere besides XPCOM shutdown now? I worry that it's still possible for inst->Stop() to re-enter and start new plugins that will get leaked and might hang shutdown -- or cause us to reach past the end of mInstances. We should open a follow-up to improve this.

Comment 13

[John Schoenick [:johns]]

Apparently splinter-review doesn't detect mid-airs :(

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 740928 [details] [diff] [review]
Neuter parts of navigator.plugins.reload, rev. 1

Yep.  One of many reasons to not use splinter-review... ;)

Comment 15

[Benjamin Smedberg]

> If plugins are still running, do these old versions ever get removed from
> the list?

No. I can file a followup to make the plugin tag inactive in some way, which I'd prefer anyways.

> >  void 
> > -nsPluginHost::DestroyRunningInstances(nsTArray<nsCOMPtr<nsIDocument> >* aReloadDocs,
> > -                                      nsPluginTag* aPluginTag)
> 
> Is this called anywhere besides XPCOM shutdown now? I worry that it's still
> possible for inst->Stop() to re-enter and start new plugins that will get
> leaked and might hang shutdown -- or cause us to reach past the end of
> mInstances. We should open a follow-up to improve this.

It is shutdown-only now, and we should be able to totally remove this in a followup, yes.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/faafe91695d9

Comment 17

[Benjamin Smedberg]

Comment on attachment 740928 [details] [diff] [review]
Neuter parts of navigator.plugins.reload, rev. 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Longstanding issue, believed to be causing crashes on monthly Adobe Flash updates
Testing completed (on m-c, etc.): passes unit tests. I manually verified that normal Flash upgrades appear to work correctly.
Risk to taking this patch (and alternatives if risky): This is a moderately risky patch: it might affect the way some plugin installers work. But I also think it's pretty important for stability.
String or IDL/UUID changes made by this patch: IID changes to nsIPluginHost.

I do not intend to uplift this to beta due to the risk.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 740928 [details] [diff] [review]
Neuter parts of navigator.plugins.reload, rev. 1

Let's get this on aurora, and have some qa testing once it's landed - we'll want to make sure that we don't have any unexpected fallout once this gets to beta in a couple weeks.

Comment 19

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66ff4f31d731

Comment 20

[u279076]

Apart from trying to reproduce crashes with Firefox running during plugin updates/installs, is there any other high risk areas we should attempt in verifying this is fixed?

Comment 21

[Mihai Morar, (:MihaiMorar)]

I confirm this is fixed in FF 23b3 on Mac OS 10.8, Windows 7x64 and Ubuntu 13.04 x86.