702300 - Add about:compartments page that just lists live JS compartments

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

It's confusing that some compartments are hidden in non-verbose mode [1, 2].  Since searching for zombie compartments is such a common use of about:memory, why don't we just show all compartments, even in the non-verbose mode?

I'll do this if we can agree that it's virtuous.

Comment 1

[Justin Lebar (not reading bugmail)]

[1] er...this is from dev.platform, but it's not on Google Groups yet, so I don't think I can link to it.  But the gist is, Honza, the Firebug developer, is confused that compartments appear and disappear when he refreshes about:memory.  The likely cause is that he's not in verbose mode.

[2] http://jlebar.com/2011/11/13/The_carrot%2C_the_stick%2C_and_the_wrench%3A_Add-on_leaks_are_everyone%27s_problem..html#comment-362876830

Comment 2

[Hugh Nougher [:Hughman]]

I cannot agree that this is a good move. Mainly because it would add 70+ lines to my about:memory in non-verbose mode (unless the tree under each compartment was hidden which I believe would make non-verbose mode neater).

Comment 3

[Justin Lebar (not reading bugmail)]

The idea would be to unconditionally display only the compartment line.  If a compartment would today not be shown at all, then with this patch, only that one line would be shown.

Comment 4

[Nicholas Nethercote [inactive]]

I can sympathize with the goal, but I don't much like the idea of having to special-case compartment reports in about:memory.

Comment 5

[Andrew McCreight [:mccr8]]

What about a listing like "(5 omitted, including 5 compartments)"?  Though that would still require some special casing.

Comment 6

[Justin Lebar (not reading bugmail)]

I eliminated one compartment special case by switching to text-overflow:ellipsis; does that mean I can add one?

In all seriousness, this special case should be really simple in terms of the about:memory code.

Comment 7

[Justin Lebar (not reading bugmail)]

Another option would be to have about:compartments which lists all the compartments.

about:compartments could also do a gc/cc automatically, cutting out another step that add-on developers would have to do.  We want to make this as low-friction as possible, since people apparently get scared by all the data in about:memory.

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> I eliminated one compartment special case by switching to
> text-overflow:ellipsis; does that mean I can add one?

:P

What would the output look like?  This?

 ├────5.00 MB (00.75%) -- compartment(https://bugzilla.mozilla.org/show_bug.cg...)
 │    └──5.00 MB (00.75%) -- (9 omitted)

where the percentage can be lower than 0.50% (the current minimum?)

Is this really a documentation problem?  My blog post about zombie compartments wasn't aimed at add-on devs, it could certainly be cleaned up and simplified and should be on a wiki page somewhere anyway.

This also makes me think of bug 687724.  First, with compartment-per-global we'll be getting many more compartments in the future.  Second, we'll be able to report them on a per-tab basis, which makes we wonder if it'll make zombie compartments more obvious.

Comment 9

[Andrew McCreight [:mccr8]]

This is a little off topic for here, but now that we have all this extra info maybe we could clean up the output a little.  For instance, with this one:

│  │   │  ├───2.66 MB (00.57%) -- arena
│  │   │  │   └──2.66 MB (00.57%) -- (3 omitted)

Is having that extra line really useful?  The (3 omitted) could be added to the arena line or just removed.

Could we just generally skip the omitted line?  Or would that be too confusing?

Do we want to adjust how deeply we nest depending on how many top level things there is?

Also, the "Other measurements" section has really bloated up.  Maybe we should split out JS into its own "Other JS measurements" section?  They comprise more than half of the lines here.  With those moved to another section, the rest of the lines are not so bad.

Comment 10

[Justin Lebar (not reading bugmail)]

Andrew, yes, that sounds pretty off-topic.  :)

