818009 - "Unknown" plugin in CTP plugins list

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Paul Silaghi, QA [:pauly]]

STR:
1. Set plugins.click_to_play=TRUE
2. Open https://bugzilla.mozilla.org/attachment.cgi?id=612565 (testcase from bug 742753)
3. Click on the doorhanger near the location bar

Actual results:
"Unknown" plugin http://img255.imageshack.us/img255/9088/ctp.png

Tested on Nightly 20.0a1 (2012-12-02) on Win 7

Comment 1

[Georg Fritzsche [:gfritzsche]]

This works fine in release/17.0.1 and regressed in beta/18.

Comment 2

[Paul Silaghi, QA [:pauly]]

Last good nightly: 2012-10-17
First bad nightly: 2012-10-18

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-10-17&enddate=2012-10-17

Comment 3

[Paul Silaghi, QA [:pauly]]

Sorry, here is the correct pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-10-17&enddate=2012-10-18

Comment 4

[Georg Fritzsche [:gfritzsche]]

Bug 797043 is the only directly click-to-play related change in there AFAICT.

Comment 5

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

From what I can tell, bug 797043 merely exposed a bug that was present basically the entire time. canActivatePlugin should really only return true for plugins that actually are click-to-play types.

Jared, here's another patch. I know I've been asking a lot of reviews from you lately, so if you want me to find someone else to do some of them, just let me know.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 688417 [details] [diff] [review]
patch

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

Thank you for fixing this! I have noticed it too and was curious as to the cause of it :)

::: browser/base/content/browser-plugins.js
@@ +248,5 @@
>  
> +    // A plugin can only be activated if it is not already activated, if there
> +    // is not a permission preventing it from being activated, if its
> +    // fallback type is a click-to-play type, and if its fallback type is not
> +    // PLUGIN_PLAY_PREVIEW

This comment seems redundant will get out of date with the actual code below.

@@ +253,3 @@
>      return !objLoadingContent.activated &&
>             pluginPermission != Ci.nsIPermissionManager.DENY_ACTION &&
> +           objLoadingContent.pluginFallbackType >= Ci.nsIObjectLoadingContent.PLUGIN_CLICK_TO_PLAY &&

Can you also add a <= Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW for future-proofing?

::: browser/base/content/test/browser_bug818009.js
@@ +11,5 @@
> +  });
> +  Services.prefs.setBoolPref("plugins.click_to_play", true);
> +
> +  var newTab = gBrowser.addTab();
> +  gBrowser.selectedTab = newTab;

gBrowser.selectedTab = gBrowser.addTab();

Comment 7

[John Schoenick [:johns]]

