Closed
Bug 1098969
Opened 10 years ago
Closed 10 years ago
RemoteAddonsParent/ContentPolicyParent gets nsIContentPolicy implementation the wrong way
Categories
(Firefox :: Extension Compatibility, defect)
Firefox
Extension Compatibility
Tracking
()
RESOLVED
FIXED
Firefox 36
People
(Reporter: nmaier, Assigned: billm)
References
Details
(Keywords: addon-compat, regression)
Attachments
(2 files, 1 obsolete file)
3.45 KB,
patch
|
Details | Diff | Splinter Review | |
3.14 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
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...
Assignee | ||
Comment 2•10 years ago
|
||
Thanks Nils. This is about as helpful a bug report as I've ever gotten.
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/83147bd55260
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/83147bd55260
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in
before you can comment on or make changes to this bug.
Description
•