661474 - Add per-compartment memory reporters

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 625305 proposed a heavyweight approach to getting per-compartment memory information, introducing a new page (about:compartments) and two new XPCOM interfaces (nsICompartmentInfoReceiver and nsICompartmentInfoService).

I've been working on a more lightweight approach that incorporates per-compartment memory reporting into about:memory using the existing nsIMemoryReporter interface.  Basically, each time a new compartment is created, several memory reporters are attached to it (using CompartmentPrivate and CompartmentCallback).  Here's example output from about:memory:

151,701,046 B (100.0%) -- explicit
├───99,059,528 B (65.30%) -- js
│   ├──81,788,928 B (53.91%) -- gc-heap
│   ├───8,388,608 B (05.53%) -- stack-space-committed
│   ├───5,083,796 B (03.35%) -- compartment([System Principal])
│   │   ├──2,183,314 B (01.44%) -- scripts
│   │   ├──1,966,080 B (01.30%) -- mjit-code
│   │   ├────476,552 B (00.31%) -- object-slots
│   │   ├────280,752 B (00.19%) -- tjit-data
│   │   │    ├──148,000 B (00.10%) -- allocators-reserve
│   │   │    └──132,752 B (00.09%) -- allocators-main
│   │   ├────131,072 B (00.09%) -- tjit-code
│   │   └─────46,026 B (00.03%) -- string-chars
│   ├───1,881,163 B (01.24%) -- compartment(http:||www.theage.com.au|)
│   │   ├────720,896 B (00.48%) -- mjit-code
│   │   ├────633,403 B (00.42%) -- scripts
│   │   ├────244,464 B (00.16%) -- tjit-data
│   │   │    ├──148,000 B (00.10%) -- allocators-reserve
│   │   │    └───96,464 B (00.06%) -- allocators-main
│   │   ├────132,296 B (00.09%) -- object-slots
│   │   ├────131,072 B (00.09%) -- tjit-code
│   │   └─────19,032 B (00.01%) -- string-chars
│   ├─────537,592 B (00.35%) -- compartment(atoms)
│   │     ├──286,056 B (00.19%) -- string-chars
│   │     ├──186,000 B (00.12%) -- tjit-data
│   │     │  ├──148,000 B (00.10%) -- allocators-reserve
│   │     │  └───38,000 B (00.03%) -- allocators-main
│   │     ├───65,536 B (00.04%) -- mjit-code
│   │     ├────────0 B (00.00%) -- scripts
│   │     ├────────0 B (00.00%) -- object-slots
│   │     └────────0 B (00.00%) -- tjit-code
│   ├─────502,796 B (00.33%) -- mjit-data
│   ├─────354,858 B (00.23%) -- string-chars
│   ├─────267,601 B (00.18%) -- compartment(http:||view.atdmt.com|AUS|iview|282680669|direct|01|5363015?click=http:||ad-apac.doubleclick.net|6k%3Bh%3Dv8|3b1a|3|0|%2a|v%3B239211737%3B0-0%3B0%3B52684720%3B4307-300|250%3B39598670|39616457|1%3B%3B%7Eaopt%3D3|1|a341|2%3B%7Esscs%3D%3f)
│   │     ├──186,000 B (00.12%) -- tjit-data
│   │     │  ├──148,000 B (00.10%) -- allocators-reserve
│   │     │  └───38,000 B (00.03%) -- allocators-main
│   │     ├───65,536 B (00.04%) -- mjit-code
│   │     ├────8,608 B (00.01%) -- string-chars
│   │     ├────5,168 B (00.00%) -- object-slots
│   │     ├────2,289 B (00.00%) -- scripts
│   │     └────────0 B (00.00%) -- tjit-code
│   ├─────254,186 B (00.17%) -- compartment(moz-nullprincipal:{de6ee864-7989-4b00-ab8c-c446d6524313})
│   │     ├──186,000 B (00.12%) -- tjit-data
│   │     │  ├──148,000 B (00.10%) -- allocators-reserve
│   │     │  └───38,000 B (00.03%) -- allocators-main
│   │     ├───65,536 B (00.04%) -- mjit-code
│   │     ├────2,464 B (00.00%) -- object-slots
│   │     ├──────186 B (00.00%) -- scripts
│   │     ├────────0 B (00.00%) -- string-chars
│   │     └────────0 B (00.00%) -- tjit-code
│   └───────────0 B (00.00%) -- stack-space-uncommitted


