Closed Bug 1085710 Opened 7 years ago Closed 7 years ago

Stop trying to send nsIPluginTag over IPC

Categories

(Firefox :: General, defect)

34 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix-ctp (obsolete) — Splinter Review
We have some code for click-to-play that's trying to send an nsIPluginTag over the message manager. I guess this probably sort of works, but it's definitely not kosher. We really only use the name property of the tag, so I made a patch that just sends that.
Attachment #8508338 - Flags: review?(mconley)
Comment on attachment 8508338 [details] [diff] [review]
fix-ctp

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

You might want somebody more familiar with nsBlocklistService.js (irving?) to give this a thumbs up - but everything else here looks like the right idea.

I assume you've run the tests?
Attachment #8508338 - Flags: review?(mconley) → review+
Comment on attachment 8508338 [details] [diff] [review]
fix-ctp

Irving, can you look at the blocklist stuff?
Attachment #8508338 - Flags: review?(irving)
Comment on attachment 8508338 [details] [diff] [review]
fix-ctp

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

I started reviewing this and quickly got to a place where I think it would be better for Mossop to do it. My main question is:

We currently *only* use the plugin name to identify plugins for the blocklist. If we expect things to stay that way, I'd prefer to change the existing API and remove the version that takes a PluginTag and support only ByName. If not that, then I'd change the underlying implementation in nsBlocklistService.js so that getPlugin*() calls getPlugin*ByName() rather than the other way around.

As an aside, there are several places in the Doxygen comments in nsIBlocklistService.idl where the comments don't match the code, e.g. describing an 'id' parameter when the parameter is actually 'addon'.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +1088,5 @@
>  
>      return this._createBlocklistURL(blockEntry.blockID);
>    },
>  
> +  getPluginBlocklistURLByName: function Blocklist_getPluginBlocklistURL(pluginName) {

Incorrect (copy/pasted?) function name, and we don't need the function name any more (and you could even use the new ES6 method syntax)

getPluginBlocklistURLByName: function(pluginName) { ...
},

or

getPluginBlocklistURLByName(pluginName) { ...
},
Attachment #8508338 - Flags: review?(irving) → review?(dtownsend+bugmail)
Comment on attachment 8508338 [details] [diff] [review]
fix-ctp

It looks like I totally misread this code and the patch is incorrect. I'll have to come up with something different.
Attachment #8508338 - Flags: review?(dtownsend+bugmail)
Attachment #8508338 - Flags: review+
What was incorrect? What did I miss?
Flags: needinfo?(wmccloskey)
The code in nsBlocklistService.js funnels into hasMatchingPluginName, which is a terrible name for the function. It sounds like it's checking the plugin's name against the blocklist. In fact it's checking a bunch of properties of the plugin: filename, description, and name. So sending the name alone is insufficient.
Flags: needinfo?(wmccloskey)
Attached patch fix-ctp v2Splinter Review
I guess this is all we need. I still don't really understand why it works to pass a normal JSObject into an XPCOM interface that expects an nsIPluginTag. But it seems to work, so I won't question.
Attachment #8508338 - Attachment is obsolete: true
Attachment #8511323 - Flags: review?(mconley)
(In reply to Bill McCloskey (:billm) from comment #7)
> Created attachment 8511323 [details] [diff] [review]
> fix-ctp v2
> 
> I guess this is all we need. I still don't really understand why it works to
> pass a normal JSObject into an XPCOM interface that expects an nsIPluginTag.
> But it seems to work, so I won't question.

xpconnect automatically makes whatever object you pass an instance of whatever interface it expects. It's magic of the blackest kind.
(In reply to Dave Townsend [:mossop] from comment #8)
> xpconnect automatically makes whatever object you pass an instance of
> whatever interface it expects. It's magic of the blackest kind.

My usual experience is that xpconnect throws NS_NOINTERFACE if your object doesn't have a QueryInterface method. I've traced through the code and I can't figure out why we can get away with not having one in this case.
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedJSClass.cpp#451
Comment on attachment 8511323 [details] [diff] [review]
fix-ctp v2

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

Assuming those are all of the fields you need, looks good to me.
Attachment #8511323 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/ee7994387e65
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.