1294341 - Ask to Activate doesn't work on http://converticon.com/ after landing patches from bug #1186948

Status: VERIFIED FIXED

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

Description

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

[Tracking Requested - why for this release]: Regression

STR:
1. Make sure that Flash plugin is set to "Ask to Activate"
2. Open this webpage http://converticon.com/
3. and see that you get this information
"This content requires the Adobe Flash Player. Get Flash"
instead of the real webpage, no doorhanger or icon notifications present

Regression window Pushlog (mozilla-inbound-win32):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=80c7885e432cec558c128379d2fb86d9935fb0ad&tochange=aa4557a5485fd8266f3f529d979215e296afac97

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: 1294341.png

I got a block icon. It asked to activate when I clicked it.

Comment 2

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

Attachment: issue.png

In my case I got nothing there (even in Safe Mode or with new profile) on Mozilla Firefox 64bit on Windows 7 64bit.

Comment 3

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

Here is the flash detection code:

	if (navigator.plugins != null && navigator.plugins.length > 0) {
		if (navigator.plugins["Shockwave Flash 2.0"] || navigator.plugins["Shockwave Flash"]) {

The issue here is that if the only plugin you have installed is Flash and Flash is click to play, navigator.plugins.length will now be 0 so we never get to the navigator.plugins["Shockwave Flash"] check which will cause the plugin activation UI to show. 

To test this theory, I enabled acrobat reader and with that enabled I get the plugin activation UI (because the length of navigator.plugins is now 1). Perhaps the right thing to do here is to show the UI if navigator.length is queried. Another option would be to report the length of plugins and hidden plugins, though that might get dicier.

Comment 4

[Johnny Stenback (:jst)]

Include a bogus plugin in navigator.plugins in this case? :)

Comment 5

[Andrew Overholt [:overholt]]

qDot, what do you think?

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I think showing the UI if length is queried might be a good idea. We're slowly filtering down to just flash anyways. The bogus plugin would work but seems a bit hacky. :)

Comment 7

[Andrew Overholt [:overholt]]

I hate to ask but can you do that work, Kyle?

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

:|

Comment 9

[Benjamin Smedberg]

I don't think we should do this (yet, anyway). We are likely going to be trading false negatives for false positives, and we don't have any data/evidence for why this would improve things instead of making things worse. We're going to turn off the navigator.plugins change in bug 1296004. If we do decide to do this, we need a bunch of data, and ddurst's team will be working on this as part of the Test Pilot experiment.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

'k. Feel free to reassign to me if it does become a thing.

Comment 11

[Andrew Overholt [:overholt]]

Based on comment 9 I'm setting this to unaffected for 50.

Comment 12

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

(In reply to Benjamin Smedberg [:bsmedberg] from comment #9)
> I don't think we should do this (yet, anyway). We are likely going to be
> trading false negatives for false positives, and we don't have any
> data/evidence for why this would improve things instead of making things
> worse. We're going to turn off the navigator.plugins change in bug 1296004.
> If we do decide to do this, we need a bunch of data, and ddurst's team will
> be working on this as part of the Test Pilot experiment.

We can always WONTFIX this, as it's kinda "broken" webpage coding per using nasty hacks, so it's mainly Tech Evangelism issue, not a very big deal for sure.

Comment 13

[Benjamin Smedberg]

Just because a website is poorly written doesn't mean we can ignore this. If users used to be able to use this site, and then they can't, that's a problem in Firefox that we usually need to address.

Comment 14

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

The same could be said about Firefox extensions (like in bug #1294344) ;)

Comment 15

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

Fixing flags per bug #1296004 Comment 4

Comment 16

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

Attachment: dummy_plugin.patch

Comment 17

[Benjamin Smedberg]

Comment on attachment 8784199 [details] [diff] [review]
dummy_plugin.patch

>diff --git a/dom/base/nsPluginArray.cpp b/dom/base/nsPluginArray.cpp

>+  if (mPlugins.Length() == 0 && mCTPPlugins.Length() != 0) {
>+    const char* emptyArray[1] = {""};

Reading the nsPluginTag constructor, I don't think we need this at all (you should be able to pass nullptr).

But if we do need the array, shouldn't it { nullptr } and not { "" }?
Also, static char const *const emptyArray[1] so that it gets put in rodata.

Need tests here to make sure this doesn't break accidentally.

Comment 18

[Marcia Knous [:marcia]]

Not tracking this for 50/51 based on status being disabled in both cases.

Comment 19

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

Attachment: dummy_plugin.patch

Comment 20

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

Attachment: pref_nightly.patch

patch for reference, I'll need to update it to tip

Comment 21

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

Attachment: dummy_plugin_test.patch

Comment 22

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

Attachment: pref_for_hidden_plugin.patch

Comment 23

[Benjamin Smedberg]

Comment on attachment 8785478 [details] [diff] [review]
dummy_plugin_test.patch

I believe that the synchronous pref setting here will fail randomly with e10s, because setting a pref requires a round trip to the chrome process and isn't always immediately visible: I recommend using SpecialPowers.pushPrefEnv({set: ["plugins.navigator.hidden_ctp_plugin", testPluginName]}, callback)

see http://searchfox.org/mozilla-central/source/dom/html/test/test_object_plugin_nav.html#93 for an example

Comment 24

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

Attachment: dummy_plugin_test.patch

Comment 25

[Benjamin Smedberg]

Comment on attachment 8787404 [details] [diff] [review]
dummy_plugin_test.patch

In general this looks fine. I don't think we need the cleanup function at all, though: IIRC pushPrefEnv does all of the necessary cacheing and restoring itself. mconley can you confirm my understanding?

Comment 26

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

Comment on attachment 8787404 [details] [diff] [review]
dummy_plugin_test.patch

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

Just a few questions - see below. Thanks for the test!

::: dom/plugins/test/mochitest/plugin-utils.js
@@ +33,5 @@
>      // propagate to the child. Yuck!
>      SpecialPowers.Services.tm.currentThread.processNextEvent(true);
>    }
>    SimpleTest.registerCleanupFunction(function() {
>      SpecialPowers.setTestPluginEnabledState(oldEnabledState, pluginName);

In the case that oldEnabledState is false-y, how will setTestPluginEnabledState take that when passed that false-y value?

::: dom/plugins/test/mochitest/test_hidden_plugin.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +<head>
> +  <title>Test whether windowed plugins with opacity:0 get their window set correctly</title>

I don't think this title matches the test.

@@ +8,5 @@
> +
> +<body>
> +  <script type="application/javascript;version=1.7">
> +  "use strict"
> +  {

Could you please add some documentation at the top of the test for what this test tests for?

@@ +18,5 @@
> +    let prefName = "plugins.navigator.hidden_ctp_plugin";
> +    try {
> +      oldPrefVal = SpecialPowers.getCharPref(prefName);
> +    } catch (ex) {}
> +    let promise = new Promise (res => SpecialPowers.pushPrefEnv({ set: [[prefName, testPluginName]]}, res))

SpecialPowers.pushPrefEnv now returns a Promise if you don't pass it a callback, so I think you can just do:

let promise = SpecialPowers.pushPrefEnv({ set: [[prefName, testPluginName]] });

@@ +20,5 @@
> +      oldPrefVal = SpecialPowers.getCharPref(prefName);
> +    } catch (ex) {}
> +    let promise = new Promise (res => SpecialPowers.pushPrefEnv({ set: [[prefName, testPluginName]]}, res))
> +    promise.then(function() {
> +      SimpleTest.registerCleanupFunction(function() {

bsmedberg is correct - pushPrefEnv will do the cleanup for you when your test is done - this cleanup is not necessary.

@@ +28,5 @@
> +          return new Promise(res => SpecialPowers.pushPrefEnv({ clear: [[prefName, "CHAR"]]}, res));
> +        }
> +      });
> +      for (let i = 0; i < plugins.length; i++) {
> +        let plugin = plugins[i]

Nit - missing semicolon

@@ +40,5 @@
> +              setTestPluginEnabledState(oldState, plugin.name);
> +          });
> +        }
> +      }
> +      is(navigator.plugins.length, 1, "navigator.plugins length should be 1");

Remind me again, what is the value of navigator.plugins.length in these sorts of mochitests when the hidden_ctp_plugin pref isn't set? I assume we have several plugins available by default?

Comment 27

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

(In reply to Mike Conley (:mconley) - (Digging through needinfos and reviews) from comment #26)
> Comment on attachment 8787404 [details] [diff] [review]
> dummy_plugin_test.patch
> 
> Review of attachment 8787404 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few questions - see below. Thanks for the test!
> 
> ::: dom/plugins/test/mochitest/plugin-utils.js
> @@ +33,5 @@
> >      // propagate to the child. Yuck!
> >      SpecialPowers.Services.tm.currentThread.processNextEvent(true);
> >    }
> >    SimpleTest.registerCleanupFunction(function() {
> >      SpecialPowers.setTestPluginEnabledState(oldEnabledState, pluginName);
> 
> In the case that oldEnabledState is false-y, how will
> setTestPluginEnabledState take that when passed that false-y value?
I think this utility function is just wrong. If the old enabled state was 0 (disabled) then we just fail when trying to set it to something else currently. I think whoever wrote this thought that a false return value meant failure.

> 
> Remind me again, what is the value of navigator.plugins.length in these
> sorts of mochitests when the hidden_ctp_plugin pref isn't set? I assume we
> have several plugins available by default?
I'm seeing 16 plugins on my system (before this test disables them), of which I think 7 are test plugins.
This test disables all the plugins except for one that it makes c2p and hides.

Comment 28

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

Attachment: dummy_plugin_test.patch

Comment 29

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

Comment on attachment 8789079 [details] [diff] [review]
dummy_plugin_test.patch

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

Looks good to me, assuming all green on try. Thanks for the test!

Comment 30

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

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Try push has test failures. Please add proper commit messages to the patches while you're at it.

https://treeherder.mozilla.org/logviewer.html#?job_id=27320625&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=27323208&repo=try

Comment 32

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

Attachment: test_fixes.patch

two test issues. Debug builds assert that the plugin's file name has a period in it (so it can strip off the extension). This makes the file name dummy.plugin to work around that.

Linux 32bit test machines have a DivX plugin that shows up in navigator.plugins even when all the plugins are disabled, so this patch disables its own test on that platform.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f4db84874d&selectedJob=28081096

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/154d446586b4
put dummy plugin in navigator.plugins when a click to play plugin is hidden and its the only plugin r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/be81e6841555
pref to control which CTP plugins should be hidden r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a582bbcc549
test for dummy plugin r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9c9d933a42
disable test on linux 32 and add a fake file name to avoid an assertion in debug builds r=bsmedberg

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/183727ce6551
Also skip the test on linux64 because tests were failing

Comment 35

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

We were still intermittently failing on linux64 (twice so far that I've noticed), so I pushed a followup to disable the test on linux64: https://treeherder.mozilla.org/logviewer.html#?job_id=36648596&repo=mozilla-inbound

Comment 36

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

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cd47fc2d3202 because now I've seen the test intermittently fail on Windows like https://treeherder.mozilla.org/logviewer.html#?job_id=36662375&repo=mozilla-inbound#L2747

Comment 37

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

Benjamin, I'm coming to the conclusion that our test machines are a mess with a bunch of plugins we can't disable. Disabling all the plugins is the pre-req for testing this, not the thing we're trying to test. I'm of the opinion we should land this without the test. What say you?

Comment 38

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

Attachment: fake_filename.patch

this is the same change you already r+'d minus disabling the test for linux 32

Comment 39

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

Attachment: fix_test.patch

This changes the test to only assert that navigator.plugins is not empty, which is really all we care about 

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

Comment 40

[Benjamin Smedberg]

Comment on attachment 8796213 [details] [diff] [review]
fix_test.patch

Maybe we can do better when Flash-only lands in 43, but this is good enough for now.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a480a95a9e6d
put dummy plugin in navigator.plugins when a click to play plugin is hidden and its the only plugin r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2d1ce84a07
pref to control which CTP plugins should be hidden r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd48531bded
test for dummy plugin r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d536cd5e5ab
add a fake filename to avoid an assertion that plugin filenames have periods in them r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/71e521c11c8b
only assert that navigator.plugins is not empty when there is a hidden CTP plugin r=bsmedberg

Comment 42

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a480a95a9e6d
https://hg.mozilla.org/mozilla-central/rev/4f2d1ce84a07
https://hg.mozilla.org/mozilla-central/rev/6bd48531bded
https://hg.mozilla.org/mozilla-central/rev/8d536cd5e5ab
https://hg.mozilla.org/mozilla-central/rev/71e521c11c8b

Comment 43

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

I want to confirm that, the issue is fixed with latest Nightly 52.0a1 (2016-10-04) (64-bit) with "plugins.navigator_hide_disabled_flash" preference set to "true" in about:config (per bug #1296004).

Thank you very much.

Comment 44

[IU]

Maybe I'm misunderstanding this bug, but when I visit: http://, with "Ask to Activate" enabled (and "Click to Play per-element" extension installed), I'm still getting asked to install flash -- regardless of whether "plugins.navigator_hide_disabled_flash" is true or false.

Comment 45

[IU]

Oops.  That link should be: http://www.ctvnews.ca/video

Comment 46

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

(In reply to IU from comment #44)
> Maybe I'm misunderstanding this bug, but when I visit: http://, with "Ask to
> Activate" enabled (and "Click to Play per-element" extension installed), I'm
> still getting asked to install flash -- regardless of whether
> "plugins.navigator_hide_disabled_flash" is true or false.
+
(In reply to IU from comment #45)
> Oops.  That link should be: http://www.ctvnews.ca/video

Looks like you need to refresh that site manually, so it will be working, at least it works for me,
and don't forget that Click to Play per-element extension is broken (see bug #1294344) with "plugins.navigator_hide_disabled_flash" preference set to "true" in about:config (per bug #1296004).

Comment 47

[Robert Kaiser]

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). Emptying plugins.navigator.hidden_ctp_plugin makes the site work as expected. Bug 1312091 may play a role there, as they may use an iframe for running the actual Flash content.