Closed Bug 846019 Opened 11 years ago Closed 11 years ago

Identify and aggregate extensions' compartments in about:memory

Categories

(Toolkit :: about:memory, defect)

Product:
Toolkit
Component:
about:memory
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: justin.lebar+bug, Assigned: nmaier)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(3 files, 13 obsolete files)

370 add-on compartments
172.04 KB, text/plain
Details
Part 1 - amIAddonManager: Map URIs to AddonIDs. r=bmcbride
23.72 KB, patch
Details | Diff | Splinter Review
Part 2 - Prefix memory reports with add-on ids. r=njn
24.75 KB, patch
Details | Diff | Splinter Review 
I have a compartment in about:memory that looks like

├───29,619,048 B (04.85%) -- compartment([System Principal], jar:file:///Users/jlebar/Library/Application%20Support/Firefox/Profiles/xxxxxx.default/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js (from: resource://gre/modules/XPIProvider.jsm:3702))

Pretty mysterious.  But if you google the UUID, you'll see that it's actually ABP!

So that's problem 1: We're not translating the UUID into the extension name.

Problem 2 is that we don't aggregate together these extension compartments.  As a result, if an extension creates many small compartments, they are essentially hidden memory!  I have a few jetpack add-ons installed and have about 370 add-on compartments.  Am I leaking some?  Who knows?  :)

I'll attach my full list of add-on compartments so we can cry.
My enabled add-ons are

Adblock Plus2.2.4a.3637 {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d}
Add-on Builder Helper1.5 jid0-t3eeRQgGANLCH9c50lPqcTDuNng@jetpack
Bugzilla Secure Mail Viewer for GMail0.1.1 bzsecuremail@mozilla.org
BugzillaJS3.1.0 jid0-NgMDcEu2B88AbzZ6ulHodW9sJzA@jetpack
Easy YouTube Video Downloader6.7 {c0c9a2c7-2e5c-4447-bc53-97718bc91e1b}Gecko Profiler1.11.11 jid0-edalmuivkozlouyij0lpdx548bc@jetpack
Github tweaks for Bugzilla1.9 jid0-AWShpy08txla2QGDYvv5bed4sjs@jetpack
Mass Password Reset1.05 masspasswordreset@johnathan.nightingale
Send to Instapaperinitial.rev24 jid0-qKBqRbzCENBZCPjcqF5kJaKNuEE@jetpack
You're right that this would be very good.  In fact, Nils wrote an extension that does exactly this!  https://github.com/nmaier/about-addons-memory.  

I looked at incorporating it into about:memory a while back.  IIRC getting the extension IDs from the add-on manager is a problem -- it has a JS-only interface, but we really need to do this at the C++ level when creating the memory reporters' paths.  Nils' extension did it via post-processing of the memory reports in JS, which isn't really compatible with how about:memory works.
I think it should be simple to write an XPCOM component which exposes this to C++.  I'll see what I can do.
(In reply to Nicholas Nethercote [:njn] from comment #2)
> You're right that this would be very good.  In fact, Nils wrote an extension
> that does exactly this!  https://github.com/nmaier/about-addons-memory.  

Yeah, I need to update that thing to support current path names.

> I looked at incorporating it into about:memory a while back.  IIRC getting
> the extension IDs from the add-on manager is a problem -- it has a JS-only
> interface, but we really need to do this at the C++ level when creating the
> memory reporters' paths.  Nils' extension did it via post-processing of the
> memory reports in JS, which isn't really compatible with how about:memory
> works.

I honestly wouldn't attempt to stuff that information into the memory reports themselves.
I think about js post-processing is the way to go, that's what about:memory/about:compartments does anyway.

You'll be using an async API to get the add-on info in something that is very synchronous by nature either way. Doing this in aboutMemory.js before generating/updating the content would be the least painful solution, instead of breaking all the assumptions that reporters deliver their reports synchronously that are made throughout the code.

I can create a patch to about:compartments adding per-addon sections. I already got an idea how to do it with a minimal amount of hacks. ;)
Should I give it a try?
> Should I give it a try?

I'm most of the way through a patch to get the information (synchronously).  Give me a day or two...
Blair suggested that if we expanded the scope of bug 793139 to include add-on ID --> name mappings, we could get away without the sync DB access.

That sounds like a good approach to me...
Updated about:addons-memory to work with latest Firefox Stable - Nightly + Android and made it available on AMO, pending prelim. review:
https://addons.mozilla.org/addon/about-addons-memory/
> Should I give it a try?

Does this offer still stand?  :)  I haven't written any code that's useful if we take the approach from comment 6.

I'd still like us not to post-process the data in about:memory; instead, I'd like us to change the memory reporters' names so they're right.  I think this is better because sometimes we read memory reporters without going through about:memory.

Perhaps a good starting point would be to aggregate extensions' compartments.  Then we can (in a separate bug, or a separate patch) plug in with bug 793139 and translate their IDs.

I was thinking that we might want to aggregate some other compartments, such as those coming from omni.ja, while we're at it.  But that's up to you.

I don't know how njn feels about doing a bunch of string operations in XPCJSRuntime to figure out which add-on a compartment belongs to, but I'm fine with that hack.
> I don't know how njn feels about doing a bunch of string operations in
> XPCJSRuntime to figure out which add-on a compartment belongs to, but I'm
> fine with that hack.

Sounds ok in principle.  We already do some non-trivial stuff in that file to work out if a JS compartment belongs to a window object, and if so, what the window's URL is.
(In reply to Justin Lebar [:jlebar] from comment #8)
> > Should I give it a try?
> 
> Does this offer still stand?  :)  I haven't written any code that's useful
> if we take the approach from comment 6.

Sure.

> I'd still like us not to post-process the data in about:memory; instead, I'd
> like us to change the memory reporters' names so they're right.  I think
> this is better because sometimes we read memory reporters without going
> through about:memory.
> 
> Perhaps a good starting point would be to aggregate extensions'
> compartments.  Then we can (in a separate bug, or a separate patch) plug in
> with bug 793139 and translate their IDs.

Aggregating without any other knowledge about the add-on first does not really work.
- You cannot do it by id, as the compartment location information does not have any ids, just URIs, and not even (useful) URIs are a given.
- You cannot really aggregate by those URIs either, because add-ons are free to register whatever number of and names for chrome/resource URIs they feel like, and in particular the more complex add-ons do often register multiple chrome packages and resource substitutions. So you need to resolve chrome/resource URIs first to the underlying file or jar uri.
- The underlying file uris have a common prefix per add-on, but you don't know what it is and cannot guess it reliably (registry installed add-ons, proxy-file installed ones, *nix package manager installed ones, ...)
- If the underlying uri is a jar uri, you can get to the jar file easily, but is it a .xpi or a chrome.jar in an em:unpack add-on?
- Full stop at this point.

So at the moment the max post-processing you can do without information from the add-on manager is to resolve the URIs until you get to a final file URI that isn't pretty helpful by itself.

You could however use the info in extensions.installCache and get path (common prefix)/addon id pairs and do the final mapping to ids. I don't know how up-to-date that information in that cache is at any point, though, and if it is a good idea to use that information at all.
Blair?

From that one could either amend the existing reporters to include the id, or spit out new reports like those non-explicit compartment ones, or both.
What would you prefer?

> I was thinking that we might want to aggregate some other compartments, such
> as those coming from omni.ja, while we're at it.  But that's up to you.

Even omni.ja isn't a given. There are potentially multiple onmni.ja. The build may be jar or flat packaged. Mobile will directly use the .apk, and I have no idea about what B2G does in this regard ATM.
However, one could just assume that all system compartments not belonging to an add-on are in fact application compartments.

> I don't know how njn feels about doing a bunch of string operations in
> XPCJSRuntime to figure out which add-on a compartment belongs to, but I'm
> fine with that hack.

I'd do a "uri to id" service instead, either in js (easier but xpconnect involved) or C++. That service could cache the parsed installCache and would be useful to other code, such as window-object/image-data mapping or my about:add-ons memory, or even the unresponsive script warning.

Any other thoughts before I get into it?
> ...
> - Full stop at this point.

Okay, it definitely sounds like you are the right person to write this patch, and I was in way over my head.  :)

