If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Moving a msg from IMAP to POP then undoing, message is lost

VERIFIED FIXED

Status

MailNews Core
Backend
--
critical
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: esther, Assigned: Navin Gupta)

Tracking

({dataloss, regression})

Trunk
dataloss, regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 rtm])

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
Using branch build 20020418 on linux and branch 20020412 on winxp (also happens
as far back as 3-28 trunk on linux too) if a user moves by drag n drop a message
from an IMAP folder to a POP folder then does UNDO, the message leaves the POP
folder, but never show up in the IMAP folder again.  It's lost.

1. Launch app, open a profile that has an IMAP account and POP account
2. Select a message in the IMAP folder and drag it to one of your POP folders or
a Local Folder.
3. Open the destination folder to see that the message is there,
4. Undo the move.  verify it left the folder where you moved it to
5. Look in the folder where it came from

Result: the message is no where to be found. I did a redo move, search on the
folder, exited and relaunched, looked at that same folder on another system 
it's gone.  This is bad, nominating nsbeta1
s
(Reporter)

Comment 1

16 years ago
adding dataloss (lost messages) and reqression (did not happen with 6.2 builds).
Keywords: dataloss, nsbeta1, regression
QA Contact: gayatri → esther
(Reporter)

Comment 2

16 years ago
Happen when moving IMAP folder to IMAP folder different accounts
Does not happen when moving IMAP folder to IMAP folder same account
Does not happen when moving from POP to IMAP 
Does not happen when moving POP to POP (same or different accounts)

Comment 3

16 years ago
i will research this bug ,please give me some time.

Comment 4

16 years ago
Created attachment 83499 [details] [diff] [review]
This is a new patch for this bug,i'm sure it can fix it.please rv= and sr=

i found that when mozilla undo moving,the src folder is imap,we should judge
the (NS_SUCCEEDED(rv) && deleteModel == nsMsgImapDeleteModels::IMAPDelete)
if result is false,we shouldn't execute CheckForToggleDelete()
GetImapDeleteModel(srcFolder, &deleteModel);
so please rv my patch and sr,i need your advice.Thank you!

Comment 5

16 years ago
Please ask Navin for a review first.
(Assignee)

Comment 6

16 years ago
Comment on attachment 83499 [details] [diff] [review]
This is a new patch for this bug,i'm sure it can fix it.please rv= and sr=

This patch is not good. correct fix upcoming
(Assignee)

Comment 7

16 years ago
Created attachment 83559 [details] [diff] [review]
proposed fix

The problem was GetMessageHeader was creating a new header if it didn't find
one in the db. The fix is to use ContainsKey to look for existing hdrs and if
we don't find that key, just return NS_OK, we always intialize aResult to the
correct default value.
(Assignee)

Comment 8

16 years ago
taking, Cavin, can I get r=? thx
Assignee: mscott → naving

Comment 9

16 years ago
Comment on attachment 83559 [details] [diff] [review]
proposed fix

Should check the output argument:

NS_ENSURE_ARG(aResult);

Apart from that, r=cavin.
Attachment #83559 - Flags: review+
(Assignee)

Comment 10

16 years ago
cavin, I will do so. david, can you sr ? thx.
Status: NEW → ASSIGNED

Comment 11

16 years ago
Why was Antonio's patch wrong? Could you elaborate for his sake and so I don't
need to figure it out for myself?

You're not changing what CheckForToggleDelete returns in this situation, are
you? Is the problem that we've created this message header that we shouldn't
have, and that causes problems at some later point?
(Assignee)

Comment 12

16 years ago
Antonio's patch copies all the imap delete model code from nsImapUndoTxn.cpp. we
don't need to check imap delete model in local code, if there is way to avoid
it. The problem is basically we have deleted the header from db and when we do
undo, we are trying to check the delete flag on the deleted header in
CheckToggleForDelete(). GetMessageHeader creates the header for that key (which
was not expected). So we need to check if we have that header in the db,
ContainsKey does that, if we don't have the header we can just return and do a
normal undo (restore the deleted header in imap folder). Also I have added
comments to explain what is going on

Comment 13

16 years ago
did the msg header created by GetMessageHeader have the IMAP_DELETED flag set?
Is that what was causing the problem?
(Assignee)

Comment 14

16 years ago
No, GetMessageHeader() calls GetMsgHdrForKey() which created a totally new
header for that key so it has IMAP_DELETED flag *not* set (I'd imagine no flags
will be set because we are creating new hdr). This caused us to return false
when we should have just bailed out so I added this code to check if there is an
existing hdr with IMAP_DELETED flag. 

Comment 15

16 years ago
Ah, the key is "we always intialize aResult to the correct default value." and
that really means the correct default value in case the header is not in the db.
OK, I see now.

Comment 16

16 years ago
Comment on attachment 83559 [details] [diff] [review]
proposed fix

sr=bienvenu (but you might want to extend the comment to be a little clearer
about initializing aResult for the case were the key is not in the db)
Attachment #83559 - Flags: superreview+
(Assignee)

Comment 17

16 years ago
fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Keywords: adt1.0.0
Resolution: --- → FIXED

Comment 18

16 years ago
If the GetMsgDatabase fails to return a non-null db,
nsMsgTxn::CheckForToggleDelete will return an uninitialized nsresult.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/util/nsMsgTxn.cpp&mark=113,118,132#110
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 19

16 years ago
please don't reopen bugs for different problems - you can file a new one if you
like. You are technically correct but that does NOT affect this bug, since in
the real world, the changes of that happening are practically nil.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

16 years ago
yes, please file separate bugs about any such issues. It is easier for tracking 
purposes. 

Comment 21

16 years ago
I've added a patch for an uninitialized rv to bug 59673.
(Reporter)

Comment 22

16 years ago
Using 20020517 trunk build on winxp & linux, this is fixed, still testing mac
9.1  & 10.1. 
IMAP to POP or Local = OK, move,undo move, redomove
IMAP to another IMAP = OK, move,undo move,redo move.
(Reporter)

Comment 23

16 years ago
OK, verified on Mac 9.1 an 10.1 using 20020517trunk build, can be checked into
branch when ready.
(Reporter)

Comment 24

16 years ago
Note: this is verified on 5-17 trunk build, still a bug on 5-17 branch builds,
Marking verified so it can be checked into branch for further verification
Status: RESOLVED → VERIFIED

Updated

16 years ago
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2 rtm]

Comment 25

16 years ago
adding adt1.0.0+.  Please get drivers approval and then check into the 1.0 branch.
Keywords: adt1.0.0 → adt1.0.0+

Comment 26

16 years ago
changing to adt1.0.1+ for checkin to the 1.0 branch for the Mozilla1.0.1
milestone.  Please get drivers approval before checking in.
Keywords: adt1.0.0+ → adt1.0.1+

Updated

16 years ago
Attachment #83559 - Flags: approval+

Comment 27

16 years ago
please checkin to the 1.0.1 branch ASAP. once there please remove the
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword.
Keywords: mozilla1.0.1+

Comment 28

16 years ago
fix checked into 1.01 banch
Keywords: mozilla1.0.1+ → fixed1.0.1
(Reporter)

Comment 29

16 years ago
Using branch build 20020610 on winxp, mac os10.1 & linux this is verified fixed
on branch. 
IMAP A to IMAP B  =OK
IMAP A to Local Folder = OK
IMAP A to POP Folder = OK
Keywords: fixed1.0.1 → verified1.0.1
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.