Some consequences of this approach:

- Because bug 625305 introduced a new interface, it could report more things,
  e.g. things other than byte counts (such as object counts).

- You can no longer easily get a number like "mjit-code for the entire process";  if you want that you have to manually add up mjit-code for each compartment.  I think per-compartment totals are more interesting, though.

Other things to note:

- One thing to note is that "js/gc-heap" is not yet split between compartments.  This is because it currently tracks at the level of Chunks, and each Chunk contains multiple arenas, and these arenas might belong to several different compartments.

- Also note that this output is from a stack of patches I have, which is why it includes reporters like "object-slots" and "string-chars" (from in-progress bug 571249) and "js/stack-space-{,un}committed" (from in-progress bug 546477).

- The URLS in the compartment names have had all '/' chars replaced with '|'.  This is a temporary hack because '/' is already in-use as the separator in memory reporter paths.

Things I've already found with this per-compartment reporting

- The atomsCompartment is allocating space for mjit and tjit code unnecessarily (bug 661068).

- On some sites we create heaps of tiny compartments.  E.g. techcrunch.com's front page has over 70, most of them are just for Facebook "like" buttons that are attached to each story.  If we allocated the mjit and tjit space for them more lazily it would save some more space.  (No bug filed yet for that.)

Thoughts about how this compares with bug 625305 are welcome.  Thanks.

Comment 1

[The 8472]

> - You can no longer easily get a number like "mjit-code for the entire
> process";  if you want that you have to manually add up mjit-code for each
> compartment.  I think per-compartment totals are more interesting, though.

As i am suffering from severe tabitis this would make about:memory very verbose, i think all compartments should be tallied up by default, showing the overall stats and only break it down to individual compartments when you unfold it.

There are basically two different needs:
a) identify which site gobbles up memory
b) identify which mozilla component gobbles up memory

Both views are needed.


> - On some sites we create heaps of tiny compartments.  E.g. techcrunch.com's
> front page has over 70, most of them are just for Facebook "like" buttons
> that are attached to each story.  If we allocated the mjit and tjit space
> for them more lazily it would save some more space.  (No bug filed yet for
> that.)
If they're from the same origin domain, wouldn't it be better to re-use compartments?

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> 
> As i am suffering from severe tabitis this would make about:memory very
> verbose, i think all compartments should be tallied up by default, showing
> the overall stats and only break it down to individual compartments when you
> unfold it.

about:memory isn't very flexible at the moment, that's true.  In the default (non-verbose) mode any sub-tree that accounts for less than 0.5% of explicit memory usage is omitted, though, so if you have 100 tabs open you'll only get entries for some of them.

The interface to the memory reporters also isn't very flexible, which makes presenting multiple views (eg. split by compartment first vs. split by component first) very difficult to do.  I'm thinking about how to improve that, but I think per-compartment reporters will be useful enough that it's worth doing them sooner rather than later.

> > - On some sites we create heaps of tiny compartments.
> If they're from the same origin domain, wouldn't it be better to re-use
> compartments?

See bug 664067.

Comment 3

[The 8472]