> From that one could either amend the existing reporters to include the id, or spit out 
> new reports like those non-explicit compartment ones, or both.
> What would you prefer?

I think ideally we'd have something like

  explicit/extensions/Adblock Plus/compartment(something relatively short, that doesn't include unnecessary information like paths or UUIDs)

Then we could rename js-non-window to something like "built-in-js", and then we could also strip the file paths from those compartments and perhaps aggregate them in a sane way (e.g. there are a bunch of sync compartments in there).  All of this is follow-up work, of course.  I'm just imagining a pie in the sky...

> I'd do a "uri to id" service instead

That sounds reasonable to me.  But see caveats above.  :)
For some prior art, you can look at the patch from bug 761950 which created a component to map URIs to addons.
Thanks. I created my own prior art (see comment #2), but will definitively have a look.
This is a first rough implementation of the "uri to id" service, which I named amIAddonInfo. Maybe amIAddonInfo can later get additional stuff, like sync mapping of ids to names and versions, etc.

Blair, do you think it is OK to implement the service as part of the existing addonManager.js integration service, or should it better go into a separate file? I for one thought it is an appropriate place.

Are there any show-stoppers with this approach? Would be good to know now before I proceed to polish up this thing and start using it to map compartments to ids.
Assignee: nobody → MaierMan
Attachment #720312 - Flags: feedback?(bmcbride)
(In reply to Nils Maier [:nmaier] from comment #10)
> You could however use the info in extensions.installCache and get path
> (common prefix)/addon id pairs and do the final mapping to ids. I don't know
> how up-to-date that information in that cache is at any point, though, and
> if it is a good idea to use that information at all.

Is guaranteed to be accurate on startup, but ONLY on startup - it doesn't get updated during runtime. Bug 731489 would fix that, but that's a decent amount of work. 

However, you only need to worry about bootstrapped extensions changing, so there's extensions.bootstrappedExtensions... but that too is currently only accurate on startup :\ Good news: I also consider that a bug, and its pretty simple to fix! Filed bug 847867, will finish up the patch for it tomorrow.
(In reply to Justin Lebar [:jlebar] from comment #11)
> I think ideally we'd have something like
> 
>   explicit/extensions/Adblock Plus/compartment(something relatively short,
> that doesn't include unnecessary information like paths or UUIDs)

IMO, using the addon name like that will make the data much harder to analyse, due to localised names and non-unique names. At the very least, it should only be in the UI.
> IMO, using the addon name like that will make the data much harder to
> analyse, due to localised names and non-unique names. At the very least, it
> should only be in the UI.

I'm not sure what you mean by "only in the UI"... about:memory is a pretty literal presentation of the underlying memory reporters.

And surely a name is easier to analyze than a UUID?  Also, about:memory isn't localised, FWIW...
Comment on attachment 720312 [details] [diff] [review]
Rough Part 1 - Implement amIAddonInfo service to map URIs to AddonIDs

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

You appear to be missing a certain new IDL file here :)

(In reply to Nils Maier [:nmaier] from comment #14)
> Blair, do you think it is OK to implement the service as part of the
> existing addonManager.js integration service, or should it better go into a
> separate file? I for one thought it is an appropriate place.

Having the API accessible via that component seems appropriate, but its mostly only used for C++ integration - the API should be available via the normal JS interface too (ie, AddonManager.jsm). Also, this implementation is very specific to just one type of add-on (XPI extensions) - other providers should be able to implement it too, just like all the other Add-ons Manager functionality. 

But its a minimal amount of juggling to make all that happen:
* Move most of this implementation to XPIProvider.jsm
* Add a function to AddonManagerInternal that resolves the URI and iterates over this.providers (optional for providers to implement, first answer wins)
* Add a wrapper function to AddonManager
* Add a wrapper function to the component in addonManager.js
* Rename amIAddonInfo to amIAddonManager, because I've been meaning to provide C++ access to a few other APIs for tools like this


And instead of the onInstalled event, you'll want to hook into loadBootstrapScope(). (But either way, you don't seem to need bug 847867, so you can ignore most of comment 15.)


> Are there any show-stoppers with this approach?

Can't think of anything.
Attachment #720312 - Flags: feedback?(bmcbride) → feedback+
(In reply to Nicholas Nethercote [:njn] from comment #17)
> And surely a name is easier to analyze than a UUID?

Example time! (All real-world examples)

Assuming this is pasted into a bug report, what extension is it referring to?
  explicit/extensions/آدبلوك بلس/compartment

Is it the same extension as this, from another bug report?
  explicit/extensions/فوق تبلیغ شکن/compartment

(Hint: There are the same.)


What about this one?
  explicit/extensions/Translate This!/compartment
There are two add-ons using this name, but with different IDs. One is on AMO, the other is malware. There are other cases where two legit add-ons use the same name but different IDs (generally one is a fork of the other).


And assuming you're writing a tool to automatically do some analysis/correlation of collected data, the code would have to jump through some hoops to determine what this is referring to (even assuming its a unique name):
  explicit/extensions/Επιλογές φόρτωσης αναζήτησης/compartment


> Also, about:memory isn't localised, FWIW...

But add-on names are.
> And assuming you're writing a tool to automatically do some analysis/correlation of 
> collected data

We're not.  We never collect about:memory from users automatically, because

 1) It's slow to generate, and
 2) It contains private information (URLs, but also fragments of JS strings and unsalted SHA1s of blobs)

In the cases when we do automated analysis, we're getting our data from telemetry.

Anyway, it sounds like we should include both the add-on name and ID in the about:memory dump for disambiguation purposes.
> In the cases when we do automated analysis, we're getting our data from telemetry.

Telemetry does use some memory reporters, but has a separate way of reporting which add-ons are installed.  The memory reporters that would generate the add-on memory usage data are in the class of slow ones, so I don't think we'd consider running them automatically, in their current state.
Alrighty then, cleaned up the patch, addressed the awesome feedback, in particular from comment #18, and added some tests.

It should be noted it would be a good base to fix bug 793139 as well /methinks...
Attachment #720312 - Attachment is obsolete: true
Attachment #721852 - Flags: review?(bmcbride)
Comment on attachment 721852 [details] [diff] [review]
Part 1 - amIAddonManager: Map URIs to AddonIDs

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

Looking good :) Only non-nit is just about keeping with API convention by not throwing exceptions when no match is found.

General notes:
* Name all anonymous functions

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +1455,5 @@
>      this.getInstallsByTypes(null, aCallback);
>    },
>  
>    /**
> +   * Synchronously map a URI to the corresponding Addon ID.

This sounds a bit ambiguous, for anyone looking at this for the first time. What kind of URI?

@@ +1464,5 @@
> +   *         URI cannot be mapped
> +   * @return string containing the Addon ID
> +   * @see    amIAddonManager.mapURIToAddonID
> +   */
> +  mapURIToAddonID: function(aURI) {

The existing convention for this type of API is for providers to return null when they don't have a matching result to return. Similarly, this method should do the same.

And conveniently, callProvider() handles most of the machinery for all that - which removes most of the code here :)

