724863 - about:memory's gTogglesBySafeId does the wrong thing if a memory reporter goes from insignificant (hidden) to significant

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

gTogglesBySafeId[nodeName] is true if the node has been toggled and false otherwise.  When we update the tree, we look to see if gTogglesBySafeId[nodeName] is true; if so, the node's visibility is flipped.

But this is the incorrect behavior if we'd toggled a node open and then, upon refresh, the node became open by default.  In that case, we'll force the node closed!

I should have caught this during review.  Sorry, Nick.

Comment 1

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #0)
> gTogglesBySafeId[nodeName] is true if the node has been toggled and false
> otherwise.

It's never false, a true entry is deleted if it's toggled.

But you're right in general.  I guess I need to record a 2-bit state instead of a 1-bit state, i.e. if the toggled node is expanded or collapsed.

How did you find this?  Did you see it happen or just work it out from the code?

Comment 2

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> (In reply to Justin Lebar [:jlebar] from comment #0)
> > gTogglesBySafeId[nodeName] is true if the node has been toggled and false
> > otherwise.
> 
> It's never false, a true entry is deleted if it's toggled.

Yes, right!

> How did you find this?  Did you see it happen or just work it out from the code?

I'd seen somewhat suspicious behavior with the button, but I noticed the problem in the code when reviewing one of your other patches.

Comment 3

[Nicholas Nethercote [inactive]]

From bug 723402:

> I guess it's OK that unhidden nodes don't count as toggle()'d.  It's bit
> weird,
> though, since if you refreshed and the memory reporter became valid, you'd
> want
> the path to it to remain expanded, so you could see the new value.  Up to you
> if you want to change it.

This should be relatively easy to fix once gTogglesBySafeId is changed to have 2-bit values.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch uses two bits for recording collapse/expand state.  It doesn't do 
the change mentioned in comment 3, it's a bit annoying to implement and 
should be rare.

Comment 5

[Justin Lebar (not reading bugmail)]

+  if (gShowSubtreesBySafeTreeId[safeTreeId]) {
+    delete gShowSubtreesBySafeTreeId[safeTreeId];
   } else {
-    gTogglesBySafeTreeId[safeTreeId] = true;
+    gShowSubtreesBySafeTreeId[safeTreeId] = isExpansion;
   }

Shouldn't the first line be

  if (gShowSubtreesBySafeTreeId[safeTreeId] !== undefined)

?  Otherwise, this isn't symmetrical. 

Ideally we'd update the test to catch this.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

You're right, good catch.  The sequence required for it to be noticeable is
as follows:

- Start with a significant sub-tree.
- Collapse it (so it's recorded with 'false' in the table).
- Expand it (the table entry should be deleted, but instead it's erroneously
  set to 'true').
- Update, *and* the sub-tree changes to insignificant -- because of the
  erroneous 'true' marking it's shown expanded when it shouldn't be.

I haven't tested for this particular sequence because of its obscurity, but
I have made the testing more thorough.  Hopefully that's enough to satisfy
you :)

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc7f2784452

Comment 8

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/bdc7f2784452