929120 - Most compartments for an SDK add-on aren't listed in the add-ons section

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Dave Townsend [:mossop]]

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?

Comment 1

[Nicholas Nethercote [inactive]]

Nils might know.

Comment 2

[Nils Maier [:nmaier]]

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/[e-mail]!/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/[e-mail]!/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).

Comment 3

[Nils Maier [:nmaier]]

Attachment: PoC: Add a way to get compartment locations ignoring non-addon locations.

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?

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

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?

Comment 6

[Nils Maier [:nmaier]]

(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.

Comment 7

[Nils Maier [:nmaier]]

(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.

Comment 8

[Nils Maier [:nmaier]]

Attachment: Add a way to get compartment locations ignoring non-addon locations.

Cleaned up that patch a bit.

Not sure about tests, though?

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Nils Maier [:nmaier]]

Attachment: Add a way to get compartment locations ignoring non-addon locations. r=njn

Addressed nit. Carrying over r=njn

Comment 11

[Nils Maier [:nmaier]]

Part of this green(-ish) try:
https://tbpl.mozilla.org/?tree=Try&rev=eac3d574aaf6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eac93c93b680

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/eac93c93b680

Comment 14

[[e-mail]]

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.)