1152651 - [maildir] moving multiple messages moves only one, leaves trace of the moved message in the source location

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[mcmurchy1917-bugzilla]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20150225144845

Steps to reproduce:

Create a sub folder called MultipleMove under Local Folders

Select 2 messages from inbox

Drag the 2 messages from inbox to the MultipleMove folder




Actual results:

Look in the Folder MultipleMove, there will be only one message, not the two expected

In the Inbox. One of the messages is still selected and has the original content. 
When selecting the other message from the Inbox List, which corresponds to the message actually moved to the MultipleMove Folder, a MessageBox is displayed in the lower pane which states

<quote>

File not found

The file mailbox:///home/alex/.thunderbird/pxgvw4yz.default/Mail/mail.btinternet.com/Inbox?number=102 cannot be found. Please check the location and try again.

    Check the file name for capitalization or other typing errors.
    Check to see if the file was moved, renamed or deleted.

</quote>


I had to repair the Inbox list as follows <Right Click> on Inbox in the Left Pane and select <Properties> from the drop down list, the Folder Properties dialog will be presented. Push the <Repair Folder> button. The message that had been successfully moved to the MultipleMove Folder but gave the “File not found”  MessageBox disappears from the Inbox folder list.



Expected results:

Both messages should have been moved to the MultipleMove folder and both messages should have been completely removed from the Inbox Folder. 

Moving one message works as expected i.e. message is moved to the MultipleMove folder and removed from the Inbox folder.

Comment 1

[Kent James (:rkent)]

I can also see this. When I confirm, it is a copy from a local maildir folder to a local mbox folder.

Comment 2

[Kent James (:rkent)]

Attachment: Fix incorrect reference in maildir CopyMessages

This patch will fix an issue with maildir-to-maildir copies, caused by missing a change in some of the fixes done to maildir for Thunderbird 38.

The actual fix is a one-line change that is pretty obvious. The rest of the patch is test code to show the problem.

In comment 1, I claim I saw this bug for a maildir to mbox copy. I don't believe this fix will help there. I need to investigate that case and see if it is real, or if I was mistaken in comment 1.

Comment 3

[Kent James (:rkent)]

I've tested multiple copies from maildir->mbox and that also has a problem for a different reason.

