Closed Bug 209189 Opened 21 years ago Closed 9 years ago

Undo delete of message in local Trash folder causes corruption of Mail summary file with blank subject and body and crazy message date 1964/1969

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(thunderbird39 wontfix, thunderbird40 wontfix, thunderbird41 fixed, thunderbird42 fixed, thunderbird_esr3841+ fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 42.0
Tracking Flags:
Tracking Status
thunderbird39 --- wontfix
thunderbird40 --- wontfix
thunderbird41 --- fixed
thunderbird42 --- fixed
thunderbird_esr38 41+ fixed

People

(Reporter: mark.kohler, Assigned: jorgk-bmo)

References

(Depends on 1 open bug)

Details

(Keywords: dataloss, testcase)

Attachments

(2 files, 5 obsolete files)

Clear the undo/redo stack when deleted from the trash or shift deleting (take 3)
1.96 KB, patch
rkent
: review+
rkent
: feedback+
rkent
: approval-comm-beta+
rkent
: approval-comm-esr38+
Details | Diff | Splinter Review
root cause patch with Jorgk's nits fixed
2.54 KB, patch
rkent
: review+
rkent
: approval-comm-beta+
rkent
: approval-comm-esr38+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529

If you delete a message that is in the Trash folder, and then try to undelete,
the list of messages gets corrupted. Specifically, your Inbox will get a
messages with no subject and a date of 1964. If you exit and restart another 
messages like this appears. You can fix this problem by exiting Mozilla, deleting the Inbox.msf file, and restarting. (I found this with local files, not IMAP.)

Reproducible: Always

Steps to Reproduce:
1. Delete message from Inbox.
2. Go to Trash folder, and delete the message.
3. Click Edit->Undo.
4. Go to Inbox and see message with date of 1964.

Actual Results:  
Message with blank subject and body and invalid data appears in Inbox.

Expected Results:  
Nothing. You can't undo messages from the Trash, so that option should not
even have been possible.

I wasn't sure if this is a Normal, Major, or Critical bug, but I think it's
Critical, because if you don't notice the problem, your Inbox can get quite
messed up with corrrupted messages.
confirming, I can reproduce this.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I tried this with a 6-12 trunk build on linux IMAP & Local and did not see this
I tried this with a 6-11 branch build on linux IMAP & Local and did not see this
I tried this with a 6-12 branch build on winxp IMAP,POP & Local and did not see
this.  Is there more to reproducing this than deleting a mail message then
Undoing the delete?

did you follow the steps exactly, including the trash part?
No, I did miss the delete from trash and undo from trash part.  Bad!


Esther, does this happen on the 1.4 branch builds?
OK, I think the problem is that deleting the message in the trash deletes the
message hdr that we're using to undo the delete from the inbox (when you're
undoing the delete, you're undoing the delete from the inbox, not from the
trash, since you can't undo delete from the trash currently)
Component: Mail Database → Mail Back End
So, there are a few possible fixes:

1. If you delete the message from the trash, we could disable undo of the
original delete, since it won't work.
2. Make it work (this would entail remembering more about the msg than just the
msg key - i.e., the msg hdr)
3. Make undo delete from the trash work, so that first you'd undo the delete
from the trash, and then the delete from the inbox would just work. There's
already a bug out there about making undo delete from trash, or shift delete
from any folder, work, bug 132121
Product: MailNews → Core
*** Bug 292573 has been marked as a duplicate of this bug. ***
*** Bug 298808 has been marked as a duplicate of this bug. ***
FWIW doesn't matter in what folder the initial deletion occurs (I started with sent folder)
Summary: Undo delete of message in Trash folder causes corruption of Mail summary file. → Undo delete of message in Trash folder causes corruption of Mail summary file with blank subject and body and crazy message date 1964/1969
Severity: critical → major
Keywords: dataloss
OS: Windows 2000 → All
QA Contact: esther → backend
Just to calify, I did not move the messages to the trash folder.  I moved them from "Suspicious Mail" where email from address not in my address book go to, to the folder called Sort, intending to go though them at a later date.  I changed my mind and decided to do it then, that is when I hit Ctrl+Z and lost them.  I did not "delete" I just moved them.
Oh and also the duplicate is Comment 11 was my bug.

I contest my bug being a duplicate, since I DID NOT DELETE the emails.  I just moved them from one folder to another, then tried to move them back useing the undo function.
Adam would know better than me, but the symptoms match even though the method of getting there is different. I'm inclined to agree with the dupe.  Also, dataloss=critical
Severity: major → critical
I have seen this in Thunderbird version 2.0.0.5 (20070716).
I assume a basic fix (although I know nothing about how Thunderbird is coded) is just an 'if' statement testing the location (folder) of origin of the command (delete), and if it is a delete from the trash to disable the undo of that (or to not put it in the undo stack)
I don't think that is the fix Jeremy.  Because when I had this problem (dup in Comment 11) the emails where never put in the trash folder, rather I moved 30 or so emails from one folder to another.  Then changed my mind (yes, the new one worked better :p )and used Ctrl Z to move them back.  I got a stack of empty emails from 1970.
Product: Core → MailNews Core
I can't produce this with imap folder Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090521 Shredder/3.0b3pre. The undone message appears in trash, not inbox, and date is good.

But it does fail when using local folder.
Can be restored with r-click, properties, reindex
Keywords: testcase
Summary: Undo delete of message in Trash folder causes corruption of Mail summary file with blank subject and body and crazy message date 1964/1969 → Undo delete of message in local Trash folder causes corruption of Mail summary file with blank subject and body and crazy message date 1964/1969
if you see this in version 2, can you reproduce in version 3?
Whiteboard: closeme 2010-01-25
I really don't want to test it and have corrupted e-mail, but I see that the order number is still buggy. Isn't the order number supposed to be the number that corresponds to the e-mail that Thunderbird has received? If so, Thunderbird is buggy in the way that it is numbering the e-mails.  If it can't number the e-mails correctly, how do you expect it to correctly restore a deleted e-mail?
my reason for asking in comment 19 is to determine whether the behavior has changed in version 3.

Wayne, my understanding is it doesn't corrupt email, it only affects the index which can be resolved by reindexing. 

bienvenu, do you really want this assigned?
Depends on: 132121
Whiteboard: closeme 2010-01-25
someday I'd like to fix it, but I don't want to stand in anyone's way...
Assignee: bienvenu → nobody
Status: ASSIGNED → NEW
I was just going to create another duplicate, but found this bug just in time. It's twelve years old, so to me it looks like a candidate for having it fixed one day soon. I needed to restore a twice-deleted message yesterday, so that's how I ran into it.

Still broken in TB 37, 38 and Daily. For me the "crazy message date" is 1/1/1970.

Further comments:
Repairing the folder fixes the phantom message, however, the undeletion hasn't worked.
Of course the message is not blank, it's in the mailbox, the message status is still at X-Mozilla-Status: 0009 (read(1)+expunged(8)).
Kent: You seem to be analysing and fixing bugs with an amazing speed these days. Perhaps this is a one-to-five-line fix as well. Otherwise, I'd like to look at it after the release of TB38, so we can get this fixed for TB38.1
Flags: needinfo?(rkent)
(In reply to Jorg K from comment #28)
> ... Otherwise, I'd like to
> look at it after the release of TB38, so we can get this fixed for TB38.1

Jorg, I think you should have a go at it.

I'm currently seeing this to an annoying degree. It seems like it shouldn't be that difficult. In my case it's an imap account, just when doing undo, I'm fairly certain there is not strange locking going on

There are several bug reports, some which probably have different steps to reproduce

Bug 457099 - empty messages 1970 year in date when using deleteMessages with deleteStorage false
Bug 851462 - Thunderbird automatically creates an empty unread message from 1970
Bug 1110583 - Phantom mail(messageKey=sizeOndisk, messageSize=0, no subject, 1970/01/01) is generated, if move from local folder to local folder, and if move fails because mail is deleted by other one while moving

And wada has a ton of comments starting at https://bugzilla.mozilla.org/show_bug.cgi?id=209501#c178
for Bug 209501 - Phantom mail from 1/01/1970 11:00 AM or 12/31/1969 keeps reappearing in my inbox, often related to junk or deleting a message
I'll look at it in the next few days.
Assignee: nobody → mozilla
Flags: needinfo?(rkent)
OK, looking at deletion in local folders first:
https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1163

While deleting a message from a local folder so that it goes into the trash, nsMsgLocalMailFolder::DeleteMessages is called as follows:

deleteStorage = false (so more to trash)
isMove = false (we're not moving a message)
allowUndo = true (we should be able to undo).
This copies the message to the trash.

Then a second call:
deleteStorage = true, isMove = true, allowUndo = true.
This deletes the message from the original folder (as part of a "move to trash").

Then for the deletion from the trash: One call:
deleteStorage = false, isMove = false, allowUndo = true.
It's called with deleteStorage = false again, but since the message is already in the trash, it's just deleted.

Now the undo. Another call:
deleteStorage = true, isMove = true, allowUndo = false.
Another deletion? It's already deleted. Looks like it tried to undo the first deletion and assumes the message is still in the trash and hasn't been deleted. So it thinks it needs to be deleted from the trash before being revived in the original folder.

To be continued ... looking at undo next.

Looks like something is weird in the undo part. I tried:
1) Copy message to trash.
2) Delete from trash
3) Undo offers: Undo Copy Message. I expected: Undo Delete.

Both scenarios suggest that there is something wrong in the undo stack.
More observations:
1) Move message to trash. Undo offers: "Undo Delete Message".
2) Delete from trash.
3) Undo Delete Message.
Result: corrupted folder from where the message originally came.

1) Put something into the trash.
2) Restart TB.
3) Delete message from trash.
4) Nothing to undo.

This confirms that when deleting a message from the trash, the undo stack is broken. It tries to undo the previous operation, and that fails, sometimes silently, other times with corruption.

Actually, that's what the reporter said in comment #0:
> Expected Results:  
> Nothing. You can't undo messages from the Trash, so that option should not
> even have been possible.
Set breakpoints on 
nsTransactionManager::DoTransaction, nsTransactionManager::BeginTransaction, nsTransactionManager::EndTransaction
When deleting a message from the trash, these don't get called, so clearly the undo stack gets confused.

More analysis:
Deleting a message and moving it to the trash goes through:
https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#3212
That's the copy bit. Then nsMsgLocalMailFolder::EndMove:
https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2629
and then DoTransaction is called a little later on:
https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#2644

Moving a message from one folder to another folder also goes through the nsMsgLocalMailFolder::EndMove and does proper transaction management.

Deleting a message from the trash or deleting a message with shift+delete (no trash) goes through:
https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#3212

