115894 - expanding a thread with right arrow stops working after collapsing a selected thread

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P2)

Description

[kmurray1115]

2001-12-17-03
Win2K
This has happened for quite a while - it seems that if you leave Mail open and
return after about an hour or so, we leak up to about 36MB, and threaded mail
seems to no longer behave as expected. Specifically, when expanding collapsed
threads, they don't actually expand to expose children. However, the twisty does
move to the expanded position. I only see this behavior after the app leaks.

Comment 1

[scottputterman]

Unfortunately our app is always leaking. Do you have any steps to reproduce the
problem?  I leave my app running all of the time and have never seen this.

Comment 2

[kmurray1115]

Are you always in threaded mode? The steps are as mentioned before - leave mail
open for about 30 minutes, come back, and threads stop expanding as expected.
The twisty moves, but the corresponding messages are not exposed.

Comment 3

[kmurray1115]

This now happens 90% of the time, and doesn't require the machine to be on for a
while at all. I believe it to be the method in which I interact with threads,
but I can't reproduce the *exact* steps yet. it seems related to loading a
message and  while that message loads, clicking on a twisty to expand a thread
momentarily corrupts the thread pane. Subsequent attempts to collapse then
expand the thread moves each remaining message below the thread in question up
on, seemingly deleting a message *into* the uppper thread. However, I know that
the messages aren't really disappearing, becasue after I restart mail (not the
app, just mail), the thread pane is back to normal. I will keep trying to find
out how I get into that situation, but it happens almost every time I launch
mail and expand a thread. 

cahnging the summary to remove the "leak" inference, as i don't think that's the
problem. i speculate that it has something to do with a conflict that occurs
while we try to paint an expanded thread. Something is going on that prevents it
from occurring normally and reliably. 

Last build this was evident: 2001-01-25-03

Comment 4

[David :Bienvenu]

there was a problem with painting after doing multiple selection that might be
related - Seth, do you remember that bug number? It was assigned to some
non-mailnews person, IIRC. Clicking next to the twisty can cause the whole
expanded thread to be selected, which might be related.

Kevin, does this happen to all folders at once, or to one folder at a time? You
imply that it happens to all folders at once, but I want to make sure. In other
words, once this happens, can you no longer expand any threads in any folders?

Comment 5

[kmurray1115]

I don't think it's related to the multiple selection issues we're seeing right
now, as I saw this behavior way before that (if you mean the flaky selection
behavior going on, where selection does not go away after moving from message to
message, giving the appearance of multiple selection). I will see if it affects
other threads the next time this happens.

Comment 6

[David :Bienvenu]

are you saying this only affects one thread at a time? So it's only one thread
that you can't expand/collapse once it gets in that state? I missed that before.

Comment 7

[kmurray1115]

That's exactly it. I just encountered this and it only affects one folder/thread
at a time. The only thing I remember about what I did before it happened was
that I used the arrow keys to navigate in the thread pane to a particualr
folder/thread, and then used the right arrow key to expand it. This is pretty
much how I get there every time, although that specific procedure doesn't cause
it every time.

Comment 8

[Peter Trudelle]

nsbeta1- per ADT triage, ->1.2, blocks 'miracle bug' 122274

Comment 9

[mozbug1]

Shouldn't this be targeted for before 1.0?  This is pretty serious.

Comment 10

[David :Bienvenu]

I can't reproduce it - if you have reproducible steps to recreate this problem,
then there's a much better chance I could get a fix and convince the powers that
be to accept it.

Comment 11

[mozbug1]

I can't figure out how to reproduce it either, but it keeps happening to me.  In
once case it happened to me with a thread at the end of the list and I
discovered by clicking the next button that it actually expanded the rest of the
thread at the *top* of the list.  I can't find the bug where I mentioned that
though.  I think the key to reproducing this may be to have a really stuffed inbox.

Comment 12

[David :Bienvenu]

is it possible that getting a new message into the thread while the thread is
expanded might cause this? jks, what build are you using? There was a fix
checked in having to do with how we insert new messages into an already expanded
thread a couple weeks ago.

Comment 13

[kmurray1115]

I'm so glad someone else is seeing this! This doesn't seem to correlate with the
receipt of a message in the thread. My only consistent behavior for reproducing
this is using the arrow keys to expand/collapse a thread. 

Comment 14

[mozbug1]

I'm using 0.9.9 now.  I can't reproduce it right now, but it was always an
intermittent problem.

Comment 15

[David :Bienvenu]

Please keep me posted - the fix I'm thinking of was checked in March 1, which I
think means it made it onto the .9.9 branch, which was made March 2. Kevin, have
you seen this problem with a trunk build from after March 1, or a .9.9 build?

Comment 16

[kmurray1115]

Yes - I'm using 2002-03-14-03 :-(

Comment 17

[David :Bienvenu]

