Closed
Bug 929120
Opened 11 years ago
Closed 11 years ago
Most compartments for an SDK add-on aren't listed in the add-ons section
Categories
(Toolkit :: about:memory, defect)
Tracking
()
RESOLVED
FIXED
mozilla28
Tracking | Status | |
---|---|---|
firefox28 | --- | fixed |
People
(Reporter: mossop, Assigned: nmaier)
Details
(Whiteboard: [MemShrink:P2][qa-])
Attachments
(1 file, 2 obsolete files)
8.92 KB,
patch
|
nmaier
:
review+
|
Details | Diff | Splinter Review |
Most of the compartments that an SDK add-on creates aren't listed in the add-ons section, they're instead under the system zone. How can we fix that?
Assignee | ||
Comment 2•11 years ago
|
||
The compartment location "parser" will look at something like this: resource://gre/modules/commonjs/method/core.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/.default/extensions/firefoxaddon@flattr.com.xpi!/bootstrap.js -> resource://gre/modules/commonjs/toolkit/loader.js:198) find the first OK URI: resource://gre/modules/commonjs/method/core.js and try to map it to an add-on, which cannot be mapped to a particular add-on of course. It should be possible to just blacklist resource://gre prefixes in http://hg.mozilla.org/mozilla-central/file/c6fc35c53c37/js/xpconnect/src/XPCJSRuntime.cpp#l339 which would then cause the parser to pick up: jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/.default/extensions/firefoxaddon@flattr.com.xpi!/bootstrap.js, which then could be mapped. There is still other stuff in my profile, though, such as: resource://gre/modules/commonjs/sdk/tabs/utils.js (from: resource://gre/modules/commonjs/toolkit/loader.js:198) I think this is coming from Scriptish. This can be potentially fixed by having loader.js always throw in some more annotations (derived from the js stack, or whatever).
Flags: needinfo?(maierman)
Updated•11 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 3•11 years ago
|
||
Proof of concept on how to ignore resource://gre/ stuff so that JetPack compartments will be mapped again. Other stuff could be blacklisted as well, like built-in chrome:// prefixes. Previously the location was parsed once and then cached, but I dropped that in this PoC for now for simplicity reasons (would need to cache two things now). Doesn't really seem to influence performance much, anyway. Also found an off-by-one error in the parser, that will chop off one byte too many so that bootstrap.js became bootstrap.j. Does not really matter at the moment, as the only consumer, the add-on mapper, does not care about that. Should be fixed properly, in another bug preferably (when I find some time). Right now, only the memory reporter mapping add-ons uses GetLocationURI, so it would be possible to just rename the API to something sane containing "addon" and have it always apply the blacklist... Or keep GetLocationURI around like this PoC does... Any thoughts in general?
Attachment #820094 -
Flags: feedback?(n.nethercote)
Comment 4•11 years ago
|
||
Comment on attachment 820094 [details] [diff] [review] PoC: Add a way to get compartment locations ignoring non-addon locations. Review of attachment 820094 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me. Two thumbs up for making it less stateful.
Attachment #820094 -
Flags: feedback?(n.nethercote) → feedback+
Comment 5•11 years ago
|
||
BTW, in debug builds when I load about:memory I see this once for each process: ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)" location: "JS frame :: file:///home/njn/moz/mi4/d64/dist/bin/components/addonManager.js :: amManager :: line 43" data: no] ************************************************************ Any idea what it means?
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5) > BTW, in debug builds when I load about:memory I see this once for each > process: > > ************************************************************ > * Call to xpconnect wrapped JSObject produced this error: * > [Exception... "Component returned failure code: 0x80570016 > (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" nsresult: > "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)" location: "JS frame :: > file:///home/njn/moz/mi4/d64/dist/bin/components/addonManager.js :: > amManager :: line 43" data: no] > ************************************************************ > > Any idea what it means? Before of after that patch? Anyway, it means ".getService(...) returned failure" for whatever reason.
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #5) > Any idea what it means? Understood the issue now I think, and filed it as bug 929297.
Updated•11 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 8•11 years ago
|
||
Cleaned up that patch a bit. Not sure about tests, though?
Assignee: nobody → maierman
Attachment #820094 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #824352 -
Flags: review?(n.nethercote)
Comment 9•11 years ago
|
||
Comment on attachment 824352 [details] [diff] [review] Add a way to get compartment locations ignoring non-addon locations. Review of attachment 824352 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/xpcprivate.h @@ +3742,5 @@ > { > public: > + enum LocationHint { > + eLocationHintRegular, > + eLocationHintAddon The 'e' prefix isn't a common style; just drop it.
Attachment #824352 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Addressed nit. Carrying over r=njn
Attachment #824352 -
Attachment is obsolete: true
Attachment #824864 -
Flags: review+
Assignee | ||
Comment 11•11 years ago
|
||
Part of this green(-ish) try: https://tbpl.mozilla.org/?tree=Try&rev=eac3d574aaf6
Keywords: checkin-needed
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eac93c93b680
Keywords: checkin-needed
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eac93c93b680
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment 14•10 years ago
|
||
Comment on attachment 824864 [details] [diff] [review] Add a way to get compartment locations ignoring non-addon locations. r=njn >+ static NS_NAMED_LITERAL_CSTRING(kGRE, "resource://gre/"); >+ static NS_NAMED_LITERAL_CSTRING(kToolkit, "chrome://global/"); >+ static NS_NAMED_LITERAL_CSTRING(kBrowser, "chrome://browser/"); Strings have constructors and destructors and should not be made static. (If it becomes possible to make named literal strings static, then this will be done as part of the macro anyway.)
status-firefox28:
--- → fixed
Whiteboard: [MemShrink:P2] → [MemShrink:P2][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•