818118 - click-to-play: pluginHost.getPermissionStringForType calls need to check if the mime type is a known plugin first

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

nsIPluginHost::GetPermissionStringForType can fail if given a type that is not associated with any installed plugin. As such, any calls to it in browser-plugins.js need to be protected by a try/catch block.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Jared - I'd appreciate you taking a look at this, thanks.
In particular, is the try/catch style I'm using acceptable?

Comment 2

[Paul Silaghi, QA [:pauly]]

STR in https://bugzilla.mozilla.org/show_bug.cgi?id=746374#c21

Comment 3

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

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

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

::: browser/base/content/browser-plugins.js
@@ +242,5 @@
>      let pluginPermission = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> +    // If objLoadingContent.actualType is null or is not associated with any
> +    // installed plugin, this will fail. That's okay - we just treat it as
> +    // an unknown plugin.
> +    try {

As we talked about on IRC, see if you can use objectLoadingContent.getContentTypeForMIMEType here and check for a truthy return value instead of doing the try/catch here.

I think this will be better since it will handle the error case that we know about, but continue to let us find out about other error conditions that we are presently unaware of.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

Good call - I think this is a much better way of going about this.

Comment 5

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

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

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

::: browser/base/content/browser-plugins.js
@@ +237,5 @@
>      }
>    },
>  
> +  isKnownPlugin: function PH_isKnownPlugin(objLoadingContent) {
> +    return (objLoadingContent.getContentTypeForMIMEType(objLoadingContent.actualType) == Ci.nsIObjectLoadingContent.TYPE_PLUGIN);

nit: break this apart in to two lines so it is easier to read.

@@ +377,5 @@
>        pluginPermission = Services.perms.testPermission(browser.currentURI, permissionString);
>      }
> +    else { // if this isn't a known plugin type, what are we doing here?
> +      return;
> +    }

Can this be changed to:
if (!gPluginHandler.isKnownPlugin(objLoadingContent))
  return;
let permissionString = pluginHost.getPermissionStringForType(objLoadingContent.actualType);
pluginPermission = Services.perms.testPermission(browser.currentURI, permissionString);

Only because it seems more concise :)

@@ +543,5 @@
>    _setPermissionForPlugins: function PH_setPermissionForPlugins(aBrowser, aPermission, aPluginList) {
>      let pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
>      for (let plugin of aPluginList) {
>        let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +      if (gPluginHandler.canActivatePlugin(objLoadingContent)) {

does this still need to check if the plugin is known?

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2.1

This is the patch as checked in (I had to rebase it and update a comment clarifying a case where it wasn't necessary to check again if a plugin was a known type).
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d63a613d14a

Just for completeness, here were some try runs:
https://tbpl.mozilla.org/?tree=Try&rev=781e1ef13872
https://tbpl.mozilla.org/?tree=Try&rev=6f087c779013

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3d63a613d14a

Comment 8

[Paul Silaghi, QA [:pauly]]

If there are plugins missing and the "missing plugins notification" shows up, the CTP doorhanger is shown now.
Verified fixed on Nightly 2012-12-10 Win 7 x64.