889788 - Plugin doorhanger breaks when plugins were removed from the page

Status: VERIFIED FIXED

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

Description

[Georg Fritzsche [:gfritzsche]]

Attachment: 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

Comment 1

[Georg Fritzsche [:gfritzsche]]

(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).

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Ok, i'll see to hiding the notification in this bug then.

Comment 4

[Benjamin Smedberg]

I have most of a patch, and other things stacked up in the same area, so I'll take this.

Comment 5

[Benjamin Smedberg]

Attachment: 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

Comment 6

[John Schoenick [:johns]]

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.

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[John Schoenick [:johns]]

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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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 {

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Something in this push was causing mochitest-bc failures. Backed out.
http://hg.mozilla.org/integration/mozilla-inbound/rev/e6d3454da7a7

https://tbpl.mozilla.org/php/getParsedLog.php?id=25380390&tree=Mozilla-Inbound

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/678e1cb9f587

Comment 12

[Benjamin Smedberg]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c5c2d41b0572

Comment 15

[Paul Silaghi, QA [:pauly]]

The CTP doorhanger disappears after removing the plugin.
Verified fixed FF 24b1, 25.0a1 2013-08-05