142065 - when I right click delete a message, the wrong message gets deleted

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[(not reading, please use [e-mail] instead)]

when I right click delete a message, the wrong message gets deleted

I found this on my imap inbox, but I can reproduce on a local folder.

I've got a reproducable test case, let me attach it.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: save this file under "Local Folders" on disk

Comment 2

[(not reading, please use [e-mail] instead)]

after you save that folder to disk, open the folder, view it as threaded.

select the last message.  then right click delete the top most message.

we delete the wrong one.

I'll screen shot for you.

Comment 3

[Oliver Klee]

This looks like bug 142061 - submitted twice?

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of the steps

Comment 5

[Boris Zbarsky [:bzbarsky]]

*** Bug 142061 has been marked as a duplicate of this bug. ***

Comment 6

[Sean Su]

seth, your attachment is missing the message you deleted when you were testing
out the bug.  However, I was able to reproduce this on today's branch build.

but whenever I brough up a debug build (mozilla or ns), it worked fine.  Also
the optimized daily mozilla build works fine too... this is rather weird...

Comment 7

[Ben Ruppel]

I've seen this on the nightly builds.  I can always reproduce it with the
following steps: 
1)select message with at least two messages above it in thread pane.
2)right-click and delete the message two messages up

result: the message between the two afformentioned messages is deleted.

Comment 8

[Sven Krohlas]

I think I've seen this several times on Linux, too. But for me it seems as if 
the correct messages gets deleted, but the list gets corrupted. When opening 
another folder and then the first one again everything seems to be correct.

Comment 9

[Ben Ruppel]

I suggest moving this up to major.  This is a great opportunity for dataloss if
the user isn't paying attention, it's been around since the feature was
implemented and now it's getting old.  Mozilla shouldn't delete messages that we
don't tell it to.

Comment 10

[Peter Mularien]

I just tested this on trunk build 2002100808. It is correct. The test case is
very simple:

1. Go to a folder with a lot of messages (my Inbox has 413).
2. Click on a message in the folder so it is highlighted.
3. Right-click on a message in the list and select Delete.
4. The message directly above the message you highlighted will appear to be deleted.

The one commenter is correct that if you click away and click back to the
folder, the correct message was deleted. But it sure is confusing.

I don't know whether the number or kind of messages in the folder makes a
difference. I tried to create a test case with another junk folder I had (35
messages) and I wasn't able to do it.

Comment 11

[Sean Su]

agh, I'm finally able to reproduce this problem.  not sure what's causing this,
but this is very very confusing.

Comment 12

[Sean Su]

I also noticed that if I left-click on the message that I thought I deleted,
that row gets redrawn and the correct message header appears there.

no need to select a different folder then back.

Comment 13

[Sean Su]

nominating

Comment 14

[Samir Gehani]

Mail triage team: nsbeta1+/adt1

Comment 15

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 16

[Ben Ruppel]

Well, the wrong message isn't actually deleted.  The wrong message /appears/ to
be deleted.  Perhaps someone should change the summary.

This could still cause dataloss because of the ensuing confusion, but it's not
as straightforward as the summary makes it out to be.

Comment 17

[Sean Su]

*** Bug 168317 has been marked as a duplicate of this bug. ***

Comment 18

[Sean Su]

The more I dig into this bug the uglier it gets.  I have a feeling that this was
caused by this big check in.  I'm still investigating though:

 <[e-mail]> 28 Mar 2002 18:44
 bugs 110156, 110155
 Removing support for <outliner> tags
 Removing <tree> layout code and moving <tree> tags to outliner layout
 Convert all usage of <outliner> to tree tags
 Convert all usage of <tree> tags to new <tree> syntax or <listbox>
 r=cmanske,varga sr=hyatt,sspitzer a=asa

 Also includes fixes by Jan Varga ([e-mail]) for bugs 132020, 133451,
 131393, 115894, and 129327

So far, I've traced the problem down to:
  http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgDBView.cpp#5302

mTreeSelection->GetRangeAt() is returning the wrong value if the delete command
was invoked via the right-mouse click context menu.  This function works fine if
the delete key is used.

Trying to track down how this is changed...

Comment 19

[Sean Su]

I finally tracked down what's going on.  The code as it is used to work just
fine.  I'm guessing that some backend layout functionality was changed/fixed,
and is now exposing this bug.

I debugged it down to a point where I found out that in the file:

  mailnews\base\resource\content\mailContextMenu.js

the function RestoreSelectionWithoutContentLoad() should not be called when a
delete or move command is issued via the context popup menu.  I'll check to make
sure the folder pane does not have the same problem.

Looking for ways to fix this now.

Comment 20

[Sean Su]

Attachment: patch v1.0

This patch fixes:
 * the delete/move redraw problem
 * logic to determine which message is selected/loaded after a delete/move
