Closed
Bug 418615
Opened 17 years ago
Closed 12 years ago
nsPluginHost's handling of navigator.plugins.refresh() is broken
Categories
(Core Graveyard :: Plug-ins, defect, P1)
Core Graveyard
Plug-ins
Tracking
(firefox22+ fixed, firefox23+ verified)
VERIFIED
FIXED
mozilla23
People
(Reporter: Biesinger, Assigned: benjamin)
References
()
Details
Attachments
(1 file)
(deleted),
patch
|
johns
:
review+
bzbarsky
:
superreview+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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!?)
Reporter | ||
Comment 1•17 years ago
|
||
That is, it should just ask the element to attempt to reinstantiate somehow.
Assignee | ||
Comment 2•12 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Comment 3•12 years ago
|
||
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.
Assignee: nobody → jschoenick
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Priority: -- → P3
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
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)
Status: REOPENED → NEW
Summary: Pluginservice shouldn't need to reframe to reload plugins → nsPluginHost's handling of navigator.plugins.refresh() is broken
Assignee | ||
Comment 8•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6387618aa9b4 test failures in
* test_nice_plugin_name.js
* test_persist_in_prefs.js
* test_plugins.js
Assignee: jschoenick → benjamin
Priority: P3 → P1
Comment 9•12 years ago
|
||
Those depend on ReloadPlugins() finding new (test-)plugin libraries and sorting them before the previous ones due to newer mtime.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #740928 -
Flags: superreview?(bzbarsky)
Attachment #740928 -
Flags: review?(jschoenick)
Comment 11•12 years ago
|
||
Comment on attachment 740928 [details] [diff] [review]
Neuter parts of navigator.plugins.reload, rev. 1
sr=me
Attachment #740928 -
Flags: superreview?(bzbarsky) → superreview+
Comment 12•12 years ago
|
||
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.
Attachment #740928 -
Flags: superreview?(bzbarsky)
Attachment #740928 -
Flags: superreview+
Attachment #740928 -
Flags: review?(jschoenick)
Attachment #740928 -
Flags: review+
Comment 13•12 years ago
|
||
Apparently splinter-review doesn't detect mid-airs :(
Comment 14•12 years ago
|
||
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... ;)
Attachment #740928 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 15•12 years ago
|
||
> 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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago → 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Assignee | ||
Comment 17•12 years ago
|
||
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.
Attachment #740928 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Updated•12 years ago
|
status-firefox23:
--- → fixed
tracking-firefox23:
--- → +
Comment 18•12 years ago
|
||
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.
Attachment #740928 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•12 years ago
|
||
Comment 20•12 years ago
|
||
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•11 years ago
|
||
I confirm this is fixed in FF 23b3 on Mac OS 10.8, Windows 7x64 and Ubuntu 13.04 x86.
Status: RESOLVED → VERIFIED
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•