698930 - Always show negative numbers in about:memory, and highlight them

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Jesse Ruderman]

When heap-unclassified is negative, for example, it should be shown despite being below the cutoff.  When it's buggy it should be visible.

Comment 1

[Justin Lebar (not reading bugmail)]

Let's also highlight them while we're at it, so they're easier to see visually.

Comment 2

[Justin Lebar (not reading bugmail)]

Wow, I said "see visually".  I'm sorry, Mrs. Janoff.

Comment 4

[Justin Lebar (not reading bugmail)]

This is tricker than I thought it would be at first.  But I'm working on it...

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 576065 [details] [diff] [review]
Patch v1

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

On the right track, but I'd like to see another revision.

::: toolkit/components/aboutmemory/content/aboutMemory.css
@@ +43,2 @@
>    color: #400;
>  }

Why did you split .mrValue's rule in two?

@@ +79,5 @@
> +.bad-value:after {
> +  color: #b45f04;
> +  content: " (?!)";
> +  -moz-user-select: text;
> +}

We already use "[*]" for unknown values (with the corresponding style |mStar|), generated by aboutMemory.js instead of via CSS.  It shows up in the test.  Can you do something like that?  Maybe "[?]" as the symbol?

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +612,5 @@
> +    for (var i = 0; i < aNode._kids.length; i++) {
> +      markMayOmit(aNode._kids[i]);
> +      omit = omit && aNode._kids[i]._mayOmit;
> +    }
> +    aNode._mayOmit = omit;

You should mention _mayOmit in the comment within TreeNode's constructor... but as we discussed on IRC, trying to do tree value arithmetic with negative numbers is kinda bogus.  Just showing them seperately would avoid complicating the tree handling code.

@@ +740,5 @@
>    } else {
> +    // We have to handle negative values of aBytes here, rather than let
> +    // formatInt handle it for us.  We pass only the number of megabytes to
> +    // formatInt, but "-0" == 0, so formatInt won't properly handle negative
> +    // values of absolute value less than 1mb!

Handling this in formatInt would be better.  If x equals 0 or -0, in JS you can determine which it is with 1/x -- it either gives Infinity or -Infinity.

Comment 7

[Nicholas Nethercote [inactive]]

Thinking about this some more...

Sequestering negative values when the memory reporters are accessed won't detect a negative heap-unclassified, because it's derived from other reporters.  Furthermore, heap-unclassified is by far the most likely value to be negative precisely because it is the only non-trivial derived value:

- Reports from reporters are unlikely to be negative, because that'd be a pretty weird bug.

- The non-leaf nodes in the trees are also derived, but they only involve addition and so won't be negative if none of the reporters are negative.

In comparison, heap-unclassified's derivation involves a subtraction, and so any over-counting by a reporter can lead to it being negative.

With all that in mind, I wonder if it's worth only checking if heap-unclassified is negative, and if so, displaying it as "0 B (00.00%) -- heap-unclassified [?]".  That would be simple and catch the case we've actually seen in practice multiple times.

Comment 8

[Andrew McCreight [:mccr8]]

That sounds like a reasonable compromise to me, to simplify things.  Not to bikeshed, but I think the failure should be a little more emphatic than a question mark.  Would it be possible to do something like "?? B (??.??%) -- heap-unclassified"?

Comment 9

[Jesse Ruderman]

"??" sounds like "I don't know". It would be clearer to show the negative number (which is somewhat useful for figuring out which reporter is buggy) and add "WTF??" after it.

Comment 10

[Justin Lebar (not reading bugmail)]

There was that canvas memory reporter which went negative occasionally (not at my awesomebar, don't have the bug).  Any memory reporter which uses a counter, instead of walking the graph of live objects, could go negative.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: Patch 1

This patch takes a slightly different approach.  It highlights bogus values,
but doesn't do anything to ensure they aren't omitted in non-verbose mode.
However, it does print a warning at the top of the process's info (much like
is done in bug 715453) to indicate that something went wrong.  I think this
is good enough, and it's easier than the other approaches.

I added some testing, and I also beefed up the testing of |kUnknown| in 
test_aboutmemory.xul.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Patch 2

This patch does some clean-ups:

- Renames |asString| as |_asString|, for consistency with other data
  properties.

- Renames |_hasProblem| as |_isUnknown| (and similar cases), for consistency
  with |_isInvalid| and |kUnknown|.

- Removes a dead |hasProblem| local variable.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: screenshot of a highlighted invalid value

Comment 14

[Justin Lebar (not reading bugmail)]

I don't like "hey, there was an error, but we're probably hiding it.  Be skeptical."

How about we

 * switch to verbose mode upon detecting an erroneous value, or
 * display the name(s) of the erroneous reporter(s) in the error message?

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: patch 1, v2: highlight bogus values

This version reports the names of memory reporters that give bogus values.  
And because we have expandable trees now, if the entry isn't visible you can
click-to-expand and find it.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: new screenshot

New screenshot from patch 1, v2.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: patch 2, v2

This patch is basically unchanged from v1.  Carrying over the r+.

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: patch 3: make unsanitized things more obvious

Names, paths and descriptions need to be sanitized before putting them into
HTML.  The way this was done was a bit ad hoc -- it was sometimes hard to
tell if a thing had been sanitized or not, and it was making me nervous.

This patch makes things much clearer by using "safe"/"unsafe" prefixes for 
names everywhere that is appropriate.  If JS had static types I'd use them
to enforce the unsafe/safe distinction, but this is still much better than
what we had.  For example, while doing this I discovered that several paths 
in assertion failure messages weren't being sanitized -- not a big deal but
still nice to fix.

I also discovered that the description for "heap-unclassified" was 
truncated...  I missed a '+' operator and JS's stupid semi-colon insertion
slapped me in the face, sigh.

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: patch 3b: make unsanitized things more obvious

Attach the right patch this time.

Comment 20

[Justin Lebar (not reading bugmail)]

> This version reports the names of memory reporters that give bogus values.  
> And because we have expandable trees now, if the entry isn't visible you can
> click-to-expand and find it.

Now that we have click-to-expand, can we automatically expand the path to any invalid reporter?  Presumably this should be simple to do, right?

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 592023 [details] [diff] [review]
patch 3b: make unsanitized things more obvious

This is pretty ugly, but I understand the need and don't have a better idea.  r=me.

Comment 22

[Justin Lebar (not reading bugmail)]

Comment on attachment 592017 [details] [diff] [review]
patch 1, v2: highlight bogus values

>@@ -804,7 +843,7 @@ function formatBytes(aBytes)
>   } else {
>     var mbytes = (aBytes / (1024 * 1024)).toFixed(2);
>     var a = String(mbytes).split(".");
>-    s = formatInt(a[0]) + "." + a[1] + " " + unit;
>+    s = formatInt(Number(a[0])) + "." + a[1] + " " + unit;

Can you please add a comment explaining that formatInt hanadles negative
Number(a[0]), even -0?

>@@ -1053,6 +1100,15 @@ function genTreeText(aT, aProcess)
>       return s;
>     }
> 
>+    // Determine if we should show the sub-tree below this entry;  this
>+    // involves reinstate any previous toggling of the sub-tree.

