Closed Bug 138342 Opened 18 years ago Closed 18 years ago
Moving a msg from IMAP to POP then undoing, message is lost
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).
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
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+
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
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 ago → 18 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
adding adt1.0.0+. Please get drivers approval and then check into the 1.0 branch.
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.
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.
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
You need to log in before you can comment on or make changes to this bug.