Closed Bug 1098969 Opened 5 years ago Closed 5 years ago

RemoteAddonsParent/ContentPolicyParent gets nsIContentPolicy implementation the wrong way

Categories

(Firefox :: Extension Compatibility, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: nmaier, Assigned: billm)

References

Details

(Keywords: addon-compat, regression)

Attachments

(2 files, 1 obsolete file)

Right now |ContentPolicyParent| caches the |entry|:
http://hg.mozilla.org/mozilla-central/file/7f0d92595432/toolkit/components/addoncompat/RemoteAddonsParent.jsm#l169
and later uses the caches |entry| trying to get the service:
http://hg.mozilla.org/mozilla-central/file/7f0d92595432/toolkit/components/addoncompat/RemoteAddonsParent.jsm#l137

This is wrong. According to the nsContentPolicy implementation, |value| must be used to get the service:
http://hg.mozilla.org/mozilla-central/file/7f0d92595432/xpcom/glue/nsCategoryCache.cpp#l47
|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. https://addons.mozilla.org/addon/adblock-plus-pop-up-addon/ 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.

STR:
- Install https://addons.mozilla.org/addon/adblock-plus-pop-up-addon/
- See console spammed and add-on not working correctly

Expected:
- 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
Status: NEW → ASSIGNED
Attachment #8523238 - Flags: review?(mconley)
Comment on attachment 8523238 [details] [diff] [review]
fix-content-policy

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

::: toolkit/components/addoncompat/RemoteAddonsParent.jsm
@@ +108,5 @@
>      let ppmm = Cc["@mozilla.org/parentprocessmessagemanager;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 (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICategoryManager#getCategoryEntry%28%29) 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+
https://hg.mozilla.org/mozilla-central/rev/83147bd55260
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.