In nsMailboxProtocol::Initialize we have code:

  if (RunningMultipleMsgUrl())
  {
    rv = OpenFileSocketForReuse(aURL, m_msgOffset, aMsgSize);

RunningMultipleMsgUrl just looks at the count of urls to decide if the file will be opened for reuse, but actually that is only valid for mbox. Also OpenFileSocketForReuse uses GetFileFromURL to locate the file, which assumes that a single mbox file is appropriate, also incorrect for maildir.

What needs to happen is these file opens need to go through the message store GetMsgInputStream which reports whether the file is reusable, and returns the individual message file in the case of maildir.

Comment 4

[Kent James (:rkent)]

Attachment: Part 2, Fix maildir->mbox moves

The message store references were actually unused, these are removed. Note also that I tried to keep the indent the same, and moved around some if statements to make that happen.

I still despise the style of:

rv == ...
if (NS_SUCCEEDED(rv)) {
  rv == ...
  if (NS_SUCCEEDED(rv)) {
    rv = ...

that this code seems to follow, as this causes the indent level to constantly increase. Not sure if others have bought into my preferred style using do { ... } while(false); and break on error, so I held my nose and kept the old style here.

This patch is risky enough that I would really like to test it in a beta for a week or so before landing in release. Not sure if we have time for that this cycle or not.

Comment 5

[[e-mail]]

Comment on attachment 8656471 [details] [diff] [review]
Part 2, Fix maildir->mbox moves

>@@ -74,42 +74,16 @@ nsresult nsMailboxProtocol::OpenMultiple
>   // XXX 64-bit
>   rv = serv->CreateInputTransport(m_multipleMsgMoveCopyStream, int64_t(offset),
>                                   int64_t(size), false,
>                                   getter_AddRefs(m_transport));
The -p -U8 options turned out to be very useful here; I did get a bit confused as to what was going on until I saw this. However there is now only one caller of OpenMultipleMsgTransport remaining and I do wonder whether it could be combined in the same way that you did with OpenFileSocketForReuse.

Comment 6

[Kent James (:rkent)]

Checked in.

http://hg.mozilla.org/comm-central/rev/cba4174a5fb6
http://hg.mozilla.org/comm-central/rev/8b519841303f

Comment 7

[Kent James (:rkent)]

Comment on attachment 8655081 [details] [diff] [review]
Fix incorrect reference in maildir CopyMessages

This is a low-risk bug that will probably be uplifted to TB 38.3.0

Comment 8

[Kent James (:rkent)]

Comment on attachment 8656471 [details] [diff] [review]
Part 2, Fix maildir->mbox moves

This is a patch that has some risk since it modifies basic copy code used by default mbox installs as well as maildir. I will decide in a few days whether to uplift to TB 38.3.0, but currently leaning against (unless we can get some solid testing on nightlies and beta prior to that).

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8656471 [details] [diff] [review]
Part 2, Fix maildir->mbox moves

At this point in 41 Beta cycle, the criteria to uplift to 41 RC (gtb tomorrow) is critical security issues that are easy to exploit, release blockers that negatively impact most of our end-users or major regressions since beta8/beta9. This is a Thunderbird issue and it is unclear why we need to uplift to FF beta branch. However, even so, this does not meet 41RC bar.

Comment 10

[Kent James (:rkent)]

I'm sorry, I missed and got mozilla- instead of comm-

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #8)
> Comment on attachment 8656471 [details] [diff] [review]
> Part 2, Fix maildir->mbox moves
> 
> This is a patch that has some risk since it modifies basic copy code used by
> default mbox installs as well as maildir. I will decide in a few days
> whether to uplift to TB 38.3.0, but currently leaning against (unless we can
> get some solid testing on nightlies and beta prior to that).

I'm inclined to agree. And I don't think we'd want to see a large refactoring on beta until it has lived first some solid days on nightly.

Comment 12

[:aceman]

There is a typo in patch part 2, in the bug number in the commit message :(

Comment 13

[Kent James (:rkent)]

Comment on attachment 8655081 [details] [diff] [review]
Fix incorrect reference in maildir CopyMessages

https://hg.mozilla.org/releases/comm-aurora/rev/4b40a0f14dea
https://hg.mozilla.org/releases/comm-beta/rev/722eb27dc247
https://hg.mozilla.org/releases/comm-esr38/rev/663123420b3f

This patch will be in Thunderbird 38.3.0

Comment 14

[Kent James (:rkent)]

Comment on attachment 8656471 [details] [diff] [review]
Part 2, Fix maildir->mbox moves

https://hg.mozilla.org/releases/comm-aurora/rev/4b40a0f14dea

This patch will not be in Thunderbird 38.3.0 nor in beta41. Landing in aurora will cause it to appear in beta42. We'll decide later whether to include in TB 38.4.0

Comment 15

[Kent James (:rkent)]

Unfortunately the patches for the bug are going to end up split between versions. Overall bug status reflects the status whether all patches have been landed in a version.

Comment 16

[mcmurchy1917-bugzilla]

If I want to test the fix what version should I download?

Comment 17

[Kent James (:rkent)]

(In reply to mcmurchy1917-bugzilla from comment #16)
> If I want to test the fix what version should I download?

Last night's aurora and nightly builds had fixes for both the maildir->maildir copy and the maildir->mbox copy.

The upcoming beta 41.0b2 and 38.3.0 will have the fix for maildir->maildir but not maildir->mbox. These are not available as releases, but tinderbox builds are available with the fix. See https://treeherder.mozilla.org/#/jobs?repo=comm-esr38 or https://treeherder.mozilla.org/#/jobs?repo=comm-beta. For example, a build of the Windows version of Thunderbird 38.3.0 is available here:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2015/09/2015-09-15-03-02-34-comm-esr38/thunderbird-38.2.0.en-US.win32.installer.exe

Comment 18

[Kent James (:rkent)]

Part 2 backed out due to mozmill failures:

https://hg.mozilla.org/comm-central/rev/6c73fa15d7fc
https://hg.mozilla.org/releases/comm-aurora/rev/53f8ab1d339d

Comment 19

[:aceman]

Yes, looks like it helped on trunk.

Comment 20

[:aceman]

We now had similar failures in eml tests on trunk, while your patch is not there. I've pushed your patch again to try, maybe it is not the cause of the failures: https://hg.mozilla.org/try-comm-central/rev/9d54ca518377

Comment 21

[:aceman]

Sorry, it seems the patch does reaaly cause the failures. Maybe the patch works for copying between stores, but the case of moving from external eml file into a store is now incorrect?

Comment 22

[Kent James (:rkent)]

There was an error in the Part 2 patch that caused an if-else needed for opening file messages to fail. I've confirmed in a try run that fixing that resolves the test failures, so I will reland with that fixed.

Comment 23

[Kent James (:rkent)]

Attachment: Part 2, Fix maildir->mbox moves (fixed for .eml opens)

Comment 24

[Kent James (:rkent)]

Comment on attachment 8668535 [details] [diff] [review]
Part 2, Fix maildir->mbox moves (fixed for .eml opens)

I'll move to aurora and beta as soon as nightly cycles, but esr38 is less clear.

Comment 25

[Kent James (:rkent)]

Part 2 was landed originally in Thunderbird 44.

Comment 26

[Kent James (:rkent)]

Comment on attachment 8668535 [details] [diff] [review]
Part 2, Fix maildir->mbox moves (fixed for .eml opens)

Sorry, I missed the hg rev for landing.

https://hg.mozilla.org/comm-central/rev/601943b5a517

Comment 27

[Kent James (:rkent)]

Part 2:

https://hg.mozilla.org/releases/comm-aurora/rev/df96a1f5bdf3
https://hg.mozilla.org/releases/comm-beta/rev/5351979c8807

Comment 28

[Kent James (:rkent)]

Comment on attachment 8668535 [details] [diff] [review]
Part 2, Fix maildir->mbox moves (fixed for .eml opens)

https://hg.mozilla.org/releases/comm-esr38/rev/7cd62350f863