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

VERIFIED FIXED in mozilla20

Status

()

defect
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted patch patch (obsolete) — Splinter Review
Jared - I'd appreciate you taking a look at this, thanks.
In particular, is the try/catch style I'm using acceptable?
Attachment #688355 - Flags: review?(jaws)
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.
Attachment #688355 - Flags: review?(jaws) → feedback+
Posted patch patch v2 (obsolete) — Splinter Review
Good call - I think this is a much better way of going about this.
Attachment #688355 - Attachment is obsolete: true
Attachment #688880 - Flags: review?(jaws)
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?
Attachment #688880 - Flags: review?(jaws) → review+
Posted patch patch v2.1Splinter Review
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
Attachment #688880 - Attachment is obsolete: true
Attachment #689374 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3d63a613d14a
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Summary: click-to-play: pluginHost.getPermissionStringForType calls need to be protected by try/catch → click-to-play: pluginHost.getPermissionStringForType calls need to check if the mime type is a known plugin first
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.