1289802 - replace "install flash" content with click to play UI when flash is click to play

Status: VERIFIED FIXED

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

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: replace_flash_place_holders.patch

This patch keys this action off of navigator.plugins being queried, assuming that content is checking for the presence of flash and replacing the flash content with these banners off of that. There is probably a variety of urls they link to for the download, this takes care of two that I've found.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8775165 [details] [diff] [review]
replace_flash_place_holders.patch

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

I assume this is all part of the Flashkill roadmap, and that this strategy has the thumbs up? If so, feedback+, but some minor points below.

::: browser/modules/PluginContent.jsm
@@ +703,5 @@
>      let plugins = cwu.plugins;
>      let pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
>  
>      let pluginFound = false;
> +    let standInFound = false;

Is standInFound actually useful?

If we're iterating the plugins, and we see one with mozPluginClickToPlayStandIn, can't we just _not_ set pluginFound instead of setting standInFound?

Like, down below:

if (!plugin.mozPluginClickToPlayStandIn) {
  pluginFound = true;
}

I think that's effectively the same, no? And then we can get rid of standInFound.

@@ +740,5 @@
>      }
>      this.updateNotificationUI();
>    },
>  
> +  _replaceInstallPluginFrames: function() {

Needs documentation.

@@ +741,5 @@
>      this.updateNotificationUI();
>    },
>  
> +  _replaceInstallPluginFrames: function() {
> +    const inIDOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);

Unused

@@ +745,5 @@
> +    const inIDOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +    let links = this.content.document.getElementsByTagName("a");
> +    for (let link of links) {
> +      switch(link.href) {
> +      case "http://www.adobe.com/go/getflash/":

Are we likely to add more and more here? If so, maybe it makes sense to add these to an Array or Set, and when iterating the links, check for presence, instead of adding more and more variations to this switch.

@@ +746,5 @@
> +    let links = this.content.document.getElementsByTagName("a");
> +    for (let link of links) {
> +      switch(link.href) {
> +      case "http://www.adobe.com/go/getflash/":
> +      case "https://get.adobe.com/flashplayer":

Rest of this function puts the { on the same line, so probably should be:

case "https://get.adobe.com/flashplayer": {
  // ...
}

@@ +748,5 @@
> +      switch(link.href) {
> +      case "http://www.adobe.com/go/getflash/":
> +      case "https://get.adobe.com/flashplayer":
> +        {
> +          let rect = link.getBoundingClientRect();

So we're looping here, modifying the DOM, and then getting rects... I think that's going to cause "reflow thrash", where we have a high likelihood of reflowing during each tick of this loop where these links are found.

Is getting the rect really necessary?

@@ +751,5 @@
> +        {
> +          let rect = link.getBoundingClientRect();
> +          let parent = link.parentNode;
> +          let obj = parent.ownerDocument.createElement("object");
> +          obj.mozPluginClickToPlayStandIn = true;

I guess by virtue of Xray-wrappers, content can't fool us by setting this themselves, right?

@@ +752,5 @@
> +          let rect = link.getBoundingClientRect();
> +          let parent = link.parentNode;
> +          let obj = parent.ownerDocument.createElement("object");
> +          obj.mozPluginClickToPlayStandIn = true;
> +          obj.type="application/x-shockwave-flash";

spaces on either side of =

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: replace_place_holder_css.patch

I think doing this via CSS is a little cleaner

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8776095 [details] [diff] [review]
replace_place_holder_css.patch

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

r-'ing until I know more about this PluginPlaceholderReplaced event, which I can't seem to find being fired anywhere. Is there a dependency here that I'm not aware of?

::: browser/modules/PluginContent.jsm
@@ +38,5 @@
>      this.pluginCrashData = new Map();
>  
>      // Note that the XBL binding is untrusted
>      global.addEventListener("PluginBindingAttached", this, true, true);
> +    global.addEventListener("PluginPlaceholderReplaced", this, true, true);

Where is this event getting fired? A DXR around didn't reveal it... is there a missing patch here?

@@ +415,5 @@
>  
>      let plugin = event.target;
>      let doc = plugin.ownerDocument;
>  
> +    if (eventType ==  "PluginPlaceholderReplaced") {

Nit - one too many spaces after ==.

@@ +419,5 @@
> +    if (eventType ==  "PluginPlaceholderReplaced") {
> +      plugin.removeAttribute("href");
> +      let overlay = this.getPluginUI(plugin, "main");
> +      this.setVisibility(plugin, overlay, true);
> +      let inIDOMUtils = Components.classes["@mozilla.org/inspector/dom-utils;1"]

We should be able to use Cc and Ci in here.

@@ +421,5 @@
> +      let overlay = this.getPluginUI(plugin, "main");
> +      this.setVisibility(plugin, overlay, true);
> +      let inIDOMUtils = Components.classes["@mozilla.org/inspector/dom-utils;1"]
> +            .getService(Components.interfaces.inIDOMUtils);
> +      inIDOMUtils.addPseudoClassLock(plugin, "-moz-handler-clicktoplay");

Was the pluginProblem.xml binding already applied before we do this? If not, then by adding this pseudoclass, we're applying the binding, which should cause the PluginBindingAttached event to fire, which means that I don't think we'll need the click event handler on 426.

@@ +654,5 @@
>    },
>  
>    // Event listener for click-to-play plugins.
>    _handleClickToPlayEvent: function (plugin) {
> +    Cu.reportError("handle click to play event");

Leftover logging

@@ +660,5 @@
>      let pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
> +    let permissionString;
> +    if (plugin instanceof Ci.nsIDOMHTMLAnchorElement) {
> +      // We only have replacement content for Flash installs
> +      permissionString= pluginHost.getPermissionStringForType("application/x-shockwave-flash");

Nit - space before =. Same on line 671.

Also, we're using "application/x-shockwave-flash" enough that we might as well define it as a constant at the top of PluginContent.jsm.

@@ +761,5 @@
>        }
>      }
>  
>      // If there are no instances of the plugin on the page any more, what the
> +    // user probably needs is for us to allow and then refresh or if  we have

Nit - too many spaces after "if".

@@ +763,5 @@
>  
>      // If there are no instances of the plugin on the page any more, what the
> +    // user probably needs is for us to allow and then refresh or if  we have
> +    // a placeholder, the page needs to be refreshed to insert its plugins
> +    if (newState != "block" && (!pluginFound || placeHolderFound)) {

Okay, this makes more sense now. Thanks for updating the comment.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8776095 [details] [diff] [review]
replace_place_holder_css.patch

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

::: toolkit/pluginproblem/content/pluginProblem.xml
@@ +81,5 @@
> +
> +<binding id="replacement" extends="chrome://pluginproblem/content/pluginProblem.xml#pluginProblem" inheritstyle="false" chromeOnlyContent="true" bindToUntrustedContent="true">
> +  <implementation>
> +    <constructor>
> +      this.dispatchEvent(new CustomEvent("PluginPlaceholderReplaced"));

the event is being dispatched here

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8776095 [details] [diff] [review]
replace_place_holder_css.patch

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

Ah - I think I accidentally only reviewed PluginContent.jsm last time. Sorry about that. I've looked at the other files now.

I think my only concern, along with the nits I raised, is when this binding gets attached. See my question below.

::: toolkit/pluginproblem/content/pluginProblemBinding.css
@@ +27,5 @@
>      opacity: 1 !important;
>      -moz-binding: url('chrome://pluginproblem/content/pluginProblem.xml#pluginProblem') !important;
>  }
> +
> +a[href='https://get.adobe.com/flashplayer/'],

So if I'm understanding correctly, this doesn't get keyed off of navigator.plugins or anything... this is going to apply to any link that happens to link to downloading the flashplayer, and replacing it with the click-to-play UI.

I understand if we want to do that when links are presented as part of a site's fallback if Flash isn't found, but this looks like it'd apply to just an empty page with one link pointed at http://get.adobe.com/flashplayer.

Or am I misunderstanding something?

::: toolkit/pluginproblem/content/pluginProblemContent.css
@@ +73,5 @@
>  
>  .mainBox[chromedir="rtl"] {
>    direction: rtl;
>  }
> +a .hoverBox,

Nit - needs newline above.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: replace_place_holder_css.patch

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8778309 [details] [diff] [review]
replace_place_holder_css.patch

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

I'm slightly worried about potential performance impacts here. It might be worth doing a Try push to Talos tp5 to see if there's any impact with this patch - but the idea of adding this style wholescale across every website, which will do these lookups to <body> to check for the class worries me. Are those worries unfounded?

::: browser/modules/PluginContent.jsm
@@ +425,5 @@
> +      let overlay = this.getPluginUI(plugin, "main");
> +      this.setVisibility(plugin, overlay, true);
> +      let inIDOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
> +            .getService(Ci.inIDOMUtils);
> +      inIDOMUtils.addPseudoClassLock(plugin, "-moz-handler-clicktoplay");

I just want to double-check - we're certain that when we add this pseudoclass and return to the main thread, we don't happen to detach the replacement binding and attach the pluginProblem binding? Because if so, we're going to fire the PluginBindingAttached event, which should (I believe) attach the click event listener that you're adding on 430.

Or am I misunderstanding?

::: toolkit/pluginproblem/content/pluginProblemBinding.css
@@ +27,5 @@
>      opacity: 1 !important;
>      -moz-binding: url('chrome://pluginproblem/content/pluginProblem.xml#pluginProblem') !important;
>  }
> +
> +body.moz-hidden-plugin-touched a[href='https://get.adobe.com/flashplayer/'],

I worry about potential performance issues here. We're going to be scanning every <a>, and then going up the tree all the way to the body to check for the moz-hidden-plugin-touched class. I'm not sure if style calculations have been optimized for this sort of thing - if so, fine, but if not, that's a lot of tree-walks-to-root for each <a>.

Have we measured to see if this impacts our tp5 set at all?

I wonder if it might be better to wait until the page touches the navigator.plugins array, scanning for Flash, and then when we detect that, to then do a querySelectorAll for those links, and set a pseudoclass which applies the binding. I don't know - that's just off the top of my head.

I'm just wary about adding this rule that's going to be applied to all content everywhere that's adding a bunch of look-ups to see if every <a> ever belongs to a body with a particular class.

Comment 9

[Chris Peterson [:cpeterson]]

In bug 1282486, Felipe suggested detection of "Install Flash" content should look for the words "Adobe", "Flash", and possibly "install" or "download" in several popular languages instead of just looking for adobe.com URLs.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #7)
> Comment on attachment 8778309 [details] [diff] [review]
> replace_place_holder_css.patch
> 
> Review of attachment 8778309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm slightly worried about potential performance impacts here. It might be
> worth doing a Try push to Talos tp5 to see if there's any impact with this
> patch - but the idea of adding this style wholescale across every website,
> which will do these lookups to <body> to check for the class worries me. Are
> those worries unfounded?
My knowledge of the perf impacts of selectors is limited. I'll push to try.

> ::: browser/modules/PluginContent.jsm
> @@ +425,5 @@
> > +      let overlay = this.getPluginUI(plugin, "main");
> > +      this.setVisibility(plugin, overlay, true);
> > +      let inIDOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
> > +            .getService(Ci.inIDOMUtils);
> > +      inIDOMUtils.addPseudoClassLock(plugin, "-moz-handler-clicktoplay");
> 
> I just want to double-check - we're certain that when we add this
> pseudoclass and return to the main thread, we don't happen to detach the
> replacement binding and attach the pluginProblem binding? Because if so,
> we're going to fire the PluginBindingAttached event, which should (I
> believe) attach the click event listener that you're adding on 430.
> 
> Or am I misunderstanding?
sorry, meant to answer this comment earlier. The binding for the -moz-handler-clicktoplay psuedo class only applies to object, embed and applet tags, so no we're not rebinding.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=6682b05784a2

tp5o results should be here when they're ready https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=0ba72e8027cf&newProject=try&newRevision=6682b05784a2&framework=1&filter=tp5o&showOnlyImportant=0

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 8778309 [details] [diff] [review]
replace_place_holder_css.patch

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

::: browser/modules/PluginContent.jsm
@@ +413,5 @@
>          return;
>        }
>        this._showClickToPlayNotification(pluginTag, false);
> +      let doc = event.target.defaultView.document;
> +      doc.body.classList.add("moz-hidden-plugin-touched");

I do not feel like adding webpage-visible change to the body element.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)
> tp5o results should be here when they're ready
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> inbound&originalRevision=0ba72e8027cf&newProject=try&newRevision=6682b05784a2
> &framework=1&filter=tp5o&showOnlyImportant=0
There is amazingly no overlap in the test results, possibly due to windowsxp now being windows7-xp. That said, comparing windowsxp to windows7-xp shows no regression.


(In reply to Masatoshi Kimura [:emk] from comment #12)
> I do not feel like adding webpage-visible change to the body element.
I'm not particularly concerned with it being webpage visible (k17e was arguing it should be for websites that really really really need to know). That said, its a simple change to make it a pseudo class.

Another alternative would be to add a stylesheet with the a[hrref=*] rules dynamically.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: replace_place_holder_dynamic_css.patch

This alternative adds the style sheet dynamically and doesn't add a class to the body.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8778974 [details] [diff] [review]
replace_place_holder_dynamic_css.patch

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

Yeah, I think I like this dynamic approach better. Thanks blassey! (It'd be great to have some automated tests for this)

::: browser/modules/PluginContent.jsm
@@ +409,5 @@
>      }
>  
>      if (eventType == "HiddenPlugin") {
> +      let win = event.target.defaultView;
> +      if (win.mozHiddenPluginTouched != true) {

if (!win.mozHiddenPluginTouched) {

@@ +433,5 @@
> +      plugin.removeAttribute("href");
> +      let overlay = this.getPluginUI(plugin, "main");
> +      this.setVisibility(plugin, overlay, true);
> +      let inIDOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
> +            .getService(Ci.inIDOMUtils);

Nit - let's line up the . with the [

@@ +434,5 @@
> +      let overlay = this.getPluginUI(plugin, "main");
> +      this.setVisibility(plugin, overlay, true);
> +      let inIDOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"]
> +            .getService(Ci.inIDOMUtils);
> +      inIDOMUtils.addPseudoClassLock(plugin, "-moz-handler-clicktoplay");

Let's mention why we're adding this (presumably for the styling).

::: toolkit/pluginproblem/content/pluginReplaceBinding.css
@@ +9,5 @@
> +a[href='https://get.adobe.com/flashplayer'],
> +a[href='http://get.adobe.com/flashplayer'],
> +a[href='https://www.adobe.com/go/getflash/'],
> +a[href='http://www.adobe.com/go/getflash/'] {
> +    display: inline-block;

Nit - 2 space indentation please.

Comment 16

[Benjamin Smedberg]

Brad, I've updated the user story with what I believe is the intended behavior of this feature. Can you verify that this is the thing that was implemented?

I don't think I need to review the code here: but I do think that we should have some careful unit tests before we turn this on by default.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Note, I will set plugins.navigator_hide_disabled_flash to true when I land this.

One point to correct, when the user clicks the in content UI, we won't show the door hanger, we'll just reload the page with the plugin activated.

Comment 18

[Benjamin Smedberg]

> One point to correct, when the user clicks the in content UI, we won't show
> the door hanger, we'll just reload the page with the plugin activated.

This is not correct/desired behavior. We want to protect this UI against clickjacking, which is why we have the in-content plugin UI always show the doorhanger rather than immediately activating Flash. It also gives the user clear visual feedback about how to reverse their decision later if they desire.

> Note, I will set plugins.navigator_hide_disabled_flash to true when I land
> this.

I don't think we should do this. We still have significant false-negative and false-positive bugs which need to addressed.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #18)
> > One point to correct, when the user clicks the in content UI, we won't show
> > the door hanger, we'll just reload the page with the plugin activated.
> 
> This is not correct/desired behavior. We want to protect this UI against
> clickjacking, which is why we have the in-content plugin UI always show the
> doorhanger rather than immediately activating Flash. It also gives the user
> clear visual feedback about how to reverse their decision later if they
> desire.
I was mistaken, we do show the door hanger. So your user story is correct.
> 
> > Note, I will set plugins.navigator_hide_disabled_flash to true when I land
> > this.
> 
> I don't think we should do this. We still have significant false-negative
> and false-positive bugs which need to addressed.
What false negative bugs are you referring to? I'd argue that given that the plugin icon is subtle false positives are pretty benign and shouldn't block this.

Also, if your concern is block listed plugins, we don't need to hide them from navigator.plugins.

Comment 20

[Benjamin Smedberg]

There are two classes of false-negative bugs:

* Not showing the plugin icon at all: you've addressed the two known cases where this happens. We can be confident that there are more, since the patch that landed doesn't detect any kind of navigator.plugins enumeration, and we know that happens sometimes. There's risk around this, and we don't have a good way to know how much risk until we hit beta or release. 
* Users don't see the plugin icon. We already know this is true from bugs that have been filed, plus the prior user research that caused us to add the notification bar in the first place. In particular, if we do more prominent prompting by showing the notification bar, then the false-positive cases get a lot worse.

I'm happy to let the code live behind the pref, and do a bunch of experimental testing. That's one of the reasons we're running the test pilot study after all. But hiding Flash in navigator.plugins is not an essential part of making Flash CTP by default, and so I want to focus our design and engineering energy on the bugs related to the test pilot experiment: Felipe is using bug 1277346 as the metabug for that work.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #20)
> There are two classes of false-negative bugs:
> 
> * Not showing the plugin icon at all: you've addressed the two known cases
> where this happens. We can be confident that there are more, since the patch
> that landed doesn't detect any kind of navigator.plugins enumeration, and we
> know that happens sometimes. There's risk around this, and we don't have a
> good way to know how much risk until we hit beta or release. 
As you said, the known cases have been addressed. We won't find the unknown without turning this on.
> * Users don't see the plugin icon. We already know this is true from bugs
> that have been filed, plus the prior user research that caused us to add the
> notification bar in the first place. In particular, if we do more prominent
> prompting by showing the notification bar, then the false-positive cases get
> a lot worse.
This bug addresses this point, but again we won't know how well unless we turn this on.
> 
> I'm happy to let the code live behind the pref, and do a bunch of
> experimental testing. That's one of the reasons we're running the test pilot
> study after all. But hiding Flash in navigator.plugins is not an essential
> part of making Flash CTP by default,
Given the experience of having Flash CTP right now, I think that's naive
> and so I want to focus our design and
> engineering energy on the bugs related to the test pilot experiment: Felipe
> is using bug 1277346 as the metabug for that work.
What design and engineering energy are you conserving by keeping this off?

Comment 22

[Mike Conley (:mconley) (:⚙️)]

I'd originally needinfo'd myself to make sure we were all on the same page about the plan for this project. As https://wiki.mozilla.org/Flash/Hidden_CTP is now the plan that bsmedberg and blassey have agreed upon, clearing that needinfo.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb51f3bada90
Replace 'install flash' content with click to play UI when flash is click to play r=mconley

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb51f3bada90

Comment 25

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on Fx 51.0.1.
Verified fixed on a couple of Flash games pages (miniclip.com, wickedgoodgames.com/flash.shtml) on Fx 52b9 on Win 10, Ubuntu 14.04, OS X 10.11.