926605 - The click-to-activate doorhanger should show all plugins the page tried to use, even if they were removed

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

Some pages try to use a plugin, and if it doesn't work they immediately remove the plugin from the page. We should continue to include those plugins in the click-to-activate doorhanger until the next navigation.

I believe this will require changing how we show the doorhanger: currently we re-create the PopupNotifications object every time a plugin is added to the page; we should try to create the PopupNotification only once, and merely add more items to it they are discovered.

Comment 1

[Benjamin Smedberg]

Attachment: part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it

This was surprisingly not as invasive as I thought it was going to be. There is a second part that I haven't written yet: if a site removes a plugin and the user subsequently chooses to allow, we should refresh the page.

I started try run https://tbpl.mozilla.org/?tree=Try&rev=61f44f18eae1 on these.

Comment 2

[Benjamin Smedberg]

Attachment: test fixup.

Comment 3

[Benjamin Smedberg]

Attachment: part B - automatically refresh the page if the users chooses to enable a plugin but its no longer on the page

I ran a separate try run with this extra change: https://tbpl.mozilla.org/?tree=Try&rev=dfb19415e569

Comment 4

[Benjamin Smedberg]

Attachment: more test fixup - browser_CTP_drag_drop needs to wait for focus before opening the plugin doorhanger, because the window "open" event can actually occur before the widget is visible, which means that the popup may be dismissed by its own parent's delayed

Comment 5

[Benjamin Smedberg]

Attachment: part D - fix activation via the context menu: I didn't realize nsContextMenu.js was calling _showClickToPlayNotification

This re-enables this test on Linux because I believe I understand what was previously causing it to fail (asynchronous popup focus after the popupshowing DOM event was fired). But if this causes intermittent linux-only orange it can be re-disabled (bug 909342).

Comment 6

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

Comment on attachment 821306 [details] [diff] [review]
part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it

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

There are a lot of CSS changes in this patch, of which I'm not sure why any of them are required.

