Plugin doorhanger breaks when plugins were removed from the page

VERIFIED FIXED in Firefox 24

Status

()

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: gfritzsche, Assigned: benjamin)

Tracking

(Blocks 1 bug, {reproducible})

Trunk
mozilla25
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 verified, firefox25 verified)

Details

Attachments

(2 attachments)

Posted file test case
* load page with plugin
* remove plugin from page
* check out plugin doorhanger

AR:
* doorhanger can still be opened and is broken

Expected:
One of:
(1) doorhanger is not accessible anymore
(2) doorhanger is still accessible and allows to set permission for plugins that were on the page
(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> (2) doorhanger is still accessible and allows to set permission for plugins
> that were on the page

I'm personally leaning toward that option as that would also deal with bug 876362 (i.e. handling CTP on pages that remove plugin elements after a short time if they failed to activate).
I agree, and I am hoping to fix that in bug 876362. But that is in fact pretty hard to do with the current implementation, because of how it (re)constructs the plugin list when the doorhanger is shown. I think the simpler thing to do here for beta is to add a listener for plugins being removed and hide the doorhanger when that happens.
Ok, i'll see to hiding the notification in this bug then.
Assignee: nobody → georg.fritzsche
I have most of a patch, and other things stacked up in the same area, so I'll take this.
Assignee: georg.fritzsche → benjamin
Comment on attachment 772954 [details] [diff] [review]
Plugin doorhanger breaks when plugins are dynamically removed from a page. This implements the short-term solution of hiding the plugin doorhanger when no more plugins are on the page (the better long-term solution will keep showing the UI and track all t

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

I don't think the event firing here is what we want - it's possible for a tag to change to an unrelated plugin without firing any events to the front end, as well as to fire a PluginRemoved event and then continue running the plugin.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +228,1 @@
>                                         mEvent, true, true);

Should SimplePluginEvent also be doing some magic to ensure these are chrome-only?

@@ +684,5 @@
>      UnloadObject();
>    }
> +  nsCOMPtr<nsIRunnable> ev = new nsSimplePluginEvent(thisContent->GetCurrentDoc(),
> +                                           NS_LITERAL_STRING("PluginRemoved"));
> +  NS_DispatchToCurrentThread(ev);

This will only fire when an object tag is removed from the document -- not when it changes types, e.g. from plugin -> image or plugin -> different plugin type. This also doesn't seem to cover the case where a plugin is reparented -- it will be unbound from the tree and rebound, but never re-instantiate.

If we only want to fire an event when a tag that has initialized as a plugin changes to another MIME type, plugin or otherwise, you could add an |if (stateChange & eParamContentTypeChanged && oldType == eType_Plugin)| check to fire the event around here: http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#l1860

Another option would be to fire a "PluginTagReset" event or some such in UnloadObject, which will fire often (like twice when a tag is initially added to the document and opens a channel...) but always be followed by a PluginBindingAttached or PluginInstantiated if other fallback/plugins are loaded into the tag.
Attachment #772954 - Flags: review?(jschoenick) → review-
johns, I realize that there are some holes here; I'm mainly looking for the quick-n-dirty approach. Here are some factors:

* I don't need to know at all when we switch from one plugin type to another (yet): we only need to know if the plugin notification should be present at all
* We theoretically would want to know if something that *was* a plugin is now rendered using an iframe. But I consider that such an edge case that we don't need to worry about it for now
* We most-importantly need to know if a plugin which is not playing (in some click-to-play state) is removed from the document. UnloadObject doesn't catch those states.
Flags: needinfo?(jschoenick)
Comment on attachment 772954 [details] [diff] [review]
Plugin doorhanger breaks when plugins are dynamically removed from a page. This implements the short-term solution of hiding the plugin doorhanger when no more plugins are on the page (the better long-term solution will keep showing the UI and track all t

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> johns, I realize that there are some holes here; I'm mainly looking for the
> quick-n-dirty approach. Here are some factors:
> 
> * I don't need to know at all when we switch from one plugin type to another
> (yet): we only need to know if the plugin notification should be present at
> all
> * We theoretically would want to know if something that *was* a plugin is
> now rendered using an iframe. But I consider that such an edge case that we
> don't need to worry about it for now

What about plugin -> nothing, e.g. |obj.data = null| ? Or plugin -> display:none?

> * We most-importantly need to know if a plugin which is not playing (in some
> click-to-play state) is removed from the document. UnloadObject doesn't
> catch those states.

UnloadObject is called on UnbindFromTree for all states except eType_Image and running plugins -- for the latter it will be called when the plugin is actually stopped (not reparented)

I'm happy to r+ if you're okay with all the caveats and there is a proper fix on the way, but this method has a lot of broken edge cases.
Attachment #772954 - Flags: review- → review+
Comment on attachment 772954 [details] [diff] [review]
Plugin doorhanger breaks when plugins are dynamically removed from a page. This implements the short-term solution of hiding the plugin doorhanger when no more plugins are on the page (the better long-term solution will keep showing the UI and track all t

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

Apologies for the slow turn-around-time on the review.

> Also bug 887088 - Short-term UI solution: when a user enables multiple tabs
> on a site and enables a plugin on one of them, the plugins are not enabled
> on other tabs but the "continue allowing" button does nothing. This patch
> at makes the "Continue Allowing" button enable existing plugins of that type.

s/enables multiple tabs on a site/loads multiple tabs from a site/
s/patch at makes/patch makes/

::: browser/base/content/urlbarBindings.xml
@@ +1584,5 @@
> +          default:
> +            if (this.notification.options.primaryPlugin) {
> +              this._setState(this._states.MULTI_COLLAPSED);
> +            }
> +            else {

} else {
Attachment #772954 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/678e1cb9f587
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Flags: needinfo?(jschoenick)
Comment on attachment 772954 [details] [diff] [review]
Plugin doorhanger breaks when plugins are dynamically removed from a page. This implements the short-term solution of hiding the plugin doorhanger when no more plugins are on the page (the better long-term solution will keep showing the UI and track all t

[Approval Request Comment]
See approval notes in bug 888292
Attachment #772954 - Flags: approval-mozilla-aurora?
Attachment #772954 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 897348
The CTP doorhanger disappears after removing the plugin.
Verified fixed FF 24b1, 25.0a1 2013-08-05
Status: RESOLVED → VERIFIED
Blocks: 920527
Depends on: 922349
You need to log in before you can comment on or make changes to this bug.