(I'm fine with addonManager.js throwing when nothing is found if you think that's appropriate - there's no established convention there, and it doesn't need to copy AddonManager.jsm since they can never have the same interface anyway.)

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3489,5 @@
> +  mapURIToAddonID: function(aURI) {
> +    this._ensureURIMappings();
> +    let resolved = this._resolveURIToFile(aURI).spec;
> +    for (let [id, spec] in Iterator(this._uriMappings)) {
> +      // str.lastIndexOf(str2, 0) === 0 is the str.startswith(str2) for JS

JS actually has str.startsWith() these days :)

@@ +3862,5 @@
>      let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>                   createInstance(Ci.mozIJSSubScriptLoader);
>  
> +    // Add a mapping for XPIProvider.mapURIToAddonID
> +    this._addURIMapping(aId, aFile);

Hmm, dictionaries are loaded through this function too. They can't contain any JS, instead we load our own bootstrap code (see below) to make them restartless - so a dictionary's sandbox isn't representative of the dictionary itself. Still, I can't see any harm in including a mapping for dictionaries - maybe it could be helpful, dunno. Just wanted to make sure that's known - up to you whether to include it.
Attachment #721852 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #23)
> General notes:
> * Name all anonymous functions

What anonymous functions? The only ones in the code are those property functions, and those get their names auto-generated. I was under the impression that auto-generated names should be preferred by now, as they usually are conveying more information while shrinking in-file code size.

> >    /**
> > +   * Synchronously map a URI to the corresponding Addon ID.
> 
> This sounds a bit ambiguous, for anyone looking at this for the first time.
> What kind of URI?

You do mean to write nsIURI here, right?
Or are you after stating what kind of URI-schemes are accepted? This would be impossible to know as providers are free to map whatever URI they fell like.

> JS actually has str.startsWith() these days :)

Eek. How did I miss that :p

> Hmm, dictionaries are loaded through this function too. They can't contain
> any JS, instead we load our own bootstrap code (see below) to make them
> restartless - so a dictionary's sandbox isn't representative of the
> dictionary itself. Still, I can't see any harm in including a mapping for
> dictionaries - maybe it could be helpful, dunno. Just wanted to make sure
> that's known - up to you whether to include it.

Well, it doesn't harm really I guess. And it is an add-on after all.
(In reply to Nils Maier [:nmaier] from comment #24)
> (In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from
> comment #23)
> > General notes:
> > * Name all anonymous functions
> 
> What anonymous functions? The only ones in the code are those property
> functions, and those get their names auto-generated. 

Yea, those. The auto-generated names are good for stack traces, but obviously don't help at all when it comes to navigating through a huge file :\ With consistent naming I know exactly what text to search for to get me to a specific method on a specific object.

> You do mean to write nsIURI here, right?
> Or are you after stating what kind of URI-schemes are accepted? This would
> be impossible to know as providers are free to map whatever URI they fell
> like.

No, I mean it should state that the URI is expected to eventually resolve to some file on disk that is belonging to some add-on. The way its worded right now, someone could half expect to be able to pass in the URL of the add-on's homepage :)
Status: NEW → ASSIGNED
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #25)
> > You do mean to write nsIURI here, right?
> > Or are you after stating what kind of URI-schemes are accepted? This would
> > be impossible to know as providers are free to map whatever URI they fell
> > like.
> 
> No, I mean it should state that the URI is expected to eventually resolve to
> some file on disk that is belonging to some add-on. The way its worded right
> now, someone could half expect to be able to pass in the URL of the add-on's
> homepage :)

I see your point, but I have no real idea how to significantly improve that...
Alright, I got something working...
I implemented another MultiMemoryReporter in XPCJSRuntime.cpp - based on the existing compartments/ one - that dumps out the paths like addons/@id@/@compartment-name@, so that I could get a feel for what is required, and hacked aboutMemory.js (about:compartments) to actually collect and display these reports.
I also wrote a nice little helper function that will map the compartment location to an URI if the location is not an URI already.
Like in: somesandbox (from: uri1 -> uri2 -> uri3:200)
(You'll want uri3, of course, or if that doesn't work uri2, etc. :p)

Result output so far with a bunch of regular, bootstrapped and jetpack add-ons installed, either popular ones or mine or both:
https://gist.github.com/nmaier/5111547

I didn't make it filter out the add-on compartments from the general list, so the duplicates are by design for now ;)

Let me finish stuff that up tomorrow when I'm less tired, and I'll post a revised amIAddonManager patch for review and this work for further discussion.
You folks may in the meantime think about where and how to display the add-on info exactly.


(In reply to Nils Maier [:nmaier] from comment #26)
> > No, I mean it should state that the URI is expected to eventually resolve to
> > some file on disk that is belonging to some add-on. The way its worded right
> > now, someone could half expect to be able to pass in the URL of the add-on's
> > homepage :)
> 
> I see your point, but I have no real idea how to significantly improve
> that...

Nah, still not really an idea... Can I leave it as is?
(In reply to Nils Maier [:nmaier] from comment #26)
> > No, I mean it should state that the URI is expected to eventually resolve to
> > some file on disk that is belonging to some add-on. The way its worded right
> > now, someone could half expect to be able to pass in the URL of the add-on's
> > homepage :)
> 
> I see your point, but I have no real idea how to significantly improve
> that...

How about:

/**
 * Synchronously map a URI that resolves to a local file belonging to an
 * add-on, to that add-on's ID.
 */

(Not my greatest literary achievement, but it gets the point across.)
(In reply to Nils Maier [:nmaier] from comment #27)
> You folks may in the meantime think about where and how to display the
> add-on info exactly.

I originally pictured something like:

extensions
├───addon1@whatever.com
│   ├───resource://addon1/whatever.jsm
│   │   ├───objects
│   │   ├───shapes
│   │   └───strings
│   └───chrome://addon1/whatever.js
│       ├───objects
│       ├───shapes
│       └───strings
├───addon2@something.com
│   ├───resource://addon2/something.jsm
│   │   ├───objects
│   │   ├───shapes
│   │   └───strings
│   └───chrome://addon2/something.js
│       ├───objects
│       ├───shapes
│       └───strings


(Not sure how well that fits into the existing about:memory, though that UI just frustrates me in general.)
> (Not sure how well that fits into the existing about:memory, though that UI
> just frustrates me in general.)

How so?
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #28)
> How about:
> 
> /**
>  * Synchronously map a URI that resolves to a local file belonging to an
>  * add-on, to that add-on's ID.
>  */
> 
> (Not my greatest literary achievement, but it gets the point across.)

That isn't necessarily true, however. A custom provider may also map remote stuff...

How about just adding another sentence like:
Mappable URIs are limited to in-application resources belonging to the add-on, such as Javascript compartments, XUL windows, XBL bindings, etc. but do not include URIs from meta data, such as the add-on homepage.
(In reply to Nils Maier [:nmaier] from comment #31)
> How about just adding another sentence like:
> Mappable URIs are limited to in-application resources belonging to the
> add-on, such as Javascript compartments, XUL windows, XBL bindings, etc. but
> do not include URIs from meta data, such as the add-on homepage.

Sounds good to me!
Ok, I changed the API to return null, except for the service, added function names, switched to callProvider, fixed up the comments and used the newly discovered String.startsWith().

New tests pass.
Attachment #721852 - Attachment is obsolete: true
Attachment #722846 - Flags: review?(bmcbride)
This patch adds a new non-explicit memory reporter that will map compartments to addon ids.
It started as a copy of the non-explicit compartments reporter.
Also modified CompartmentPrivate to not throw away the location URI when GetLocation() is accessed and also adds some location parsing function GetCompartmentLocationURI().

aboutMemory.js is modified to generate an Add-on Compartments section based on this reporter, while also using AddonManager to map the id to the name.

Now, it needs to be decided where to display add-on info and how.
I think that displaying something like this patch demonstrates in about:compartments would be a good start, but also think it would be great if about:memory did display something along the lines of what Blair proposed in comment #29
Attachment #722866 - Flags: feedback?(justin.lebar+bug)
We use the memory reporters outside of about:memory.  For example, AWSY captures the memory reporters.

Any about:memory post-processing has to be duplicated by non-about:memory consumers.  Even relatively simple post-processing steps such as computing heap-unclassified have given us headaches as recently as this week.

Therefore I am strongly in favor of requiring minimal (ideally zero) additional post-processing of the memory reporters.  Is there some reason we can't modify the existing memory explicit memory reporter to give the paths we want?
Comment on attachment 722846 [details] [diff] [review]
Part 1 - amIAddonManager: Map URIs to AddonIDs

Turns out this causes a lot of logging to the Error Console when using the service, because JS Component expections are always reported (except for QI).

Better change the amIAddonManager signature to return some bool :p
Attachment #722846 - Flags: review?(bmcbride)
(In reply to Justin Lebar [:jlebar] (PTO 3/11-3/15, then GMT+9 until 3/24) from comment #35)
> Therefore I am strongly in favor of requiring minimal (ideally zero)
> additional post-processing of the memory reporters.  Is there some reason we
> can't modify the existing memory explicit memory reporter to give the paths
> we want?

Depends on what exactly those paths should contain... The add-on ID is a no brainer... The example memory reporter from the patch already does that.

Getting the corresponding name for the ID is nasty though, as there is currently only the asynchronous AddonManager API, and you don't really want to use become async or spin your own event loop ;)

If you want names in reporters themselves, I'd do that in a follow up bug.
If it's OK to just have add-on IDs and google the names or whatever when you get raw reports, then we're already there.
I think we should have both the ID and the name in the memory reporter (I tried to say this in comment 20, but I realize now this was kind of oblique; sorry).  A goal of memory reports is for them to be easy to use, reducing the energy required for people to figure out what's a bug.  I think having to google all the IDs of your memory reporters doesn't satisfy that goal, and post-processing in about:memory to get the names is undesirable for other reasons.

I'm happy to get the names in there as a follow-up.
> If it's OK to just have add-on IDs and google the names or whatever when you get raw reports, then 
> we're already there.

Do you mean we're already there with this patch, or without it?

To be clear, the issues I'd like to solve with the existing extension memory reporters are that
 
 a) They're not aggregated by add-on, and
 b) You have to Google the add-on's ID.

I'm happy to tackle these two separately.
Comment on attachment 722866 [details] [diff] [review]
Rough Part 2: Report add-on compartments in about:compartments

Per above, I don't think this is the approach we want to take.  If we don't want to translate IDs to names in this bug, I think we should just to modify the existing explicit/ reporter so that addons' compartments are aggregated.
Attachment #722866 - Flags: feedback?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] (PTO 3/11-3/15, then GMT+9 until 3/24) from comment #39)
> > If it's OK to just have add-on IDs and google the names or whatever when you get raw reports, then 
> > we're already there.
> 
> Do you mean we're already there with this patch, or without it?
> To be clear, the issues I'd like to solve with the existing extension memory
> reporters are that
>  
>  a) They're not aggregated by add-on, and
>  b) You have to Google the add-on's ID.

