Closed Bug 818009 Opened 7 years ago Closed 7 years ago

"Unknown" plugin in CTP plugins list

Categories

(Core :: Plug-ins, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox18 + verified
firefox19 + verified
firefox20 + verified
firefox-esr17 19+ verified

People

(Reporter: pauly, Assigned: keeler)

References

Details

Attachments

(3 files, 2 obsolete files)

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
This works fine in release/17.0.1 and regressed in beta/18.
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
Bug 797043 is the only directly click-to-play related change in there AFAICT.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → dkeeler
Attachment #688417 - Flags: review?(jaws)
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();
Attachment #688417 - Flags: review?(jaws) → review+
(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
(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?
(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?
(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.
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #688417 - Attachment is obsolete: true
Attachment #688895 - Flags: review+
Attachment #688895 - Flags: feedback?(jschoenick)
Attachment #688895 - Flags: feedback?(jschoenick) → feedback+
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.
Attachment #688895 - Flags: feedback+
Tracking, given this is a regression and bug 810082 (targeted for FF18) will make this even more visible.
Attached patch patch v2.1Splinter Review
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
Attachment #688895 - Attachment is obsolete: true
Attachment #688952 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e8e24d4bed52
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Attached patch patch for auroraSplinter Review
(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
Attachment #689806 - Flags: review+
Attachment #689806 - Flags: approval-mozilla-aurora?
Attached patch patch for betaSplinter Review
(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:
Attachment #689807 - Flags: review+
Attachment #689807 - Flags: approval-mozilla-beta?
Attachment #689806 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 689807 [details] [diff] [review]
patch for beta

low risk, needed for CTP, approving.
Attachment #689807 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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 ?
(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.
bug 820708 filed
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b4
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 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.
Attachment #689807 - Flags: approval-mozilla-esr17?
Attachment #689807 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
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.
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
You need to log in before you can comment on or make changes to this bug.