Closed Bug 1186948 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- disabled
firefox51 --- disabled
firefox52 --- disabled

People

(Reporter: blassey, Assigned: blassey)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Keywords: qawanted, site-compat)

Attachments

(5 files, 10 obsolete files)

16.34 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.12 KB, patch
Details | Diff | Splinter Review
3.06 KB, patch
mconley
: review+
Details | Diff | Splinter Review
8.10 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
20.29 KB, patch
Details | Diff | Splinter Review
No description provided.
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).
Depends on: 757726, 934107
Attached patch c2p_nav_plug.patch (obsolete) — Splinter Review
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.
Assignee: nobody → blassey.bugs
Attachment #8638354 - Flags: feedback?(dougt)
Attachment #8638354 - Flags: feedback?(cpeterson)
Attachment #8638354 - Flags: feedback?(bugs)
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/.
Attachment #8638354 - Flags: feedback?(cpeterson) → feedback+
(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.
Attached patch c2p_nav_plug.patch (obsolete) — Splinter Review
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.
Attachment #8638354 - Attachment is obsolete: true
Attachment #8638354 - Flags: feedback?(dougt)
Attachment #8638354 - Flags: feedback?(bugs)
Attachment #8638596 - Flags: review?(jst)
Attachment #8638596 - Flags: feedback?(dougt)
Attachment #8638596 - Flags: feedback?(bugs)
Attached patch c2p_nav_plug.patch (obsolete) — Splinter Review
needed to rev the uuid of nsIPluginHost
Attachment #8638596 - Attachment is obsolete: true
Attachment #8638596 - Flags: review?(jst)
Attachment #8638596 - Flags: feedback?(dougt)
Attachment #8638596 - Flags: feedback?(bugs)
Attachment #8638602 - Flags: review?(jst)
Attachment #8638602 - Flags: feedback?(dougt)
Attachment #8638602 - Flags: feedback?(bugs)
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.
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.
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.
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.
Why are we messing with this at all? What's the value we're trying to provide?
(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.
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.
awesome..  will mark the current patch as unreviewed.
Attachment #8638602 - Flags: review?(jst)
Attachment #8638602 - Flags: feedback?(dougt)
Attachment #8638602 - Flags: feedback?(bugs)
Attached patch c2p_nav_plug.patch (obsolete) — Splinter Review
Attachment #8638602 - Attachment is obsolete: true
Attachment #8642693 - Flags: review?(jst)
Attachment #8703125 - Attachment is obsolete: true
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 :)
Attachment #8642693 - Flags: review?(jst) → review-
Attached patch c2p.patch (obsolete) — Splinter Review
Attachment #8642693 - Attachment is obsolete: true
Attached patch c2p.patch (obsolete) — Splinter Review
Attachment #8756666 - Attachment is obsolete: true
Attachment #8756668 - Flags: review?(jst)
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"?
(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
Attached patch c2p.patchSplinter Review
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
Attachment #8756668 - Attachment is obsolete: true
Attachment #8756668 - Flags: review?(jst)
Attachment #8756968 - Flags: review?(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
Attachment #8756968 - Flags: review?(jst) → review+
Pushed by blassey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7383d49bc7c8
remove plugins that are click-to-play from navigator.plugins (restricted to Flash) r=jst
https://hg.mozilla.org/mozilla-central/rev/7383d49bc7c8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Flags: needinfo?(blassey.bugs)
Keywords: qawanted
(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
Flags: needinfo?(blassey.bugs)
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).
(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.
Summary: remove plugins that are click-to-play from navigator.plugins → remove Flash from navigator.plugins when it's click-to-play
Ugh, sorry I missed :dao's comment. Will fix the doc shortly.
Depends on: 1277760
Depends on: 1277825
Depends on: 1277832
(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.
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?
Flags: needinfo?(cpeterson)
Flags: needinfo?(blassey.bugs)
We can figure it out after the merge. But I hope before we release aurora by the end of next week.
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?
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.
I backed this out for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6684c07cc6
Status: RESOLVED → REOPENED
Flags: needinfo?(cpeterson)
Resolution: FIXED → ---
Flags: needinfo?(blassey.bugs)
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
(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?
Flags: needinfo?(lhenry)
(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.
Attached patch QI_tag.patch (obsolete) — Splinter Review
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.
Attachment #8761025 - Flags: review?(mconley)
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.
(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 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.
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)
Flags: needinfo?(wkocher)
Flags: needinfo?(lhenry)
Flags: needinfo?(cpeterson)
That backout appears to be on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/ff6684c07cc6
Flags: needinfo?(wkocher)
Yes, the backout landed on central on Sunday before the Aurora merge: https://hg.mozilla.org/mozilla-central/rev/ff6684c07cc6
Flags: needinfo?(cpeterson)
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?
Ah, I see - it's in the other patch.
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.
Attachment #8761025 - Flags: review?(mconley)
(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.
Attached patch QI_tag.patch (obsolete) — Splinter Review
Attachment #8761025 - Attachment is obsolete: true
Attachment #8762168 - Flags: review?(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.
Attachment #8762168 - Flags: review?(mconley) → review-
Attached patch QI_tag.patchSplinter Review
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.
Attachment #8762215 - Flags: review?(mconley)
Attachment #8762168 - Attachment is obsolete: true
Duplicate of this bug: 1281943
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 {
...
}
Attachment #8762215 - Flags: review?(mconley) → review+
(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
Pushed by blassey@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/776f0bb21587
https://hg.mozilla.org/mozilla-central/rev/aa4557a5485f
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(blassey.bugs)
(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?
Flags: needinfo?(blassey.bugs)
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?
Flags: needinfo?(blassey.bugs)
We probably should have put this feature behind a pref so it would be easier to disable if we find more problems.
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?
Flags: needinfo?(blassey.bugs)
(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.
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.
Target Milestone: mozilla49 → mozilla50
Depends on: 1284203
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla50 → ---
Depends on: 1284445
Attached patch check_same_doc.patch (obsolete) — Splinter Review
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.
Attachment #8769188 - Flags: review?(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?
Flags: needinfo?(blassey.bugs)
(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.
Flags: needinfo?(blassey.bugs)
(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.
Flags: needinfo?(blassey.bugs)
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.
Flags: needinfo?(blassey.bugs)
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.
Attachment #8769188 - Flags: review?(mconley) → review+
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
(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
Attached patch use_event.patchSplinter Review
Need a DOM peer's review since this is adding a webidl interface
Attachment #8769188 - Attachment is obsolete: true
Attachment #8772414 - Flags: review?(mrbkap)
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.
Attachment #8772414 - Flags: review?(mrbkap) → review+
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
Pushed by ihsiao@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21d15da870e8
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1289496
Is the intention of this bug to break click-to-play for flash?  Every time it lands, click-to-play stops working for flash.
Depends on: 1289670
Depends on: 1290474
Depends on: 1292644
Depends on: 1292317
No longer depends on: 1292644
Depends on: 1292410
(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.
Depends on: 1295984
Depends on: 1296004
Depends on: 1292644
Depends on: 1298083
Duplicate of this bug: 1235161
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.
Keywords: dev-doc-needed
Depends on: 1312091
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).
(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.
Emptying plugins.navigator.hidden_ctp_plugin works, so it's bug 1294341. I expected to find that in dependencies here somewhere but didn't.
Duplicate of this bug: 1244140
Depends on: 1329725
Duplicate of this bug: 1319105
You need to log in before you can comment on or make changes to this bug.