Actually, you want even more. Without patches there aren't even add-on ids in reports ;)

- Part 1: Mapping URIs to Add-on IDs in general. But reports remain the same

- Part 2: a) Put out memory reports containing addon/@id@/@compartment@ paths as a demo. But what reporters should finally include the add-on id and how exactly this should look like is still up for discussion. I'm fine with modifying any existing reporters to include the add-on id instead or as well, as long as somebody comes up with a good idea how these new paths should look like.

- Part 2: Somewhat b) via post-processing in about:addons.

Another issue Blair already pointed out is that while about:memory and memory reporters are not localized, addon names are. So you would not only need an API to sync get the addon name, but the API must support to return the (canonical) en-US name if any.
Changed the amIAddonManager signature to:
boolean mapURIToAddonID(in nsIURI aURI, out AUTF8String aID);
This is pretty ugly but required to not have XPConnect litter the Error Console.
Attachment #722846 - Attachment is obsolete: true
Attachment #722887 - Flags: review?(bmcbride)
Updated to reflect changes in Part1.
For reference.
Attachment #722866 - Attachment is obsolete: true
There's some sort of google-able ID in most of the memory reports (see attachment 719198 [details]).  For example

├────2,529,016 B (00.41%) -- compartment([System Principal], resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/geckoprofiler/lib/main.js (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///Users/jlebar/Library/Application%20Support/Firefox/Profiles/xxxxxx.default/extensions/jid0-edalmuivkozlouyij0lpdx548bc@jetpack.xpi!/bootstrap.js -> resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/toolkit/loader.js:143))

I'd been calling "jid0-edalmuivkozlouyij0lpdx548bc" the "addon ID", but that's probably wrong.  What should I call these IDs that I currently see in the memory reports?

> I'm fine with modifying any existing reporters to include the add-on id instead or as 
> well, as long as somebody comes up with a good idea how these new paths should look 
> like.

I was thinking that, for every compartment associated with addon X, we'd have memory reporters with the prefix

  explicit/addons/X/compartment(...)/

X can be the add-on's ID, or a string constructed from the ID and name, or whatever.  All of the existing per-compartment memory reporters would be underneath this path.

Is that still ambiguous, or do you think we should do it differently somehow?
Nils, it's very helpful if you include example about:memory output when you attach a patch that affects it :)
Comment on attachment 722889 [details] [diff] [review]
Rough Part 2: Report add-on compartments in about:compartments

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +1760,5 @@
>      assertInput(aAmount === 1,          "bad compartments amount");
>      assertInput(aDescription === "",    "bad compartments description");
>  
> +    let c = new Tracker(unsafeNames[2]);
> +    c.isSystemCompartment = isSystemCompartment;

The coding style guidelines say that JS properties should begin with '_'.
> I was thinking that, for every compartment associated with addon X, we'd
> have memory reporters with the prefix
> 
>   explicit/addons/X/compartment(...)/

That's also what I was thinking.
(In reply to Justin Lebar [:jlebar] (PTO 3/11-3/15, then GMT+9 until 3/24) from comment #44)
> There's some sort of google-able ID in most of the memory reports (see
> attachment 719198 [details]).  For example
> 
> ├────2,529,016 B (00.41%) -- compartment([System Principal],
> resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/geckoprofiler/lib/
> main.js (from: resource://gre/modules/XPIProvider.jsm ->
> jar:file:///Users/jlebar/Library/Application%20Support/Firefox/Profiles/
> xxxxxx.default/extensions/jid0-edalmuivkozlouyij0lpdx548bc@jetpack.xpi!/
> bootstrap.js ->
> resource://jid0-edalmuivkozlouyij0lpdx548bc-at-jetpack/addon-sdk/lib/toolkit/
> loader.js:143))
> 
> I'd been calling "jid0-edalmuivkozlouyij0lpdx548bc" the "addon ID", but
> that's probably wrong.  What should I call these IDs that I currently see in
> the memory reports?

That's just a resource substituation (or chrome package name for chrome uris). These are free-from and the SDK folks did something sensible and kinda included the id. But authors may still override that IIRC. And it is SDK only.
For example, DownThemAll! will have chrome://dta-modules/content/glue.jsm and I'm not going to change that anytime soon as other add-ons rely on existing URIs to extend DownThemAll! (how cool is that the platform allows to create add-ons for add-ons ;)
Anyway, see my gist for some more examples of non-id-ish urls and how those are still mapped correctly with the patches.


> I was thinking that, for every compartment associated with addon X, we'd
> have memory reporters with the prefix
> 
>   explicit/addons/X/compartment(...)/
> 
> X can be the add-on's ID, or a string constructed from the ID and name, or
> whatever.  All of the existing per-compartment memory reporters would be
> underneath this path.
> 
> Is that still ambiguous, or do you think we should do it differently somehow?

That would be doable, but you'd have to remove these compartments from the regular paths to avoid double counting, i.e.:
explicit/js-non-window/compartments/non-window-global
explicit/window-objects/.../js/
Hence, I don't really know if this is a sensible thing to do.

I'd still prefer some non-explicit listing like comment #29, having the usual explicit memory reporter just dump out the non-explicit, add-on id prefixed paths as well, instead of modifying the explicit paths.
I don't know enough about AWSY and telemetry to know if this would cause any problems, though.

Also, I think that the about:compartments stuff from Rough Part 2, post-processing the name in or not, is something we should do *as well* (not necessarily the actual implementation, but the spirit). While it likely does not help automated tools operating on raw collected reports, it will help developers and probably users and bug reports where users dump their about:compartments.
I consulted about:compartments countless times before to check if my new restartless add-on shutdown code really made the compartments go away.
Attached file Output after Rought Part 2 (obsolete) — 
(In reply to Nicholas Nethercote [:njn] from comment #45)
> Nils, it's very helpful if you include example about:memory output when you
> attach a patch that affects it :)

Here you are, after the latest changes. But it still is the same as the gist I posted before.

about:memory output wasn't changed.
> That would be doable, but you'd have to remove these compartments from the
> regular paths to avoid double counting, i.e.:
> explicit/js-non-window/compartments/non-window-global
> explicit/window-objects/.../js/
> Hence, I don't really know if this is a sensible thing to do.

We already do something like this for the js-non-window/window-objects split.  We pass in a table containing info about window objects, and for each compartment, we look it up in that table.  If it's present, we created a window-objects/... path using the URI in the table.  Otherwise, we create a js-non-window/... path.
(In reply to Nicholas Nethercote [:njn] from comment #46)
> The coding style guidelines say that JS properties should begin with '_'.

For private members, which this is not really. ;)
But this is just an oversight from creating that Tracker thing, not an intensional change.

(In reply to Nicholas Nethercote [:njn] from comment #50)
> > That would be doable, but you'd have to remove these compartments from the
> > regular paths to avoid double counting, i.e.:
> > explicit/js-non-window/compartments/non-window-global
> > explicit/window-objects/.../js/
> > Hence, I don't really know if this is a sensible thing to do.
> 
> We already do something like this for the js-non-window/window-objects
> split.  We pass in a table containing info about window objects, and for
> each compartment, we look it up in that table.  If it's present, we created
> a window-objects/... path using the URI in the table.  Otherwise, we create
> a js-non-window/... path.

I'm not worrying about the processing, but about taking away what might be useful information (window/non-window?, which window?).
Example: Let's say an add-on framescript blows up only on facebook. The facebook bit would be lost with explicit/addons/X/compartment(...)/
(In reply to Justin Lebar [:jlebar] (PTO 3/11-3/15, then GMT+9 until 3/24) from comment #39)
>  b) You have to Google the add-on's ID.

Discussion here has been really focused on whether/how to include the add-on name in the path - is there any reason why we can't just add a separate field to memory reporters? Something like a property-bag, and anything relating to add-ons puts the add-on name in the 'Add-on name' key. Then it can be surfaced in about:memory however we want.

(In reply to Nicholas Nethercote [:njn] from comment #30)
> > (Not sure how well that fits into the existing about:memory, though that UI
> > just frustrates me in general.)
> 
> How so?

Happy to discuss this on IRC, since its kinda tangential to this bug.
Comment on attachment 722887 [details] [diff] [review]
Part 1 - amIAddonManager: Map URIs to AddonIDs

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

Good to go :)

::: toolkit/mozapps/extensions/amIAddonManager.idl
@@ +14,5 @@
> +interface amIAddonManager : nsISupports
> +{
> +  /**
> +   * Synchronously map a URI to the corresponding Addon ID.
> +   * 

Nit: Trailing whitespace! Oh noes!!!!
Attachment #722887 - Flags: review?(bmcbride) → review+
No longer depends on: 847867
> is there any reason why we can't just add a separate field to memory reporters? Something like a
> property-bag, and anything relating to add-ons puts the add-on name in the 'Add-on name' key. Then 
> it can be surfaced in about:memory however we want.

This is better than some other ways of associating arbitrary data with an add-on, but it still has the problem that every consumer of memory reporters which wants to surface this data has to do it specially.  That means that when we add or change a property, we have to change every consumer.  I'd prefer to avoid that if we can.
I should say, every /such/ consumer.  Which would at least be our diff tool, in this case.
How about this then:
The code will get the add-on id, and if there is any, insert an addons/@id@/ prefix behind explicit/, effectively aggregating add-ons.

This got more messy than I wanted. At the point where the code processes compartments (phase 1) I cannot actually call amIAddonManager, as the js heap is not idle and hence the call would assert/crash. So only get the location URI at this point and try to map it later (phase 2).

I'd like to do the same aggregation for window objects and workers.

Keep in mind that this is just a WIP. Right now it also has a notice perf impact, although I'm not sure how much of this is because of --enable-debug
Attachment #722889 - Attachment is obsolete: true
Attachment #724129 - Flags: feedback?(justin.lebar+bug)

        Attachment #722902 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 724129 [details] [diff] [review]
Rough Part 2: Prefix js compartment reports

njn, would you mind looking at this so I can focus on some b2g issues?

        Attachment #724129 -
        Flags: feedback?(justin.lebar+bug) → feedback?(n.nethercote)

    
    

    
  
Nils, I want to try this out in a build but the patches have bit-rotted.  Would you mind updating them?  For the first patch, XPIDL_SOURCES variables have moved from Makefile.in files to moz.build files.  Thanks!

    
    

    
  


  
Un-bitrotted.

        Attachment #722887 -
        Attachment is obsolete: true

    
    

    
  


  
Un-bitrotted.

The zones stuff pulled some things out of the compartments and into the top-level zone that now cannot attributed to the compartment anymore, such as string-chars.
This contributes to addon memory usage being underreported a bit.

Also, I still didn't tackle workers and window objects. Still waiting on what (this?) approach to take...

I could spin a try-build if anybody is interested?

        Attachment #724129 -
        Attachment is obsolete: true

        Attachment #724131 -
        Attachment is obsolete: true

        Attachment #724129 -
        Flags: feedback?(n.nethercote)

        Attachment #727319 -
        Flags: feedback?(n.nethercote)

    
    

    
  
Comment on attachment 724129 [details] [diff] [review]
Rough Part 2: Prefix js compartment reports

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

Sorry I took so long with this.  It looks pretty good!

- The output is good.  I wonder if "addons/" should be "extensions/"... if we're being pedantic, "add-ons" include plugins like Flash, right?  But maybe that sense of "addons" is common enough to be ok.  If so, it should probably bee "add-ons" instead of "addons" :)

- I like what you've done with CompartmentStatsExtras.

- The parsing of the sandbox names is grotty.  More about that below.

- The fact that the location-getting is in stage 1 and the id-getting is in stage 2 doesn't worry me much.

- Doing it for window objects and workers would be good.  It's a shame that zones made the string blame game impossible;  there's a trade-off between reportability and compactness :/


Here's an example:

> ├───10,946,560 B (10.84%) -- addons
> │   ├───5,340,408 B (05.29%) -- memchaser@quality.mozilla.org
> │   │   ├──4,787,856 B (04.74%) ++ js-non-window/zones/zone(7f98fa341000)
> │   │   └────552,552 B (00.55%) -- window-objects
> │   │        ├──276,640 B (00.27%) ++ top(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/widget/widget.html, id=18)/active/window(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/widget/widget.html)/js-compartment(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/widget/widget.html)
> │   │        └──275,912 B (00.27%) ++ top(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/panel/context.html, id=23)/active/window(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/panel/context.html)/js-compartment(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/panel/context.html)
> │   ├───3,987,304 B (03.95%) ++ jid1-LW6w2lZXp4PutQ@jetpack/js-non-window/zones/zone(7f98fa341000)
> │   ├───1,198,856 B (01.19%) ++ enter.selects@agadak.net/js-non-window/zones/zone(7f98fa341000)
> │   ├─────157,360 B (00.16%) ++ picpasso@sdzinvest.de/js-non-window/zones/zone(7f98fa341000)/compartment([System Principal], chrome://picpasso/content/browser.xul)
> │   ├──────87,464 B (00.09%) ++ domfuzz@squarefree.com/js-non-window/zones/zone(7f98fa341000)/compartment([System Principal], jar:file:///home/njn/.mozilla/firefox/esssu3vp.addons/extensions/domfuzz@squarefree.com.xpi!/components/domfuzzhelperobserver.js)
> │   ├──────83,840 B (00.08%) ++ restartless.restart@erikvold.com/js-non-window/zones/zone(7f98fa341000)/compartment([System Principal], jar:file:///home/njn/.mozilla/firefox/esssu3vp.addons/extensions/restartless.restart@erikvold.com.xpi!/bootstrap.js (from: resource://gre/modules/XPIProvider.jsm:3862))
> │   ├──────62,416 B (00.06%) ++ {64161300-e22b-11db-8314-0800200c9a66}/js-non-window/zones/zone(7f98fa341000)/compartment([System Principal], chrome://speeddial/content/speeddialOverlay.xul)
> │   └──────28,912 B (00.03%) ++ {4fa0d965-cd01-4d08-9bdb-0d8c47cfd5d8}/js-non-window/zones/zone(7f98fa341000)/compartment([System Principal], chrome://smartsearch/content/smartsearchOverlay.xul)

Some add-ons have an id that's just hex crap.  Is that because the add-on fails to provide a human-readable id?

::: dom/workers/WorkerPrivate.cpp
@@ +1430,5 @@
>      // aCompartmentStats->{extra1,extra2} are char pointers.
>  
>      // This is the |cJSPathPrefix|.  Each worker has exactly two compartments:
>      // one for atoms, and one for everything else.
>      nsAutoCString cJSPathPrefix(mRtPath);

|cJSPathPrefix| is dead now.  The comment needs updating -- change |cJSPathPrefix| to |JSPathPrefix|.

@@ +1439,4 @@
>  
>      // This should never be used when reporting with workers (hence the "?!").
> +    extras->DOMPathPrefix.AssignLiteral("explicit/workers/?!/");
> +    aCompartmentStats->extra1 = extras;

I think you remove the |extra2| member now?  And rename |extra1| as |extra|.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1398,5 @@
>      }
>  }
>  
> +static 
> +bool GetCompartmentLocationURIHelper(nsIURI** uri, const nsACString& uristr)

It's hard to tell what this function does.  Is |uri| an outparam?  If so, it should be the last parameter.  A brief comment would also help.

Also, there appears to be a bunch of trailing whitespace in the patch.  Please remove.

@@ +1417,5 @@
> +    return true;
> +}
> +
> +static 
> +bool GetCompartmentLocationURI(JSCompartment* c, nsIURI** uri) {

Nit: Opening brace on its own line.

@@ +1422,5 @@
> +    CompartmentPrivate *compartmentPrivate = GetCompartmentPrivate(c);
> +    if (!compartmentPrivate)
> +        return false;
> +
> +    // Already stored an URI?

s/an/a/

@@ +1426,5 @@
> +    // Already stored an URI?
> +    if (compartmentPrivate->GetLocationURI(uri))
> +        return true;
> +
> +    // Need to parse the URI

Nit: I like full stops at the end of comment sentences... here and in numerous other places :)

@@ +1436,5 @@
> +    int32_t idx = location.Find(NS_LITERAL_CSTRING("(from:"));
> +    if (idx < 0)
> +        return GetCompartmentLocationURIHelper(uri, location);
> +
> +    // Special Sandbox handling, for getting insane fast

I don't understand what "getting insane fast" means.

@@ +1440,5 @@
> +    // Special Sandbox handling, for getting insane fast
> +    // Maybe either "some name (from: ownerURI:lineno)"
> +    // or "some name (from: ownerURI -> ownerURI -> ownerURI ...:lineno)"
> +    // or "realURI (from: ownerURI:lineno)"
> +    // or "realURI (from: ownerURI -> ownerURI -> ownerURI ...:lineno)"

Make that a bulleted list, e.g.:

// One of the following:
// - "some name (from: ownerURI:lineno)"
// - ...
// First try to parse realURI...

It'd also be great if the comment explained what we are extracting in each of these cases.  Working that out from the code is difficult.  (I think it's the rightmost ownerURI?)


Having said that, this parsing is a bit grotty.  We have control over paths used for sandboxes.  Could we somehow make them more regular in a way that makes this step simpler?  For example, we always write compartment names as "compartment(...)".  Maybe we could do something similar with the URIs we're extracting.

@@ +1628,5 @@
> +        if (NS_SUCCEEDED(addonManager->MapURIToAddonID(extras.Location,
> +                                                       addonId, &ok))
> +            && ok) {
> +            // Insert the add-on id as "addons/@id@/" behind "explicit/" to
> +            // aggregate add-on compartments

I found "behind" confusing... "after" would be better.

@@ +1631,5 @@
> +            // Insert the add-on id as "addons/@id@/" behind "explicit/" to
> +            // aggregate add-on compartments
> +            addonId.Insert(NS_LITERAL_CSTRING("addons/"), 0);
> +            addonId += "/";
> +            cJSPathPrefix.Insert(addonId, 9);

Maybe do |size_t offset = strlen("explicit/")| instead of hardcoding |9|.  Or even |size_t explicitLen = 0;| :)

@@ +1933,5 @@
>      size_t gcTotal = 0;
>      for (size_t i = 0; i < rtStats.compartmentStatsVector.length(); i++) {
>          JS::CompartmentStats cStats = rtStats.compartmentStatsVector[i];
> +        xpc::CompartmentStatsExtras *extras
> +            = static_cast<xpc::CompartmentStatsExtras*>(cStats.extra1);

Nit: put the '=' at the end of the preceding line.

::: js/xpconnect/src/xpcpublic.h
@@ +377,5 @@
>  DOM_DefineQuickStubs(JSContext *cx, JSObject *proto, uint32_t flags,
>                       uint32_t interfaceCount, const nsIID **interfaceArray);
>  
> +// ReportJSRuntimeExplicitTreeStats will expect this in the extra1 member
> +// of JS::<CompartmentStats

Truncated comment?

@@ +382,5 @@
> +class CompartmentStatsExtras {
> +public:
> +    nsAutoCString JSPathPrefix;
> +    nsAutoCString DOMPathPrefix;
> +    nsCOMPtr<nsIURI> Location; // May be undefined

Undefined, or null?  The latter seems preferable.

        Attachment #724129 -
        Flags: feedback+

    
    

    
  
Comment on attachment 727319 [details] [diff] [review]
Part2 - Prefix js compartment reports

Hmm, somehow I looked at the older "Rough part 2" bug.  Hopefully not much changed between it and the newer one.

        Attachment #727319 -
        Flags: feedback?(n.nethercote) → feedback+

    
    

    
  
(In reply to Nicholas Nethercote [:njn] from comment #62)
> - The output is good.  I wonder if "addons/" should be "extensions/"... if
> we're being pedantic, "add-ons" include plugins like Flash, right?  But
> maybe that sense of "addons" is common enough to be ok.  If so, it should
> probably bee "add-ons" instead of "addons" :)

I'd prefer "add-ons". "extensions" isn't used that much these days in mozilla properties and with users as far as I can tell.

> Some add-ons have an id that's just hex crap.  Is that because the add-on
> fails to provide a human-readable id?

An add-on only has one id (install.rdf em:id), human-readable or not.
It used to be with Firefox 1 and below that ids needed to be a GUID. Now GUIDs and the at-form (something@whatever) are supported.
https://developer.mozilla.org/en-US/docs/Install_Manifests#id
Lots of add-ons that have been around for a very long time hence use a GUID. Switching ids (no matter what form) is not supported, as it breaks updates, leads to multiple simultaneously installed copies of an add-on and so on.
So we're stuck with GUIDs instead of the potentially more readable at-form.
Also, the add-on SDK, while using the at-form, will generate those uuid-like random jetpack ids by default, although authors can use their own ids. (SDK program ids become install.rdf em:id-s, and hence cannot be changed after the initial release)
https://addons.mozilla.org/en-US/developers/docs/sdk/latest/dev-guide/guides/program-id.html

Fun fact: All those mozilla applications also use a GUID. https://addons.mozilla.org/en-US/firefox/pages/appversions/

As already discussed, one could use the addon manager to map the id to a human readable name, but there is no non-localized, XPCOM-accessible, synchronous API to do so at the moment.


> @@ +1439,4 @@
> >  
> >      // This should never be used when reporting with workers (hence the "?!").
> > +    extras->DOMPathPrefix.AssignLiteral("explicit/workers/?!/");
> > +    aCompartmentStats->extra1 = extras;
> 
> I think you remove the |extra2| member now?  And rename |extra1| as |extra|.
> 

I thought about that, too, but didn't really want to break the public js API.
But if that's ok...


> @@ +1440,5 @@
> > +    // Special Sandbox handling, for getting insane fast
> > +    // Maybe either "some name (from: ownerURI:lineno)"
> > +    // or "some name (from: ownerURI -> ownerURI -> ownerURI ...:lineno)"
> > +    // or "realURI (from: ownerURI:lineno)"
> > +    // or "realURI (from: ownerURI -> ownerURI -> ownerURI ...:lineno)"
> 
> Make that a bulleted list, e.g.:
> 
> // One of the following:
> // - "some name (from: ownerURI:lineno)"
> // - ...
> // First try to parse realURI...
> 
> It'd also be great if the comment explained what we are extracting in each
> of these cases.  Working that out from the code is difficult.  (I think it's
> the rightmost ownerURI?)
> 
> 
> Having said that, this parsing is a bit grotty.  We have control over paths
> used for sandboxes.

It's not strictly Sandboxes, but all users of SetLocationForGlobal(JSObject *global, const nsACString& location) that potentially put non-URI strings as the location.
We could kill that API and only have SetLocationForGlobal(JSObject *global, nsIURI *locationURI) present. It would then be the responsibility of the caller to parse the URI from Sandboxes and such... But that is just shifting the burden and may lead to code duplication and wasted CPU should those location URIs never get accessed.

When it comes to sandboxes, we only have control to some extent actually at the moment.
The sandbox name is constructed from a caller provided name and js stack frame file name, which in turn also is caller provided (somewhat).

We have multiple things at play here:
- The piece of code that actually assembles the location for sandbox compartments:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#3645
- Code that creates those " -> " chains, e.g. Cu.Sandbox callers, Cu.evalInSandbox callers and
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSSubScriptLoader.cpp#285
- Cu.evalInSandbox that accepts basically a free form filename that will end up as the js stack frame file name:
https://developer.mozilla.org/en-US/docs/Components.utils.evalInSandbox#Optional_Arguments

The "->" chains are somewhat relied upon by a lot of code, such as the add-on SDK, devtools (profiler), etc.

> Could we somehow make them more regular in a way that
> makes this step simpler?  For example, we always write compartment names as
> "compartment(...)".  Maybe we could do something similar with the URIs we're
> extracting.

Well, these names already have a certain form:
"<sandboxName> (from: <js stack filename>:<lineno>)"
But one needs to consider a couple of things:
- sandboxName may contain a valid URI, which should best be used then. E.g. all bootstrap.js compartments of restartless add-ons will have the sandboxName set to the uri of the bootstrap script.
- However, if sandboxName is not an URI, e.g. anonymous sandboxes and such, then one has to look into js stack file name.
- js stack file name, is, as mentioned before somewhat free from as well and may or may not be a "->" chain

Hence my "insane" code does the following:
Try to parse sandboxName. E.g.
"jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/0svgadk4.default/extensions/uriloader@pdf.js.xpi!/bootstrap.js (from: resource://gre/modules/XPIProvider.jsm:3702)"
-> jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/0svgadk4.default/extensions/uriloader@pdf.js.xpi!/bootstrap.js
We don't want the XPIProvider.jsm URI here.

If that fails, try to parse js stack filename, which can be actually a "->" chain of free form items. We're interested in rightmost chain item that is actually an URI. E.g.
"[anonymous sandbox] (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/0svgadk4.default/extensions/jid1-obqnuXXRROigLA@jetpack.xpi!/bootstrap.js:202)"
-> jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/0svgadk4.default/extensions/jid1-obqnuXXRROigLA@jetpack.xpi!/bootstrap.js:202
Or:
"[anonymous sandbox] (from: resource://gre/modules/XPIProvider.jsm -> jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/0svgadk4.default/extensions/jid1-obqnuXXRROigLA@jetpack.xpi!/bootstrap.js -> mysandboxscript.js:1)"
-> jar:file:///Users/maierman/Library/Application%20Support/Firefox/Profiles/0svgadk4.default/extensions/jid1-obqnuXXRROigLA@jetpack.xpi!/bootstrap.js

I should probably state a BNF for all that. :p


To make sandbox things simpler, one would have to refine Sandbox APIs to only accept actual URIs in various places, that is all of the following:
- Either make sandboxName only accept URIs or abandon the concept entirely. But both would break a lot of existing add-ons incl. all SDK add-ons.
- Kill those "->" chains. But these chains might have a purpose and contain some interesting information, and when generated by mozJSSubScriptLoader apparently also fix some kind of security issue, and a lot of core and maybe add-on code relies on their presence.
- Make the optional evalInSandbox filename argument only accept valid URIs, but again, this would break lots of code.

So I prefer the "grotty" approach (after a bit of polishing) as hackish, loosely-coupled and string-based as it might be.

    
    

    
  
> I'd prefer "add-ons".

Fair enough.


> An add-on only has one id (install.rdf em:id), human-readable or not.
> It used to be with Firefox 1 and below that ids needed to be a GUID. Now
> GUIDs and the at-form (something@whatever) are supported.

Ok.  It looks like you can use Google to find the add-on name when you have the GUID, which is better than nothing.


> I thought about that, too, but didn't really want to break the public js API.
> But if that's ok...

Totally ok! :)


> So I prefer the "grotty" approach (after a bit of polishing) as hackish,
> loosely-coupled and string-based as it might be.

Ok -- you understand it much better than I do.  But better comments (perhaps including some of what you just wrote) would be great!

    
  
Depends on: 857690

    
    

    
  
(In reply to Nils Maier [:nmaier] from comment #64:
> Switching ids (no matter what form) is not supported, as it breaks updates,
> leads to multiple simultaneously installed copies of an add-on and so on.

Tangential nit: This is supported now days.

    
    

    
  
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #66)
> (In reply to Nils Maier [:nmaier] from comment #64:
> > Switching ids (no matter what form) is not supported, as it breaks updates,
> > leads to multiple simultaneously installed copies of an add-on and so on.
> 
> Tangential nit: This is supported now days.

What? How is it possible to change IDs? Could you post a link to doc for that please?

    
    

    
  
(In reply to Dave Garrett from comment #67)
> What? How is it possible to change IDs? Could you post a link to doc for
> that please?

This is getting a bit off-topic :)

See the Gecko 2.0 note here: https://developer.mozilla.org/en-US/docs/Extension_Versioning,_Update_and_Compatibility#Automatic_Add-on_Update_Checking

Implemented in bug 412819, shipped in Firefox 4.

Update manifest just works the same as always, pointing to the XPI of the new version. New version can have a new ID, the Add-ons Manager will automatically detect that and replace the old add-on, and everything magically work.

    
  
Whiteboard: [MemShrink]

    
    

    
  
(In reply to Nicholas Nethercote [:njn] from comment #65)
> > An add-on only has one id (install.rdf em:id), human-readable or not.
> > It used to be with Firefox 1 and below that ids needed to be a GUID. Now
> > GUIDs and the at-form (something@whatever) are supported.
> 
> Ok.  It looks like you can use Google to find the add-on name when you have
> the GUID, which is better than nothing.

FWIW, users may actually match the ids of add-ons they have installed to names via about:support. ;)
Google is another option, as is add-ons mxr (most of the time and provided you have access).

    
    

    
  


  
This patch prefixes essentially all reports I could think of that have some sort of location attached to them: JS compartments, workers and windows.
I think this is a good start.
The reports will be prefixed like explicit/add-ons/<addonid>/. As the add-on name is not readily available due to the lack of a fast and synchronous API to query for it, it is not included for now (as discussed before).

Green try: https://tbpl.mozilla.org/?tree=Try&rev=ddb6f322f53c

        Attachment #727319 -
        Attachment is obsolete: true

        Attachment #760922 -
        Flags: review?(n.nethercote)
Whiteboard: [MemShrink] → [MemShrink:P1]

    
    

    
  
Comment on attachment 760922 [details] [diff] [review]
Part 2 - Prefix memory reports with add-on ids

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

I've just looked at the code, and it looks good.  A few nits below.

Next I'll try a build with these patches and check the output.

::: dom/workers/WorkerPrivate.cpp
@@ +1839,5 @@
>      NS_ASSERTION(mWorkerPrivate, "Disabled more than once!");
>      mWorkerPrivate = nullptr;
>    }
> +
> +  // Only call this from the main thread and under mMutex lock.

Add a call to AssertIsOnMainThread() at the top of the function to assert the first part.  And I *think* you can do   mMutex.AssertCurrentThreadOwns() to assert the second part.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +228,5 @@
> +{
> +    // Already tried parsing the location before
> +    if (locationWasParsed) {
> +      return false;
> +    }

No need to brace this.  (xpconnect style is a mish-mash, but you haven't braced single-line blocks below in this function.  intra-function consistency seems worthwhile.)

@@ +245,5 @@
> +    // <js-stack-frame-filename> furthermore is "free form", often using a
> +    // "uri -> uri -> ..." chain. The following code will and must handle this
> +    // common case.
> +    // It should be noted that other parts of the code may already rely on the
> +    // "format" of these strings, such as the add-on SDK.

Nice comment!

@@ +267,5 @@
> +    // Not in <sandboxName> so we need to inspect <js-stack-frame-filename> and
> +    // the chain that is potentially contained within and grab the rightmost
> +    // item that is actually a URI.
> +
> +    // First, hack off the :lineno) part as well

Use :<lineno>) for consistency with the comment above.

@@ +304,5 @@
> +    nsAutoCString scheme;
> +    if (NS_FAILED(uri->GetScheme(scheme)))
> +        return false;
> +
> +    // Cannot really map data: and blob:

Full stop at end of sentence, please :)

@@ +2253,5 @@
> +{
> +    nsCOMPtr<amIAddonManager> am =
> +      do_GetService("@mozilla.org/addons/integration;1");
> +    return ReportJSRuntimeExplicitTreeStats(rtStats, rtPath, am.get(), cb,
> +                                            closure, rtTotalOut);

Do we need two versions of ReportJSRuntimeExplicitTreeStats()?  Could the two versions just be merged?

@@ +2495,5 @@
>      // stats seems like a bad idea.
>  
> +    nsCOMPtr<amIAddonManager> addonManager =
> +      do_GetService("@mozilla.org/addons/integration;1");
> +    XPCJSRuntimeStats rtStats(windowPaths, topWindowPaths, addonManager);

The 3rd arg of this constructor is a boolean, but you're passing a pointer.  Changing it to |!!addonManager| might make the types clearer.

Or you could do this:

  bool getLocations = !!addonManager;
  XPCJSRuntimeStats rtStats(windowPaths, topWindowPaths, getLocations);

        Attachment #760922 -
        Flags: review?(n.nethercote) → review+

    
    

    
  
Testing with patch 1 and 2, when I load about:memory after starting a profile with multiple add-ons, I get eight occurrences of this:

  WARNING: NS_ENSURE_TRUE(pWindow) failed: file /home/njn/moz/mi4/dom/base/nsWindowMemoryReporter.cpp, line 60


I also saw this.  Note the nested |add-ons|, which suggests a problem with the location parsing.

├───11,271,008 B (07.72%) -- add-ons
│   ├───5,600,680 B (03.83%) -- memchaser@quality.mozilla.org
│   │   ├──4,454,872 B (03.05%) ++ js-non-window/zones/zone(0x7fe889add800)
│   │   ├────678,520 B (00.46%) ++ window-objects
│   │   └────467,288 B (00.32%) -- add-ons/memchaser@quality.mozilla.org/window-objects
│   │        ├──234,624 B (00.16%) ++ top(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/widget/widget.html, id=21)/active/window(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/widget/widget.html)/js-compartment(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/widget/widget.html)
│   │        └──232,664 B (00.16%) ++ top(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/panel/context.html, id=28)/active/window(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/panel/context.html)/js-compartment(resource://memchaser-at-quality-dot-mozilla-dot-org/memchaser/data/panel/context.html)


Other than that, it looks good.  Thanks!

    
    

    
  
(In reply to Nicholas Nethercote [:njn] from comment #71)
> @@ +2253,5 @@
> > +{
> > +    nsCOMPtr<amIAddonManager> am =
> > +      do_GetService("@mozilla.org/addons/integration;1");
> > +    return ReportJSRuntimeExplicitTreeStats(rtStats, rtPath, am.get(), cb,
> > +                                            closure, rtTotalOut);
> 
> Do we need two versions of ReportJSRuntimeExplicitTreeStats()?  Could the
> two versions just be merged?

I suppose they can be merged. I didn't want amIAddonManager in xpcpublic.h - where ReportJSRuntimeExplicitTreeStats is defined - and still have a fast-path in case a pointer to amIAddonManager was already obtained by the caller.
I could either
- add the amIAddonManager* parameter to xpcpublic.h
- or remove the fast-path
- or leave as is.
What do you prefer?

(In reply to Nicholas Nethercote [:njn] from comment #72)
> Testing with patch 1 and 2, when I load about:memory after starting a
> profile with multiple add-ons, I get eight occurrences of this:
> 
>   WARNING: NS_ENSURE_TRUE(pWindow) failed: file
> /home/njn/moz/mi4/dom/base/nsWindowMemoryReporter.cpp, line 60
> 
> 
> I also saw this.  Note the nested |add-ons|, which suggests a problem with
> the location parsing.
> 
> ├───11,271,008 B (07.72%) -- add-ons
> │   ├───5,600,680 B (03.83%) -- memchaser@quality.mozilla.org
> │   │   └────467,288 B (00.32%) --
> add-ons/memchaser@quality.mozilla.org/window-objects

Yeah, that happens because a top window and js-compartment both get prefixed individually...

The js-compartment report generate should just check that the path isn't prefixed already. This however would assume that each window has exactly (or at most) one js-compartment (the one housing the window global). Just to be sure: This assumption is correct?

    
    

    
  
> > Do we need two versions of ReportJSRuntimeExplicitTreeStats()?  Could the
> > two versions just be merged?
> 
> I could either
> - add the amIAddonManager* parameter to xpcpublic.h
> - or remove the fast-path
> - or leave as is.

Just leave as is.  When I wrote that I hadn't seen the second calling context, and I was meaning to delete that comment but clearly I forgot to :)



> > I also saw this.  Note the nested |add-ons|, which suggests a problem with
> > the location parsing.
> > 
> > ├───11,271,008 B (07.72%) -- add-ons
> > │   ├───5,600,680 B (03.83%) -- memchaser@quality.mozilla.org
> > │   │   └────467,288 B (00.32%) --
> > add-ons/memchaser@quality.mozilla.org/window-objects
> 
> Yeah, that happens because a top window and js-compartment both get prefixed
> individually...
> 
> The js-compartment report generate should just check that the path isn't
> prefixed already. This however would assume that each window has exactly (or
> at most) one js-compartment (the one housing the window global). Just to be
> sure: This assumption is correct?

Yes:  every window object (a.k.a. global object) has exactly one compartment.  That's exactly what "compartment-per-global" (bug 650353) was about, and it's an invariant that is baked in deeply now.

    
    

    
  
(In reply to Nicholas Nethercote [:njn] from comment #74)
> Yes:  every window object (a.k.a. global object) has exactly one
> compartment.  That's exactly what "compartment-per-global" (bug 650353) was
> about, and it's an invariant that is baked in deeply now.

Well, I was wondering if there could be more than one compartment "attached" to a window, e.g. sandboxes or XBL globals or frame scripts...
Doesn't seem to be the case.

    
    

    
  


  
Addressed the nits.
Also, the NS_ENSURE_TRUE assertions should be gone now.
Fixed the double-prefixing of js-compartments under windows by making sure the paths aren't prefixed twice.

        Attachment #760922 -
        Attachment is obsolete: true

        Attachment #762392 -
        Flags: review?(n.nethercote)

    
    

    
  
Comment on attachment 762392 [details] [diff] [review]
Part 2 - Prefix memory reports with add-on ids

Looks good!  I guess these two patches are ready to land, correct?  I.e. nothing else remains to be done, AFAIK.

        Attachment #762392 -
        Flags: review?(n.nethercote) → review+

    
    

    
  


  
Un-bitrotted

        Attachment #727316 -
        Attachment is obsolete: true

    
    

    
  


  
Rebased

        Attachment #762392 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Nicholas Nethercote [:njn] from comment #77)
> I guess these two patches are ready to land, correct?  I.e.
> nothing else remains to be done, AFAIK.

Yep, anything else, like also adding add-on names or pimping about:compartments can be done in follow-up bugs, or not at all.
Feel free to commit.

JF the commiter's I: Green try with mostly the same code:
https://tbpl.mozilla.org/?tree=Try&rev=ddb6f322f53c
Keywords: checkin-needed

    
    

    
  
> Feel free to commit.
> 
> JF the commiter's I: Green try with mostly the same code:
> https://tbpl.mozilla.org/?tree=Try&rev=ddb6f322f53c

It's Saturday here.  I'll do it on Monday, assuming nobody else (RyanVM?) beats me to it.

    
    

    
  
Thanks, Ryan!

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a331728c7d97
https://hg.mozilla.org/mozilla-central/rev/6503457c2561
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

    
  
Depends on: 888693

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: