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

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
Plug-ins
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

(Blocks: 1 bug)

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26+ fixed, b2g-v1.2 fixed)

Details

Attachments

(7 attachments)

30.62 KB, patch
jaws
: review+
Details | Diff | Splinter Review
18.80 KB, patch
jaws
: review+
Details | Diff | Splinter Review
2.12 KB, patch
jaws
: review+
Details | Diff | Splinter Review
1.51 KB, patch
jaws
: review+
Details | Diff | Splinter Review
4.59 KB, patch
jaws
: review+
Details | Diff | Splinter Review
8.54 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.03 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 745187
(Assignee)

Comment 1

4 years ago
Created 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

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.
Attachment #821306 - Flags: review?(jaws)
(Assignee)

Comment 2

4 years ago
Created attachment 821307 [details] [diff] [review]
test fixup.
Attachment #821307 - Flags: review?(jaws)
(Assignee)

Updated

4 years ago
Assignee: georg.fritzsche → benjamin
(Assignee)

Comment 3

4 years ago
Created attachment 821315 [details] [diff] [review]
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
Attachment #821315 - Flags: review?(jaws)
(Assignee)

Comment 4

4 years ago
Created 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
Attachment #821459 - Flags: review?(jaws)
(Assignee)

Comment 5

4 years ago
Created attachment 821473 [details] [diff] [review]
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).
Attachment #821473 - Flags: review?(jaws)
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.
Attachment #821306 - Flags: review?(jaws)
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.
Attachment #821306 - Flags: review-
Tracking so this stays on our radar for FF26, would want this in if possible when we ship CTP by default.
tracking-firefox26: --- → +
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.
Attachment #821307 - Flags: review?(jaws)
Attachment #821315 - Flags: review?(jaws)
Attachment #821459 - Attachment description: 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 → 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
Attachment #821459 - Flags: review?(jaws)
Attachment #821473 - Flags: review?(jaws)
(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.
Attachment #821306 - Flags: review-
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.
Attachment #821306 - Flags: review+
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.
Attachment #821307 - Flags: review+
Attachment #821315 - Flags: review+
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.
Attachment #821459 - Flags: review+
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.
Attachment #821473 - Flags: review+
Created attachment 822531 [details] [diff] [review]
Part E - Followups from the previous set of patches

Pushed the full series to try,
https://tbpl.mozilla.org/?tree=Try&rev=57a790618db1
Attachment #822531 - Flags: review?(dkeeler)
Blocks: 909342
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?
Attachment #822531 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 17

4 years ago
> 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.
(Assignee)

Comment 18

4 years ago
> 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.
(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.
(Assignee)

Comment 20

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/69ccec8b45fa
https://hg.mozilla.org/integration/fx-team/rev/809832214af3
https://hg.mozilla.org/mozilla-central/rev/809832214af3
https://hg.mozilla.org/mozilla-central/rev/69ccec8b45fa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 22

4 years ago
Typo?
2587 	#plugins-notification-icon {

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

Comment 23

4 years ago
Created attachment 823266 [details] [diff] [review]
followup to fix icon on Windows,

Good catch.

Comment 24

4 years ago
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
Attachment #823266 - Flags: review+
(Assignee)

Comment 25

4 years ago
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.
Attachment #821306 - Flags: approval-mozilla-beta?
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.
Attachment #821306 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 27

4 years ago
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.
status-firefox26: --- → fixed
Do we have websites where this is known to fail so QA can verify this is fixed in Firefox 26.0b1?
(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.
Keywords: verifyme
(Assignee)

Comment 30

4 years ago
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.
(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.
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
status-b2g-v1.2: --- → fixed
Depends on: 932786
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.
Status: RESOLVED → VERIFIED
status-firefox26: fixed → verified
Keywords: verifyme
Actually i think it would be better to wait the fixes for bug 745187 and bug 932786 first.
Status: VERIFIED → RESOLVED
Last Resolved: 4 years ago4 years ago
status-firefox26: verified → fixed
(Assignee)

Updated

4 years ago
Duplicate of this bug: 840944
Duplicate of this bug: 940958
You need to log in before you can comment on or make changes to this bug.