OK, I managed to recreate this and debug it a little - what's happening is that
the outliner is losing track of the currently selected index.
nsOutlinerSelection::GetCurrentIndex is returning -1, even though the selection
is displaying correctly. the vk_right code in outliner.xml checks
getcurrentindex, and if it's not -1, expands the thread. But since
getcurrentindex is -1, it can't expand the thread. I'm not sure what gets us
into that state, or how to fix it, but I suspect now that it's an outliner bug.

Comment 18

[David :Bienvenu]

OK, I found reproducible steps:

1. with a collapsed thread, click on the thread icon, which expands the thread,
and selects all the messages.
2. Collapse the thread by clicking on the twisty

at this point, the right arrow stops working to expand the thread. However,
clicking on the twisty works, or clicking on the thread icon. It's really mainly
the arrow keys that get confused at this point, though there is a wide range of
confusion possible when we think the current index is -1. I also determined that
it's not getting set to -1 from outside the selection object (I set a
conditional breakpoint on ::SetCurrentIndex for someone passing in -1 and didn't
hit it).

Comment 19

[David :Bienvenu]

this is a general outliner problem. I've recreated it in history as well. All
you have to do is collapse a selected hierarchy with a twisty, and after that,
the selected index gets set to -1. I have a trivial fix (though someone familiar
with the code could almost certainly come up with a better one). Renominating
now that we can reproduce this, and I think, trivially fix it.

Comment 20

[David :Bienvenu]

Attachment: proposed fix

this fixes the problem, for mailnews, history and bookmarks - there may be
other cases that need to be fixed, or it could be that we don't want to clear
mCurrentIndex in this code all the time:

  // adjust mCurrentIndex, if necessary
  if ((mCurrentIndex != -1) && (aIndex <= mCurrentIndex)) {
    // if we are deleting and the delete includes the current index, reset it
    if (aCount < 0 && (mCurrentIndex <= (aIndex -aCount -1))) {
	mCurrentIndex = -1;
    }
    else {
	mCurrentIndex += aCount;
    }
  }

my guess is that a better fix would involve changing this code to check if any
of the items in the selected range aren't being deleted, and if so, setting
mCurrentIndex to one of them. But I'll defer to hyatt or jan.

Comment 21

[Jan Varga [:janv]]

I think there is also similar issue in bug 125687.
Yeah, problem happens if we have current index in the thread we're closing.
I wonder if it is a good idea to move current index to parent index in such
situation instead of assigning value -1.
Or even better, just select parent index.

Comment 22

[Jan Varga [:janv]]

ok, I checked ns4x behaviour on windows. We should select parent index in such
situation.
David, thanks for detailed analysis.
Patch coming.

Comment 23

[Jan Varga [:janv]]

Attachment: alternate fix

Comment 24

[David :Bienvenu]

what does that patch do if you have the first msg selected and then click on the
twisty? Does it cause the parent of the first msg to get selected? That doesn't
seem right, or am I misreading the patch?

Comment 25

[Jan Varga [:janv]]

No, it doesn't select parent of the first msg.

          if (obj.value == "twisty") {
+           if (b.selection.currentIndex >= 0) {
+            var parentIndex = b.view.getParentIndex(b.selection.currentIndex);
+             if (parentIndex == row.value)
+               b.selection.select(parentIndex);
+           }
            b.view.toggleOpenState(row.value);
            return;
          }

First I check if we have a valid current index.
Then I need to find out if the current index is in the thread I'm closing,
simple way to do that is to check parent index of the current index to clicked row.

Now I see there is also possibility to have current index deeper in row
hierarchy, I'll try to fix that too.

Comment 26

[Jan Varga [:janv]]

Attachment: new patch

Comment 27

[David :Bienvenu]

Comment on attachment 75446 [details] [diff] [review]
new patch

r=bienvenu - I've run with this patch, and it fixes the problem, and I haven't
seen any regressions. Jan, do you want to reassign this to yourself?

Comment 28

[Jan Varga [:janv]]

taking, thanks for review

Comment 29

[Jan Varga [:janv]]

Attachment: almost the same patch with a small optimization

Comment 30

[David :Bienvenu]

Comment on attachment 76214 [details] [diff] [review]
almost the same patch with a small optimization

r=bienvenu

Comment 31

[Peter Trudelle]

nsbeta1- per Nav triage team, Jan doesn't need plus to checkin anyway.

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 76214 [details] [diff] [review]
almost the same patch with a small optimization

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 33

[Joe Hewitt (gone)]

Comment on attachment 76214 [details] [diff] [review]
almost the same patch with a small optimization

sr=hewitt

Comment 34

[Jan Varga [:janv]]

Joe landed for me.
Fixed.

Comment 35

[Jan Varga [:janv]]

sigh

Comment 36

[esther]

Using build 20020402 on winxp, 20020401 on linux and 20020402 on mac 9.1 this is
fixed.  Verified,