reinstating.

r=me, but if you decide to expand the path down to invalid reporters (I'd like it if we did!), I'd like to take a look at that change.

Comment 23

[Nicholas Nethercote [inactive]]

> This is pretty ugly, but I understand the need and don't have a better idea.
> r=me.

Yep :/


> Now that we have click-to-expand, can we automatically expand the path to
> any invalid reporter?  Presumably this should be simple to do, right?

With the current page generation strategy -- generate a big HTML string and then set innerHTML -- it's a bit difficult.  By the time you realize you should expand parent nodes you've already generated the HTML code for them.

I'm planning to investigate instead building the DOM directly with things like document.createElement.  The main motivation is to hopefully reduce the amount of memory used when generating the page, but it would also make this expansion a lot easier because we'd have a live data structure to fiddle with instead of a half-formed string.

Comment 24

[Justin Lebar (not reading bugmail)]

> By the time you realize you should expand parent nodes you've already generated the HTML code for 
> them.

But you could still toggle() all the relevant nodes, no?

Comment 25

[Nicholas Nethercote [inactive]]

I'd have to record them somewhere and do it after setting innerHTML.  I'd rather wait and be able to do it on the spot.

Comment 26

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #25)
> I'd have to record them somewhere and do it after setting innerHTML.  I'd
> rather wait and be able to do it on the spot.

Okay, I think I understand what the issue is.  If you have a plan to change it, that's fine.

Comment 27

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cba1d921a6d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f989793c20bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/50080b09ae04

Comment 28

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #23)
> > This is pretty ugly, but I understand the need and don't have a better idea.
> > r=me.
> 
> Yep :/

I just learnt that if you don't generate HTML strings but instead build the page via DOM methods and properties you don't need to escape all those characters.  Still, I'll probably keep the safe/unsafe names as long as the backslash flipping is needed.  (I do hope to get rid of that one day.)

Comment 29

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/cba1d921a6d5
https://hg.mozilla.org/mozilla-central/rev/f989793c20bd
https://hg.mozilla.org/mozilla-central/rev/50080b09ae04