::: browser/base/content/browser-plugins.js
@@ +729,5 @@
>        }
>      }
>    },
>  
> +  _showClickToPlayNotification: function PH_showClickToPlayNotification(aBrowser, aPlugin, aShowNow) {

This function is now extremely long and does many things. Why remove the _makeCenterActions function?

@@ +780,5 @@
> +      pluginInfo.hidden = true;
> +
> +      let overlay = this.getPluginUI(plugin, "main");
> +      if (overlay && overlay.style.visibility != "hidden" && overlay.style.visibility != "") {
> +        pluginInfo.hidden = false;

Why is this checking if the overlay's visibility property is an empty string? Only comparing against "hidden" should be sufficient (we never use visibility:collapse for this overlay).

@@ +845,5 @@
>      }
>  
> +    if (notification) {
> +      // Don't modify the notification UI while it's on the screen, that would be
> +      // jumpy and might allow clickjacking.

I think this clickjacking concern is a bit of a stretch, although I can understand the desire to stay away from a jumpy UI.

::: browser/base/content/browser.xul
@@ -534,5 @@
>              <image id="webapps-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="plugins-notification-icon" class="notification-anchor-icon" role="button"/>
>              <image id="web-notifications-notification-icon" class="notification-anchor-icon" role="button"/>
> -            <image id="alert-plugins-notification-icon" class="notification-anchor-icon" role="button"/>
> -            <image id="blocked-plugins-notification-icon" class="notification-anchor-icon" role="button"/>

I don't understand why this change is necessary to fix this bug.

Comment 7

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

Comment on attachment 821306 [details] [diff] [review]
part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it

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

::: browser/base/content/browser-plugins.js
@@ -720,5 @@
> -      return;
> -    }
> -
> -    centerActions.sort(function(a, b) {
> -      return a.pluginName.localeCompare(b.pluginName);

The sorting of the center actions shouldn't have been removed.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking so this stays on our radar for FF26, would want this in if possible when we ship CTP by default.

Comment 9

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

The entirety of these patches feels very risky to consider for a last-day Beta uplift. I'm trying to put together a simpler patch that would be less risky.

Comment 10

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

(In reply to Jared Wein [:jaws] from comment #9)
> The entirety of these patches feels very risky to consider for a last-day
> Beta uplift. I'm trying to put together a simpler patch that would be less
> risky.

Sorry, I misunderstood the nature of this urgency. This would be a last-day Aurora uplift, or early Beta uplift. I'm still concerned about the reach of these patches, but that does make me feel a bit better.

Comment 11

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

Comment on attachment 821306 [details] [diff] [review]
part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it

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

r=me on these changes, I'm handing nits/issues in a followup patch.

Comment 12

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

Comment on attachment 821307 [details] [diff] [review]
test fixup.

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

::: browser/base/content/test/general/browser_pluginnotification.js
@@ +441,5 @@
>    var objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
>    ok(!objLoadingContent.activated, "Test 18f, Plugin should not be activated");
>  
> +  notification.options.eventCallback = function() {
> +    notification.options.eventCallback = gPluginHandler._clickToPlayNotificationEventCallback;

This looks broken. The event callback will no longer actually call the original event callback, it just sets the callback.

Comment 13

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

Comment on attachment 821459 [details] [diff] [review]
more test fixup - browser_CTP_drag_drop needs to wait for focus before opening the plugin doorhanger, because the window "open" event can actually occur before the widget is visible, which means that the popup may be dismissed by its own parent's delayed

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

::: browser/base/content/test/general/browser_CTP_drag_drop.js
@@ +69,4 @@
>  }
>  
>  function part6() {
>    gNewWindow.removeEventListener("load", handleEvent);

This line is obsolete now.

Comment 14

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

Comment on attachment 821473 [details] [diff] [review]
part D - fix activation via the context menu: I didn't realize nsContextMenu.js was calling _showClickToPlayNotification

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

::: browser/base/content/test/general/Makefile.in
@@ -58,5 @@
>    $(NULL)
>  endif
>  
>  # browser_CTP_context_menu.js fails intermittently on Linux (bug 909342)
> -ifndef MOZ_WIDGET_GTK

I'd rather not try to solve this in the same bug. We can land this change (as well as removing the above comment) in bug 909342.

Comment 15

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

Attachment: Part E - Followups from the previous set of patches

Pushed the full series to try,
https://tbpl.mozilla.org/?tree=Try&rev=57a790618db1

Comment 16

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 822531 [details] [diff] [review]
Part E - Followups from the previous set of patches

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

LGTM.

::: browser/base/content/browser-plugins.js
@@ +750,5 @@
>        let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                               .getInterface(Ci.nsIDOMWindowUtils);
>        // cwu.plugins may contain non-plugin <object>s, filter them out
>        plugins = cwu.plugins.filter((plugin) =>
> +      plugin.getContentTypeForMIMEType(plugin.actualType) == Ci.nsIObjectLoadingContent.TYPE_PLUGIN);

Should this be indented more as an indication it's not it's own statement?

Comment 17

[Benjamin Smedberg]

> There are a lot of CSS changes in this patch, of which I'm not sure why any
> of them are required.

Previously we would give the notification a different icon ID based on whether it was normal/hidden/insecure. But that status can now change dynamically as plugins are added, so I changed things so that we use a single icon ID and add the other styles via classes. I didn't see a low-risk way to make PopupNotifications let you change the icon ID of a notification after it was created.


> 
> ::: browser/base/content/browser-plugins.js
> @@ +729,5 @@
> >        }
> >      }
> >    },
> >  
> > +  _showClickToPlayNotification: function PH_showClickToPlayNotification(aBrowser, aPlugin, aShowNow) {
> 
> This function is now extremely long and does many things. Why remove the
> _makeCenterActions function?

Primarily because it no longer operates on all plugins in a page, just a subset, but we could keep it factored out if that would be better.

> 
> @@ +780,5 @@
> > +      pluginInfo.hidden = true;
> > +
> > +      let overlay = this.getPluginUI(plugin, "main");
> > +      if (overlay && overlay.style.visibility != "hidden" && overlay.style.visibility != "") {
> > +        pluginInfo.hidden = false;
> 
> Why is this checking if the overlay's visibility property is an empty
> string? Only comparing against "hidden" should be sufficient (we never use
> visibility:collapse for this overlay).

Right after an element is inserted into the DOM but before reflow, it doesn't have any computed style at all, so it was actually returning '' in some cases. This was necessary to fix tests.

> This looks broken. The event callback will no longer actually call the
> original event callback, it just sets the callback.

That is true but actually irrelevant in this case because the only thing the showing callback does is telemetry. Wouldn't hurt to forward, but it doesn't break anything.

Yes it should be indented.

Comment 18

[Benjamin Smedberg]

> The sorting of the center actions shouldn't have been removed.

Actually I removed it intentionally. Previously I had to sort them because I needed a stable ordering and that was the only way to get it. Now, though, they are sorted by the order they were actually used on the page, which I think is a better choice because advertising-only plugins are more likely to be later/last in the list.

Comment 19

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

(In reply to Benjamin Smedberg  [:bsmedberg] (Away 24-25 October) from comment #18)
> because advertising-only plugins are more likely to
> be later/last in the list.

I'm not sure what data you have to show this, but I doubt users will understand it. Unless we were to include data on the popup notification stating the order that plugins were instantiated (which is too technical for users), I highly doubt that users will understand why the plugins are displayed in instantiation order. Absent of an obvious-to-users natural sorting order, we should continue to sort the plugins alphabetically.

Further, we don't update the popup if it is already being displayed, so I'm not worried about users accidentally clicking on the wrong plugin based on a new one getting inserted in the middle.

Comment 20

[Benjamin Smedberg]

https://hg.mozilla.org/integration/fx-team/rev/69ccec8b45fa
https://hg.mozilla.org/integration/fx-team/rev/809832214af3

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/809832214af3
https://hg.mozilla.org/mozilla-central/rev/69ccec8b45fa

Comment 22

[Alfred Kayser]

Typo?
2587 	#plugins-notification-icon {

This should be:
2587 	#plugins-notification-icon:active {

Comment 23

[:Gijs (he/him)]

Attachment: followup to fix icon on Windows,

Good catch.

Comment 24

[:Gijs (he/him)]

Comment on attachment 823266 [details] [diff] [review]
followup to fix icon on Windows,

Landed directly on m-c with approval from the sheriffs over IRC, as https://hg.mozilla.org/mozilla-central/rev/c5e4bb7db0b9

Comment 25

[Benjamin Smedberg]

Comment on attachment 821306 [details] [diff] [review]
part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Missing feature in CtP plugins, required for correct functioning of some Java sites as well as Google Earth
User impact if declined: We need to disable CtP-by-default
Testing completed (on m-c, etc.): in today's nightly, and I've done some personal focused testing
Risk to taking this patch (and alternatives if risky): This patch is not tiny, but is limited to the CtP code, so regressions are unlikely to affect the rest of the browsing experience and are likely to show up with some targeted QA.
String or IDL/UUID changes made by this patch: None. There are theme changes that might affect theme authors, so I'll set addon-compat.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 821306 [details] [diff] [review]
part A - When a plugin is removed from a page, continue showing it in the plugin doorhanger, to deal with the cases where the site removes a plugin immediately after trying to use it

Approving for uplift to Beta (26) now that we've completed the merge. This should go into our first Beta.

Comment 27

[Benjamin Smedberg]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d9759153b18
https://hg.mozilla.org/releases/mozilla-beta/rev/00d300f0de04
https://hg.mozilla.org/releases/mozilla-beta/rev/f966e94fd7c4

Some merging was required, but only for the mochitest manifest file and for the file move from tests/ to tests/general, so no functional changes.

Comment 28

[u279076]

Do we have websites where this is known to fail so QA can verify this is fixed in Firefox 26.0b1?

Comment 29

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #28)
> Do we have websites where this is known to fail so QA can verify this is
> fixed in Firefox 26.0b1?

Google Earth, I think.

Comment 30

[Benjamin Smedberg]

Currently this does not work with Google Earth. I'm investigating as to why.

This certainly does work with the "Remove Plugin" at http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html

I'm getting some odd results at https://www.mojebanka.cz/InternetBanking/ where the icon shows up if I click several times, but I think there's still an issue here when the plugin is removed quickly.

Comment 31

[u279076]

(In reply to Benjamin Smedberg  [:bsmedberg] (Away 24-25 October) from comment #30)
> Currently this does not work with Google Earth. I'm investigating as to why.

Okay, thanks. Paul reported this is still not working in bug 745187.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Automatic merge from m-b to b2g26:
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5d9759153b18
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/00d300f0de04
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f966e94fd7c4

Comment 33

[Bogdan Maris [:bogdan_maris], Desktop QA]

Using this demo http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html I can see that this is fixed on Windows 7 x64 and Mac OS X 10.9 with Firefox 26 beta 1.

Comment 34

[Bogdan Maris [:bogdan_maris], Desktop QA]

Actually i think it would be better to wait the fixes for bug 745187 and bug 932786 first.