1186948 - remove Flash from navigator.plugins when it's click-to-play

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Comment 1

[Chris Peterson [:cpeterson]]

In bug 757726, I hid plugins from navigator.plugins enumeration (to prevent trackers from fingerprinting all available plugins) while allowing content to query for individual plugins by name, e.g. navigator.plugins["Shockwave Flash"]. Unfortunately, this broke a lot of content (even after we whitelisted Flash to not be hidden) because all the plugin detection example code that web developers copy/paste (and also SWFObject) enumerates navigator.plugins, comparing every plugin name for the desired plugin name.

In Tech Evangelism bug 934107, we reached out to some websites to fix their plugin detection, but this was a Sisyphean effort so I removed the plugin hiding feature in bug 1169945.

http://zombo.com/ is an example of a website that directly embeds Flash without plugin detection.

http://trailers.apple.com/ is an example of a website that uses navigator.plugins enumeration to detect the QuickTime plugin (bug 934108).

Comment 2

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

Attachment: c2p_nav_plug.patch

this works for e10s tabs, but not non-e10s tabs.

My test case has been twitch. I'd be interested if you have any other sites you thing would be good to test against.

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 8638354 [details] [diff] [review]
c2p_nav_plug.patch

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

Very interesting. So when a website enumerates navigator.plugins, it does not see click-to-play plugins? But if the website asks for a click-to-play plugin by name (e.g. navigator.plugins["Shockwave Flash"]), we then show the click-to-play door hanger?

So the result is that most websites don't detect click-to-play plugins (because most use navigator.plugins enumeration to detect a given plugin), but websites that *really* care can rewrite their plugin detection code to query navigator.plugins by name?

