656773 - about:memory error in private browsing mode, page is blank

Status: VERIFIED FIXED

(Toolkit :: about:memory, defect)

Description

[Christian Ascheberg]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:6.0a1) Gecko/20110512 Firefox/6.0a1
Build Identifier: http://hg.mozilla.org/mozilla-central/rev/2a0fbd6eedbd

Error: mappedHeapUsedTmr is undefined
Source File: chrome://global/content/aboutMemory.js
Line: 241

Reproducible: Always

Steps to Reproduce:
1. run firefox, start private browsing mode
2. open about:memory
3. wait some time / refresh page a few times

Actual Results:  
about:memory is blank, above error shown in console

Expected Results:  
about:memory should be displayed
seems to work fine in non-private browsing mode

Comment 1

[Nicholas Nethercote [inactive]]

I found the problem.  In a memory reporter's path, ':' has a special meaning.  It's used to separate the process name from the rest of the path.  But it turns out that storage/ can create a reporter with the name "heap-used/storage/sqlite/:memory:/cache-used".

The obvious thing to do is choose a different special char to indicate the separation.  Maybe '^'?  Seems unlikely to be used in a reporter's path.

A more robust alternative, but one that requires more work, would be to change nsIMemoryReporter so that the process is stored separately.

Comment 2

[Nicholas Nethercote [inactive]]

Just to clarify:  there's nothing special about private browsing mode here, other than it causes this ":memory:" reporter to be used.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch implements the robust alternative -- it creates a separate "process" field in nsIMemoryReporter.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 533893 [details] [diff] [review]
patch

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

r=sdwilsh

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +242,5 @@
>   *
>   * @param aProcess
>   *        The name of the process
> + * @param aMrs
> + *        Table of memory reporters for this process

I find aMrs to be a poor variable name.  aTmrs was at least close to timers such that it was obvious, but I find I keep having to look up what aMrs is.  Can we go with aReporters or aMemReporters here?

@@ +545,5 @@
>   * Gets the byte count for a particular memory reporter and sets its _done
>   * property.
>   *
> + * @param aMrs
> + *        Table of memory reporters for this process

ditto here regarding aMrs (in fact, it applies elsewhere in your patch, but I'll stop commenting on it)

::: xpcom/base/nsIMemoryReporter.idl
@@ +43,5 @@
>  [scriptable, uuid(d298b942-3e66-4cd3-9ff5-46abc69147a7)]
>  interface nsIMemoryReporter : nsISupports
>  {
>    /*
> +   * The name of the process containing this reporter.  If it's "" it

nit: I think you want a comma after the ""

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> > + * @param aMrs
> > + *        Table of memory reporters for this process
> 
> I find aMrs to be a poor variable name.  aTmrs was at least close to timers
> such that it was obvious, but I find I keep having to look up what aMrs is. 
> Can we go with aReporters or aMemReporters here?

aTmrs wasn't short for 'timers' :)

Would aMRs be better?  It's just that I use that name (and slight variations) all over the place, and I find Huffman encoding (common things deserve short names) to be a good principle.

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> aTmrs wasn't short for 'timers' :)
Bah, right.  That's what I get for doing too many reviews in a day.

> Would aMRs be better?  It's just that I use that name (and slight variations)
> all over the place, and I find Huffman encoding (common things deserve short
> names) to be a good principle.
I usually agree with that, but in this case, aMRs doesn't really convey what it is still.  It's great for people who know the code, but someone new approaching it will have a harder time understanding it.  I'd really prefer aReporters.

Comment 7

[Nicholas Nethercote [inactive]]

How's this for a compromise?  Change this:

    for (var _tpath in aTmrs) {
      var tmr = aTmrs[_tpath];

To this:

    for (var _tpath in aReporters) {
      var r = aReporters[_tpath];


So "aReporters" when it's the full table, but 'r' for a single reporter.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> So "aReporters" when it's the full table, but 'r' for a single reporter.
That works great, in fact I use that trick all the time (but I suggest you use let instead of var).

Comment 9

[Benjamin Smedberg]

Comment on attachment 533893 [details] [diff] [review]
patch

This doesn't make a lot of sense to me: it seems that you're making most of the memory reporters return "" for the process field unconditionally: but that's obviously incorrect for Fennec/Firefox with content processes, unless there's something in the remoting layer which sets a "better" process field for them.

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> Comment on attachment 533893 [details] [diff] [review] [review]
> patch
> 
> This doesn't make a lot of sense to me: it seems that you're making most of
> the memory reporters return "" for the process field unconditionally: but
> that's obviously incorrect for Fennec/Firefox with content processes, unless
> there's something in the remoting layer which sets a "better" process field
> for them.

Yes, all reporters start out with "" for the process.  When the parent asks the child process for its reporters, the "" gets replaced.  See http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#325  It's hard to see from this patch because this patch doesn't change ContentChild.cpp.

So I haven't changed the function at all;  it's just that previously the process (be it "" or something else) was a prefix on the 'path', and now it's a separate field.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to comment #10)
> 
> Yes, all reporters start out with "" for the process.

I will add a comment to nsIMemoryReporter.idl to explain this.

Comment 12

[Nicholas Nethercote [inactive]]

Oh, is nsIMemoryReporter's uuid supposed to change?

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> Oh, is nsIMemoryReporter's uuid supposed to change?

yes

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: new patch

Updated patch:  adds a comment about always using the empty string for the process, and updates the uuid for nsIMemoryReporter.idl.

Trying a different super-reviewer.  roc, nsIMemoryReporter::path previously had a path with an optional process in front of it (with a ':' as a separator between the process and path).  E.g. it could be "a/b/c" or "foo:a/b/c".  This patch just splits off the process into it's own field, nsIMemoryReporter::process.  This avoids the private browsing problem, which was that some reporter paths could contain ':' and so the string splitting got all confused.

Comment 16

[Nicholas Nethercote [inactive]]

I added |tracking-firefox6?| because Firefox 6 includes the revamped about:memory, and this is a minor bug from that revamp that has been reported twice (here and in bug 661480).

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 537068 [details] [diff] [review]
new patch

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

Comment 20

[AndreiD[QA]]

The issue is reproducible, also, on Windows 7 x86:
 Mozilla/5.0 (Windows NT 6.1; rv:6.0a2) Gecko/20110602 Firefox/6.0a2

*Changing the platform to All. Reproducible on Mac, also (reported in bug 661780).

Comment 21

[Nicholas Nethercote [inactive]]

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

Comment 22

[Johnathan Nightingale [:johnath]]

(In reply to comment #16)
> I added |tracking-firefox6?| because Firefox 6 includes the revamped
> about:memory, and this is a minor bug from that revamp that has been
> reported twice (here and in bug 661480).

Yeah, I hear that - we discussed this one in triage today and felt that it didn't need to be on the FF6 short list, but we'd like to see an approval request with a cost/benefit analysis - it's already baking on trunk so hopefully it's an easy calculus?

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 537068 [details] [diff] [review]
new patch

Requesting approval-mozilla-aurora.  The patch fixes a problem several people have reported.  I think the risk is low, it really just splits one string in nsIMemoryReporter that had two parts separated by ':' into two.  And it has baked on trunk for a while without problem.

Comment 24

[LegNeato]

Comment on attachment 537068 [details] [diff] [review]
new patch

We don't want this on mozilla-aurora as the reward isn't really there.

Comment 25

[AndreiD[QA]]

Verified fixed on the latest Nightly:
Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110703 Firefox/7.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110704 Firefox/7.0a1
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110704 Firefox/7.0a1