(In reply to Jared Wein [:jaws] from comment #6)
> Can you also add a <= Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW for
> future-proofing?

If you do this make sure you update the comment to note the assumption:
http://dxr.mozilla.org/mozilla-central/content/base/src/nsObjectLoadingContent.h.html#l80
http://dxr.mozilla.org/mozilla-central/content/base/public/nsIObjectLoadingContent.idl.html#l52

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to John Schoenick [:johns] from comment #7)
> (In reply to Jared Wein [:jaws] from comment #6)
> > Can you also add a <= Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW for
> > future-proofing?
> 
> If you do this make sure you update the comment to note the assumption:
> http://dxr.mozilla.org/mozilla-central/content/base/src/
> nsObjectLoadingContent.h.html#l80
> http://dxr.mozilla.org/mozilla-central/content/base/public/
> nsIObjectLoadingContent.idl.html#l52

Wouldn't it be better to put such logic in one place (e.g. provide nsIObjectLoadingContent::isPlaceHolderType, etc.)? Or is something making this more complicated?

Comment 9

[Dana Keeler (she/her) [:keeler]]

(In reply to Jared Wein [:jaws] from comment #6)
> @@ +253,3 @@
> >      return !objLoadingContent.activated &&
> >             pluginPermission != Ci.nsIPermissionManager.DENY_ACTION &&
> > +           objLoadingContent.pluginFallbackType >= Ci.nsIObjectLoadingContent.PLUGIN_CLICK_TO_PLAY &&
> 
> Can you also add a <= Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW for
> future-proofing?

I guess shouldn't this boil down to:

>    return !objLoadingContent.activated &&
>           pluginPermission != Ci.nsIPermissionManager.DENY_ACTION &&
>           objLoadingContent.pluginFallbackType >= Ci.nsIObjectLoadingContent.PLUGIN_CLICK_TO_PLAY &&
>           objLoadingContent.pluginFallbackType <= Ci.nsIObjectLoadingContent.PLUGIN_VULNERABLE_NO_UPDATE;

since that's the range of click-to-play types (which is what we care about here)?

(In reply to John Schoenick [:johns] from comment #7)
> (In reply to Jared Wein [:jaws] from comment #6)
> > Can you also add a <= Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW for
> > future-proofing?
> 
> If you do this make sure you update the comment to note the assumption:
> http://dxr.mozilla.org/mozilla-central/content/base/src/
> nsObjectLoadingContent.h.html#l80
> http://dxr.mozilla.org/mozilla-central/content/base/public/
> nsIObjectLoadingContent.idl.html#l52

I'm not sure I understand what you're asking here. Do you want a comment in browser-plugins.js about what the values of pluginFallbackType can be and what they mean?

Comment 10

[John Schoenick [:johns]]

(In reply to David Keeler from comment #9)
> I'm not sure I understand what you're asking here. Do you want a comment in
> browser-plugins.js about what the values of pluginFallbackType can be and
> what they mean?

In this case, we're assuming that everything between CLICK_TO_PLAY and VULNERABLE_NO_UPDATE are CTP types, but that might break if we add a new type in the wrong spot, so I'm saying we should note that in the OBJC comment, similar to how we note that >= CLICK_TO_PLAY are reserved for plugin-replacement types.

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Wouldn't it be better to put such logic in one place (e.g. provide
> nsIObjectLoadingContent::isPlaceHolderType, etc.)? Or is something making
> this more complicated?

That would also be fine. Right now the only assumption is the >= CTP one, so adding a new IDL function seemed unnecessary.

Comment 11

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2

Ok - I think I've addressed everyone's comments. Carrying over r+, but anyone stop me if there's an objection.
Just started a try run: https://tbpl.mozilla.org/?tree=Try&rev=ca0a740bf410

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 688895 [details] [diff] [review]
patch v2

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

Is it true that you want to leave out the PLUGIN_PLAY_PREVIEW? The original patch didn't do so, and I'm not sure how that will affect Shumway. Since PLUGIN_PLAY_PREVIEW doesn't need to interact with click-to-play, I'm thinking that this patch is more correct.

::: content/base/public/nsIObjectLoadingContent.idl
@@ +51,5 @@
>    const unsigned long PLUGIN_USER_DISABLED        = 7;
>    /// ** All values >= PLUGIN_CLICK_TO_PLAY are plugin placeholder types that
>    ///    would be replaced by a real plugin if activated (playPlugin())
> +  /// ** Furthermore, values >= eFallbackClickToPlay and
> +  ///    <= eFallbackVulnerableNoUpdate are click-to-play types.

The IDL doesn't mention any "eFallbackClickToPlay" name, so this comment should only use PLUGIN_CLICK_TO_PLAY and PLUGIN_VULNERABLE_NO_UPDATE, respectively.

Comment 13

[Alex Keybl [:akeybl]]

Tracking, given this is a regression and bug 810082 (targeted for FF18) will make this even more visible.

Comment 14

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2.1

Fixed the comment.
canActivate isn't called (and shouldn't be called) by the playPreview code, so we're good there.

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

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Dana Keeler (she/her) [:keeler]]

Attachment: patch for aurora

(patch rebased for aurora - carrying over r+)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play / differentiating by type
User impact if declined: users will see an "Unknown" plugin in the list of plugins to activate, which is confusing and possibly even scary
Testing completed (on m-c, etc.): tested on try, has landed on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 17

[Dana Keeler (she/her) [:keeler]]

Attachment: patch for beta

(rebased for beta, carrying over r+)

[Approval Request Comment] - all of this is the same as for the aurora patch (see above)
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 689807 [details] [diff] [review]
patch for beta

low risk, needed for CTP, approving.

Comment 19

[Dana Keeler (she/her) [:keeler]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4d85abbda8c
https://hg.mozilla.org/releases/mozilla-beta/rev/f8eeaa6920ab

Comment 20

[Paul Silaghi, QA [:pauly]]

What has been done so far is good. But I've noticed one small issue:
1. Open addons manager and disable flash
2. Restart Firefox
3. Open http://mozqa.com/data/firefox/plugins/plugin-test.html --> see the missing plugin notification instead of flash
4. Open addons manager and enable flash
5. Reload the tab from step 3

Actual results:
"unknown plugin" instead of flash

Works fine after restarting FF. So...does it worth to be fixed ?

Comment 21

[Alex Keybl [:akeybl]]

(In reply to Paul Silaghi [QA] from comment #20)
> What has been done so far is good. But I've noticed one small issue:
> 1. Open addons manager and disable flash
> 2. Restart Firefox
> 3. Open http://mozqa.com/data/firefox/plugins/plugin-test.html --> see the
> missing plugin notification instead of flash
> 4. Open addons manager and enable flash
> 5. Reload the tab from step 3
> 
> Actual results:
> "unknown plugin" instead of flash
> 
> Works fine after restarting FF. So...does it worth to be fixed ?

Somebody please correct me if I'm wrong, but this does not appear to be a common user scenario. We should get a bug on file regardless.

Comment 22

[Paul Silaghi, QA [:pauly]]

bug 820708 filed

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4

Comment 24

[Paul Silaghi, QA [:pauly]]

I stumbled upon this again when trying to verify bug 820497 on FF ESR. Wouldn't be better to land this in ESR too ?

Comment 25

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 689807 [details] [diff] [review]
patch for beta

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed for click-to-play
User impact if declined: users may see an "Unknown" plugin in the click-to-play UI, which is scary and undesirable
Fix Landed on Version: 20
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/68a030fcf213

I pushed this with ba=akeybl since the IDL changes made were only to add comments and don't affect compatibility.

Comment 27

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 19b4, Aurora 20.0a2 (2013-02-05), ESR 17.0.2 2013-02-05-03-45-01-mozilla-esr17 on Win 7, Ubuntu 12.04 and Mac OS X 10.7.5