*No* transaction management. There's the problem, but how to fix it?
Neil, I know you have review powers on /mailnews, so perhaps you know this code well an can enlighten me on how it's meant to work. I've just observed that deleting from the trash or shift-deleting bypasses the transaction management and confuses the undo stack which leads to problems later.

Once I sorted out the local folders, I'll take a look at the IMAP folders.
Flags: needinfo?(neil)
Stack trace for more clarity:

Delete to trash/Move to trash:
1st step: Copy to trash:
xul.dll!nsMsgCopyService::CopyMessages() Line 438	C++
xul.dll!nsMsgLocalMailFolder::DeleteMessages() Line 1200	C++
xul.dll!nsMsgDBView::DeleteMessages() Line 3212	C++
xul.dll!nsMsgDBView::ApplyCommandToIndices() Line 2895	C++
xul.dll!nsMsgDBView::DoCommand() Line 2568	C++
http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#658

2nd step: Delete from source, no idea where that's called from.
xul.dll!nsMsgBrkMBoxStore::DeleteMessages() Line 750	C++
xul.dll!nsMsgLocalMailFolder::DeleteMessages() Line 1221	C++
xul.dll!nsMsgLocalMailFolder::EndMove() Line 2630	C++ <== transaction management here
xul.dll!nsCopyMessageStreamListener::EndCopy() Line 134	C++
xul.dll!nsCopyMessageStreamListener::OnStopRequest() Line 144	C++
xul.dll!nsMsgProtocol::OnStopRequest() Line 333	C++
xul.dll!nsMailboxProtocol::OnStopRequest() Line 388	C++
xul.dll!nsInputStreamPump::OnStateStop() Line 718	C++
xul.dll!nsInputStreamPump::OnInputStreamReady() Line 437	C++
xul.dll!nsInputStreamReadyEvent::Run() Line 93	C++
xul.dll!nsThread::ProcessNextEvent() Line 846	C++
xul.dll!NS_ProcessNextEvent() Line 265	C++
xul.dll!nsThread::Shutdown() Line 645	C++
(do we need more stack trace?)

