Closed Bug 138342 Opened 18 years ago Closed 18 years ago

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

Categories

(MailNews Core :: Backend, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: esther, Assigned: naving)

Details

(Keywords: dataloss, regression, Whiteboard: [adt2 rtm])

Attachments

(2 files)

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
adding dataloss (lost messages) and reqression (did not happen with 6.2 builds).
QA Contact: gayatri → esther
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)
i will research this bug ,please give me some time.
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!
Please ask Navin for a review first.
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
Attached patch proposed fixSplinter Review
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.
taking, Cavin, can I get r=? thx
Assignee: mscott → naving
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+
cavin, I will do so. david, can you sr ? thx.
Status: NEW → ASSIGNED
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?
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
did the msg header created by GetMessageHeader have the IMAP_DELETED flag set?
Is that what was causing the problem?
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. 
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 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+
fixed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: adt1.0.0
Resolution: --- → FIXED
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 → ---
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
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
yes, please file separate bugs about any such issues. It is easier for tracking 
purposes. 
I've added a patch for an uninitialized rv to bug 59673.
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.
OK, verified on Mac 9.1 an 10.1 using 20020517trunk build, can be checked into
branch when ready.
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
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt2 rtm]
adding adt1.0.0+.  Please get drivers approval and then check into the 1.0 branch.
Keywords: adt1.0.0adt1.0.0+
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+
Attachment #83559 - Flags: approval+
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+
fix checked into 1.01 banch
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
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.