(In reply to comment #2)
> so if you have 100 tabs open you'll only get entries for some of them.
Which would - theoretically - mean that if I have 200+ evenly loaded compartments i would get no information at all.

I know this is not a particularly common or important case, but for me it could actually end up making about:memory less useful than it is in its current state.

Comment 4

[Nicholas Nethercote [inactive]]

So then you can fallback to the verbose view.  I understand the presentation has shortcomings, but IMO the value of the per-compartment information outweighs any reduction in usability.  I've already filed bug 661068 and bug 664067 based on the output, and I've barely even used it yet.  It'll also help with finding leaks.

Comment 5

[The 8472]

Hah, totally forgot about the verbose view.

Comment 6

[Nicholas Nethercote [inactive]]

Here's the non-verbose output for www.cad-comic.com/cad/.  Note the truncated compartment names:

80.04 MB (100.0%) -- explicit
├──36.54 MB (45.66%) -- js
│  ├──15.00 MB (18.74%) -- gc-heap
│  ├───8.00 MB (10.00%) -- stack
│  ├───5.41 MB (06.76%) -- compartment(http://www.cad-comic.com/)
│  │   ├──2.06 MB (02.58%) -- mjit-code
│  │   ├──1.98 MB (02.47%) -- scripts
│  │   ├──0.51 MB (00.64%) -- tjit-data
│  │   │  └──0.51 MB (00.64%) -- (2 omitted)
│  │   ├──0.49 MB (00.62%) -- mjit-data
│  │   └──0.37 MB (00.46%) -- (3 omitted)
│  ├───4.37 MB (05.46%) -- compartment([System Principal])
│  │   ├──2.01 MB (02.51%) -- scripts
│  │   ├──1.19 MB (01.48%) -- mjit-code
│  │   ├──0.75 MB (00.94%) -- (4 omitted)
│  │   └──0.42 MB (00.53%) -- object-slots
│  ├───1.32 MB (01.65%) -- (5 omitted)
│  ├───1.15 MB (01.44%) -- compartment(http://www.facebook.com/plugins/like.php...)
│  │   └──1.15 MB (01.44%) -- (7 omitted)
│  ├───0.78 MB (00.98%) -- compartment(http://s7.addthis.com/static/r07/sh44.ht...)
│  │   └──0.78 MB (00.98%) -- (7 omitted)
│  └───0.51 MB (00.64%) -- compartment(atoms)
│      └──0.51 MB (00.64%) -- (7 omitted)


Here's the verbose output, which doesn't truncate compartment names.

83,873,012 B (100.0%) -- explicit
├──38,383,434 B (45.76%) -- js
│  ├──15,728,640 B (18.75%) -- gc-heap
│  ├───8,388,608 B (10.00%) -- stack
│  ├───5,828,691 B (06.95%) -- compartment(http://www.cad-comic.com/)
│  │   ├──2,293,760 B (02.73%) -- mjit-code
│  │   ├──2,072,485 B (02.47%) -- scripts
│  │   ├────545,536 B (00.65%) -- mjit-data
│  │   ├────537,312 B (00.64%) -- tjit-data
│  │   │    ├──296,000 B (00.35%) -- allocators-reserve
│  │   │    └──241,312 B (00.29%) -- allocators-main
│  │   ├────206,208 B (00.25%) -- object-slots
│  │   ├────131,072 B (00.16%) -- tjit-code
│  │   └─────42,318 B (00.05%) -- string-chars
│  ├───4,811,382 B (05.74%) -- compartment([System Principal])
│  │   ├──2,113,958 B (02.52%) -- scripts
│  │   ├──1,441,792 B (01.72%) -- mjit-code
│  │   ├────444,040 B (00.53%) -- object-slots
│  │   ├────380,368 B (00.45%) -- mjit-data
│  │   ├────234,384 B (00.28%) -- tjit-data
│  │   │    ├──148,000 B (00.18%) -- allocators-reserve
│  │   │    └───86,384 B (00.10%) -- allocators-main
│  │   ├────131,072 B (00.16%) -- tjit-code
│  │   └─────65,768 B (00.08%) -- string-chars
│  ├───1,205,949 B (01.44%) -- compartment(http://www.facebook.com/plugins/like.php?href=http%3A%2F%2Fwww.cad-comic.com%2Fcad%2F20110617&layout=button_count&show_faces=false&width=100&action=like&font=arial&)
│  │   ├────392,160 B (00.47%) -- tjit-data
│  │   │    ├──296,000 B (00.35%) -- allocators-reserve
│  │   │    └───96,160 B (00.11%) -- allocators-main
│  │   ├────330,697 B (00.39%) -- scripts
│  │   ├────262,144 B (00.31%) -- mjit-code
│  │   ├────131,072 B (00.16%) -- tjit-code
│  │   ├─────46,760 B (00.06%) -- object-slots
│  │   ├─────42,024 B (00.05%) -- mjit-data
│  │   └──────1,092 B (00.00%) -- string-chars
│  ├─────818,783 B (00.98%) -- compartment(http://s7.addthis.com/static/r07/sh44.html#)
│  │     ├──220,272 B (00.26%) -- tjit-data
│  │     │  ├──148,000 B (00.18%) -- allocators-reserve
│  │     │  └───72,272 B (00.09%) -- allocators-main
│  │     ├──196,608 B (00.23%) -- mjit-code
│  │     ├──194,459 B (00.23%) -- scripts
│  │     ├──131,072 B (00.16%) -- tjit-code
│  │     ├───39,984 B (00.05%) -- object-slots
│  │     ├───34,324 B (00.04%) -- mjit-data
│  │     └────2,064 B (00.00%) -- string-chars
│  ├─────537,468 B (00.64%) -- compartment(atoms)
│  │     ├──285,932 B (00.34%) -- string-chars
│  │     ├──186,000 B (00.22%) -- tjit-data
│  │     │  ├──148,000 B (00.18%) -- allocators-reserve
│  │     │  └───38,000 B (00.05%) -- allocators-main
│  │     ├───65,536 B (00.08%) -- mjit-code
│  │     ├────────0 B (00.00%) -- scripts
│  │     ├────────0 B (00.00%) -- object-slots
│  │     ├────────0 B (00.00%) -- mjit-data
│  │     └────────0 B (00.00%) -- tjit-code
│  ├─────287,647 B (00.34%) -- compartment(http://openx.blindferret.com/www/delivery/afr.php?zoneid=63&target=_blank)
│  │     ├──186,000 B (00.22%) -- tjit-data
│  │     │  ├──148,000 B (00.18%) -- allocators-reserve
│  │     │  └───38,000 B (00.05%) -- allocators-main
│  │     ├───65,536 B (00.08%) -- mjit-code
│  │     ├───32,320 B (00.04%) -- object-slots
│  │     ├────2,767 B (00.00%) -- scripts
│  │     ├────1,024 B (00.00%) -- string-chars
│  │     ├────────0 B (00.00%) -- mjit-data
│  │     └────────0 B (00.00%) -- tjit-code
│  ├─────267,532 B (00.32%) -- compartment(about:home)
│  │     ├──186,000 B (00.22%) -- tjit-data
│  │     │  ├──148,000 B (00.18%) -- allocators-reserve
│  │     │  └───38,000 B (00.05%) -- allocators-main
│  │     ├───65,536 B (00.08%) -- mjit-code
│  │     ├───10,864 B (00.01%) -- object-slots
│  │     ├────4,898 B (00.01%) -- scripts
│  │     ├──────234 B (00.00%) -- string-chars
│  │     ├────────0 B (00.00%) -- mjit-data
│  │     └────────0 B (00.00%) -- tjit-code
│  ├─────254,532 B (00.30%) -- compartment(http://www.cad-comic.com/cad/)
│  │     ├──186,000 B (00.22%) -- tjit-data
│  │     │  ├──148,000 B (00.18%) -- allocators-reserve
│  │     │  └───38,000 B (00.05%) -- allocators-main
│  │     ├───65,536 B (00.08%) -- mjit-code
│  │     ├────2,592 B (00.00%) -- object-slots
│  │     ├──────404 B (00.00%) -- scripts
│  │     ├────────0 B (00.00%) -- string-chars
│  │     ├────────0 B (00.00%) -- mjit-data
│  │     └────────0 B (00.00%) -- tjit-code
│  └─────254,202 B (00.30%) -- compartment(moz-nullprincipal:{ec481ca4-6219-4f0d-8a1d-9e312765496a})
│        ├──186,000 B (00.22%) -- tjit-data
│        │  ├──148,000 B (00.18%) -- allocators-reserve
│        │  └───38,000 B (00.05%) -- allocators-main
│        ├───65,536 B (00.08%) -- mjit-code
│        ├────2,464 B (00.00%) -- object-slots
│        ├──────202 B (00.00%) -- scripts
│        ├────────0 B (00.00%) -- string-chars
│        ├────────0 B (00.00%) -- mjit-data
│        └────────0 B (00.00%) -- tjit-code


This is a great tool for understanding just how much crap websites foist on their users!  (blindferret.com is an advertising/publishing/media company, in case you were wondering.)

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: JS and xpconnect changes (against TM 70766:e3c7aa315ca5)

This is the JS and xpconnect changes:

- Adds per-compartment reporters to xpcjsruntime.cpp.  A lot of the relevant
  changes involving tweaking existing reporters so that they are
  per-compartment.

- The per-compartment reporters are stored in CompartmentPrivate.  I added a
  JSCOMPARTMENT_SETPRIV callback operation for compartment callbacks.  This
  was necessary because we can't register the memory reporters until the
  compartment's private has been set, which happens after it's
  constructed.  A bit hacky, but seems hard to avoid something like this.

- GetGCChunk was dead, as was the overloading of ReleaseGCChunk that took a
  jsuword as the 2nd argument, so I removed them both.

- Changes the mjit-data reporters to be computed on demand by traversing
  data structures, instead of keeping around the current value.  This make
  mjit-data more like other JS memory reporters.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: toolkit changes (against TM 70766:e3c7aa315ca5)

toolkit changes:

- No longer includes the names of omitted sub-trees in the tool-tip for an
  "omitted" entry, because they can get really big now.

- Sort "omitted" entries correctly.

- Adds some code to handle compartment names having forward slashes in them,
  and some code to truncate compartment names in non-verbose mode, because
  they can be very long.

Comment 9

[avada]

What does "heap-unclassified" include? It's usually responsible for 50%+ of firefox's memory usage when I check.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

heap-unclassified = all of the memory the allocator knows it's allocated - the total the various memory reporters have reported.

In other words, it's things that aren't hooked up yet.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> What does "heap-unclassified" include? It's usually responsible for 50%+ of
> firefox's memory usage when I check.

We're working on reducing it, see bug 563700.

Comment 12

[Nicholas Nethercote [inactive]]

I cancelled the reviews because bug 666075 will make this much easier.

Comment 13

[Mardeg]

Once bug 591737 lands would <summary> and <details> be useful to have throughout the display?

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> Once bug 591737 lands would <summary> and <details> be useful to have
> throughout the display?

Maybe, though bug 654917 is a suggestion for a more comprehensive overhaul of how the information in about:memory is presented.

Comment 15

[Nicholas Nethercote [inactive]]

I'm making good progress on rewriting the patches to use memory multi-reporters (bug 666075).  It's much nicer that way, there's no need to register/unregister patches every time a compartment is created/destroyed, no need to store stuff in CompartmentPrivate, and I'll end up getting a more detailed breakdown of the gc-heap measurement.  I should have a patch up for review early next week.

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 539988 [details] [diff] [review]
toolkit changes (against TM 70766:e3c7aa315ca5)

After reworking, the toolkit portion was unchanged.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: JS and xpconnect changes, v2, on top of patch from bug 666075

JS and xpconnect changes:

- Changes the mjit-data reporters to be computed on demand by traversing
  data structures, instead of keeping around the current value.  This make
  mjit-data more like other JS memory reporters.

- GetGCChunk was dead, as was the overloading of ReleaseGCChunk that took a
  jsuword as the 2nd argument, so I removed them both.

- Completely overhauls IterateCells.  It now gets three callbacks:  one for
  compartments, one for arenas, one for cells.  The name changed to
  IterateCompartmentsArenasCells accordingly.  The |compartment| and
  |traceKind| arguments are no longer necessary.

- Adds per-compartment reporters to xpcjsruntime.cpp.  Uses the new
  multi-reporter interface (see bug 666075) so that a single traversal of
  the JS heap (in XPConnectJSCompartmentsMultiReporter) can be used to make
  all the measurements we need.


Here's the verbose output for a single compartment:

│  ├───9,377,488 B (13.60%) -- compartment(http://www.nytimes.com/)
│  │   ├──4,485,120 B (06.50%) -- gc-heap
│  │   │  ├──2,614,928 B (03.79%) -- objects
│  │   │  ├──1,342,400 B (01.95%) -- shapes
│  │   │  ├────374,016 B (00.54%) -- strings
│  │   │  ├─────64,288 B (00.09%) -- arena-unused
│  │   │  ├─────63,208 B (00.09%) -- arena-padding
│  │   │  ├─────26,280 B (00.04%) -- arena-headers
│  │   │  └──────────0 B (00.00%) -- xml
│  │   ├──1,875,968 B (02.72%) -- mjit-code
│  │   ├──1,523,110 B (02.21%) -- scripts
│  │   ├────445,788 B (00.65%) -- mjit-data
│  │   ├────440,118 B (00.64%) -- string-chars
│  │   ├────241,928 B (00.35%) -- object-slots
│  │   ├────234,384 B (00.34%) -- tjit-data
│  │   │    ├──148,000 B (00.21%) -- allocators-reserve
│  │   │    └───86,384 B (00.13%) -- allocators-main
│  │   └────131,072 B (00.19%) -- tjit-code

There's also now js/gc-heap-chunk-unused and js/gc-heap-chunk-admin
reporters.

I think we're going to get some interesting data on fragmentation from
these new reporters.

Comment 18

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

Comment on attachment 542074 [details] [diff] [review]
JS and xpconnect changes, v2, on top of patch from bug 666075

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

Looks good to me. Perhaps eventually the GC can provide a stats API so that we can hide some of these calculations better. Some of this stuff will change soon and I'm afraid we'll forget to update xpcjsruntime.cpp.

::: js/src/jsgc.cpp
@@ +2797,1 @@
>  {

Please stick in a CHECK_REQUEST(cx) here to make sure we're in a request.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ +1431,5 @@
> +
> +    #endif  // JS_TRACER
> +
> +    static void
> +    CompartmentCallback(JSContext *cx, void *vdata, JSCompartment *compartment) {

I don't know what style is preferred in XPConnect, but it seems like your bracing isn't consistent here with the rest of the functions. Could you give the whole thing a once-over?

@@ +1526,5 @@
> +                NS_ERROR("couldn't create context for memory tracing");
> +                return NS_ERROR_FAILURE;
> +            }
> +            js::IterateCompartmentsArenasCells(cx, &data, CompartmentCallback, ArenaCallback,
> +                                               CellCallback);

Please wrap this with JS_BeginRequest(cx) and JS_EndRequest(cx).

@@ +1670,3 @@
>  
> +        callback->Callback(p, NS_LITERAL_CSTRING("explicit/js/gc-heap-chunk-unused"),
> +                           nsIMemoryReporter::MR_HEAP,

Shouldn't this be JS_GC_HEAP_KIND?

@@ +1672,5 @@
> +                           nsIMemoryReporter::MR_HEAP,
> +                           NS_LITERAL_CSTRING(
> +    "Memory on the garbage-collected JavaScript heap, within chunks, that "
> +    "could be holding useful data but currently isn't."),
> +                           gcHeapChunkUnused, closure);

Do you want to subtract gcHeapChunkAdmin out of gcHeapChunkUnused?

@@ +1677,3 @@
>  
> +        callback->Callback(p, NS_LITERAL_CSTRING("explicit/js/gc-heap-chunk-admin"),
> +                           nsIMemoryReporter::MR_HEAP,

Seems like this should also be JS_GC_HEAP_KIND.

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 539988 [details] [diff] [review]
toolkit changes (against TM 70766:e3c7aa315ca5)

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

*yoink* ...stealing from sdwilsh.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +621,5 @@
> +function truncateCompartmentName(aStr)
> +{
> +  return (gVerbose)
> +       ? aStr
> +       : aStr.replace(/compartment\((.{40}).*\)/, 'compartment($1...)');

You could do this with CSS instead of script, thanks to our fancy new text-overflow: ellipsis. Which would also give you a proper ellipsis (…) instead of three periods.

But I suspect the overlap between "typography geek" and "memory analysis geek" is small enough that it doesn't matter, so fine as-is.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: toolkit changes, v2

This version sanitizes URLs better, and also updates the name of the JS GC heap reporter in Telemetry.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: toolkit changes, v3

Like v2, but I added a test for the URL sanitizing.  I confirmed that the alert pops up if escapeAll() is replaced with escapeQuotes().

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 542718 [details] [diff] [review]
toolkit changes, v3

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

Thanks for fixing this. :)

Comment 23

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fb03584dd823

Comment 24

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/fb03584dd823

Comment 25

[Vlad [QA]]

Setting resolution to Verified Fixed on 
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101) Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101) Firefox/7.0