Move between folders:
1st step: Copy (didn't track that down, sorry I'm lazy)
2st step: Delete from source: Same as above with transaction management in "EndMove".

Copy to folder:
1st step: Copy (didn't track that down, sorry I'm lazy)
2st step: EndMove calling nsTransactionManager::DoTransaction

Delete from trash, shift-delete case:
xul.dll!nsMsgBrkMBoxStore::DeleteMessages() Line 750	C++
xul.dll!nsMsgLocalMailFolder::DeleteMessages() Line 1221	C++
xul.dll!nsMsgDBView::DeleteMessages() Line 3212	C++
xul.dll!nsMsgDBView::ApplyCommandToIndices() Line 2895	C++
xul.dll!nsMsgDBView::DoCommand(int command) Line 2568	C++
No transaction management.
Attached patch WIP for feedback (obsolete) — — Splinter Review 
We know the cases where the deletion can't be undone, those are the cases were we issue a warning (unless suppressed). In these cases we might clear the undo/redo stack. I'd still have to work out the code to do this.

Or maybe this is the wrong approach, since the transaction management (for local folders) happens elsewhere (nsMsgLocalMailFolder/nsLocalMailFolder.cpp).

So I'd really need some instructions on how to pursue the matter further since I don't know how it's meant to work.
Attachment #8617646 - Flags: feedback?(neil)
This does the trick. As per my previous comment, I don't know whether it's the right approach.

This works for local folders and IMAP folders (although I've noticed that the undo for operation on IMAP folders doesn't actually work; anyway, the undo gets cleared).
Attachment #8617646 - Attachment is obsolete: true
Attachment #8617646 - Flags: feedback?(neil)
Attachment #8620500 - Flags: feedback?(neil)
Comment on attachment 8620500 [details] [diff] [review]
Clear the undo/redo stack when deleted from the trash or shift deleting

Oops, I've just seen that Neil has 45 reviews in the queue, so perhaps Kent, you can tell me whether this is the right approach.
Attachment #8620500 - Flags: review?(rkent)
Comment on attachment 8620500 [details] [diff] [review]
Clear the undo/redo stack when deleted from the trash or shift deleting

OK I spent a few hours getting familiar with the undo transaction code so that I could comment on this.

I don't think this code should be in the view. The view does not currently know anything about transaction details, while the folder and copy service do. Also, really the root issue is that the mailnews code tries to process a non-existent message. Shouldn't we be fixing the root problem?

I'll attach a patch that shows and fixes the root problem, though it is incomplete.
Attachment #8620500 - Flags: review?(rkent) → review-
This patch stops the incorrect delete, but it is not properly cleaning up the transaction stack, so that the UI continues to show that there is a possible undo delete transaction.

