Use the blocklist to inform click-to-play plugins

RESOLVED FIXED in mozilla16

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

We want the ability to make vulnerable plugins click-to-play much in the way we have the ability to block plugins via the blocklist right now.
Created attachment 629282 [details] [diff] [review]
prototype
Assignee: nobody → dkeeler
Attachment #629282 - Flags: feedback?(jaws)
(Assignee)

Updated

5 years ago
Blocks: 738698
Depends on: 746374

Comment 2

5 years ago
Comment on attachment 629282 [details] [diff] [review]
prototype

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

::: content/base/src/nsObjectLoadingContent.h
@@ +44,1 @@
>  };

please comment what these are (and what VUA and VNU) mean, as you have in other parts of the patch

::: dom/plugins/base/nsPluginHost.h
@@ +91,1 @@
>  

any particular reason you're not adding this to nsIPluginHost ? just curious.. :)
I assume you mean GetBlocklistStateByType? It didn't occur to me to add it to the interface, because every time nsIPluginHost is used in nsObjectLoadingContent.cpp, it's static_casted to nsPluginHost, so I just re-used that style. Is there a good reason for this behavior? Also, should I add the function to nsIPluginHost?
(In reply to David Keeler from comment #1)
> Created attachment 629282 [details] [diff] [review]
> prototype

Sorry I didn't get to this review yet. I will get to it later today.
Comment on attachment 629282 [details] [diff] [review]
prototype

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

I didn't do a great review of the nsPluginHost.cpp and related files since I'm not super comfortable with some of those code areas.

::: browser/base/content/browser.js
@@ +7343,5 @@
>      let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
>      // The overlay is null if the XBL binding is not attached (element is display:none).
>      if (overlay) {
>        overlay.addEventListener("click", function(aEvent) {
> +        if (aEvent.target instanceof XULElement && 

Out of curiosity, what does checking for XULElement get us here?

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2047,5 @@
> +    if (pluginHost) {
> +      rv = pluginHost->GetBlocklistStateByType(aContentType.get(), &state);
> +      if (NS_SUCCEEDED(rv)) {
> +        if (state == nsIBlocklistService::STATE_UPDATE_AVAILABLE)
> +          return ePluginClickToPlayVUA;

nit: In Gecko code, there should be braces on all if() { } else if {} else {}. Now's a good time to fix it for this function.

::: dom/plugins/base/nsPluginHost.cpp
@@ +1286,5 @@
> +  if (plugin) {
> +    nsCOMPtr<nsIBlocklistService> blocklist = do_GetService("@mozilla.org/extensions/blocklist;1");
> +    if (blocklist) {
> +      nsresult rv = blocklist->GetPluginBlocklistState(plugin, EmptyString(),
> +                                                       EmptyString(), aState);

What are the EmptyString()s here for?

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +46,5 @@
>  const MAX_BLOCK_LEVEL                 = 3;
>  const SEVERITY_OUTDATED               = 0;
> +const CLICKTOPLAY_NONE                = 0;
> +const CLICKTOPLAY_UPDATE_AVAILABLE    = 1;
> +const CLICKTOPLAY_NO_UPDATE           = 2;

The difference in naming between CLICKTOPLAY_NONE and CLICKTOPLAY_NO_UPDATE is hard to discern when only looking at these three lines. Can you pick a better name for this?
Attachment #629282 - Flags: feedback?(jaws) → feedback+
Created attachment 631463 [details] [diff] [review]
patch

While we don't have the ui/ux nailed down yet, this patch implements the functionality we want. I've incorporated the feedback I've gotten and restructured some things so this patch no longer depends on any other bug, so we can get moving faster on this.
Attachment #629282 - Attachment is obsolete: true
Attachment #631463 - Flags: review?(joshmoz)
(Assignee)

Updated

5 years ago
No longer depends on: 746374

Comment 7

5 years ago
Comment on attachment 631463 [details] [diff] [review]
patch

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

I wonder if this patch should be split up into two pieces - one to re-do the mShouldPlay and gClickToPlayPlugins variable usage, and the other to do what you really want here. I'm worried that the logic is getting too complicated to be dealt with carefully. I'm not saying this is necessary, but if you wanted to do it I wouldn't mind.

r- mostly for the IsPluginClickToPlay return value, but I'm also concerned about the meaning of nsObjectLoadingContent::mShouldPlay. Let's talk about the latter on irc if you're unsure about what to do about it.

::: browser/base/content/browser-plugins.js
@@ +142,5 @@
>  
> +      case "PluginVulnerableUpdatable":
> +        let updateLink = doc.getAnonymousElementByAttribute(plugin, "class", "checkForUpdatesLink");
> +        self.addLinkClickCallback(updateLink, "updatePlugins");
> +        /* FALLTHRU */

If you fall through here are you going to open the update page and then play the plugin instance anyway?

@@ +161,2 @@
>        let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      if (overlay && self.isTooSmall(plugin, overlay))

I don't understand the changes to this 'if' block - can you explain?

@@ +232,5 @@
>    },
> + 
> +  // Callback for user clicking on the link in a click-to-play plugin
> +  // (where the plugin has an update)
> +  updatePlugins: function (aEvent) {

'updatePlugins' is a little vague - maybe use something more descriptive like "openPluginUpdateURL"?

::: content/base/src/nsObjectLoadingContent.cpp
@@ +479,5 @@
>    }
>  
> +  nsresult result = pluginHost->IsPluginClickToPlayForType(aMIMEType.get());
> +  if (NS_FAILED(result)) {
> +    mShouldPlay = true;

This is pretty awkward logic - I don't like the mapping of success/fail exception codes to a boolean answer. I wouldn't naturally expect a failed code to mean to go ahead and play the instance. Why not just make "IsPluginClickToPlayForType" return a bool?

@@ +574,5 @@
> +  if (!mShouldPlay) {
> +    return false;
> +  } else {
> +    return enabled;
> +  }

The above usage of "mShouldPlay" makes me think its meaning is getting a little too vague. It's not clear under what conditions it is true or false. Does it reflect whether a plugin is enabled or disabled? Is it simply whether a plugin is click-to-play or not? How does it relate to content permissions? This ambiguity is going to make the code harder to understand.

::: dom/plugins/base/nsPluginHost.h
@@ +84,5 @@
>                                 nsIURI *aURL,
>                                 nsIPluginInstanceOwner *aOwner);
>    nsresult IsPluginEnabledForType(const char* aMimeType);
>    nsresult IsPluginEnabledForExtension(const char* aExtension, const char* &aMimeType);
> +  nsresult IsPluginClickToPlayForType(const char *aMimeType);

Make this return a bool, this is not a good use of nsresult (which is essentially an exception code).
Attachment #631463 - Flags: review?(joshmoz) → review-
Comment on attachment 631463 [details] [diff] [review]
patch

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

*sigh* The blocklist service really needs an overhaul.

Is there any reason why we would mark a plugin as outdated, but not enable click-to-play? We'd only mark a plugin as outdated if there is a benefit in telling the user - it seems like any such reason would benefit from click-to-play (potential security issues aren't the only reason for click-to-play). And it would be nice to avoid any unnecessary complexity here - we could just have STATE_OUTDATED and STATE_OUTDATED_UPDATE_AVAILABLE.
If we want to get rid of STATE_OUTDATED and the associated outdated plugin warning mechanisms, I'd be all for it, but that should probably be a separate bug. Also, STATE_NO_UPDATE is intended to convey something a bit stronger than STATE_OUTDATED - that is, the plugin is known to be vulnerable and there is no update, rather than it's simply old (any suggestions for a better name that conveys this yet isn't 80 characters long would be appreciated).
Created attachment 638517 [details] [diff] [review]
patch v2
Attachment #631463 - Attachment is obsolete: true
Attachment #638517 - Flags: review?(joshmoz)

Updated

5 years ago
Attachment #638517 - Flags: review?(joshmoz) → review+
I pushed v2 of the patch to tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=0e3a9dfd3993
Status: NEW → ASSIGNED
Comment on attachment 638517 [details] [diff] [review]
patch v2

I'm working on fixing the browser chrome test, but in the meantime, I thought it would be good to have another set of eyes on this, so I'm asking Blair for a second review.
Attachment #638517 - Flags: review?(bmcbride)
Comment on attachment 638517 [details] [diff] [review]
patch v2

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

Ok, then I think we should frame the blocklist API changes to better reflect comment 9, as they're quite disconnected at the moment. The existing blocklist code is describing the status, not what to do - would like to keep that consistent. And the naming suggestions below end up mapping nicely to the event names PluginVulnerableUpdatable and PluginVulnerableNoUpdate.

You'll also need to update the Add-on Manager UI (toolkit/mozapps/extensions/content/extensions.js and extensions.xul), since we display notifications for STATE_OUTDATED there. Will need to display similar notifications for the two new states.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +46,5 @@
>  const MAX_BLOCK_LEVEL                 = 3;
>  const SEVERITY_OUTDATED               = 0;
> +const CLICKTOPLAY_NOT_APPLICABLE      = 0; // indicates "ignore this field"
> +const CLICKTOPLAY_UPDATE_AVAILABLE    = 1;
> +const CLICKTOPLAY_NO_UPDATE           = 2;

CLICKTOPLAY_NOT_APPLICABLE -> VULNERABILITYSTATUS_NONE
etc

@@ +1051,5 @@
>    if (versionRangeElement && versionRangeElement.hasAttribute("severity"))
>      this.severity = versionRangeElement.getAttribute("severity");
>    else
>      this.severity = DEFAULT_SEVERITY;
> +  if (versionRangeElement && versionRangeElement.hasAttribute("clicktoplay"))

"clicktoplay" -> "vulnerabilitystatus"

@@ +1052,5 @@
>      this.severity = versionRangeElement.getAttribute("severity");
>    else
>      this.severity = DEFAULT_SEVERITY;
> +  if (versionRangeElement && versionRangeElement.hasAttribute("clicktoplay"))
> +    this.clicktoplay = versionRangeElement.getAttribute("clicktoplay");

this.clicktoplay -> this.vulnerabilityStatus

::: xpcom/system/nsIBlocklistService.idl
@@ +24,5 @@
>    const unsigned long STATE_OUTDATED    = 3;
> +  // Indicates that the item is vulnerable and there is an update.
> +  const unsigned long STATE_UPDATE_AVAILABLE   = 4;
> +  // Indicates that the item is vulnerable and there is no update.
> +  const unsigned long STATE_NO_UPDATE   = 5;

STATE_VULNERABLE_UPDATE_AVAILABLE and STATE_VULNERABLE_NO_UPDATE
Attachment #638517 - Flags: review?(bmcbride) → review-
Created attachment 640664 [details] [diff] [review]
patch v3

I've updated the names of the constants and fields.
A concern about the UI: we're looking to land this before the migration date. Since there's additional UI work we have to do anyway (that we're pushing to the next release), would you be alright with me doing the addon manager UI later as a follow-up to this?
Attachment #638517 - Attachment is obsolete: true
Attachment #640664 - Flags: review?(bmcbride)
(In reply to David Keeler from comment #14)
> would you be alright with me doing the addon
> manager UI later as a follow-up to this?

Yep, that's fine. Can you file a dependent followup bug so it doesn't get forgotten?
Comment on attachment 640664 [details] [diff] [review]
patch v3

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

r+ with a couple of nits fixed.

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +25,5 @@
>  <!ENTITY tapToPlayPlugin                                     "Tap here to activate plugin.">
>  <!ENTITY clickToPlayPlugin                                   "Click here to activate plugin.">
> +<!ENTITY clickToPlayPluginVulnerableUpdateAvailable          "Click here to activate vulnerable plugin.">
> +<!ENTITY clickToPlayPluginVulnerableNoUpdate                 "Click here to activate vulnerable plugin (no update available).">
> +<!ENTITY checkForUpdates                                     "Check for updates…">

I assume these strings are part of the followup UI work? I guess they'll do if we don't use it until the followup work is done.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +954,5 @@
>          else if (!plugin.disabled && state != Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
>            if (state == Ci.nsIBlocklistService.STATE_OUTDATED) {
>              gPref.setBoolPref(PREF_PLUGINS_NOTIFYUSER, true);
>            }
> +          else if (state != Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE && state != Ci.nsIBlocklistService.STATE_VULNERABLE_NO_UPDATE) {

Wrap this line, line break at the &&

@@ +1051,5 @@
>      this.severity = versionRangeElement.getAttribute("severity");
>    else
>      this.severity = DEFAULT_SEVERITY;
> +  if (versionRangeElement && versionRangeElement.hasAttribute("vulnerabilitystatus")) {
> +    this.vulnerabilitystatus = versionRangeElement.getAttribute("vulnerabilitystatus");

Camel case the property name (this.vulnerabilityStatus)
(Not the attribute - that should stay as-is)
Attachment #640664 - Flags: review?(bmcbride) → review+
Summary: use the blocklist to inform click-to-play plugins → Use the blocklist to inform click-to-play plugins
(Assignee)

Updated

5 years ago
Depends on: 772897
Created attachment 641084 [details] [diff] [review]
patch v4

Fixed nits. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=90f965562bbd
Also filed bug 772897 for the ui follow-up (and yes, those strings are just placeholders until we get the final ui).
Attachment #640664 - Attachment is obsolete: true
Attachment #641084 - Flags: review+
Between that try run and this one: https://tbpl.mozilla.org/?tree=Try&rev=e1b52fbda573 (an earlier one where the only changes were identifier names), I think it's safe to call this good to go. Marking checkin-needed.
Keywords: checkin-needed
pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18d69de4ff67
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/18d69de4ff67
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 773761

Updated

5 years ago
Depends on: 819964

Updated

4 years ago
Depends on: 919493
You need to log in before you can comment on or make changes to this bug.