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.

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

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

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.

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

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?

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

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.