Closed Bug 1098969 Opened 7 years ago Closed 7 years ago

RemoteAddonsParent/ContentPolicyParent gets nsIContentPolicy implementation the wrong way


(Firefox :: Extension Compatibility, defect)

Not set



Firefox 36


(Reporter: nmaier, Assigned: billm)



(Keywords: addon-compat, regression)


(2 files, 1 obsolete file)

Right now |ContentPolicyParent| caches the |entry|:
and later uses the caches |entry| trying to get the service:

This is wrong. According to the nsContentPolicy implementation, |value| must be used to get the service:
|entry| is a free-form field and not guaranteed to contain the contract ID.

This causes a couple of add-ons to not work anymore, and spamming the console with "TypeError: Cc[policyCID] is undefined" messages.
E.g. is affected.

On a related note, the code will return an error from ContentPolicyParent.shouldLoad upon the first service not being available (anymore). Not sure if this is indented behavior now, but the original nsContentPolicy implementation will just script any services that fail loading and/or where shouldLoad returns an error.

- Install
- See console spammed and add-on not working correctly

- Console should not be spammed
- Add-on should work correctly if possible (as far as there are no other e10s compat issues) or should at least not hinder other content policies from running.
I'm not familar with the code and I'm not having the time to see this through, but this patch seems to address the issue...
Attached patch fix-content-policy (obsolete) — Splinter Review
Thanks Nils. This is about as helpful a bug report as I've ever gotten.
Assignee: nobody → wmccloskey
Attachment #8523238 - Flags: review?(mconley)
Comment on attachment 8523238 [details] [diff] [review]

Review of attachment 8523238 [details] [diff] [review]:

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +108,5 @@
>      let ppmm = Cc[";1"]
>                 .getService(Ci.nsIMessageBroadcaster);
>      ppmm.addMessageListener("Addons:ContentPolicy:Run", this);
> +    this._policies = {};

Seems like a perfect use-case for a Map.

@@ +113,4 @@
>    },
> +  addContentPolicy: function(name, cid) {
> +    this._policies[name] = cid;

So if "entry" is a free-form field... do we need to worry about collision?

@@ +130,5 @@
>      }
>    },
>    shouldLoad: function(aData, aObjects) {
> +    for (let name in this._policies) {

... because then we get

for (let policyCID of this._policies.values()) {
  // ...
Attachment #8523238 - Flags: review?(mconley)
It appears that the category |entry| part (also called the name) must be unique. The service uses a hashtable keyed on the entry to store this stuff. Also, there's a function getCategoryEntry ( that returns a single value for a given name. The documentation on this isn't very clear, but I don't see how it could work any other way.
Attachment #8523238 - Attachment is obsolete: true
Attachment #8524133 - Flags: review?(mconley)
Comment on attachment 8524133 [details] [diff] [review]
fix-content-policy v2

Review of attachment 8524133 [details] [diff] [review]:

Looks good - thanks billm for the patch, and nmaier for the great write-up.
Attachment #8524133 - Flags: review?(mconley) → review+
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.