Closed Bug 724863 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: about:memory, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: njn)

Details

Attachments

(1 file, 1 obsolete file)

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.
(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?
(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.
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.
Assignee: nobody → n.nethercote
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #607069 - Flags: review?(justin.lebar+bug)
+  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.
Attached patch patch, v2Splinter Review
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 :)
Attachment #607069 - Attachment is obsolete: true
Attachment #607069 - Flags: review?(justin.lebar+bug)
Attachment #607397 - Flags: review?(justin.lebar+bug)
Attachment #607397 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/mozilla-central/rev/bdc7f2784452
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.