I'm still uncertain about what all is needed for this bug. I think we should at least stop the root issue like this, but perhaps we should also do the clear that you suggest - only I don't think that it should be in the view, but in the folder or lower (transaction or copy infrastructure).

What do you think?
Attachment #8630266 - Flags: feedback?(mozilla)
Attachment #8620500 - Flags: feedback?(neil)
(In reply to Kent James (:rkent) from comment #40)
> Shouldn't we be fixing the root problem?

Yes, we should. Thanks for looking at it. In comment #38 and comment #39 I was questioning whether my approach "glossing over the problem" was the right approach, as I already had doubts. I only requested a review to get the ball rolling. I have no time immediately (due to travel), but I'll look at it later in the week.

Thanks again for looking at it!
Flags: needinfo?(neil)
Comment on attachment 8630266 [details] [diff] [review]
Don't process a non-existent message

I looked at it a little bit and ran these test cases:

1)
- start TB
- move message from folder A to B.
- shift delete message from folder C (some other message)
- undo offers "undo move" and that works.
- maybe we should clear the undo stack, but it's not necessary, although confusing to undo "out of order".

2)
- start TB
- shift delete message.
- no undo offered, so no need to clear the undo stack.

3)
- start TB
- delete message to trash
- delete this message from trash
- undo offers "undo delete", but nothing happens.

Due to Kent's patch the corruption is stopped and we get this on the console:
WARNING: Cannot get old msg header: file h:/mozilla-source/comm-central/mailnews/local/src/nsLocalUndoTxn.cpp, line 310

So for this case, it would have been good to
a) clear the undo stack or b) display a message saying "sorry can't undo after all ;-("

It's fantastic that the corruption is stopped, but the system should not allow the undo or give feedback. Maybe together with case 1) it's easiest to always clear the undo stack in the right place ;-) when a permanent non-undoable transaction happens.

(That's all the time I can dedicate to it right now).
Attachment #8630266 - Flags: feedback?(mozilla) → feedback+
For the record:
[5472] WARNING: Cannot get old msg header: file h:/mozilla-source/comm-central/mailnews/local/src/nsLocalUndoTxn.cpp, line 310
And then:
JavaScript error: resource:///modules/activity/autosync.js, line 210: uncaught exception: 2147746065
Maybe unrelated.

Seems like the activity manager is not happy. I don't understand the error since there *is* a try/catch:
https://dxr.mozilla.org/comm-central/source/mail/components/activity/modules/autosync.js#210
Is the following problem relevant?

* Go into offline mode
* Move a message from one folder to another
* Press "Undo"

Expected: message appears back in original folder
Result: blank message with 1970 date appears in original folder

This seems 100% reproducible.

Gerv
But also: I'm against clearing the undo stack if a non-undoable delete is performed. I think instead, if the user tries to undo a non-undoable operation (whether it's non-undoable by nature, such as full delete, or non-undoable by subsequent action, such as the move-to-trash of a message later fully deleted) then we should just pop a non-model "unable to Undo" warning and pop that operation off the stack. So the user can then undo other things.

Gerv
(In reply to Gervase Markham [:gerv] from comment #45)
> Is the following problem relevant?
> * Go into offline mode
> * Move a message from one folder to another [using offline IMAP folders]
> * Press "Undo"

Yes, it's relevant, but IMHO not in the context of this bug. I'd report another bug.

Note, there is bug 857465 reporting a problem in offline mode.
(In reply to Kent James (:rkent) from comment #40)
> Shouldn't we be fixing the root problem?

Inspired by comment #45 I tried the "delete, delete, undo" and "shift delete, undo" on an (online) IMAP folder. Both work:

2 - IMAP)
- shift delete message.
- undo offered, and works!

3 - IMAP)
- delete message to trash
- delete this message from trash
- undo offers "undo delete", and it works!

So could "fixing the root problem" actually mean: Allow "permanent deletion" (shift delete; delete to trash, then delete from trash) be undoable on local folders? The message is still there, so can it be resurrected?
(In reply to Jorg K from comment #48)
> (In reply to Kent James (:rkent) from comment #40)
> > Shouldn't we be fixing the root problem?
...
> So could "fixing the root problem" actually mean: Allow "permanent deletion"
> (shift delete; delete to trash, then delete from trash) be undoable on local
> folders? The message is still there, so can it be resurrected?

There are myriad problems with the undo code, starting with the fact that it is not async, plus the mailnews implementations do not really follow the pattern established by the Editor (which owns the base code) of actually doing the work in transactions. Attempts to expand the scope of this bug to improve undo in various ways are not likely to succeed.

So I suggest that you keep the focus as narrow as possible, which means respecting the existing design (where permanent delete is not undo-able) and just make that work. "Work" would include both fixing root core issues that are causing unacceptable corruption when the UI violates expectations (like asking to do an impossible undo) as well as making the UI properly report what undo is possible. It should not include expanding the scope to allow undo of permanent deletes.
(In reply to Kent James (:rkent) from comment #49)
> "Work" would include both a) fixing root core issues that
> are causing unacceptable corruption when the UI violates expectations (like
> asking to do an impossible undo) as well as b) making the UI properly report
> what undo is possible. It should not include expanding the scope to allow
> undo of permanent deletes.

Fine by me ;-)

So a) is already covered and for b) we need to stick the code for clearing the undo-stack or displaying an error somewhere. Despite comment #46 I'm in favour of clearing the stack in order not to undo the 2nd last transaction since the last one is undoable.
Moved the code one level down. This is part b) of the solution.
Attachment #8620500 - Attachment is obsolete: true
Attachment #8631284 - Flags: feedback?(rkent)
Oops, the previous patch crashed when sending a message and deleted the saved draft.
Attachment #8631284 - Attachment is obsolete: true
Attachment #8631284 - Flags: feedback?(rkent)
Attachment #8632523 - Flags: feedback?(rkent)
Grammar messed up:
The previous patch crashed while sending a message when the saved draft was deleted.

Now it works.
Comment on attachment 8632523 [details] [diff] [review]
Clear the undo/redo stack when deleted from the trash or shift deleting (take 3)

OK I think this is a reasonable approach. I thought about trying to clear only the affected transaction. But that would be quite confusing from a UI point of view, that you have a stack of undos that don't actually succeed because on of them is missing (and it's not like you can get any information on what, exactly, is missing).

So short of adding all of the complexity to allow undoing a permanent delete, I think this is the correct approach.
Attachment #8632523 - Flags: feedback?(rkent) → feedback+
So can we then merge the two patches and land them? Do we need a /mailnews reviewer for that?
Flags: needinfo?(rkent)
(In reply to Jorg K from comment #55)
> So can we then merge the two patches and land them? Do we need a /mailnews
> reviewer for that?

You need to prepare the patch you believe is ready to land, and ask for review of that. I can review it.
Flags: needinfo?(rkent)
Comment on attachment 8632523 [details] [diff] [review]
Clear the undo/redo stack when deleted from the trash or shift deleting (take 3)

Kent, someone needs to review your part, and I suppose you can't do that yourself ;-)
I don't think it's a wise idea to merge the two patches, since they need to be tested separately, that is, with my part active, the your new code will never be reached.

Let's find an unbureaucratic way to fix this 12 y/o bug.
Attachment #8632523 - Flags: review?(rkent)
(In reply to Jorg K from comment #57)
> Comment on attachment 8632523 [details] [diff] [review]
> Clear the undo/redo stack when deleted from the trash or shift deleting
> (take 3)
> 
> Kent, someone needs to review your part, and I suppose you can't do that
> yourself ;-)
> I don't think it's a wise idea to merge the two patches, since they need to
> be tested separately, that is, with my part active, the your new code will
> never be reached.
> 
> Let's find an unbureaucratic way to fix this 12 y/o bug.

jcranmer, who is the current module owner for mailnews, has stated a policy that a peer is able to decide who is an acceptable reviewer. Generally, for non-controversial issues (such as this), practically that means that two people need to be involved, one of which is a peer. So from a review perspective, you and I are working together, when we both agree on the solution we can land it, regardless of who wrote which piece of code. That is the unbureaucratic solution, particularly given our current slow review process.

So you need to propose a solution you are happy with (which might include portions of my code), then I will review it, and that is sufficient.
One other thing, if you are happy with my patch as is, change your flag setting to r+. Make sure that you do what you believe is a review-quality inspection of the work.
OK, I will give your patch a "quality review" within the next 24 hours, but please don't complain if I find a nit ;-)
Comment on attachment 8630266 [details] [diff] [review]
Don't process a non-existent message

Review of attachment 8630266 [details] [diff] [review]:
-----------------------------------------------------------------

This is my first (semi-official) review, so please excuse any silly comments ;-)
Obviously this is a learning exercise for me, so please let me know if my comments are invalid.