> What would the output look like?  This?
> 
>  ├────5.00 MB (00.75%) -- compartment(https://bugzilla.mozilla.org/show_bug.cg...)
>  │    └──5.00 MB (00.75%) -- (9 omitted)

I was hoping we could get rid of the omitted line, since it doesn't add anything here.

I've been hearing about compartment-per-global for a while.  I'd rather solve our problem now and iterate than wait until it happens.

> Is this really a documentation problem?

I don't think we can rely on users reading documentation.   See (1) in comment 1, for instance.

But the first comment thread at [1] suggests to me that it's bigger than that.  Mike Kaply was trying to follow the zombie compartment instructions, but it seems like he was just overwhelmed by the instructions and the detail in about:memory?verbose.

The more I dig into this, the more I think we need to make this kind of debugging *dead simple* if our goal is to affect improvements in addons.

We may want an about:compartments page which makes it simpler to detect compartment leaks.  But people are still going to screw up and look at about:memory, which also lists compartments, and be confused.  It's not the user's fault...

[1] http://jlebar.com/2011/11/13/The_carrot%2C_the_stick%2C_and_the_wrench%3A_Add-on_leaks_are_everyone%27s_problem..html#comments

Comment 11

[Justin Lebar (not reading bugmail)]

For instance, the line could be

5.00 MB (00.75%) -- compartment (http://zombocom.com) (9 omitted)

Comment 12

[Hugh Nougher [:Hughman]]

(In reply to Justin Lebar [:jlebar] from comment #11)
> 5.00 MB (00.75%) -- compartment (http://zombocom.com) (9 omitted)

I think the "9 omitted" is just confusing. Either it needs to be "9 children omitted" so its known that they are all part of the compartment or somehow go with a tree structure which is the usual way of omitting children.

Comment 13

[Nicholas Nethercote [inactive]]

Hughman++.

Is really a documentation problem?
 
> I don't think we can rely on users reading documentation.   See (1) in
> comment 1, for instance.
> 
> But the first comment thread at [1] suggests to me that it's bigger than
> that.  Mike Kaply was trying to follow the zombie compartment instructions,
> but it seems like he was just overwhelmed by the instructions and the detail
> in about:memory?verbose.

Well, so Kaply found the documentation.  Anyway, you think that non-verbose mode has sufficiently little information to not confuse people?  For zombie compartment hunting, ideally we'd just have a list of compartments with nothing else.  Is this solution close enough to that to suffice?

Comment 14

[Justin Lebar (not reading bugmail)]

> Anyway, you think that non-verbose mode has sufficiently little information to not confuse people?  
> For zombie compartment hunting, ideally we'd just have a list of compartments with nothing else.  Is 
> this solution close enough to that to suffice?

I'm not sure -- I think the list of compartments with nothing else may be the right solution.  My concern is that:

> people are still going to screw up and look at about:memory, which also lists compartments, and be 
> confused.

but if we all agree on about:compartments, maybe try that on for size first.

Comment 15

[Justin Lebar (not reading bugmail)]

So listing all the compartments in about:memory would be useful for me (and jesup?), since I'm often looking for zombie compartments and verbose mode is a pain.  So I selfishly think we should start with this and see later if we want to add about:compartments.

We've put so much effort into zombie compartments, it seems pretty natural to me to list them all there.

But anyway, it's not a big deal; I won't cry if we wontfix this and decide that rtfm is the answer.

Comment 16

[Hugh Nougher [:Hughman]]

Here is my proposal and reasons why certain choices have been made.

How about making the current static tree into a dynamic tree where each parent can be clicked on to expand/collapse its subtree. When a subtree to first expanded it will have nothing omitted. This is so when you click on a compartment it would expand to show a full breakdown of what the compartment is using.

By default all children of "explicit" are collapsed except "js" and all children of "js" are collapsed. Resulting structure below (values from one of my testing instances). Notice how clean it is to look at without all the exact breakdowns.

108.14 MB (100.0%) -- explicit
├───90.90 MB (84.06%) -- heap-unclassified
├───11.36 MB (10.51%) -- js
│   ├───7.92 MB (07.32%) -- compartment([System Principal], 0x866a000)
│   ├───1.32 MB (01.22%) -- gc-heap-chunk-dirty-unused
│   ├───1.07 MB (00.99%) -- compartment(atoms)
│   └───1.05 MB (00.97%) -- (some other compartment)
├────3.25 MB (03.01%) -- storage
├────1.36 MB (01.26%) -- layout
├────0.89 MB (00.82%) -- xpti-working-set
└────0.37 MB (00.34%) -- (other stuff)

Now my reasoning for this structure. The biggest question is "what do you want to see when you first look at about:memory?" or thinking from what a UI designer would think "what do you need to look at first to find what you are looking for?".

The answer to me seems straight forward for these questions. I go looking for a specific compartment or a specific type of allocation. I dont usually want to see the specific details of each compartment until I know which compartment I am looking at. Its a similar case with the other memory types under "explicit".

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Also, the "Other measurements" section has really bloated up.  Maybe we
> should split out JS into its own "Other JS measurements" section?  They
> comprise more than half of the lines here.  With those moved to another
> section, the rest of the lines are not so bad.

Is there a bug somewhere for cleaning up this "other" section? I think it also needs some tree structure but that can wait for the other bug.

Comment 17

[Nicholas Nethercote [inactive]]

> The biggest question is "what do you
> want to see when you first look at about:memory?" or thinking from what a UI
> designer would think "what do you need to look at first to find what you are
> looking for?".

That's not the biggest question for me.  I'm very often interested in a single entry or subset of entries, and I usually use about:memory?verbose and then search for what I'm looking for.  I'd support something like this if about:memory showed the current subset and you could drill down by clicking, if about:memory?verbose stayed the way it is, i.e. showing everything from the start.  There's a bug open about XULifying about:memory but I can't find it now.

I don't want to add a tree structure to the Other Measurements, for two reasons.  First, it doesn't make sense -- not all of those measurements are subsets of other measurements, and some aren't even measured in bytes.  Second, it doesn't solve any existing problems.

Comment 18

[Justin Lebar (not reading bugmail)]

I've thought about click-to-expand, but I'm not convinced it's worthwhile.  Just about the only thing I'd want to click to expand in the non-verbose about:memory is the list of compartments.  And about:memory isn't so huge that I think we need to hide large parts by default.

click-to-expand is tricky with respect to refreshes.  You'd want to persist the expanded/collapsed state across refreshes, even when we add or remove nodes from the tree.

Click-to-expand also complicates our copy-paste story.

If you want to discuss this further, could you please file a separate bug?

Comment 19

[Nicholas Nethercote [inactive]]

Bug 719335 is open for click-to-expand.  It's working well w.r.t. refreshes and cut-and-paste.  The only problem is that it might be slower and take more memory.  More investigation is needed there.

I thought about adding an about:memory?compartments that shows what Hugh suggested in comment 16, but having played with it in my click-to-expand build there's still too much extraneous stuff: gc-heap-chunk-dirty-unused, storage, layout, etc.  My preference now is just to have a separate about:compartments page.  I've taken the liberty of renaming this bug accordingly.

Forcing GC/CC/MMU on loading is probably a good idea, esp. since it's aimed so heavily at leak detection.  Maybe we could have about:compartments?nogc (or somesuch) just in case you didn't want to run the GC/CC/MMU.

It would also be a good idea to have two lists, one for system compartments and one for user compartments.

Having about:compartments would simplify the docs at https://developer.mozilla.org/en/Zombie_Compartments quite a bit.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: draft patch

Attached is a draft patch implementing about:compartments.  about:memory is slightly broken -- it lists all compartments in the "Other Measurements" list.

aboutCompartments.js is about 320 lines.  About 70 lines are copied directly from aboutMemory.js.  Most of the remainder is heavily inspired by aboutMemory.js.  Part of me wants to avoid this duplication and just have a "compartments" section in about:memory, possibly unexpanded by default.  But I suspect the "ease-of-use for users trumps code cleanness" argument will raise its head.

Three memory pressure events are fired when about:compartments is loaded.  I'll attach a screenshot shortly.

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: screenshot from draft patch

Screenshot when techcrunch.com is open.  Random thoughts:

- The fact that the moz-nullprincipal compartment shows up under "User Compartments" is unfortunate.

- I wonder if emphasizing the domain name (as is done in the address bar) is a good idea.

- I wonder if having non-verbose/verbose modes is a good idea, where the former truncates long URLs like about:memory does.

Note that if there are multiple compartments with the same name, a "[2]"-style number will be appended, just like about:memory.

Note also that system compartments don't have their address shown, unlike about:memory.

Comment 23

[Justin Lebar (not reading bugmail)]

> Three memory pressure events are fired when about:compartments is loaded.

I think this is a principal reason to have a separate about:compartments.  It eliminates a confusing and tedious step.  (We're not going to auto-GC when you load about:memory, I think.)

Comment 24

[Justin Lebar (not reading bugmail)]

> aboutCompartments.js is about 320 lines.  About 70 lines are copied directly from aboutMemory.js.  
> Most of the remainder is heavily inspired by aboutMemory.js.  Part of me wants to avoid this 
> duplication and just have a "compartments" section in about:memory, possibly unexpanded by default.

Making about:compartments a special case of about:memory is not the only way to accomplish this, of course.  We could run the same JS file for both about:memory and about:compartments.

I don't like the code duplication, but I don't know if adding special cases all over about:memory would be better.

> The fact that the moz-nullprincipal compartment shows up under "User Compartments" is unfortunate.

Surely we can special-case this.

> I wonder if emphasizing the domain name (as is done in the address bar) is a good idea.

At the moment, we only have one compartment per origin, so we could just truncate to the origin.  Maybe you could click to see the full URI or something, or maybe we just leave that to about:memory.

> I wonder if having non-verbose/verbose modes is a good idea, where the former truncates long URLs 
> like about:memory does.

Sure, that would work, too.

> Note also that system compartments don't have their address shown, unlike about:memory.

Sounds fine to me.

Comment 25

[Nicholas Nethercote [inactive]]

> I don't like the code duplication, but I don't know if adding special cases
> all over about:memory would be better.

I could make a .jsm for the shared stuff.  Not that I know anything about them, but that seems to be what they're for.


> > The fact that the moz-nullprincipal compartment shows up under "User Compartments" is unfortunate.
> 
> Surely we can special-case this.

I'm currently relying on the JS engine's user/system categorization, but yeah, that'd be better.  (I talked to gwagner about changing the JS engine's categorization, but special casing it in aboutCompartments.js sounds simpler.)


I have this vague nagging feeling about about:compartments... something to do with the fact that it's targeted at such a specific use-case, and I worry that after a not-very-long time it'll end up being not useful, or too specialized, or something.  Though I guess we can always change it or remove it later if necessary.

Comment 26

[Nicholas Nethercote [inactive]]

Attachment: patch 1: var-to-let

I have several preliminary patches before I get to the meaty stuff.

This patch converts all instances of |var| in aboutMemory.js and its tests
to |let|, which is just better because it is properly scoped.  Almost
entirely trivial, though in 3 places in aboutMemory.js a declaration had to
be moved outside a loop because the loop variable was used after the loop.

Comment 27

[Nicholas Nethercote [inactive]]

Attachment: patch 2: fix-test-indentation

This patch just fixes some accidental 3-space indents in
test_aboutmemory.xul.  No other changes.

Comment 28

[Nicholas Nethercote [inactive]]

Attachment: patch 3: tweak-tests

This patch modifies test_aboutmemory.xul and test_aboutmemory2.xul:

- Adds an 'a' prefix to some parameters that lack it (eg. |closure| ->
  |aClosure|)

- Changes addLoadEvent to SimpleTest.waitForFocus, on Ehsan's recommendation
  -- apparently it can prevent occasional failures.

- Put the expected/actual dumping in the failure function instead of the
  clipboard function, which is a better spot for it.

- Some other very minor things.

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: patch 4: tree-descs

This patch uses multi-line string syntax for the long tree descriptions,
which I think is much nicer than concatenation.

It also inserts some newlines, which only started working recently in
tool-tips.  I'm using "\n\n" because that gives a blank line between
paragraphs.  A single "\n" just starts a new line without any indentation,
which doesn't look very nice.

Other than the newlines, the descriptions are unchanged.

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: patch 5: reporter-to-report

Before multi-reporters were added, it made sense to talk about a single
about:memory measurement as a "reporter".  But now that we have multi-reporters
it's clearer to describe a single measurement as a "report", and use "reporter"
for nsIMemoryReporter and nsIMemoryMultiReporter objects;  the former produce
single reports and the latter produce multiple reports.
  
This patch adjust names and comments accordingly.  E.g. |Reporter| -> |Report|.

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: patch 6: move-stuff

In the patch after the next one aboutMemory.js will be split into 3 parts:
code shared by about:memory and about:compartments, code specific to
about:memory, and code specific to about:compartments.  This patch purely
moves stuff around in preparation for that (and adds some separator
comments);  separating the code motion from more substantive changes makes
the next patch easier to review.

Comment 32

[Nicholas Nethercote [inactive]]

Attachment: patch 7: add the compartments multi-reporter

This patch just adds a new memory multi-reporter for compartments, and
there's a bit of infrastructure required for this.  I renamed a few
functions in MemoryMetrics.cpp to make things clearer.  And I moved
{System,User}CompartmentCount from jsapi.cpp to MemoryMetrics.cpp.

Comment 33

[Nicholas Nethercote [inactive]]

Attachment: patch 8: about-compartments

The main event.  about:compartments is implemented entirely within
aboutMemory.{xhtml,js,css}.  Changes in aboutMemory.js:

- I factored out some stuff so it could be used by both about:memory and
  about:compartments:  
  
  - gChildMemoryListener and setupChildObserversAndUpdate() for the child
    observer stuff;
  - minimizeMemoryUsage() is now passed the function to call once it's
    finished
  - processMemoryReports iterates over the single and multi-reporters, and
    uses callbacks to control the iteration.

- The about:compartments-specific code is added.  It has some similarities
  to the about:memory-specific code, but is much simpler.

- I handled long URLs by truncating them (via CSS) in normal mode, and adding
  about:compartments?verbose where they aren't truncated.

- Compartment names are now shown in alphabetical order within each section.

- moz-nullprincipal compartments are special-cased so they show up in the
  "system compartments" list.

- The GC/CC on page entry is currently disabled because it breaks the test.
  See aboutCompartmentsOnLoad() for details.  I have to work something out
  here.

- I changed |document.getElementById("content")| to the simpler
  |document.body| and removed the |id="content"| from the <body> element.
  (You learn some things when reading the DOM API! :)

Other changes:

- In test_aboutmemory.xul, I converted some "smaps" reports in a child
  process from a multi-reporter to a single reporter, which is how it would
  actually happen.  This change thus tests the filtering of "smaps" single
  reporters.

- I added test_aboutcompartments.xul.

- I tightened up the descriptions of the allowed reports in
  nsIMemoryReporter.idl.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 597728 [details] [diff] [review]
patch 7: add the compartments multi-reporter

Nothing looks obviously wrong, but I'd prefer if you asked someone who actually knows what this code does

Comment 35

[Nicholas Nethercote [inactive]]

Attachment: patch 9: desafe-descs

The last patch!  I realized that there's no longer any need for the
safe/unsafe distinction for report descriptions.  This patch removes them.
There are basically three changes:

- A bunch of fields and variables were renamed (boring).

- A flipBackslashes() call was added for the "The sum of all entries below..."
  descriptions used for non-leaf nodes.

- A flipBackslashes() call was removed where the descriptions are used for the
  |nameSpan.title = ...| description.

Overall, flipBackslashes() is called fewer times (i.e. only on non-leaf node
descriptions instead of all node descriptions).  So this should reduce the
amount of memory allocation done, but I haven't measured how big the effects
are.

Comment 36

[Nicholas Nethercote [inactive]]

I just realized all these patches have -U 4 instead of -U 8 because I just copied them from .hg/patches/ and Mercurial uses -U 4 in patch queues.  Sorry about that.  I suspect there's a better way to export patch queues.

Comment 37

[Nicholas Nethercote [inactive]]

Comment on attachment 597718 [details] [diff] [review]
patch 1: var-to-let

I've been asking for too many reviews from certain people lately.  Time to ask for reviews from some newer people.  johns, I don't know if you've done any reviews, but I'm going to give you a couple of very easy ones that require JS knowledge but no specific about:memory knowledge.

Comment 38

[Nicholas Nethercote [inactive]]

Comment on attachment 597721 [details] [diff] [review]
patch 2: fix-test-indentation

Another easy one for johns.

Comment 39

[Nicholas Nethercote [inactive]]

Comment on attachment 597728 [details] [diff] [review]
patch 7: add the compartments multi-reporter

Terrence, let me know if you're uncomfortable reviewing any part of this.

Comment 40

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 597728 [details] [diff] [review]
patch 7: add the compartments multi-reporter

I already started on this, so I might as well finish.

Comment 41

[Nicholas Nethercote [inactive]]

> I already started on this, so I might as well finish.

Sorry, Bill!  And thanks.  Terrence, feel free to take a look anyway :)

Comment 42

[Terrence Cole [:terrence]]

I'm glad Bill decided to steal the review on that: XPConnect is still foreign territory for me :-)

Comment 43

[Nicholas Nethercote [inactive]]

(In reply to Terrence Cole [:terrence] from comment #42)
> I'm glad Bill decided to steal the review on that: XPConnect is still
> foreign territory for me :-)

Me too!  But big chunks of XPCJSRuntime.cpp are very closely related to the JS engine and don't involve any XPConnect-specific details.

Comment 44

[Justin Lebar (not reading bugmail)]

btw, it looks like your patches have 3 lines of context, rather than the traditional 8.

Comment 45

[Justin Lebar (not reading bugmail)]

Comment on attachment 597723 [details] [diff] [review]
patch 3: tweak-tests

Good call on changing how the clipboard EXPECTED/ACTUAL output works.

Comment 46

[Justin Lebar (not reading bugmail)]

Comment on attachment 597725 [details] [diff] [review]
patch 5: reporter-to-report

>+  // - Make a copy of its reports into a sub-table indexed by its process.

Maybe 'report(s)' here to emphasize the role of non-multi-reporters?

>-function buildTree(aReporters, aTreeName)
>+function buildTree(aReports, aTreeName)
> {
>-  // We want to process all reporters that begin with |aTreeName|.  First we
>+  // We want to process all reports that begin with |aTreeName|.  First we
>   // build the tree but only fill the properties that we can with a top-down
>   // traversal.
> 
>-  // There should always be at least one matching reporter when |aTreeName| is
>+  // There should always be at least one matching Report when |aTreeName| is

I wonder if you capitalize "report" sometimes and not others for some reason other than tormenting me.  :)

Comment 47

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It seems like CollectRuntimeStats provides the information you need, except that it includes the address of system compartments in the name. Are you creating a new API (CollectCompartmentNamesAndKinds) because CollectRuntimeStats is too slow?

As an aside, I really don't like how the strings are handled in this patch and in the existing memory reporter API. I've also noticed that for JS debugging, it would be nice to have an easy way to get a compartment's name. What if we add a char* field to JSCompartment that would be owned by JS? It could be freed in the compartment destructor. XPConnect could provide it during compartment creation.

If we are going to have a separate API, I would rather it take the form of "iterate over all compartments and call this function on each compartment". It seems to fit what you're doing better, and it's more general.

Comment 48

[Justin Lebar (not reading bugmail)]

Comment on attachment 597729 [details] [diff] [review]
patch 8: about-compartments

>diff --git a/toolkit/components/aboutmemory/content/aboutMemory.js b/toolkit/components/aboutmemory/content/aboutMemory.js
>--- a/toolkit/components/aboutmemory/content/aboutMemory.js
>+++ b/toolkit/components/aboutmemory/content/aboutMemory.js
>@@ -52,11 +56,18 @@ const UNITS_COUNT            = Ci.nsIMem
> const UNITS_COUNT_CUMULATIVE = Ci.nsIMemoryReporter.UNITS_COUNT_CUMULATIVE;
> const UNITS_PERCENTAGE       = Ci.nsIMemoryReporter.UNITS_PERCENTAGE;
> 
>+const gVerbose = location.href === "about:memory?verbose" ||
>+                 location.href === "about:compartments?verbose";

|location.search === "?verbose"|?

> function debug(x)
> {
>-  let content = document.getElementById("content");
>-  appendElementWithText(content, "div", "legend", JSON.stringify(x));
>+  let body = document.body;
>+  appendElementWithText(body, "div", "legend", JSON.stringify(x));
> }

Nit: |appendElementWithText(document.body, ...)|?

>-function onLoad()
>+function setupChildObserversAndUpdate(aUpdateFn)

Nit: "Setup" is a noun; "set up" is a verb.  So setUpChildObserversAndUpdate().

>+function onLoad()
>+{
>+  if (location.href.indexOf("about:memory") === 0) {
>+    document.title = "about:memory";
>+    onLoadAM();
>+  } else if (location.href.indexOf("about:compartment") === 0) {
>+    document.title = "about:compartments";
>+    onLoadAC();
>+  } else {
>+    assert(false, "Unknown location");
>+  }

Can we expand "AM" and "AC", please?

> function onUnload()
> {
>   // We need to check if the observer has been added before removing; in some
>   // circumstances (eg. reloading the page quickly) it might not have because
>-  // onLoad might not fire.
>-  if (gAddedObserver) {
>+  // about{Memory,Compartments}OnLoad might not fire.

You mean onLoadA{M,C}, or some other functions?

> //---------------------------------------------------------------------------

What is this separating, exactly?  There are a bunch of these separators added in an early patch, but I'm not sure they're helping.

Separating shared code, AM-only, and AC-only makes sense.  But beyond that, I'm not convinced it's useful.

You could use "{{{" and "}}}" to set fold points in vim, which, if the sections are actually meaningful, might be useful.  It looks like emacs uses the same markers, although I will not profane to open emacs to test.  :)

http://www.emacswiki.org/emacs/FoldingMode

> function doGlobalGC()
>@@ -236,7 +328,7 @@ function doGlobalGC()
>   let os = Cc["@mozilla.org/observer-service;1"]
>             .getService(Ci.nsIObserverService);
>   os.notifyObservers(null, "child-gc-request", null);
>-  update();
>+  updateAM();
> }
> 
> function doCC()
>@@ -247,7 +339,7 @@ function doCC()
>   let os = Cc["@mozilla.org/observer-service;1"]
>             .getService(Ci.nsIObserverService);
>   os.notifyObservers(null, "child-cc-request", null);
>-  update();
>+  updateAM();
> }

Maybe you want to pass a callback function to these, rather than hardcoding updateAM()?

>@@ -368,16 +458,23 @@ Report.prototype = {
> 
> function getReportsByProcess(aMgr)
> {
>-  // Process each memory reporter:
>-  // - Make a copy of its reports into a sub-table indexed by its process.
>-  //   Each copy is a Report object.  After this point we never use the
>-  //   original memory reporter again.
>-  //
>-  // - Note that copying rOrig.amount (which calls a C++ function under the
>-  //   IDL covers) to r._amount for every report now means that the
>-  //   results as consistent as possible -- measurements are made all at
>-  //   once before most of the memory required to generate this page is
>-  //   allocated.
>+  // Ignore the "smaps" multi-reporters in non-verbose mode, and the

Nit: '"smaps" multi-reporter', singular?

>@@ -781,7 +851,7 @@ function appendWarningElements(aP, aHasK
>     {
>       appendTextNode(ul, " ");
>       appendElementWithText(ul, "li", "", 
>-        makeSafe(gUnsafePathsWithInvalidValuesForThisProcess[i]));
>+        flipBackslashes(gUnsafePathsWithInvalidValuesForThisProcess[i]));

"This" process is always the main process.  Maybe "current" would be better, because at least the "current" process might change.  Anyway...

>+//-----------------------------------------------------------------------------
>+// Code specific to about:compartments
>+//-----------------------------------------------------------------------------
>+
>+function onLoadAC()
>+{
>+  // Minimize memory usage before generating the page in an attempt to collect
>+  // any dead compartments.
>+  // njn: this causes test_aboutcompartments.xul to fail because the page
>+  //      selection happens while MMU is happening, when the page is still
>+  //      blank...
>+  //minimizeMemoryUsage3x(
>+  //  function() { setupChildObserversAndUpdate(updateAC); });

So the plan is to handle the minimize-on-load as a followup?  That's fine.

If you want to fix it now, we could follow the lead of reftest and put a CSS class in the root/body element when we're ready for the test to do its thing.  The test can poll until it sees the class.

>+  appendProcessCompartmentsElementsHelper(aP, aCompartments, false, "User");
>+  appendProcessCompartmentsElementsHelper(aP, aCompartments, true, "System");

Ouch, can you do |/* aIsSystem = */false|, or even just pass "User" and
"System" and get true/false from that within appendProcessCompartmentsElementsHelper()?

>+  <![CDATA[
>+  var acExpectedText =
>+"\
>+Main Process\n\
>+\n\
>+User Compartments\n\
>+http://foo.com/\n\
>+https://bar.com/bar?baz\n\
>+https://very-long-url.com/very-long/oh-so-long/really-quite-long.html?a=2&b=3&c=4&d=5&e=abcdefghijklmnopqrstuvwxyz&f=123456789123456789123456789\n\
>+\n\
>+System Compartments\n\
>+[System Principal] [3]\n\
>+atoms\n\
>+moz-nullprincipal:{7ddefdaf-34f1-473f-9b03-50a4568ccb06}\n\
>+\n\
>+2nd Process\n\
>+\n\
>+User Compartments\n\
>+child-user-compartment\n\
>+\n\
>+System Compartments\n\
>+child-system-compartment\n\
>+\n\

It doesn't look like you test about:compartments?verbose.  Is that intentional?

>-   * - Paths starting with "map" represent regions of virtual memory that the
>-   *   process has mapped.  The reporter immediately beneath "map" describes
>-   *   the type of measurement; for instance, the reporter "map/rss/[stack]"
>-   *   might report how much of the process's stack is currently in physical
>-   *   memory.
>+   * - Paths starting with "smaps/" represent regions of virtual memory that the
>+   *   process has mapped.  The rest of the path describes the type of
>+   *   measurement; for instance, the reporter "smaps/rss/[stack]" might report
>+   *   how much of the process's stack is currently in physical memory.
>    *
>    *   Reporters in this category must have kind NONHEAP and units BYTES.
>    *
>+   * - Paths starting with "compartments/" represent the names of JS
>+   *   compartments.  Reporters in this category must have amount 1, kind
>+   *   OTHER, units COUNT, and an empty description.

And the path must have the form compartments/{User,System}/X.

Comment 49

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #44)
> btw, it looks like your patches have 3 lines of context, rather than the
> traditional 8.

Yep, see comment 36.

Comment 50

[Nicholas Nethercote [inactive]]

> I wonder if you capitalize "report" sometimes and not others for some reason
> other than tormenting me.  :)

"Report" refers to the objects of that specific type, "report" refers to the concept in general.  I can use "Report object" instead of "Report" if that's clearer.

Comment 51

[Nicholas Nethercote [inactive]]

(In reply to Bill McCloskey (:billm) from comment #47)
> It seems like CollectRuntimeStats provides the information you need, except
> that it includes the address of system compartments in the name. Are you
> creating a new API (CollectCompartmentNamesAndKinds) because
> CollectRuntimeStats is too slow?

Yes.  CollectRuntimeStats touches every live thing on the GC heap, which is massive overkill.


> As an aside, I really don't like how the strings are handled in this patch
> and in the existing memory reporter API. I've also noticed that for JS
> debugging, it would be nice to have an easy way to get a compartment's name.
> What if we add a char* field to JSCompartment that would be owned by JS? It
> could be freed in the compartment destructor. XPConnect could provide it
> during compartment creation.

Sounds reasonable, but can we save it for a follow-up bug?  I'm happy to file it and take it.

 
> If we are going to have a separate API, I would rather it take the form of
> "iterate over all compartments and call this function on each compartment".
> It seems to fit what you're doing better, and it's more general.

So just expose IterateCompartments, basically?  The problem with that is that XPConnect would need to #include "jscompartments.h"... Ms2ger just moved CollectRuntimeStates from XPCJSRuntime.cpp into MemoryMetrics.cpp in order to avoid exactly that.

Comment 52

[Justin Lebar (not reading bugmail)]

> I suspect there's a better way to export patch queues.

Have you tried hg bzexport?

http://blog.mozilla.com/ted/2010/09/07/bzexport-a-mercurial-extension/

> "Report" refers to the objects of that specific type, "report" refers to the concept in general.  I 
> can use "Report object" instead of "Report" if that's clearer.

That's what I figured, but it wasn't totally consistent.  Anyway, it's fine.  :)

Comment 53

[Nicholas Nethercote [inactive]]

> >+const gVerbose = location.href === "about:memory?verbose" ||
> >+                 location.href === "about:compartments?verbose";
> 
> |location.search === "?verbose"|?

That doesn't work because about:compartments isn't a standard URL and so |search| and the other fields aren't set properly.


> Can we expand "AM" and "AC", please?

I had it expanded and then contracted because I found it easier to read :(  I'll revert.

 
> Separating shared code, AM-only, and AC-only makes sense.  But beyond that,
> I'm not convinced it's useful.

Just basic areas of functionality, like chapters/sections in a document.


> >+  // njn: this causes test_aboutcompartments.xul to fail because the page
> >+  //      selection happens while MMU is happening, when the page is still
> >+  //      blank...
> >+  //minimizeMemoryUsage3x(
> >+  //  function() { setupChildObserversAndUpdate(updateAC); });
> 
> So the plan is to handle the minimize-on-load as a followup?  That's fine.
> 
> If you want to fix it now, we could follow the lead of reftest and put a CSS
> class in the root/body element when we're ready for the test to do its
> thing.  The test can poll until it sees the class.

I do want to fix it now, I'll take a look at reftest, thanks.

 
> It doesn't look like you test about:compartments?verbose.  Is that
> intentional?

I do, there's this line:

  var acvExpectedText = acExpectedText;

I thought I put a comment on it explained the two were identical, but apparently not.  I'll do that.

Comment 54

[Justin Lebar (not reading bugmail)]

Ah, they're the same because the only difference is truncating long names with ellipsis?

Comment 55

[Justin Lebar (not reading bugmail)]

>> Separating shared code, AM-only, and AC-only makes sense.  But beyond that,
>> I'm not convinced it's useful.
>
> Just basic areas of functionality, like chapters/sections in a document.

I've never seen this work well in practice, which is why I bring it up.  The divisions make sense now, but as people add and remove more code, they won't know whether to put the code above or below the line, or somewhere else...

I'm still in favor of collapsible AM/AC/shared sections, because that's a hard division...

Comment 56

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #54)
> Ah, they're the same because the only difference is truncating long names
> with ellipsis?

Yes.

(In reply to Justin Lebar [:jlebar] from comment #55)
> 
> I've never seen this work well in practice, which is why I bring it up.  The
> divisions make sense now, but as people add and remove more code, they won't
> know whether to put the code above or below the line, or somewhere else...

I have seen it work.  Your criticism can be applied more generally to just about any comment...
 
> I'm still in favor of collapsible AM/AC/shared sections, because that's a
> hard division...

The {{{ thing?  Yuk, no thanks :)

Comment 57

[Justin Lebar (not reading bugmail)]

> I have seen it work.  Your criticism can be applied more generally to just about any comment...

It could be misapplied, sure!  But in those cases, it would not be a particularly good argument.  :D

The issue isn't with comments or even adding structure to a file, but rather with adding vague chapter-like structure which the people reading your code won't be able to understand, and which the people modifying your code won't be able to maintain.

If you think it's a good idea, add it, and let's revisit this file in a year.  :)

Comment 58

[Nicholas Nethercote [inactive]]

Attachment: patch 7: compartments-multi-reporter, v2

Using IterateObjects from XPCJSRuntime.cpp made this much simpler.

Comment 59

[Nicholas Nethercote [inactive]]

Attachment: patch 8: about-compartments, v2

Ehsan, I just need review for one small part of this patch.  Remember how I said on IRC that about:compartments triggers GC/CC when it loads, and then generates some content after that?  My test was failing because the cut+paste was happening before the GC/CC finished and so it was copying an empty page.  Here's my fix for that.

In aboutMemory.js, within updateAboutCompartments(), once I finish the DOM generation I dispatch a "bodygenerated" event to the document.

In test_aboutcompartments.xul, within test(), I listen for the "bodygenerated" event and don't do the cut+paste until it's arrived.

It's working fine on my Linux box.  Hopefully it's a robust approach.  Thanks!

Comment 60

[John Schoenick [:johns]]

Comment on attachment 597718 [details] [diff] [review]
patch 1: var-to-let

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +394,1 @@
>    a = appendElementWithText(div2, "a", "option", "Troubleshooting information");

This is the 'let a' on L384, after the inner a's in the if block. Not broken, but confusing.

Comment 61

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 598129 [details] [diff] [review]
patch 7: compartments-multi-reporter, v2

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

Looks good; just some style nits.

::: js/src/MemoryMetrics.cpp
@@ +351,5 @@
> +{
> +    size_t n = 0;
> +    for (size_t i = 0; i < rt->compartments.length(); i++) {
> +        if (rt->compartments[i]->isSystemCompartment) {
> +            ++n;

No braces needed around ++n. Same in UserCompartmentCount.

::: js/src/jsapi.h
@@ +2682,5 @@
> +typedef void (*JSIterateCompartmentCallback)(JSContext *cx, void *data,
> +              JSCompartment *compartment);
> +
> +/*
> + * This function calls |compartmentCallback| on every compartment.

Please add something like this: "Beware that there is no guarantee that the compartment will survive after the callback returns."

::: js/src/jsgc.cpp
@@ +3767,5 @@
> +    JSRuntime *rt = cx->runtime;
> +    JS_ASSERT(!rt->gcRunning);
> +
> +    AutoLockGC lock(rt);
> +    AutoGCSession gcsession(cx);

Now that incremental GC has landed (and assuming it sticks), this should be an AutoHeapSession.

@@ +3773,5 @@
> +    rt->gcHelperThread.waitBackgroundSweepEnd();
> +#endif
> +    AutoUnlockGC unlock(rt);
> +
> +    AutoCopyFreeListToArenas copy(rt);

Waiting for background sweeping to end and the AutoUnlockGC should be unnecessary. Same for AutoCopyFreeListToArenas.

@@ +3775,5 @@
> +    AutoUnlockGC unlock(rt);
> +
> +    AutoCopyFreeListToArenas copy(rt);
> +    for (CompartmentsIter c(rt); !c.done(); c.next()) {
> +        (*compartmentCallback)(cx, data, c);

No braces needed around the single statement.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1725,5 @@
>  NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(JsMallocSizeOf, "js")
>  
> +class JSCompartmentsMultiReporter : public nsIMemoryMultiReporter
> +{
> +public:

Bobby Holley told me that XPConnect style is now JS style. So public: should be indented two spaces (which is awesome because diffs are much more readable).

@@ +1739,5 @@
> +
> +    static void CompartmentCallback(JSContext *cx, void* data, JSCompartment *c)
> +    {
> +        Paths *paths = static_cast<Paths *>(data);
> +        if (!paths->growBy(1)) {

Similarly, all the braced single lines in this class can have their braces removed, making them less likely to get beat up after school.

@@ +1744,5 @@
> +            return;     // silent failure, but it's very unlikely
> +        }
> +        nsCString *name =
> +            static_cast<nsCString *>(xpc::GetCompartmentName(cx, c));
> +        nsCString &path = paths->back();

You're more of a Vector expert than me, but why can't you just use append here?

Comment 62

[Nicholas Nethercote [inactive]]

> > +    AutoGCSession gcsession(cx);
> 
> Now that incremental GC has landed (and assuming it sticks), this should be
> an AutoHeapSession.

I changed this...


> @@ +3773,5 @@
> > +    rt->gcHelperThread.waitBackgroundSweepEnd();
> > +#endif
> > +    AutoUnlockGC unlock(rt);
> > +
> > +    AutoCopyFreeListToArenas copy(rt);
> 
> Waiting for background sweeping to end and the AutoUnlockGC should be
> unnecessary. Same for AutoCopyFreeListToArenas.

... but I haven't changed this.  This patch is just moving code around, and other functions like IterateCompartmentsArenasCells and IterateChunks need to be updated as well, so I'd prefer to do this in a follow-up.  I've filed bug 728736 for this and assigned it to myself.


> Bobby Holley told me that XPConnect style is now JS style.

Great!  I did not know that.  One fewer code style to deal with.

Comment 63

[Nicholas Nethercote [inactive]]

> In aboutMemory.js, within updateAboutCompartments(), once I finish the DOM
> generation I dispatch a "bodygenerated" event to the document.
> 
> In test_aboutcompartments.xul, within test(), I listen for the
> "bodygenerated" event and don't do the cut+paste until it's arrived.
> 
> It's working fine on my Linux box.  Hopefully it's a robust approach. 

Do I need to remove the listener at the end of the test?

Comment 64

[Justin Lebar (not reading bugmail)]

> Do I need to remove the listener at the end of the test?

The listener lives on the content document, so I don't think so.

Comment 65

[Nicholas Nethercote [inactive]]

Comment on attachment 598135 [details] [diff] [review]
patch 8: about-compartments, v2

khuey looked at the test and said it was ok, and try server agrees, so that's good enough for me.

Comment 66

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a22548d04fc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/49bae78a33a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/397de2686f88
https://hg.mozilla.org/integration/mozilla-inbound/rev/0642fc24dacb
https://hg.mozilla.org/integration/mozilla-inbound/rev/21fc99c37626
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5598ffb1aaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/4edde0ad19d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/17927137a699
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec21e7e02464

Comment 67

[Nicholas Nethercote [inactive]]

I just finished updating https://developer.mozilla.org/en/Zombie_Compartments so I think the "dev-doc-needed" can be removed.

Comment 68

[Nicholas Nethercote [inactive]]

A tiny follow-up patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dab4b89eabc

Comment 69

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a22548d04fc3
https://hg.mozilla.org/mozilla-central/rev/49bae78a33a7
https://hg.mozilla.org/mozilla-central/rev/397de2686f88
https://hg.mozilla.org/mozilla-central/rev/0642fc24dacb
https://hg.mozilla.org/mozilla-central/rev/21fc99c37626
https://hg.mozilla.org/mozilla-central/rev/d5598ffb1aaf
https://hg.mozilla.org/mozilla-central/rev/4edde0ad19d8
https://hg.mozilla.org/mozilla-central/rev/17927137a699
https://hg.mozilla.org/mozilla-central/rev/ec21e7e02464

(Leaving open for the followup to merge)

Comment 70

[Jim Jeffery not reading bug-mail 1/2/11]

There seems to be a formatting issue.  
When going into 'more verbose' the listed URL's in about:compartments run off the right side of the boundry box, and make it impossible to copy the entire URL.

Any way to wrap the URL, or make it selectable with double-click or something ?

Comment 71

[Nicholas Nethercote [inactive]]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #70)
> There seems to be a formatting issue.  
> When going into 'more verbose' the listed URL's in about:compartments run
> off the right side of the boundry box, and make it impossible to copy the
> entire URL.

Why is it impossible to copy?  It works fine for me, either by selecting the whole thing by clicking and dragging, or triple-clicking to select the whole line.

In non-verbose mode triple-clicking selects the whole line as well, even the truncated part.

Comment 72

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Nicholas Nethercote [:njn] from comment #71)
> (In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #70)
> > There seems to be a formatting issue.  
> > When going into 'more verbose' the listed URL's in about:compartments run
> > off the right side of the boundry box, and make it impossible to copy the
> > entire URL.
> 
> Why is it impossible to copy?  It works fine for me, either by selecting the
> whole thing by clicking and dragging, or triple-clicking to select the whole
> line.
> 
> In non-verbose mode triple-clicking selects the whole line as well, even the
> truncated part.

OK, my bad.. didn't think of triple-click.  That works, thanks.

Comment 73

[Ed Morley [:emorley]]

(In reply to Nicholas Nethercote [:njn] from comment #68)
> A tiny follow-up patch:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5dab4b89eabc

https://hg.mozilla.org/mozilla-central/rev/5dab4b89eabc

Comment 74

[Jorge Villalobos [:jorgev] (he/him)]

Will about:compartments be preferred way to track zombie compartments while testing add-ons? I'd like to mention it in the Firefox 13 Compatibility post.

Comment 75

[Justin Lebar (not reading bugmail)]

(In reply to Jorge Villalobos [:jorgev] from comment #74)
> Will about:compartments be preferred way to track zombie compartments while
> testing add-ons? I'd like to mention it in the Firefox 13 Compatibility post.

Yes.  Or at least, you're welcome to continue using about:memory, but if about:compartments is somehow worse than about:memory for the purposes of tracking zombies, that's probably a bug.

Comment 76

[Nicholas Nethercote [inactive]]

https://developer.mozilla.org/en/Zombie_Compartments#Proactive_checking_of_add-ons lists steps involving about:compartments before the steps involving about:memory.