333689 - [@ nsCertTree::ToggleOpenState]

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

this code is both silly and crashy. would someone please save it?

Comment 1

[:Gijs (he/him)]

Attachment: Patch, I hope

I think this is enough to fix this bug. First C++ patch ever, though. So have mercy on my foolishness.

Comment 2

[:Gijs (he/him)]

Oops. Keeping this on people's radards.

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 218153 [details] [diff] [review]
Patch, I hope

r=kengert

Comment 4

[[e-mail]]

Comment on attachment 218153 [details] [diff] [review]
Patch, I hope

>-  if (el) el->open = !el->open;
>-  PRInt32 fac = (el->open) ? 1 : -1;
>-  if (mTree) mTree->RowCountChanged(index, fac * el->numChildren);
>+  if (el) {
>+    el->open = !el->open;
>+    PRInt32 fac = (el->open) ? 1 : -1;
>+    if (mTree) mTree->RowCountChanged(index, fac * el->numChildren);
>+  }
>   mSelection->Select(index);
My feeling is that this, the only call of mSelection in this file, is to work around the bug in the previous line. I won't keep you in suspense; the bug is that when you toggle a row then its children get added or removed after, not before the row, and thus the first parameter to RowCountChanged is 1 too low. 
sr=me with that fixed, or better still if kaie warms to the idea of removing the call to mSelection->Select(index); too ;-) Oh, and I prefer the idea of el->open ? el->numChildren : -el->numChildren rather than the multiplication.

Comment 5

[Kai Engert [:KaiE:]]

Neil, you say, in addition to the missing null check, you see an additional bug.

And you propose to fix that bug as well.

According to the explanation and examples on http://xulplanet.com/tutorials/xultu/treeviewdet.html I agree, the correct first parameter to RowCountChanged seems to be (index+1).

I agree we could simplify the code, and remove the multiplication.

I don't understand why you propose to remove the call to ->Select().
What happens if a child item is selected, and the parent is collapsed?
The selected child will no longer be visible.
I agree the authors of the intended to select the nearest reasonable item instead.
I'm fine with that, I propose we keep this behaviour unless there is a good reason.

So can we agree on this?

  if (el) {
    el->open = !el->open;
    if (mTree) {
      PRInt changeAmount = el->open ? el->numChildren : -el->numChildren;
      mTree->RowCountChanged(index + 1, changeAmount);
    }
  }
  mSelection->Select(index);

Comment 6

[Kai Engert [:KaiE:]]

s/PRInt/PRInt32/

Comment 7

[[e-mail]]

(In reply to comment #5)
>I don't understand why you propose to remove the call to ->Select().
>What happens if a child item is selected, and the parent is collapsed?
>The selected child will no longer be visible.
Actually trees already know about that, see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tree.xml&rev=1.1&root=/cvsroot&mark=750-751#745

Comment 8

[Kai Engert [:KaiE:]]

ok, I'm fine with removing the ->Select() call

Comment 9

[:Gijs (he/him)]

Attachment: New Patch

Someone had better test whether this compiles, since I want to go to bed and make doesn't like my ancient tree enough to do its work (and I really don't want to do a full new build and wait for an hour to tell if it works). My excuse for doing it at this horrific time of night is "timeless made me do it".

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 221113 [details] [diff] [review]
New Patch

it compiles

Comment 11

[:Gijs (he/him)]

Yeah, so, timeless totally committed this on the 11th. Oops. Or something.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsCertTree.cpp&rev=1.47#872