::: mailnews/local/src/nsLocalUndoTxn.cpp
@@ +306,5 @@
>            m_copiedMsgIds.AppendElement(messageId);
>          }
> +        else
> +        {
> +          NS_WARNING("Cannot get old msg header");

Should that be NS_ERROR? Isn't it a logic error, that we try to undo something that can't be undone? I don't have the experience to decide this.

@@ +319,5 @@
> +                                       false);
> +      }
> +      else
> +      {
> +        // Nothing to do because original messages deleted.

Nit. Would you mind making it "... messages are/were/have been deleted."
Or maybe the entire else-branch should be removed, see next comment.

@@ +320,5 @@
> +      }
> +      else
> +      {
> +        // Nothing to do because original messages deleted.
> +        return NS_ERROR_FAILURE;

I'm not sure that you want to return a non-zero status here. A failure will trigger a clean-up action in the caller, see:
http://mxr.mozilla.org/mozilla-central/source/editor/txmgr/nsTransactionItem.cpp#193.
(I've actually gone through the effort stepping through it in the debugger since I promised a "quality review").
Why would it be wrong just to continue the processing instead of returning early? I removed the else-branch and it works just the same.
Attachment #8630266 - Flags: review+
Let me know what you think.
(In reply to Jorg K from comment #61)
> Comment on attachment 8630266 [details] [diff] [review]
> Don't process a non-existent message
> 
> > +          NS_WARNING("Cannot get old msg header");
> 
> Should that be NS_ERROR? Isn't it a logic error, that we try to undo
> something that can't be undone? I don't have the experience to decide this.

NS_ERROR tends to generate a more severe response, which may result in a debug break while debugging. Personally I find it annoying when there is a debug break in someone else's code that I know nothing about, while I am trying to debug my own code, so I tend to avoid NS_ERROR myself.

> 
> @@ +319,5 @@
> > +                                       false);
> > +      }
> > +      else
> > +      {
> > +        // Nothing to do because original messages deleted.
> 
> Nit. Would you mind making it "... messages are/were/have been deleted."
> Or maybe the entire else-branch should be removed, see next comment.
> 
> @@ +320,5 @@
> > +      }
> > +      else
> > +      {
> > +        // Nothing to do because original messages deleted.
> > +        return NS_ERROR_FAILURE;
> 
> I'm not sure that you want to return a non-zero status here. A failure will
> trigger a clean-up action in the caller, see:
> http://mxr.mozilla.org/mozilla-central/source/editor/txmgr/nsTransactionItem.
> cpp#193.
> (I've actually gone through the effort stepping through it in the debugger
> since I promised a "quality review").
> Why would it be wrong just to continue the processing instead of returning
> early? I removed the else-branch and it works just the same.

Generally, it is important for a method where async notifications will be done to return an error if the call is not done, as the async notifications will never occur so the caller should not wait for them. Hard to say if this applies here though, since the copier is not using the listener (though there are still folder notifications done at completion).

Looking at the error recovery, it actually seems to be trying to address issues that your patch ignores by clearing the stack, namely trying to recover an undo stack that is valid even in the case of an error. So we have a very difficult cleanup problem here, and it's not clear to me what is the right thing to do. I have no confidence that the existing undo code is robust in the face of errors, so I'm sure that it really matters. But I'll remove the error. Still I would like to leave a warning.
(In reply to Kent James (:rkent) from comment #63)

> Looking at the error recovery, it actually seems to be trying to address
> issues that your patch ignores by clearing the stack, namely trying to
> recover an undo stack that is valid even in the case of an error. So we have
> a very difficult cleanup problem here, and it's not clear to me what is the
> right thing to do. I have no confidence that the existing undo code is
> robust in the face of errors, so I'm sure that it really matters. But I'll
> remove the error. Still I would like to leave a warning.

As I said, you have far more experience, so let's do whatever you're comfortable with. You asked for "review-quality inspection", so that's what I tried to do ;-)

With my part of the patch, clearing the undo-stack, this code won't get run, so the discussion is a little on the academic side. However, we should leave it in a way that your part stands up on its own and you're happy with it.

We can obsolete my "merged" patch (attachment 8638946 [details] [diff] [review]).
Comment on attachment 8638946 [details] [diff] [review]
Kent's patch (with else-branch removed) and Jorg's patch combined.

We're going to use the separate patches.
Attachment #8638946 - Attachment is obsolete: true
Attachment #8632523 - Flags: review?(rkent) → review+
Carrying over Jorgk's review, land this patch as well as his.
Attachment #8638963 - Flags: review+
Attachment #8630266 - Attachment is obsolete: true
Please land two patches.

If we want to show that we're working on TB, we could uplift this, even into TB 38.2.
Keywords: checkin-needed
For the record: I tested Kent's patch in the following case: Delete three messages to the trash, then delete one of them from the trash. Undo. Works just fine: Two messages are moved back. So all good.
As great as it will be to get this into a release, it's a very old bug that has not gotten worse in recent releases and the patch is not trivial. So it should get at least a couple weeks of baking on nightly and beta. Targetting to 38.3.0 would be safer.
(In reply to Jorg K from comment #48)
> I tried the "delete, delete, undo" and "shift
> delete, undo" on an (online) IMAP folder. Both work:
> 
> 2 - IMAP)
> - shift delete message.
> - undo offered, and works!
> 
> 3 - IMAP)
> - delete message to trash
> - delete this message from trash
> - undo offers "undo delete", and it works!

(I assume from the attached patches that nothing was done to change the online IMAP case, which to me sounds like it is using best practice; unfortunately I don't know the code well enough to know why this doesn't seem to be possible in the local folder case.)
Yep, IMAP wasn't touched.
url:        https://hg.mozilla.org/comm-central/rev/479e2eb5a8cd95a6cecba816e45a98c3dd63190d
changeset:  479e2eb5a8cd95a6cecba816e45a98c3dd63190d
user:       Jorg K <mozilla@jorgk.com>
date:       Sun Jul 12 01:28:00 2015 +0200
description:
Bug 209189 - Clear undo stack after permanent deletion. r=rkent

url:        https://hg.mozilla.org/comm-central/rev/cc47880f2301bb2f56875e53f5a45954df638567
changeset:  cc47880f2301bb2f56875e53f5a45954df638567
user:       R Kent James <rkent@caspia.com>
date:       Sat Jul 25 16:33:00 2015 +0200
description:
Bug 209189 - Don't do transaction on non-existent message. r=rkent
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 42.0
Comment on attachment 8632523 [details] [diff] [review]
Clear the undo/redo stack when deleted from the trash or shift deleting (take 3)

https://hg.mozilla.org/releases/comm-beta/rev/661436d7a39d
https://hg.mozilla.org/releases/comm-esr38/rev/4ead332237c0
Attachment #8632523 - Flags: approval-comm-esr38+
Attachment #8632523 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: