967979 - Provide a pref to prevent "This Plugin is Disabled" barrier (Tor 8312)

Status: RESOLVED INVALID

(Firefox :: General, defect, P3)

Description

[Mike Perry]

Attachment: Draft patch to remove barrier (needs pref)

The "This Plugin is Disabled" barrier interferes with the display of HTML5 videos that first attempt to load flash. This happens (or used to happen) on YouTube videos that have both HTML5 and flash versions. YouTube would try the flash version first, trigger the barrier, and then play the HTML5 video behind the barrier.

In Tor Browser, we simply removed the barrier, but ideally we could just set a pref to prevent it from displaying.

This bug is relayed to Bug 921730, but is not the same. We don't want users to have to close the barrier, as we don't want to encourage them to enable Flash, especially if there is an HTML5 video available.

Comment 1

[Justin Dolske [:Dolske]]

There are two somewhat-separate issues here:

One is discouraging users from enabling plugins. The minimal fix for that would probably be to just have a way to suppress the "Manage plugins..." link in the overlay we show for disabled plugins. Users could still manually go to the Addon Manager and enable the plugin, but that's at least a significant speedbump. Maybe you'd want some kind of message / warning in the Addon Manager too.

The other is hiding this overlay entirely. I'm a bit wary of doing that, because it's intended to serve as a way to notify the user that something in the page is non-functional / broken due the way their browser is configured -- especially if the page has no alternate / HTML5 fallback. EG, instead of getting a big mysterious blank rectangle where their game/video/whatever should be, it's pretty obvious what's going on.

That said, this isn't working so ideally already for Flash, from a quick skim of some sites:

 * YouTube is acting oddly for me. Once in a while I'll get the HTML5 version, and though our "Plugin disabled" overlay appears briefly, the HTM5 version correct plays on top. But for other videos I see our overlay briefly and then it's replaced by YouTube's usual "Flash required" error + link to install (!) Flash. Click-to-play mode Flash is OK.

* Vimeo never shows our overlay, and you just get a generic "This video can’t be played with your current setup." message from the site.

* Kongregate.com games just give an error page saying I need JS and a current Flash player. Our overlay never appears.

* Farmville (Facebook) just shows a "Loading Game... If your game does not load within 10 seconds, you may need to upgrade your version of Flash. Please do so by clicking here" with a link to install Flash.

I have much better results with Java, many of the top pages from a Google of "java demo" do show our UI.

Comment 2

[Benjamin Smedberg]

There are two variants of this: plugins with fallback content and plugins without fallback content.

For plugins with fallback content, I think we should never show the "disabled plugin" overlay: we should always show the fallback content.

For plugins without fallback content, we typically need some visible indication of something broken, just like broken images. I'd be interested to know if we could make it less visibly "broken", maybe even by using the broken-image UI instead of the grey plugin UI.

Comment 3

[Justin Dolske [:Dolske]]

Didn't shorlander do some design work for a more subtle plugin-problem UI?

Comment 4

[Stephen Horlander [:shorlander]]

Attachment: Click-to-Play Subtle Placeholder