::: browser/modules/PluginContent.jsm
@@ +209,5 @@
>  
> +  _getPluginInfoForTag: function (pluginTag, mimeType, fallbackType) {
> +    let pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
> +
> +    let tagMimetype;

tagMimeType is returned from this function but it is never initialized.

::: dom/base/nsPluginArray.h
@@ +60,5 @@
>    void EnsurePlugins();
>  
>    nsCOMPtr<nsPIDOMWindow> mWindow;
>    nsTArray<nsRefPtr<nsPluginElement> > mPlugins;
> +  nsTArray<nsRefPtr<nsPluginElement> > mAllPlugins;

If you land this patch, I think these member variables should be renamed s/mPlugins/mNonClickToPlayPlugins/ and s/mAllPlugins/mPlugins/.

Comment 4

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

(In reply to Chris Peterson [:cpeterson] from comment #3)
> Comment on attachment 8638354 [details] [diff] [review]
> c2p_nav_plug.patch
> 
> Review of attachment 8638354 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/base/nsPluginArray.h
> @@ +60,5 @@
> >    void EnsurePlugins();
> >  
> >    nsCOMPtr<nsPIDOMWindow> mWindow;
> >    nsTArray<nsRefPtr<nsPluginElement> > mPlugins;
> > +  nsTArray<nsRefPtr<nsPluginElement> > mAllPlugins;
> 
> If you land this patch, I think these member variables should be renamed
> s/mPlugins/mNonClickToPlayPlugins/ and s/mAllPlugins/mPlugins/.

this doesn't just apply to click to play addons, it also effects blocked addons. I tend to think the current names reflect reality well enough.

Comment 5

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

Attachment: c2p_nav_plug.patch

I now realize that the funky behavior I was seeing in non-e10s with this patch is the same as without this patch, so that's a separate bug.

Comment 6

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

Attachment: c2p_nav_plug.patch

needed to rev the uuid of nsIPluginHost

Comment 7

[Benjamin Smedberg]

This seriously breaks many sites when we make Flash CtP for security issues. They do navigator.plugins or navigator.mimeTypes detection to prompt users to install Flash, and then the user doesn't even have the opportunity to activate Flash and reload.

We should not do this.

Comment 8

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

not sure if I follow. If the site does the detection through navigator.plugins, we show the click to play UI with this patch. I haven't implemented navigator.mimeTypes, but can add that.

Comment 9

[Benjamin Smedberg]

I must be misunderstanding something.

Let's say we mark Flash click-to-play. This patch causes Flash to not appear in navigator.plugins. When the site iterates through navigator.plugins using code of this sort:

var isFlashPresent = false;
for (var i = 0; i < navigator.plugins.length; ++i) {
  if (navigator.plugins[i].name == "Shockwave Flash") {
    isFlashPresent = true;
  }
}

Will cause the site to show the user a "please install Flash" notice.

This is a very common cargo-cult method of doing Flash detection.

Comment 10

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

I had an idea of how to address that pattern, but I'd like to know we need it before implementing. 

Essentially, I'd have nsPluginElement.GetName() return a subclass of nsCString (let's say nsPluginCString) that would check and call to Equals() against the list of plugins and act like navigator.plugins["Shockwave Flash"] was checked.

Comment 11

[Benjamin Smedberg]

Why are we messing with this at all? What's the value we're trying to provide?

Comment 12

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> Why are we messing with this at all? What's the value we're trying to
> provide?

I've found that many sites are falling back to HTML5 video when they don't detect flash, probably to support iOS. This is a much better experience than browsing with the current C2P implementation.

Comment 13

[Benjamin Smedberg]

Brad and I talked about this today and came up with the following plan:

* Limit this to Flash. The risk associated with other plugins is much greater, and doing this with other plugins greatly complicates the plugin click-to-play requirements.
* Don't do the very complex name magic when a site enumerates navigator.plugins: just assume that if they are enumerating navigator.plugins and Flash is click-to-play that they intend to use Flash.

Comment 14

[Doug Turner (:dougt)]

awesome..  will mark the current patch as unreviewed.

Comment 15

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

Attachment: c2p_nav_plug.patch

Comment 16

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/navigator-plugins-will-no-longer-contain-click-to-play-plugins/

Comment 17

[irefra2]

Attachment: chrome.exe

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 8642693 [details] [diff] [review]
c2p_nav_plug.patch

- In browser/modules/PluginContent.jsm:

+    Services.obs.addObserver(this, "Plugin::HiddenPluginTouched", false);
[...]
+  observe: function observe(aSubject, aTopic, aData) {
+   let pluginTag = aSubject;
+   this._showClickToPlayNotification(pluginTag, false);

Given how complex the observer/listener setup is in this code it may be worth checking that the topic is what we expect in this function, assert it if you will.

- In dom/base/nsPluginArray.cpp:

- In nsPluginArray::EnsurePlugins():

I suspect we need this method to change its initial early bailout to also check if we have CTP plugins, or we'll run into a possible scenario where if this method gets called multiple times (through Refresh(), or other code paths), we could end up with multiple nsPluginElements in the CTP array for each hidden plugin. Refresh() should also clear mCTPPlugins to ensure it gets properly set up after an explicit refresh call.

+      if (nsIPermissionManager::ALLOW_ACTION == permission) {

variable == constant for consistency's sake here, please.

I'd have more warm fuzzies about this patch if we also added mCTPPlugins to the list of arguments we pass to NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(), even though it probably isn't strictly needed at this point.

r- to take a look at all that, and unsurprisingly this has bit-rotted since it was created. And yes, I did deserve the public shaming for taking this long to get to very carefully review this patch :)

Comment 19

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

Attachment: c2p.patch

Comment 20

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

Attachment: c2p.patch

Comment 21

[Chris Peterson [:cpeterson]]

Does this approach handle the (rare) case where content queries `navigator.plugins["Shockwave Flash"]` directly instead of enumerating navigator.plugins, comparing plugin names === "Shockwave Flash"?

Comment 22

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

(In reply to Chris Peterson [:cpeterson] from comment #21)
> Does this approach handle the (rare) case where content queries
> `navigator.plugins["Shockwave Flash"]` directly instead of enumerating
> navigator.plugins, comparing plugin names === "Shockwave Flash"?

It should, it is taking flash out of the internal array of plugins and holding it in a separate array of CTP plugins

Comment 23

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

Attachment: c2p.patch

previous patch had a test failure on try, this fixes that by not trying to be fancy

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90de73e38028&selectedJob=21516120

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 8756968 [details] [diff] [review]
c2p.patch

- In browser/modules/PluginContent.jsm:

+  observe: function observe(aSubject, aTopic, aData) {
+   let pluginTag = aSubject;
+   if (aTopic == "Plugin::HiddenPluginTouched") {

1-space indentation? :)

- In dom/base/nsPluginArray.h:

   nsTArray<RefPtr<nsPluginElement> > mPlugins;
+  nsTArray<RefPtr<nsPluginElement> > mCTPPlugins;

Please add a comment here explaining why we have two arrays so the next person has a chance to understand how these are different.

r=jst

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7383d49bc7c8
remove plugins that are click-to-play from navigator.plugins (restricted to Flash) r=jst

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7383d49bc7c8

Comment 27

[Benjamin Smedberg]

Brad, did you have a testing plan set up for this yet? It seems like it could use some run through a bunch of topsites.

Comment 28

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> Brad, did you have a testing plan set up for this yet? It seems like it
> could use some run through a bunch of topsites.

I don't

Comment 29

[Chris Peterson [:cpeterson]]

Our testing options are:

1. Do nothing and hope Nightly users will report bugs. (More accurately, the 1% of users with click-to-play enabled.)
2. Encourage Nightly users to dogfood click-to-play.
3. Prepare a directed test plan for Softvision QA.

I suggest option #2. After I spot check some of the common Flash sites, I can announce the change on Twitter and some Mozilla mailing lists (fx-team and/or dev-platform?). I anticipate two types of problems:

1. Sites that require Flash are broken. This is expected and they ought to work when the page is reloaded.

2. Flash video sites will most likely be broken on Windows XP and N/KN editions. The sites won't be able to fall back to H.264 HTML5 video because we don't have H.264 decoders on those operating systems. VP9 is an option for them, but practically no video sites use VP9 besides YouTube. VP9 is also more CPU-intensive than Flash video (which can use H.264 hardware decoders on XP where we can't).

Comment 30

[Dão Gottwald [:dao]]

(In reply to Kohei Yoshino [:kohei] from comment #16)
> Posted the site compatibility doc:
> https://www.fxsitecompat.com/en-US/docs/2015/navigator-plugins-will-no-
> longer-contain-click-to-play-plugins/

That's incorrect, since this is only for Flash.

Comment 31

[Kohei Yoshino [:kohei]]

Updated the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/navigator-plugins-no-longer-lists-click-to-activate-plug-ins/

Comment 32

[Kohei Yoshino [:kohei]]

Ugh, sorry I missed :dao's comment. Will fix the doc shortly.

Comment 33

[Kohei Yoshino [:kohei]]

Revised: https://www.fxsitecompat.com/en-CA/docs/2016/navigator-plugins-no-longer-lists-flash-when-it-s-click-to-activate/

Comment 34

[Pascal Chevrel:pascalc]

(In reply to Chris Peterson [:cpeterson] from comment #29)
> 2. Encourage Nightly users to dogfood click-to-play.
> 
> I suggest option #2. After I spot check some of the common Flash sites, I
> can announce the change on Twitter and some Mozilla mailing lists (fx-team
> and/or dev-platform?).

The nightly-testers mailing list is also a good target I think :)
https://mail.mozilla.org/pipermail/nightly-testers/

We also have an IRC channel (#nightly) where you could post this information, keeping in mind that currently most of the people there are from the CET timezone.

If you can draft a message explaining how people should test the feature, I could broadcast it through our community (the @FirefoxNightly twitter account for example has >9000 followers). Another option would be to put a snippet on about:home asking our Nightly users directly to test the feature, I might be able to get such snippet running before MozLondon.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Those are some pretty major regressions (breaking Facebook video, Hulu)  already. So we already have uncovered issues that probably should block the release. Do we have some other option here?  Is the plan to find these regressions and try to deal with them per-site?

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

We can figure it out after the merge. But I hope before we release aurora by the end of next week.

Comment 37

[Trevor Rowbotham [:rowbot]]

Was it intentional that this bug also caused navigator.mimeTypes to stop reporting "application/x-shockwave-flash" as a valid content type or was this supposed to be strictly limited to navigator.plugins as the bug title indicates?

Comment 38

[Benjamin Smedberg]

The navigator.mimeTypes change was intentional.

I think we should probably back this out immediately for the regressions, since clearly something isn't right when we're not even triggering the location bar icon.

Comment 39

[Chris Peterson [:cpeterson]]

I backed this out for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6684c07cc6

Comment 40

[Chris Peterson [:cpeterson]]

This game wouldn't load with this bug's patch applied, even after clicking to unhide and play Flash:

http://www.flashgames247.com/play/17152.html

Comment 41

[Chris Peterson [:cpeterson]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #35)
> Those are some pretty major regressions (breaking Facebook video, Hulu) 
> already. So we already have uncovered issues that probably should block the
> release.

Liz, do you have bug numbers or STR for the Facebook and Hulu regressions? Any other broken sites?

Comment 42

[Trevor Rowbotham [:rowbot]]

(In reply to Chris Peterson [:cpeterson] from comment #41)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #35)
> > Those are some pretty major regressions (breaking Facebook video, Hulu) 
> > already. So we already have uncovered issues that probably should block the
> > release.
> 
> Liz, do you have bug numbers or STR for the Facebook and Hulu regressions?
> Any other broken sites?

Hulu is bug 1277760 and Facebook is bug 1277825.  bug 1277832 seems to suggest that CNN broke as well. Twitch.tv was also broken when I checked.

Comment 43

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

Attachment: QI_tag.patch

This fixes all the sites that I have tested (my bank, Hulu, CNN, Facebook, the japanese video site). I think something went sideways when I was fixing the test.

Comment 44

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

Attachment: iterator_check.patch

There is a particularly terrible case that the current code doesn't cover:

function hasFlash() {
  for (var i = 0; i < navigator.plugins.length; i++) {
    if (navigator.plugins[i].name == "Shockwave Flash") {
      return true;
    }
  }
  return false;
}

I haven't actually seen this pattern in the wild. But, just in case, here's a patch that would take care of it. I hope we don't need it.

Comment 45

[Trevor Rowbotham [:rowbot]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #44)
> Created attachment 8761026 [details] [diff] [review]
> iterator_check.patch
> 
> There is a particularly terrible case that the current code doesn't cover:
> 
> function hasFlash() {
>   for (var i = 0; i < navigator.plugins.length; i++) {
>     if (navigator.plugins[i].name == "Shockwave Flash") {
>       return true;
>     }
>   }
>   return false;
> }
> 
> I haven't actually seen this pattern in the wild. But, just in case, here's
> a patch that would take care of it. I hope we don't need it.

Bug 757726 tried to prevent this type of scenario, but after skimming through the comments, it looks like it was backed out for site breakage.  I assume this bug would cause the same breakage that Bug 757726 got backed out for.

Comment 46

[Chris Peterson [:cpeterson]]

Comment on attachment 8761026 [details] [diff] [review]
iterator_check.patch

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

::: dom/base/nsPluginArray.cpp
@@ +181,5 @@
>    if (!aFound) {
>      return nullptr;
>    }
>  
> +  // If the page is iterating by index, they may need anything in the CTP list

Should this CTP list check be inside the `if (!aFound) {}` branch above? IIUC, click-to-play plugins will be in the mCTPPlugins list and not in mPlugins. Thus the hidden plugin will never be found in mPlugins above, so we would always return nullptr early and never reach this mCTPPlugins loop.

@@ +187,5 @@
> +    nsAutoString pluginName;
> +    nsPluginElement* plugin = mCTPPlugins[i];
> +    plugin->GetName(pluginName);
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    obs->NotifyObservers(plugin->PluginTag(), "Plugin::HiddenPluginTouched", pluginName.get());

If the user has H hidden plugins and N non-hidden plugins, won't this code fire a total of H*N notifications? How does the front-end UI handle that stampede of redundant notifications?

Since this code is trying to handle the ugly `if (navigator.plugins[i].name == "Shockwave Flash")` for loop case, maybe we can limit the mCTPPlugins check to when aIndex == mPlugins.Length() - 1, i.e. after the naive code has enumerated through 0–N non-hidden plugins and never found the hidden plugin. When their loop hits the end of navigator.plugins[], only then do we need to check whether there are any hidden click-to-play plugins that the user might be looking for.

I think this code could misfire "Plugin::HiddenPluginTouched" events if the user has click-to-play Flash and the page's for loop is looking for a plugin that is not installed.

Comment 47

[Liz Henry (:lizzard) (relman/hg->git project)]

Just checking, it looks like this may have backed out from inbound from comment 39. id that backout hit m-c before the merge? If not, we still might need to backout from 49.    (Then this won't block aurora 49 updates)

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

That backout appears to be on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/ff6684c07cc6

Comment 49

[Chris Peterson [:cpeterson]]

Yes, the backout landed on central on Sunday before the Aurora merge: https://hg.mozilla.org/mozilla-central/rev/ff6684c07cc6

Comment 50

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

Comment on attachment 8756968 [details] [diff] [review]
c2p.patch

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

::: browser/modules/PluginContent.jsm
@@ +206,5 @@
>               blocklistState: blocklistState,
>             };
>    },
>  
> +  _getPluginInfoForTag: function (pluginTag, tagMimetype, fallbackType) {

I'm confused. When is this used?

Comment 51

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

Ah, I see - it's in the other patch.

Comment 52

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

Comment on attachment 8761025 [details] [diff] [review]
QI_tag.patch

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

This is probably okay, but I have some questions. I'll re-review once I get the answers.

::: browser/modules/PluginContent.jsm
@@ +763,5 @@
>  
>      for (let p of plugins) {
> +      let pluginInfo;
> +      if (p instanceof Ci.nsIPluginTag) {
> +        pluginInfo = this._getPluginInfoForTag(p, p.getMimeTypes()[0], null);

As far as I can tell, this is the only caller to _getPluginInfoForTag. Why do we need the third argument if it's going to be null every time?

Also, can we be certain for all plugins that there'll be at least one mime type? If no, we should probably get a count (using the optional argument to getMimeTypes), and ensure it's greater than 1 before taking the 0th mime type.

Comment 53

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

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #52)
> Comment on attachment 8761025 [details] [diff] [review]
> QI_tag.patch
> 
> Review of attachment 8761025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is probably okay, but I have some questions. I'll re-review once I get
> the answers.
> 
> ::: browser/modules/PluginContent.jsm
> @@ +763,5 @@
> >  
> >      for (let p of plugins) {
> > +      let pluginInfo;
> > +      if (p instanceof Ci.nsIPluginTag) {
> > +        pluginInfo = this._getPluginInfoForTag(p, p.getMimeTypes()[0], null);
> 
> As far as I can tell, this is the only caller to _getPluginInfoForTag. Why
> do we need the third argument if it's going to be null every time?
getPluginInfoForTag() is a copy of most of getPluginInfo(). Originally (as in the most recently obsoleted patch) I implemented getPluginInfo() by calling getPluginInfoForTag(). I still think doing some code clean up to eliminate the code duplication would be nice in the future.

That said, we can drop the 3rd arg.
 
> Also, can we be certain for all plugins that there'll be at least one mime
> type? If no, we should probably get a count (using the optional argument to
> getMimeTypes), and ensure it's greater than 1 before taking the 0th mime
> type.

Sure, its possible. We can check it.

Comment 54

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

Attachment: QI_tag.patch

Comment 55

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

Comment on attachment 8762168 [details] [diff] [review]
QI_tag.patch

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

::: browser/modules/PluginContent.jsm
@@ +763,5 @@
>  
>      for (let p of plugins) {
> +      let pluginInfo;
> +      if (p instanceof Ci.nsIPluginTag) {
> +        pluginInfo = this._getPluginInfoForTag(p, p.getMimeTypes() > 0 ? p.getMimeTypes()[0] : null);

I thought the way that getMimeTypes worked was:

```
let out = {};
p.getMimeTypes(out);
if (out.value > 0) {
}
```

As it stands, I'm pretty sure p.getMimeTypes will always coerce to truth-y here.

Comment 56

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

Attachment: QI_tag.patch

sorry, meant to have .length there. I should note that its hard to test the "don't have a mime type" case without compiling a test NPAPI plugin, and since this is restricted to Flash, that test plugin would need to claim to be Flash.

Comment 57

[Kohei Yoshino [:kohei]]

fyi https://webkit.org/blog/6589/next-steps-for-legacy-plug-ins/

Comment 58

[jeremy9856]

Is this a duplicate ?

https://bugzilla.mozilla.org/show_bug.cgi?id=1281943

Comment 60

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

Comment on attachment 8762215 [details] [diff] [review]
QI_tag.patch

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

This looks okay to me. One minor pre-land suggestion, and one general question for my own edification.

::: browser/modules/PluginContent.jsm
@@ +120,5 @@
>      }
>    },
>  
>    observe: function observe(aSubject, aTopic, aData) {
> +    let pluginTag = aSubject.QueryInterface(Ci.nsIPluginTag);

Is there a chance this could ever throw NS_NOINTERFACE? I ask, because further down, we iterate the plugins from the DOMWindowUtils on the window, and seem to handle a case where the plugin does not implement the nsIPluginTag interface... what is that case?

@@ +763,5 @@
>  
>      for (let p of plugins) {
> +      let pluginInfo;
> +      if (p instanceof Ci.nsIPluginTag) {
> +        pluginInfo = this._getPluginInfoForTag(p, p.getMimeTypes().length > 0 ? p.getMimeTypes()[0] : null);

I think I'd prefer if we could extract the logic out, like:

if (p instanceof Ci.nsIPluginTag) {
  let mimeType = p.getMimeTypes().length() ? p.getMimeTypes()[0] : null;
  pluginInfo = this._getPluginInfoForTag(p, mimeType);
} else {
...
}

Comment 61

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

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #60)
> Comment on attachment 8762215 [details] [diff] [review]
> QI_tag.patch
> 
> Review of attachment 8762215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks okay to me. One minor pre-land suggestion, and one general
> question for my own edification.
> 
> ::: browser/modules/PluginContent.jsm
> @@ +120,5 @@
> >      }
> >    },
> >  
> >    observe: function observe(aSubject, aTopic, aData) {
> > +    let pluginTag = aSubject.QueryInterface(Ci.nsIPluginTag);
> 
> Is there a chance this could ever throw NS_NOINTERFACE? I ask, because
> further down, we iterate the plugins from the DOMWindowUtils on the window,
> and seem to handle a case where the plugin does not implement the
> nsIPluginTag interface... what is that case?
It shouldn't be possible. This notification is sent from nsPluginArray, and what it is sending is strongly typed as an nsPluginTag.
> 
> @@ +763,5 @@
> >  
> >      for (let p of plugins) {
> > +      let pluginInfo;
> > +      if (p instanceof Ci.nsIPluginTag) {
> > +        pluginInfo = this._getPluginInfoForTag(p, p.getMimeTypes().length > 0 ? p.getMimeTypes()[0] : null);
> 
> I think I'd prefer if we could extract the logic out, like:
> 
> if (p instanceof Ci.nsIPluginTag) {
>   let mimeType = p.getMimeTypes().length() ? p.getMimeTypes()[0] : null;
>   pluginInfo = this._getPluginInfoForTag(p, mimeType);
> } else {
> ...
> }
sure

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/776f0bb21587
remove plugins that are click-to-play from navigator.plugins (restricted to Flash) r=jst
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4557a5485f
Follow up, need to QI the plugin tag r=mconley

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/776f0bb21587
https://hg.mozilla.org/mozilla-central/rev/aa4557a5485f

Comment 64

[Liz Henry (:lizzard) (relman/hg->git project)]

I think this also need to be backed out of aurora since 49 is affected (and is also in the bugs that refer to this one). Can you request uplift ? Thanks.

Comment 65

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #64)
> I think this also need to be backed out of aurora since 49 is affected (and
> is also in the bugs that refer to this one). Can you request uplift ? Thanks.

why are you asking for uplift?

Comment 66

[Kohei Yoshino [:kohei]]

Updated the site compat doc: https://www.fxsitecompat.com/en-CA/docs/2016/navigator-plugins-and-navigator-mimetypes-no-longer-list-flash-when-it-s-click-to-activate/

I guess the flags are just confusing.

Comment 67

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Brad and Kohei, I appreciate the explanation in the post. It was a bit confusing and it sounds like I got the desired state wrong (I thought we were going to try again to get this change into 49)

It sounds like this change in 50 may fix the behavior on some sites to be what we intend and will help make the HTML5 video fallback visible to more users. Is there anything we can ask QA to do to support testing of the cases that made us back this out of 49? Or expand the coverage of what we do test?

Comment 68

[Chris Peterson [:cpeterson]]

We probably should have put this feature behind a pref so it would be easier to disable if we find more problems.

Comment 69

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

I can add a pref. I see two options for that, one is a simple boolean. The other option would be a list of plugins that this applies to (which would only be flash for now). Any opinions between those options?

Comment 70

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #69)
> I can add a pref. I see two options for that, one is a simple boolean. The
> other option would be a list of plugins that this applies to (which would
> only be flash for now). Any opinions between those options?

The boolean pref sounds easiest. However, you might want to postpone writing that patch until we know whether actually need it. Users can workaround any problems by making Flash not click-to-play.

Comment 71

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Fixing "Target Milestone:", as it landed on Firefox 50 after reland.

(In reply to Chris Peterson [:cpeterson] from comment #70)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #69)
> > I can add a pref. I see two options for that, one is a simple boolean. The
> > other option would be a list of plugins that this applies to (which would
> > only be flash for now). Any opinions between those options?
> 
> Users can workaround any problems by making Flash not click-to-play.

Unfortunately this bug breaks Flash click-to-play feature + Click to Play per-element extension ( https://addons.mozilla.org/addon/click-to-play-per-element/ ). Is it intended? If so, we need "addon-compat" keyword too, also "relnote-firefox" flag will be nice.

Comment 72

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

I backed this out on inbound for causing bug 1284203, which seemed annoying enough to warrant it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b295ee49d52ebb1b0e20bcf4d111b6b6294a85ca
Backed out changeset aa4557a5485f for causing bug 1284203.

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c4e1bae9775e8765d1180ff4795b3a61b0f699
Backed out changeset 776f0bb21587 for causing bug 1284203.

Comment 73

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

Attachment: check_same_doc.patch

In retrospect, this was a pretty silly oversight. Need to make sure that the notification is coming from the document that we're showing the dialog for.

To do this I need to pass the window though. Ideally, I'd just pass the nsPluginElement object, but needed members aren't scriptable, so I've added a container object with a window and a tag. Its a little ugly, so if there any other ideas here, I'm all ears.

Finally, I changed it to show the notification box where previously it only showed the plugin icon in the location bar. This change is prompted by the comments on bug 1284445.

Comment 74

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

Comment on attachment 8769188 [details] [diff] [review]
check_same_doc.patch

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

I wonder if the new interface / class is really necessary... for other things, we tend to bubble up chrome-only events, which PluginContent listens for and reacts to - that's how we know when some plugin activity belongs to a particular window. Could we not do something similar here, and dispatch a custom HiddenPluginTouched chrome-only event that PluginContent could listen for, and do its thing?

Comment 75

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

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #74)
> Comment on attachment 8769188 [details] [diff] [review]
> check_same_doc.patch
> 
> Review of attachment 8769188 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if the new interface / class is really necessary... for other
> things, we tend to bubble up chrome-only events, which PluginContent listens
> for and reacts to - that's how we know when some plugin activity belongs to
> a particular window. Could we not do something similar here, and dispatch a
> custom HiddenPluginTouched chrome-only event that PluginContent could listen
> for, and do its thing?

As far as I know, there's no way to send a generic nsISupports through with an event (without writting a one off custom event with an idl, which seems just as ugly). The problem is that we need to send and nsIPluginTag though (or an nsPluginElement, but that gets even more in the weeds in being a webidl binding and not an idl binding/nsISupports).

So, what you're suggesting would work if we were sending an event about an element in the page, but not for these nsIPluginTags.

Comment 76

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #75)
> (In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #74)
> > Comment on attachment 8769188 [details] [diff] [review]
> > check_same_doc.patch
> > 
> > Review of attachment 8769188 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I wonder if the new interface / class is really necessary... for other
> > things, we tend to bubble up chrome-only events, which PluginContent listens
> > for and reacts to - that's how we know when some plugin activity belongs to
> > a particular window. Could we not do something similar here, and dispatch a
> > custom HiddenPluginTouched chrome-only event that PluginContent could listen
> > for, and do its thing?
> 
> As far as I know, there's no way to send a generic nsISupports through with
> an event (without writting a one off custom event with an idl, which seems
> just as ugly). The problem is that we need to send and nsIPluginTag though
> (or an nsPluginElement, but that gets even more in the weeds in being a
> webidl binding and not an idl binding/nsISupports).
> 
> So, what you're suggesting would work if we were sending an event about an
> element in the page, but not for these nsIPluginTags.

Could we send the mimetype through the event instead? That way, we could use nsIPluginHost's getPluginTagForType to retrieve the tag.

If that's not going to work, we can probably take the observer approach - but I do want to make sure that we can't re-use the same mechanism we've already got in PluginContent before adding a new one.

Comment 77

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

This notification is coming from nsPluginHost to tell us when someone requested a tag for a type we're hiding. So, no that won't work.

Comment 78

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

Comment on attachment 8769188 [details] [diff] [review]
check_same_doc.patch

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

This looks okay, but I'm worried it's not going to show notifications if the plugin iteration occurs in a subframe. Can you please test that? If that needs addressing, I suppose we can do that in a follow-up.

On that note, all of this should have automated tests written for it. Happy to review that in a follow-up bug as well.

::: browser/modules/PluginContent.jsm
@@ +124,5 @@
>      if (aTopic == "Plugin::HiddenPluginTouched") {
> +      let tagAndWindow = aSubject.QueryInterface(Ci.nsITagAndWindow);
> +      let pluginTag = tagAndWindow.tag.QueryInterface(Ci.nsIPluginTag);
> +      let window = tagAndWindow.window.QueryInterface(Ci.mozIDOMWindow);
> +      if (window.document != this.content.document) {

What about subframes?

::: dom/base/nsPluginArray.cpp
@@ +259,5 @@
>      nsPluginElement* hiddenPlugin = FindPlugin(mCTPPlugins, aName);
>      if (hiddenPlugin) {
>        nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +      nsCOMPtr<nsITagAndWindow> tagAndWindow = new nsTagAndWindow(hiddenPlugin->PluginTag(), hiddenPlugin->GetParentObject());
> +      obs->NotifyObservers(tagAndWindow, "Plugin::HiddenPluginTouched", nsString(aName).get());

Is the data value even being used anymore? If not, probably best to remove.

::: dom/plugins/base/nsIPluginTag.idl
@@ +76,5 @@
>    readonly attribute nsIURI handlerURI;
>  };
> +
> +[scriptable, uuid(32fd6acf-55fa-422c-8951-d8d51114c134)]
> +interface nsITagAndWindow : nsISupports {

Aren't these supposed to be mozI prefixed now? I guess I don't care too much, but if there's a strict convention that new interfaces start with mozI, we should probably adhere to it.

Comment 79

[doomtay]

To be honest, I'm with :bsmedberg on going against the idea of basically making the site think Flash isn't installed.

If I were to disable the Flash plugin, Amazon, LEGO.com, and The Onion would resort to HTML5. However, Rooster Teeth will say "No playable sources found" and http://www.portalgraphics.net/pg/ will ask to download Flash

Comment 80

[Chris Peterson [:cpeterson]]

(In reply to bluejedi from comment #79)
> If I were to disable the Flash plugin, Amazon, LEGO.com, and The Onion would
> resort to HTML5. However, Rooster Teeth will say "No playable sources found"
> and http://www.portalgraphics.net/pg/ will ask to download Flash

At that point, Firefox will ask the user if they would like to allow Flash on portalgraphics.net. If the user opts to allow Flash, the game will then work.

Chrome plans to make this same change in Q4 2016:

https://docs.google.com/presentation/d/106_KLNJfwb9L-1hVVa4i29aw1YXUy9qFX-Ye4kvJj-4/
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/0wWoRRhTA_E/__E3jf40OAAJ

Comment 81

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

Attachment: use_event.patch

Need a DOM peer's review since this is adding a webidl interface

Comment 82

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8772414 [details] [diff] [review]
use_event.patch

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

::: browser/modules/PluginContent.jsm
@@ +45,5 @@
>      global.addEventListener("PluginRemoved",         this, true);
>      global.addEventListener("pagehide",              this, true);
>      global.addEventListener("pageshow",              this, true);
>      global.addEventListener("unload",                this);
> +    global.addEventListener("HiddenPlugin",          this, true);  

Uber-nit (passing by): Splinter points out some trailing whitespace both here and for the removeEventListener call below.

::: dom/base/nsPluginArray.cpp
@@ +237,5 @@
>      nsPluginElement* hiddenPlugin = FindPlugin(mCTPPlugins, aName);
>      if (hiddenPlugin) {
> +      HiddenPluginEventInit init;
> +      init.mTag = hiddenPlugin->PluginTag();
> +      RefPtr<nsIDocument> doc = hiddenPlugin->GetParentObject()->GetDoc();

nsCOMPtr for interfaces.

@@ +244,5 @@
> +      event->SetTarget(doc);
> +      event->SetTrusted(true);
> +      event->WidgetEventPtr()->mFlags.mOnlyChromeDispatch = true;
> +      
> +      nsCOMPtr<EventTarget> target(doc);

nsIDocument (via nsINode) inherits directly from EventTarget. I'd nuke this local entirely and call DispatchEvent directly on doc.

Comment 83

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

Attachment: patch for checkin

patch for checkin, and a try push which is colorful, but the color appears to be intermittents 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90c04deeabd3

Comment 84

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d15da870e8
remove plugins that are click-to-play from navigator.plugins (restricted to Flash). r=jst,mconley,mrbkap

Comment 85

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21d15da870e8

Comment 86

[IU]

Is the intention of this bug to break click-to-play for flash?  Every time it lands, click-to-play stops working for flash.

Comment 87

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Virtual_ManPL [:Virtual] - (ni? me) from comment #71)
> Unfortunately this bug breaks Flash click-to-play feature + Click to Play
> per-element extension (
> https://addons.mozilla.org/addon/click-to-play-per-element/ ). Is it
> intended? If so, we need "addon-compat" keyword too, also "relnote-firefox"
> flag will be nice.
(In reply to IU from comment #86)
> Is the intention of this bug to break click-to-play for flash?  Every time
> it lands, click-to-play stops working for flash.

FYI - I reported this issue in #1294344 per no reply here.

Comment 89

[Eric Shepherd [:sheppy]]

I see this is behind a pref now and disabled by default. I’m going to forgo documenting it for now, then, unless there’s a good reason to include it. Please be sure to put dev-doc-needed on the bug that enables it by default.

Comment 90

[Kohei Yoshino [:kohei]]

Updating the flags as per Bug 1296004.

Comment 91

[Robert Kaiser]

I think this change is what made "Ask to Activate" basically unusable for me because it affects a game I play (one of the two sites with Flash that I still use). Despite being set to "Allow and Remember", that site always shows me information to install Flash (it's the game http://startrek.gamesamba.com/ and I guess it needs an account to get to actually see this, which I experience when trying to enter a server).

Comment 92

[Robert Kaiser]

(In reply to Eric Shepherd [:sheppy] from comment #89)
> I see this is behind a pref now and disabled by default.

OK, I see it's off even on Nightly and plugins.navigator_hide_disabled_flash in either direction doesn't solve my issue. Need to find another bug to watch in addition, then.

Comment 93

[Robert Kaiser]

Emptying plugins.navigator.hidden_ctp_plugin works, so it's bug 1294341. I expected to find that in dependencies here somewhere but didn't.