Closed Bug 1152651 Opened 5 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Backend, defect)

x86
All
defect
Not set

Tracking

(thunderbird40 wontfix, thunderbird41 wontfix, thunderbird42 fixed, thunderbird43 fixed, thunderbird44 fixed, thunderbird_esr3842+ fixed)

RESOLVED FIXED
Thunderbird 43.0
Tracking Status
thunderbird40 --- wontfix
thunderbird41 --- wontfix
thunderbird42 --- fixed
thunderbird43 --- fixed
thunderbird44 --- fixed
thunderbird_esr38 42+ fixed

People

(Reporter: mcmurchy1917-bugzilla, Assigned: rkent)

References

(Blocks 1 open bug)

Details

(Whiteboard: [maildir])

Attachments

(2 files, 1 obsolete file)

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.
I can also see this. When I confirm, it is a copy from a local maildir folder to a local mbox folder.
Assignee: nobody → rkent
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Component: Untriaged → Backend
Product: Thunderbird → MailNews Core
Summary: Maildir moving multiple messages → [maildir] moving multiple messages moves only one, leaves trace of the moved message in the source location
Whiteboard: [maildir]
Depends on: 1193958
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.
Attachment #8655081 - Flags: review?(neil)
Attachment #8655081 - Flags: review?(neil) → review+
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.
Attached patch Part 2, Fix maildir->mbox moves (obsolete) — Splinter Review
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.
Attachment #8656471 - Flags: review?(neil)
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.
Attachment #8656471 - Flags: review?(neil) → review+
Checked in.

http://hg.mozilla.org/comm-central/rev/cba4174a5fb6
http://hg.mozilla.org/comm-central/rev/8b519841303f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 43.0
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
Attachment #8655081 - Flags: approval-mozilla-esr38?
Attachment #8655081 - Flags: approval-mozilla-beta?
Attachment #8655081 - Flags: approval-mozilla-aurora?
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).
Attachment #8656471 - Flags: approval-mozilla-esr38?
Attachment #8656471 - Flags: approval-mozilla-beta?
Attachment #8656471 - Flags: approval-mozilla-aurora?
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.
Attachment #8656471 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I'm sorry, I missed and got mozilla- instead of comm-
Attachment #8655081 - Flags: approval-mozilla-esr38?
Attachment #8655081 - Flags: approval-mozilla-beta?
Attachment #8655081 - Flags: approval-mozilla-aurora?
Attachment #8655081 - Flags: approval-comm-esr38?
Attachment #8655081 - Flags: approval-comm-beta?
Attachment #8655081 - Flags: approval-comm-aurora?
Attachment #8656471 - Flags: approval-mozilla-esr38?
Attachment #8656471 - Flags: approval-mozilla-beta-
Attachment #8656471 - Flags: approval-mozilla-aurora?
Attachment #8656471 - Flags: approval-comm-esr38?
Attachment #8656471 - Flags: approval-comm-beta?
Attachment #8656471 - Flags: approval-comm-aurora?
(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.
OS: Linux → All
There is a typo in patch part 2, in the bug number in the commit message :(
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
Attachment #8655081 - Flags: approval-comm-esr38?
Attachment #8655081 - Flags: approval-comm-esr38+
Attachment #8655081 - Flags: approval-comm-beta?
Attachment #8655081 - Flags: approval-comm-beta+
Attachment #8655081 - Flags: approval-comm-aurora?
Attachment #8655081 - Flags: approval-comm-aurora+
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
Attachment #8656471 - Flags: approval-comm-beta?
Attachment #8656471 - Flags: approval-comm-beta-
Attachment #8656471 - Flags: approval-comm-aurora?
Attachment #8656471 - Flags: approval-comm-aurora+
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.
If I want to test the fix what version should I download?
(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
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yes, looks like it helped on trunk.
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
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?
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.
Attachment #8656471 - Attachment is obsolete: true
Attachment #8656471 - Flags: approval-comm-esr38?
Attachment #8668535 - Flags: review?(neil)
Attachment #8668535 - Flags: review?(neil) → review+
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.
Attachment #8668535 - Flags: approval-comm-esr38?
Attachment #8668535 - Flags: approval-comm-beta?
Attachment #8668535 - Flags: approval-comm-aurora?
Part 2 was landed originally in Thunderbird 44.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
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
Attachment #8668535 - Flags: approval-comm-beta?
Attachment #8668535 - Flags: approval-comm-beta+
Attachment #8668535 - Flags: approval-comm-aurora?
Attachment #8668535 - Flags: approval-comm-aurora+
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
Attachment #8668535 - Flags: approval-comm-esr38? → approval-comm-esr38+
You need to log in before you can comment on or make changes to this bug.