command

Comment 21

[R.K.Aa.]

*** Bug 194056 has been marked as a duplicate of this bug. ***

Comment 22

[Jan Varga [:janv]]

Comment on attachment 114492 [details] [diff] [review]
patch v1.0

works fine
r=varga

Comment 23

[(not reading, please use [e-mail] instead)]

1)

-    gNextMessageViewIndexAfterDelete = gDBView.msgToSelectAfterDelete;

we no longer need to ask the view for the next message?  
what does this mean for about for the different imap delete models?
(mark as deleted vs move to trash).

2)

does NumberOfSelectedMessagesAboveCurrentIndex() do the right thing when we are
threaded mode?  (note, there might be other bugs about selecting the next
message after delete not working properly in threaded mode, that are pre-existing.)

3) [nit]

+  var numberOfMsgsAboveCurrentIndex; 
   gThreadPaneDeleteOrMoveOccurred = true;
+  numberOfMsgsAboveCurrentIndex =
NumberOfSelectedMessagesAboveCurrentIndex(treeSelection.currentIndex);
+  gNextMessageViewIndexAfterDelete = treeSelection.currentIndex -
numberOfMsgsAboveCurrentIndex;

as a style nit, maybe just do:

   gThreadPaneDeleteOrMoveOccurred = true;
+  var numberOfMsgsAboveCurrentIndex =
NumberOfSelectedMessagesAboveCurrentIndex(treeSelection.currentIndex);
+  gNextMessageViewIndexAfterDelete = treeSelection.currentIndex -
numberOfMsgsAboveCurrentIndex;

or, since numberOfMsgsAboveCurrentIndex is only used once:

   gThreadPaneDeleteOrMoveOccurred = true;
+  gNextMessageViewIndexAfterDelete = treeSelection.currentIndex -
NumberOfSelectedMessagesAboveCurrentIndex(treeSelection.currentIndex);

or, maybe:

   gThreadPaneDeleteOrMoveOccurred = true;
+  var currentIndex = treeSelection.currentIndex;
+  gNextMessageViewIndexAfterDelete =  currentIndex -
NumberOfSelectedMessagesAboveCurrentIndex(currentIndex);

Comment 24

[(not reading, please use [e-mail] instead)]

> what does this mean for about for the different imap delete models?

I meant:

> what does this mean for the different imap delete models?

Comment 25

[Sean Su]

Attachment: patch v1.1

updated patch given Seth's comments:

> 1)
> -    gNextMessageViewIndexAfterDelete = gDBView.msgToSelectAfterDelete;
> we no longer need to ask the view for the next message?  
> what does this mean for about for the different imap delete models?
> (mark as deleted vs move to trash).

We can't use gDBView.msgToSelectAfterDelete because the DBView does not take
into account right-click deletes (message(s) deleted might not be currently
hilighted).  The following statement will correctly figure this out and take
into account right-click deletes:
+    gNextMessageViewIndexAfterDelete = treeSelection.currentIndex -
NumberOfSelectedMessagesAboveCurrentIndex(treeSelection.currentIndex);

> 2)
> does NumberOfSelectedMessagesAboveCurrentIndex() do the right thing when we
> are threaded mode?  (note, there might be other bugs about selecting the next

> message after delete not working properly in threaded mode, that are
> pre-existing.)

yes, it will correctly figure out which message to select and load after a
delete.  The previous patch did not take into account the "Mark as deleted"
delete model, but this one does.  It also handles threaded model for either
"Mark as Deleted" or "Move to Trash".

> 3) [nit]

nit addressed.

Comment 26

[Sean Su]

Attachment: patch 1.2

new patch with function GetRemoveRowOnMoveOrDelete() optimized per conversation
with Seth.

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 117624 [details] [diff] [review]
patch 1.2

r/sr=sspitzer

thanks for all the comments, which will help us from accidentally regressing
this in the future.

nice work, sean.

Comment 28

[Sean Su]

finally! patch has been checked in.

Comment 29

[Florian Hars]

*** Bug 197339 has been marked as a duplicate of this bug. ***

Comment 30

[Ninoschka Baca]

Trunk build 2003-03-26-11: WinXP
Trunk build 2003-03-25-03: Mac 10.1.5
Verified Fixed. When verifying I used Seth's attached file and saved it to my
Local Folders. Also checked mail messages in my IMAP account.

Comment 31

[[e-mail]]

Now I see what the cause of this is - gDBView.onDeleteCompleted simply removes
the *selected* messages from the view, assuming that those are the ones that
were deleted. This was not true in the context delete case, which is why the
current hack was implemented, and is not even vaguely true in the IMAP case
because the delete is asynchronous and the user should be allowed to get on with
other things while the delete request is processed. Sigh...