Closed Bug 656773 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: about:memory, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox6 - ---

People

(Reporter: c.ascheberg, Assigned: njn)

References

Details

Attachments

(2 files)

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
Blocks: 653630
Blocks: revampaboutmemory
No longer blocks: 653630
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.
Just to clarify:  there's nothing special about private browsing mode here, other than it causes this ":memory:" reporter to be used.
Attached patch patchSplinter Review
This patch implements the robust alternative -- it creates a separate "process" field in nsIMemoryReporter.
Assignee: nobody → nnethercote
Attachment #533893 - Flags: superreview?(benjamin)
Attachment #533893 - Flags: review?(sdwilsh)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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 ""
Attachment #533893 - Flags: review?(sdwilsh) → review+
(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.
(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.
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.
(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 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.
(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.
(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.
Oh, is nsIMemoryReporter's uuid supposed to change?
(In reply to comment #12)
> Oh, is nsIMemoryReporter's uuid supposed to change?

yes
Duplicate of this bug: 661480
Attached patch new patchSplinter Review
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.
Attachment #537068 - Flags: superreview?(roc)
Attachment #537068 - Flags: review+
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 on attachment 537068 [details] [diff] [review]
new patch

Review of attachment 537068 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #537068 - Flags: superreview?(roc) → superreview+
Duplicate of this bug: 661780
Duplicate of this bug: 661804
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).
OS: Windows Vista → All
http://hg.mozilla.org/mozilla-central/rev/caf81a228fff
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #533893 - Flags: superreview?(benjamin)
(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 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.
Attachment #537068 - Flags: approval-mozilla-aurora?
Comment on attachment 537068 [details] [diff] [review]
new patch

We don't want this on mozilla-aurora as the reward isn't really there.
Attachment #537068 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Status: RESOLVED → VERIFIED
Component: General → about:memory
QA Contact: general → about.memory
Target Milestone: --- → mozilla7
Duplicate of this bug: 679722
Duplicate of this bug: 680725
You need to log in before you can comment on or make changes to this bug.