(In reply to Justin Dolske [:Dolske] from comment #3)
> Didn't shorlander do some design work for a more subtle plugin-problem UI?

I did a bunch of exploration around making the UI more subtle. It didn't cover this particular case, though it could be a good starting point.

Comment 5

[Justin Dolske [:Dolske]]

Yeah, that's the mock I was thinking of.

Comment 6

[Jesse Ruderman]

Mike, I don't think you need a new pref. Setting Flash to "never activate" instead of "ask to activate" should do what you want, which is to indicate to web sites that you don't have Flash installed when they ask.

Comment 7

[Justin Dolske [:Dolske]]

"Never activate" would accomplish that, but it also means (at least with UI that exists today) that the user has no opportunity to activate Flash as needed. Never means never!

Comment 8

[nyuszika7h]

In Nightly 40.0a1 (2015-04-24) on Windows 8.1, this issue manifests itself in a weird way:

- If e10s is ENABLED, the Flash video loads without problem.
- If e10s is DISABLED, I get this message. Setting Flash to "Never activate" loads the HTML5 video.

(The site in question is animerush.tv. I'm not providing a link here because it's not exactly legal.)

Comment 9

[Arthur Edelstein [:arthur]]

Comment on attachment 8370491 [details] [diff] [review]
Draft patch to remove barrier (needs pref)

Here is a link that tracks the latest version of the Tor Browser patch:
https://torpat.ch/8312

Comment 10

[Dave Huseby [:huseby]]

Attachment: Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

Review commit: https://reviewboard.mozilla.org/r/72762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72762/

Comment 11

[Dave Huseby [:huseby]]

Attachment: Bug 967979 - Provide a pref to prevent 'This Plugin is Disabled' barrier,

Review commit: https://reviewboard.mozilla.org/r/72764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72764/

Comment 12

[Dave Huseby [:huseby]]

I uplifted this while waiting for a clobber build to finish.

Comment 13

[Benjamin Smedberg]

Felipe, this is related to the work in bug 1277346, though this is for torbrowser specifically. NI so you're aware.

Felipe should probably be reviewer here, if that's ok.

Comment 14

[Arthur Edelstein [:arthur]]

Comment on attachment 8782704 [details]
Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

Might want to credit Mike Perry somewhere here. Otherwise looks good.

Comment 15

[Arthur Edelstein [:arthur]]

Comment on attachment 8782705 [details]
Bug 967979 - Provide a pref to prevent 'This Plugin is Disabled' barrier,

I think the logic might be flipped here. Seems to me it should be

> if (!Services.prefs.getBoolPref("privacy.plugin_disabled_barrier.enabled")) {

Comment 16

[Dave Huseby [:huseby]]

You're right, let me correct that and add a note that the patch is from Mike Perry in the commit message.  Thanks for the feedback.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8782705 [details]
Bug 967979 - Provide a pref to prevent 'This Plugin is Disabled' barrier,

https://reviewboard.mozilla.org/r/72764/#review71778

r+ with the logic flipped as mentioned

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8782704 [details]
Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

https://reviewboard.mozilla.org/r/72762/#review71782

"overlay" is more commonly used in our terminology than "barrier". Could you rename the pref to make it easier to understand?

Comment 19

[Dave Huseby [:huseby]]

Comment on attachment 8782704 [details]
Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72762/diff/1-2/

Comment 20

[Dave Huseby [:huseby]]

Comment on attachment 8782705 [details]
Bug 967979 - Provide a pref to prevent 'This Plugin is Disabled' barrier,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72764/diff/1-2/

Comment 21

[Dave Huseby [:huseby]]

Comment on attachment 8782704 [details]
Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72762/diff/2-3/

Comment 22

[Dave Huseby [:huseby]]

Comment on attachment 8782705 [details]
Bug 967979 - Provide a pref to prevent 'This Plugin is Disabled' barrier,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72764/diff/2-3/

Comment 23

[Dave Huseby [:huseby]]

Comment on attachment 8782704 [details]
Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

https://reviewboard.mozilla.org/r/72760/#review73324

Comment 24

[Dave Huseby [:huseby]]

Comment on attachment 8782704 [details]
Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72762/diff/2-3/

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 8782704 [details]
Bug 967979 - adds privacy.plugin_disabled_barrier.enabled pref

https://reviewboard.mozilla.org/r/72762/#review74254

I think this is just the thing felipe reviewed with the two suggested changes, but annoyingly mozreview is making that entirely non-obvious.

Comment 26

[Dave Huseby [:huseby]]

try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7447a537478

Comment 27

[Dave Huseby [:huseby]]

Attachment: Bug_967979.patch r+ dolske

r+ dolske doing it this way because review board won't let me land the patch.

Comment 28

[Justin Dolske [:Dolske]]

(In reply to Dave Huseby [:huseby] from comment #27)
> Created attachment 8789101 [details] [diff] [review]
> Bug_967979.patch r+ dolske
> 
> r+ dolske doing it this way because review board won't let me land the patch.

Err, this isn't the patch I reviewed -- why are you now removing &managePlugins; from pluginProblem.xml?

Comment 29

[Dave Huseby [:huseby]]

Attachment: Bug_967979.patch r+ dolske

This is the correct patch.

Comment 30

[Dave Huseby [:huseby]]

Justin,

Yes you're right, I've corrected the mistake.

Comment 31

[Dave Huseby [:huseby]]

Hrm, this patch seems to trigger a *lot* of plugin mochitest timeouts.  Might be related to e10s.

With both the pref added and the code change in browser/modules/PluginContent.jsm:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7447a537478

With just the code change in browser/modules/PluginContent.jsm:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd04a6690e5

With the patch not applied (control):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86f74a4426de

With the patch removed completely, we get a pretty clean try run, as soon as I add the code, plugin tests start timing out.  Investigating.

Comment 32

[Mats Palmgren (inactive)]

We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.