Closed Bug 1430480 Opened 6 years ago Closed 6 years ago

Images not properly parsed/copied on mail copy/move from one mailbox to another (between different accounts on same or different IMAP server)

Categories

(MailNews Core :: Networking: IMAP, defect)

Product:
MailNews Core
Component:
Networking: IMAP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(thunderbird60 fixed, thunderbird61 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 61.0
Tracking Flags:
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed

People

(Reporter: sadjuk, Assigned: gds)

References

(Blocks 1 open bug)

Details

(Keywords: assertion)

Attachments

(2 files, 10 obsolete files)

test multipart, alternative first.eml
2.86 KB, text/plain
Details
1430480-review-changes3.patch
5.32 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

Kindly see the detailed explanation (with screenshots) via this link: https://support.mozilla.org/en-US/questions/1199480.

Someone actually suggested I'd better file it as a bug, rather than ask questions about it.

Verison: Thunderbird 52.5.2.
I've looked through the report at https://support.mozilla.org/en-US/questions/1199480.

Does the target folder become good when using right-click, Properties, Repair Folder?
Also, what are the synchronisation settings for source and target folder?

I've tried to reproduce something similar and the the enclosed message will just corrupt the target folder when moved, in fact, when displaying the moved message, I see the body of the previous message displayed.

Reporter, can you try my message? Just download the attached file and drag it into the IMAP folder and then copy it to another IMAP folder.
Attachment #8942540 - Attachment mime type: message/rfc822 → text/plain
Reporter, could you send the message in question as an attachment to me. Looks like it has confidential content, so I won't ask you to attach it here.
Some more reading in bug 770888: Messages moved by a filter don't show attachments properly. Well, embedded images are also just "attached" data in a specific MIME structure.
See Also: → 770888
Last night I was able to reproduce the problem with the attached message, but revisiting the test case, I had a lot of trouble reproducing the failure.

I believe that I have a reproducible case now and I will contact one of our volunteer developers to see whether he can reproduce it. It appears to depend on the type of IMAP server that is used.
Hi Jorg,

Please see my below run-down:

1) I was able to import your message in my personal Inbox. Message is OK, shows the inline picture. Was able to also send out a confirmation of having read it.

https://i.imgur.com/pV4S7pr.png

2) I've then copied it via right-click -> Copy To -> Inbox -> folder of choice on the other mailbox. Still not showing the picture.

https://i.imgur.com/pV4S7pr.png

As I mentioned in the initial report, the copy doesn't properly build up the "parts", whereas Part1.1 is the raw text message, while Part1.2.1 should be the HTML version of the raw message with inline images pointing to the attached images. Part1.2.1 and the attached images are 0KB in size.

As far as sending you the message I am referring, I can't do that. Has confidential data indeed. But you can try with any message with inline images, just like the one you've asked me to import/copy.

BR,
Bogdan
(In reply to sunbeam906 from comment #6)
> Hi Jorg,
> 
> Please see my below run-down:
> 
> 1) I was able to import your message in my personal Inbox. Message is OK,
> shows the inline picture. Was able to also send out a confirmation of having
> read it.
> 
> https://i.imgur.com/pV4S7pr.png
> 
> 2) I've then copied it via right-click -> Copy To -> Inbox -> folder of
> choice on the other mailbox. Still not showing the picture.
> 
> https://i.imgur.com/pV4S7pr.png
> 
> As I mentioned in the initial report, the copy doesn't properly build up the
> "parts", whereas Part1.1 is the raw text message, while Part1.2.1 should be
> the HTML version of the raw message with inline images pointing to the
> attached images. Part1.2.1 and the attached images are 0KB in size.
> 
> As far as sending you the message I am referring, I can't do that. Has
> confidential data indeed. But you can try with any message with inline
> images, just like the one you've asked me to import/copy.
> 
> BR,
> Bogdan

Sorry about that, I've linked the same screenshot for 2) as in 1) :) Here's the correct one:

1) https://i.imgur.com/pV4S7pr.png

2) https://i.imgur.com/u1qEISh.png

BR,
Bogdan
Thanks for the confirmation that my message reproduces your problem. I get the same result when copying this message to a folder on another server. However, it doesn't appear to happen with all servers.

Would you happen to know which IMAP software the server uses? Dovecot? Also, repairing the mailbox (right-click, Properties, Repair Folder) makes the message show up complete again, right?

We will be looking into the problem in the next couple of weeks, so please don't expect any "quick fix" other than the suggestion to repair the folder. That's also what the reporter of bug 1428277 suggests.
Hi Jorg,

Seems to work that way: right-click -> Properties -> Repair Folder. Image shows now.

What puzzles me is I've been using an older version of Thunderbird up to now for the sole reason that some extensions I loved (like "Compose Toolbar Buttons") were not yet compatible with newer builds. And this build (31.3.0) doesn't have this issue.

So if you (the team) plan on addressing this issue, maybe revising the code from that particular build related to what you think may cause this (programming-wise) would be a good pointer in the right direction?

Thanks once more,
Bogdan
For the record, was doing a copy of problem message with big attachment from server A to server B with tb copy menus. Seemed to work OK but when tried to open the attachment on server B, tb crashed. This was logged to console:

Assertion failure: !aNew (Logic error: Trying to read part from entire message which doesn't exist), at /home/gene/comm-central/mailnews/imap/src/nsImapProtocol.cpp:9220

Typically, the folder at server B shows the problem message header info (subj, etc) but no "paper clip" attachment icon and the email opens but shows the content as the next email in the list. This time the problem email displayed OK except opening the attachment caused a crash.
https://hg.mozilla.org/comm-central/annotate/tip/mailnews/imap/src/nsImapProtocol.cpp#l9220
      MOZ_ASSERT(!aNew,
                 "Logic error: Trying to read part from entire message which doesn't exist");
in nsImapMockChannel::OnCacheEntryAvailable

An assert of course, but I checked anyway for crash reports and find none https://crash-stats.mozilla.com/search/?signature=~nsImapMockChannel%3A%3AOnCacheEntryAvailable&product=Thunderbird&date=%3E%3D2017-12-16T20%3A50%3A58.000Z&date=%3C2018-01-16T20%3A50%3A58.000Z&page=1&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-email
Severity: normal → critical
Status: UNCONFIRMED → NEW
Component: Untriaged → Networking: IMAP
Ever confirmed: true
Keywords: assertion
Product: Thunderbird → MailNews Core
Version: 52 Branch → 52
(In reply to Wayne Mery (:wsmwk) from comment #11)
> An assert of course, but I checked anyway for crash reports and find none

I could be wrong but it appears that the NS_ASSERT that was hit might only occur with a DEBUG build. So with actual releases it will not cause a crash. I have been doing some fairly stressful tests so that might have also caused it. Since then I have seen an error pop up that attachment might not be present when I tried to open the pdf attachment.
Definition: Acuse... is an large email that has a large html content and a fairly large pdf attachment, total size 371513 bytes.

For me, a fairly reliable way to reproduce the problem is to have a folder on server A that is empty except a single simple one-line text only email. Make sure the folder is compressed (expunged) and that it is set for offline usage. Check that the folder mbox file exists and contain the single simple email. Also check that the corresponding .msf file exists, but its content is (currently) beyond me.

Now on server B, place the "Acuse..." email with the pdf attachment. If it was copied there and you can't read it due to this bug, repairing the folder should fix it. Make sure the attached pdf opens too. 

Copy Acuse... from server B to folder on server A with the single email. When you do this, tb reads the email from the mbox file on server B and sends the whole Acuse... email contents to imap server A using an imap APPEND command. The whole email is transferred and always works fine. Server A stores the received file as a new email for the folder containing the simple email. When you select the destination folder you will see now two emails. The simple email and the Acuse... email but with no attachment icon. If you select the Acuse email you will only see the simple email. The mbox file for the destination folder on server A still only contains the small email and not the large Acuse email. The msf index file now seems to contain links to the two emails but they both apparently point to the simple email.

(Note: If you run another tb instance on another machine you will see on Server A the Acuse... email and the simple email. The Acuse email (html based) can be read and the pdf attachment can be opened. This proves that Server A received the data from the imap APPEND correctly.) 

Another test is to save-as the Acuse emails on server B as a .eml file. Now on server A delete the broken Acuse email and compress/expunge the folder leaving only the simple email. Now, with a file manager, drag the saved .eml file to the folder on server A. This causes the exact same imap APPEND to occur but this time you will notice that the server A destination mbox now gets much larger and contains the simple email and the large Acuse email. Now both email and attachment can be read and opened.

So, I think the problem is that the copy between servers is causing a bad write to the destination .msf file. At this time I haven't attempted to determine where or what is written bad to the .msf.

One other point: I don't think there is a problem if the email copies occurs within the same server. In that case, APPEND is not used but just a imap COPY or UID COPY occurs.
Re-reading Comment 0, I think the reporter is saying he is copying between two accounts on the same IMAP server. In this case, APPEND will still be used and not COPY/UID COPY. There should not be a problem when the copy occurs within a given account.

Also, I haven't found any connection to server type, e.g., Courier or Dovecot. I can see the problem with my source server B above being charter and server A (destination) being yahoo.
Thanks for the investigation, Gene. I'll get back to this bug soon. So basically the copy operation doesn't copy the message data to the target mbox file but stores an invalid reference to it in the database (.msf) file.

Just for the casual reader. The test cases are some Spanish e-mails, "Acuse de recibo" is simple an "acknowledgement of receipt" of an order with a detailed order description as PDF.
I have some more cases that don't work.

STR:
Store the attached messsage (attachment 8942540 [details]) in an IMAP folder in account 1.

In account 2 create a folder, called target 1. Open a window onto the mail storage and observe that "target 1.msf" is created when the folder is created. No mbox file "target 1" is created. Now copy the message from account 1 into the folder. Then click onto the folder. The mbox for "target 1" appears and the message is completely visible.

Now repeat the test with a subtle difference:
In account 2 create a folder, called target 2. Open a window onto the mail storage and observe that "target 2.msf" is created when the folder is created. No mbox file "target 2" is created. *Now click on "target 2"*. That causes the following to go wrong. Now copy the message from account 1 into the folder. Then click onto the folder again. The mbox for "target 2" DOES NOT appear and the message is TRUNCATED to the text part, no embedded image shows. 

That reproduces comment #7. It also explains that sometimes I could reproduce the problem, sometimes I couldn't. It depends on whether the mbox is visited while still empty or not.

BTW, using the "Acuse de recibo" e-mail, it doesn't matter whether the target folder is visited before the copy or not, it always copies correctly.

Also, I *cannot* reproduce the failure from comment #13. If I understood it correctly you need to prepare a folder with a simple text mail, then copy the "Acuse de recibo" e-mail into it, of course from a different account. For me, that always works. But I have seen cases where copying an e-mail into a folder shows the previous message in the folder when just clicking on the one just copied. That's also what bug 1428277 reports, quote:
> Usually they show some previous message on the same folder.
I've seen it, but I can't reproduce it.
Actually in my case happens very frequently also with folders with mails already in and also with attachments other than images.
Sometimes happens even without attachments but with large folders (thousands on messages).
The problem may have the same root as what you found, so its fantastic to have a way to surely reproduce.
But when the bug is found remember that there may be other cases to be covered.
What seems a constant is: it happens when moving mails into folders between different IMAP accounts so different servers.
It happens more frequently when either the email, or the folder or both are large (means more time tu upload, download or adjust pointers.
My plan is: Get two 100% reproducible scenarios and fix them. Then we can release a fix either in a Daily or beta and affected users can tell us whether we fixed all problems they are seeing.

So far I can only reproduce one case, let's see whether Gene can reproduce it as well and whether he can make his case reproducible for me.
(In reply to Jorg K (GMT+1) from comment #16)
> I have some more cases that don't work.
> 
> STR:
> Store the attached messsage (attachment 8942540 [details]) in an IMAP folder
> in account 1.
> 
> In account 2 create a folder, called target 1. Open a window onto the mail
> storage and observe that "target 1.msf" is created when the folder is
> created. No mbox file "target 1" is created. Now copy the message from
> account 1 into the folder. Then click onto the folder. The mbox for "target
> 1" appears and the message is completely visible.

Already had your fully readable email on account 1. When I do this I only see the text, approx "auob Bold" and no pic in target 1 folder. Also no trash 1 mbox file created.

Don't see a difference if I "visit" the folder first. Oh well, looks like we can't reproduce each other's scenario.
It's áúóß bold ;-)
Anyway, you can reproduce my case, with or without visiting the target first. I'm doing this with a current Daily, so perhaps there are some changes.

I'd somehow like to reproduce the scenario where copying a message shows another message in the target mailbox, in this case the simple text mail.
I tried a few more times and did reproduce your scenario. Don't why it didn't seem to work on my first try.
(In reply to Jorg K (GMT+1) from comment #20)
> It's áúóß bold ;-)
> Anyway, you can reproduce my case, with or without visiting the target
> first. I'm doing this with a current Daily, so perhaps there are some
> changes.
> 
> I'd somehow like to reproduce the scenario where copying a message shows
> another message in the target mailbox, in this case the simple text mail.

When I copy anything else into Target 1 folder (the one that worked ok) I see your full test email for both. Also, the mbox doesn't grow. So I think the msf gets updated OK but the data it points to in the mbox just isn't there.
(In reply to Jorg K (GMT+1) from comment #20)
> It's áúóß bold ;-)
> Anyway, you can reproduce my case, with or without visiting the target
> first. I'm doing this with a current Daily, so perhaps there are some
> changes.
> 
> I'd somehow like to reproduce the scenario where copying a message shows
> another message in the target mailbox, in this case the simple text mail.

Hi Jorg,

Your last paragraph reminded me of this one behavior: create a folder on the IMAP server, move or not a mail in there, then rename the folder. You'll see something mad nice happening :) In my case, TB doesn't show anything new happening (not sure if I have to hit Repair Folder) - as in clicking on another folder then back on this recently renamed one won't list the content of the folder. The only way I got everything back up to normal was to restart the client (close, re-open). When the main sync kicks-in, everything's back to normal.

BR,
Bogdan

P.S.: Sadly I can't tell you which kind of email server this is, maybe Zimbra or Beehive indicate anything on the lines of this? Let me know if there's any query I can do to get that info somehow.
(In reply to sunbeam906 from comment #23)
We fixed the folder rename issue in TB 52 in bug 1328814. For me, rename folder works just fine.

(In reply to gene smith from comment #22)
> When I copy anything else into Target 1 folder (the one that worked ok) I
> see your full test email for both. Also, the mbox doesn't grow. So I think
> the msf gets updated OK but the data it points to in the mbox just isn't
> there.
Hmm, in comment #16 the "target 1" folder is the one that I didn't visit after creation and where the message showed correctly after being copied. I can copy more messages there and they all look good.

Copying more messages to "target 2" folder shows the other messages OK, just not the first one copied. For me that mbox never shows up anywhere. Weird. Should I try this on Linux to see whether that behaves differently. Maybe some file locking problem?
(In reply to Jorg K (GMT+1) from comment #24)
> (In reply to sunbeam906 from comment #23)
> Hmm, in comment #16 the "target 1" folder is the one that I didn't visit
> after creation and where the message showed correctly after being copied. I
> can copy more messages there and they all look good.
> 
> Copying more messages to "target 2" folder shows the other messages OK, just
> not the first one copied. For me that mbox never shows up anywhere. Weird.
> Should I try this on Linux to see whether that behaves differently. Maybe
> some file locking problem?

Tried it again an it worked.

However, "visiting" the newly created and empty folder before appending to it has some weird effect that stops the fetch of emails with mime parts. I thought maybe it was due to imap SELECT so I reduced the max cached conns down to 1. Made no difference. If the folder was never visited, after it is appended and then visited, headers are downloaded, then entire body of smaller emails is downloaded (actually, fetch'd) and then for the larger emails (Acuse) the html part is fetched and then pdf part is fetched and all is well. If the folder was visited before the append, the process stops after the headers are downloaded and folder is messed up.
OK, so we now agree on my scenario from comment #16?

> If the folder was visited before the append, the process stops after the
> headers are downloaded and folder is messed up.
Surely we can find out where and why it stops.

How about your scenario from comment #13? Folder with a text mail, copy the "Acuse" mail to it and bang? I tried on Linux copying between jorgk.com and Pancheta and I didn't see a problem. Can you still reproduce? On my servers as well?

I know I've seen cases where after a copy clicking on an e-mail showed content from another, but I can't make it happen at will.
(In reply to Jorg K (GMT+1) from comment #26)
> OK, so we now agree on my scenario from comment #16?
> 
> > If the folder was visited before the append, the process stops after the
> > headers are downloaded and folder is messed up.
> Surely we can find out where and why it stops.
> 
> How about your scenario from comment #13? Folder with a text mail, copy the
> "Acuse" mail to it and bang? I tried on Linux copying between jorgk.com and
> Pancheta and I didn't see a problem. Can you still reproduce? On my servers
> as well?

On jorgk.com folder chile-of-inbox there is only one simple email after the folder compressed and repaired.

Just copied Acuse from charter to jorgk.com (dest folder chile-of-inox) and I see only the simple email displayed when either is selected and there is no attachment icon on Acuse in that folder. View source for both show only the simple email.

Now deleted Acuse from chile-of-inbox, compressed and repaired chile-of-inbox so now just one simple email again in mbox and msf file. Then copied Acuse from Dell folder on pancheta and same result as chile-of-inbox on jorgk.com. Both emails display the same header and text and Acuse email shows no attachement icon. 

> 
> I know I've seen cases where after a copy clicking on an e-mail showed
> content from another, but I can't make it happen at will.

I see it a lot. I think it is just a symptom that the email's content was never stored in mbox file. In chile-of-inbox mbox there is only the simple email. But in .msf I see a reference to both emails.

I won't touch chile-of-inbox for now and you can try the same experiment. Note: When you first look at it you will probably see both emails OK since your mbox and msf will be essentially newly built when you select the folder.
Hi guys,

I see this is still on-going. Let me know if I can assist in any way.

On another note, I've been able to reproduce the bug I mentioned yesterday: on any given server, creation of a new folder and moving another folder with emails to that folder renders move activity non-usable. That's the def of it; to exemplify: create folder 1, drop emails in it; close TB; reopen; create folder 2, move folder 1 into 2; try to move any other folder on same IMAP server to folder 1, see if it works. I've not been able to get moving to work without a restart of TB.

Let me know if I should open another bug ticket (unless already reported?).

BR,
Bogdan
Correction: < [...] try to move any other folder on same IMAP server to folder **2**, see if it works. >
(In reply to gene smith from comment #27)
> Just copied Acuse from charter to jorgk.com (dest folder chile-of-inox) and
> I see only the simple email displayed when either is selected and there is
> no attachment icon on Acuse in that folder. View source for both show only
> the simple email.
> 
> Now deleted Acuse from chile-of-inbox, compressed and repaired
> chile-of-inbox so now just one simple email again in mbox and msf file. Then
> copied Acuse from Dell folder on pancheta and same result as chile-of-inbox
> on jorgk.com. Both emails display the same header and text and Acuse email
> shows no attachement icon. 
You will hate me now but I can't reproduce this. This is what I've done:
Went to chile-of-inbox, saw two e-mails, deleted "Acuse", compact, repair.
Checked mbox file in editor. I had one message.
Copied "Acuse" in again from the Pancheta account.
Checked mbox file in editor. I had one message.
Visited chile-of-inbox, all good. Checked in editor, both message present.
Tried in TB 52 and TB 59 Daily.
I'll shoot you a video ;-)
I figured you'd say that. Well, it didn't work for me yesterday, will try again. But I did notice that when I copy Acuse from jorgk.com to pancheta with the same method it works too. Haven't looked at it much today, been busy "writing" other stuff (and trying to keep warm).
Thanks for the video proof! Yes, after trying again, it worked for me too, today. But I still see the problem when I copy from charter to jorgk.com.

All I can tell is after the append to the destination folder finishes, there is a fetch of a fixed set of headers for the newly appended email. This always occurs even when the copy results are bad (and the fetched header content always looks the same):

43 UID fetch 4 (UID RFC822.SIZE FLAGS BODY.PEEK[HEADER.FIELDS (From To Cc Bcc Subject Date Message-ID Priority X-Priority References Newsgroups In-Reply-To Content-Type Reply-To)])

* 2 FETCH (UID 4 RFC822.SIZE 371513 FLAGS (\Seen) BODY[HEADER.FIELDS ("From" "To" "Cc" "Bcc" "Subject" "Date" "Message-ID" "Priority" "X-Priority" "References" "Newsgroups" "In-Reply-To" "Content-Type" "Reply-To")] {450}
Message-Id: <a29559$4c4lep@ausxipmktpc11.us.dell.com>
From: "Dell Inc." <dell_espaa_internet_orders@dell.com>
To: info@hostalpancheta.es
Reply-To: "Dell Inc." <dell_espaa_internet_orders@dell.com>
Date: 14 Sep 2014 14:40:56 -0500
Subject: =?utf-8?B?QWN1c2UgZGUgcmVjaWJvIGRlIHBlZGlkbyAtIE7Dum1lcm8gZGUgcmVjaWJvIEludGVybmV0IEVTMDEzNC04NTgzLTUzOTUx?=
Content-Type: multipart/mixed; boundary=--boundary_2152_3a5f64ec-8ecc-46c3-a0a5-387d475082c6

)
43 OK FETCH completed.

That's all that happens when the copy results are bad. But when it actually works, the entire 371513 byte email body, including html and pdf attachement, is immediately fetched from the destination server:

255 UID fetch 4 (UID RFC822.SIZE BODY.PEEK[])

* 2 FETCH (UID 4 RFC822.SIZE 371513 BODY[] {371513}
X-Mozilla-Keys:                                                                                 
Return-path: <automated_email@dell.com>
Envelope-to: info@hostalpancheta.es
Delivery-date: Sun, 14 Sep 2014 21:41:00 +0200
Received: from aussmtpmrkpc107.us.dell.com ([143.166.82.87]:61837)
	by hs-1587.dedicated.hostalia.com with esmtps (TLSv1:RC4-SHA:128)
	(Exim 4.82)
:
:
ZXINCjw8L1NpemUgMjE4L0lEWzw2RUIwMERBN0YwQ0U4NjRDQTNFMjhGNTg5MUFBOUYwQT48
NDYwMEIyMEY2Rjc2MzI0Nzg1MjkwMUIyMjhCRjVBRTQ+XT4+DQpzdGFydHhyZWYNCjExNg0K
JSVFT0YNCg==
----boundary_2153_7e369c11-5f31-4582-82e8-dc6dadce2732--

----boundary_2152_3a5f64ec-8ecc-46c3-a0a5-387d475082c6--



)
255 OK FETCH completed.

Still trying to figure out what triggers (or doesn't trigger) this full body fetch.
(In reply to gene smith from comment #32)
> Still trying to figure out what triggers (or doesn't trigger) this full body
> fetch.
Thanks.

At least the symptoms are the same as in the reproducible case of copying the message attached here to a new folder that has been visited once (comment #16). So hopefully addressing that will also fix the spurious other problems. I might be able to start looking into it next week.
Hi Jorg (and everyone monitoring this bug),

I've recorded a demo of doing this: https://www.youtube.com/watch?v=eRWoCf__MRA

- import test mail to TEST folder on account #1 of given IMAP server
- move mail from TEST folder of account #1 to TEST folder of account #2 (same IMAP server)

Moved email doesn't list image content unless folder is repaired.

What's strange is that even with folder repaired and email moved back to original location, then once more moved back (last part of the video), I have to repair the folder again. Imagine in time I'd have to move more emails to given folder; I'd have to repair it with each occasion.

Be back with a demo on the other bug I mentioned above in comment #28. For me it's not fixed (maybe we're talking about different bugs).

Oh, and here's my TB version: https://imgur.com/KMVXkNC.

BR,
Bogdan
Hi again,

Apparently, if you name a folder "TEST", there's a bit of an issue trying to delete it. You can move it around, but upon deletion server will say it can't do it; some internal error. The only way I was able to do it was to rename it to "TEST1".

See: https://www.youtube.com/watch?v=o-6DGE20beM

BR,
Bogdan
And here's the video for the bug I mentioned in comment #28: https://www.youtube.com/watch?v=RbNqDXcqlw8

Summary:

Create multiple folders with or without emails in them. Select all created folders minus 1 (the last one, let's say) with either Ctrl or Shift. Move them to the last folder. You'll see only the first folder in selection gets moved properly. The rest remain where they are. After this, try to move them individually to last folder. You'll see they can't be moved. Create a new folder and try the same, move it to this last folder of your choice. You'll see it can't be moved. "Move" action is crippled based on that multiple move action. The only way I found to restore functionality is to restart Thunderbird.

Additionally, after video recording I attempted to delete the folders. Deletion works. I also tried same sequence as above, then deleted one of the folders and tried individual move again to see if deletion did reset the move capability. Didn't work.

BR,
Bogdan
(In reply to sunbeam906 from comment #34)
> Hi Jorg (and everyone monitoring this bug),
> 
> I've recorded a demo of doing this:
> https://www.youtube.com/watch?v=eRWoCf__MRA
> 

Thanks for the video demo. It looks similar to what I see. 

I see on your right click menu there is a "copy message to clipboard" selection. I don't see that on my 52.x on windows or linux. Is it coming from an "Import/Export" add-on/extension? Does it allow you to copy a message and then paste it into another folder instead of using thunderbird's expanding/cascading copy menus?

Probably the issue with moving/copying folders that you show should be in a separate bug.
Hi Gene,

I have no clue; the reason I right-clicked was indeed to copy, then paste the message. But then I noticed there's no paste feature on my right-click; didn't want to do "Copy To", as I've collapsed the mail folders for a reason (confidentiality) - Copy To shows last few folders used as destinations.

Have though tested this and can tell you it's the "ImportExportTools 3.2.4.1" extension adding that option. I can choose copy Mail or Header, but don't see a paste option. I've pasted manually via Ctrl+C just the header to a Notepad++ window, but the Mail pasting option doesn't work. Tried in TB as well as normal paste on Desktop or in a folder. But yeah, that's a different story :)

Alright, I'll log another ticket for the other bug.

BR,
Bogdan
I think I may have found a fix. The problem is that once a folder is visited (clicked into) subsequent copies into that folder from a different server potentially causes messages read in that destination folder to be have attachments missing or inline items not visible. Items dragged in and, I think, items copied or moved in from other folders on the same server can be seen fully and are not corrupted.

The same problem occurs regardless of whether the folder has an offline store (e.g., mbox file) or if it only has the .msf file. What happens is that the copy causes the header for the message to have the ::Offline flag set that is preserved at the destination. The folder processing at the destination sees the ::Offline flag that tell it that the message is already stored offline (or in cache if there is no offline store configured for the folder). So the copied message, that was correctly appended via imap to the destination imap mailbox, is fetched from the server but never streamed to the offline store or cache. When it is accessed, the .msf contains the correct headers but the full message body and mime parts are not stored (in mbox file or cache) so are missing when the message is displayed.

The fix is in mailnews/imap/src/nsImapMailFolder.cpp. If the copy is not between the same servers, a value is set to zero that causes the ::Offline flag and several other offline related attributes to not be set when the copy occurs. 

Setting of the ::Offline flag and the other offline attributes was added with the changes associated with Bug 439108. The reason for setting them was not discussed. Actually, if I only don't set the ::Offline flag the problem is fixed. However, not setting all of them is done under another condition (overwriting a draft, that I have not been able to cause) and seems to cause no harm.

The change is minimal and can be manually applied. If you do try it, I would recommend a "repair" of the corrupted folder before doing any testing. Or just delete the folder and create a new one.
Attachment #8951163 - Flags: feedback?(jorgk)
^ Where's the LIKE button? :) Awesome explanation.
Gene, thank you so much for the research!

(In reply to gene smith from comment #39)
> The fix is in mailnews/imap/src/nsImapMailFolder.cpp. If the copy is not
> between the same servers, a value is set to zero that causes the ::Offline
> flag and several other offline related attributes to not be set when the
> copy occurs.
Can you please show me where that happens.

> Setting of the ::Offline flag and the other offline attributes was added
> with the changes associated with Bug 439108. The reason for setting them was
> not discussed.
Right, bug 439108 added it here:
https://hg.mozilla.org/comm-central/rev/81b62dba7723#l7.46
and then it got reformatted here:
https://hg.mozilla.org/comm-central/rev/097bc36f180d#l52.298
Please always include your findings in your comment, so I don't have to retrace your steps.

> Actually, if I only don't set the ::Offline flag the problem
> is fixed. However, not setting all of them is done under another condition
> (overwriting a draft, that I have not been able to cause) and seems to cause
> no harm.
Can you point me to this code, too. It's always easier to follow the train of thought, when one has all the details.
 
> The change is minimal and can be manually applied. If you do try it, I would
> recommend a "repair" of the corrupted folder before doing any testing. Or
> just delete the folder and create a new one.
I'll try it later today.
Hi Gene,
sorry, why do you say that this only happens if the source folder is on a different server?
If I understand correctly sunbeam906 is reproducing the problem by moving messages inside the same server (but maybe between different accounts).

Moreover, I'm also experiencing the same issue when filters move messages automatically from the inbox of an IMAP server to a subfolder in the same account on the same server. See https://bugzilla.mozilla.org/show_bug.cgi?id=770888#c21

Do you think this issue is related?

Thanks.
Comment on attachment 8951163 [details] [diff] [review]
proposed change: between-folders-copy.diff

This fixes the test case described in comment #16 regardless of whether I copy between accounts on the same physical server or between different servers.

Looks like dstServer->Equals(srcServer, &sameServer); returns false since the account is part of the "server", like user@server, right, Gene?

I'd be happy to accept this fix as long as my questions from comment #41 are answered, in particular, why is aMessage->SetOfflineMessageSize(0); the best choice? Where else is this used and what effects does that have on the flags?
Attachment #8951163 - Flags: feedback?(jorgk) → feedback+
(In reply to Marco De Vitis from comment #42)
> Hi Gene,
> sorry, why do you say that this only happens if the source folder is on a
> different server?
> If I understand correctly sunbeam906 is reproducing the problem by moving
> messages inside the same server (but maybe between different accounts).
> 
> Moreover, I'm also experiencing the same issue when filters move messages
> automatically from the inbox of an IMAP server to a subfolder in the same
> account on the same server. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=770888#c21
> 
> Do you think this issue is related?

I saw in you comment that you didn't see a problem when you manually moved the messages on the same account on the same server. I also don't see a problem doing that. However, I haven't tried a move/copy using a filter. I will try that and see what happens. May not be the exact same problems but possibly related.
(In reply to Jorg K (GMT+1) from comment #43)
> Comment on attachment 8951163 [details] [diff] [review]
> proposed change: between-folders-copy.diff
> 
> This fixes the test case described in comment #16 regardless of whether I
> copy between accounts on the same physical server or between different
> servers.
> 
> Looks like dstServer->Equals(srcServer, &sameServer); returns false since
> the account is part of the "server", like user@server, right, Gene?

Yes, if you have two accounts on the same server they must be considered different "servers" (sameServer set false) since a copy to the same account uses imap COPY while a copy to another account (requiring a different login), even on the same server, uses imap APPEND. (COPY uses minimal network while APPEND uses a lot.)

> 
> I'd be happy to accept this fix as long as my questions from comment #41 are
> answered, in particular, why is aMessage->SetOfflineMessageSize(0); the best
> choice? Where else is this used and what effects does that have on the flags?

At this point in the current code
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#8094
the message size is set to 0 so that the "offline attributes" are not set inside SetPendingAttributes(). My proposal is to also not set them when the copy destination is to a different server. I have not yet determined what all these attributes actually do, which I will look closer at.

My original fix, that also seemed to work and not break anything that I could see was just to comment out the setting of ::Offline flag here 
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#7527
and not bother with setting the SetOfflineMessageSize(0).
(In reply to gene smith from comment #45)
> At this point in the current code
> https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/
> nsImapMailFolder.cpp#8094
> the message size is set to 0 so that the "offline attributes" are not set
> inside SetPendingAttributes(). My proposal is to also not set them when the
> copy destination is to a different server. I have not yet determined what
> all these attributes actually do, which I will look closer at.
Thanks for the clarification. Clearly you have spent more time with this code than I have :-)

> My original fix, that also seemed to work and not break anything that I
> could see was just to comment out the setting of ::Offline flag here 
> https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/
> nsImapMailFolder.cpp#7527
> and not bother with setting the SetOfflineMessageSize(0).
Right, that's the code introduced in bug 439108 here:
https://hg.mozilla.org/comm-central/rev/81b62dba7723#l7.46
Without having read or understood bug 439108 fully, I have the impression that this bug was about improving the performance of the sync following an offline operation. So if we remove the line that sets the offline flag, we won't "break" anything but we'll cause a performance regression. So I think the approach taken in the patch is the right way. I'll leave it to you to explore further. It's good to see progress!
(In reply to gene smith from comment #44)
> (In reply to Marco De Vitis from comment #42)
>
>  However, I haven't tried a move/copy using a filter. I
> will try that and see what happens. May not be the exact same problems but
> possibly related.

I double checked manual intra-account copies and it works fine for me.

I also set up a filter to copy newly received emails from INBOX to a subfolder in the same account. I sent several test message that historically have problems with their attachements and other mime items after an inter-server copy. They appear OK in the filter destination folder and the attachements and parts are perfectly accessible.

I tested with my patch applied. However, my changes are not occurring when the messages is copied by the filter (since it's the same server). But, the offline message size is still zero even though my changes are not setting it. So the set of offline attributes are still *not* being set, which may be why the attachments and parts are still accessible at the destination.

I also checked the destination mbox file and all the copied messages are present in it.

Marco, are you testing this by actually receiving new messages into INBOX or are you copying or moving message from another folder to INBOX? (I assume your filter is moving messages from INBOX, but doesn't really matter.) 

Also, do you know your imap server type, e.g., dovecot. I have test a lot with a local dovecot server but also use my ISP's openwave server.
Marco,
Based on this grep result you posted on the other bug,

$>grep -Rl "Test2 attachment" ./
.//INBOX.sbd/starless.msf
.//INBOX.sbd/starless/cur/1518351695206383
.//INBOX.msf
.//INBOX/cur/1518351684223896

it appears you are using maildir offline storage and not mbox, based on the "cur/151..." in the path. I only tested so far with mbox. Have you tested with mbox format and see the same problem? I have another unpatched tb instance (v52.6.0) that uses maildir storage and the attachments and parts in the destination folder there are fine. However, I don't have a filter set up there to do the move. I will try that now...

...seems to work fine on the maildir based tb instance running a filter. Oh well.

Also, based on your folder structure (folders under INBOX) it appears you are using dovecot, but I could be wrong.
Hi gene,
thank you very much for your tests!

To test the issue I'm composing new messages from a different account, messages composed of a small text and one PNG image attachment, and I'm sending them to the account where I have the filter active. So yes, I'm actually receiving them.

Unfortunately I do not know the IMAP server type. Connecting via telnet I can see "perdition" being announced, but I think it is just a proxy.

Yes I am using maildir local storage, but the folder tree where I performed the grep is just the local Thunderbird storage, it is not the IMAP folders on the server.
Is the local hierarchy related to the server type somehow?

Anyway I did more tests and I have new info: the issue only appears to happen if the filter is applied AFTER junk classification.
If I set it to be applied BEFORE junk classification I cannot reproduce the issue.
I see no problem using BEFORE or AFTER setting after I enabled junk processing for the account. I assume none of your problem emails are getting classified as junk? Also, are you saying all your attachments and/or inline parts are missing after being copied by the filter? Also, is this the case for *all* emails with attachments/parts? If so, that's what I was seeing with inter-server copies.
Also, just to be sure, your destination folder is in the same account as your INBOX (not copying to another account or server)?

And, by any chance do you have any prefs set to non-default via the "config editor"?
Hi gene, I'll answer your questions:

* No, the affected emails are not getting classified as junk.

* Yes, as far as I can tell all attachments are missing from emails which were moved by the filter. (Actually, as you know, the attachments are not really "missing", they are displayed in encoded format in the message source, but cannot be opened).

* Yes, my destination folder is just a subfolder of the INBOX for the receiving account.

* No, I did not modify any prefs in the config editor.

Now, to rule out IMAP server differences I just reproduced the problem with my Gmail account.
I created in TB a subfolder of Inbox and a filter to move in there all messages containing "movee" in the subject, AFTER junk classification.
Junk processing is enabled for the Gmail account, see https://imgur.com/a/B8LU9

I then sent an email with a few words and a PNG attachments from another account to my Gmail account.
Then checked mail in TB, the message was received and moved, and when opening it the image is not displayed ("broken sheet" icon) and the attachment is said to be empty.

Thanks.
Attached patch between-folders-copy-v2.diff (obsolete) — — Splinter Review 
I found a problem while testing with my last patch. If I copy message x from a folder on server A to a folder on server B, it seemed to work OK for the most part. However, if there are n messages in the server A folder, all the data from message x to n was actually copied to server B folder. (Data from message 1 to message x-1 was not copied.) I would see lots of extra stuff in the mbox file at server B that I didn't intentionally copy. But only plain text messages displayed incorrectly (correct content followed by raw data chars to the end of the mbox file).

The problem was caused by setting the message length to 0 when copying to a different server. The zero length caused the source mbox file to be read from the starting offset (offset of message x above) to the end of the mbox file instead of just the length of message x. Now I am skipping setting the ::Offline flag only when the copy is between different servers and not touching the other offline attributes.

Still need to test more with other variations: no mbox file, maildir files, etc.
 
Attached is an updated diff that shows the corrected approach.
Attachment #8951163 - Attachment is obsolete: true
Attachment #8952047 - Flags: feedback?(jorgk)
Did tests with attachment 8952047 [details] [diff] [review] using maildir storage and copy between servers worked OK.

However, did notice some unexpected things with maildir. Noted here but the subject of another potential bug:

1. If delete to trash method used, messages are moved to trash and removed from the local maildir and the server's maildir, as expected (imap move capability supported). But when trash is emptied (all trash items marked \deleted and expunged), only files in the server's Trash maildir are removed; the local maildir files remain and are unaffected by the delete and expunge. Seems like they could build up over time.

2. If mark as deleted or remove immediately method is configured, messages are just marked as deleted or disappear immediately from the display as expected. However, the local maildir files also are deleted and the server's maildir files remain in place. I would expect the server's maildir files to remain since the deleted messages just have their \deleted flag set and no expunge occurs. I wouldn't expect the local maildir files to be deleted. However there is no "compact" right-click option so there is no way to actually delete (expunge) the files from the server, so they would tend to build up over time in the same way the local trash files could build up using delete to trash.
Gene, sadly there are heaps of maildir bugs, see meta-bug 845952. Maybe bug 1317117 covers part of what you're mentioning in comment #53.
Comment on attachment 8952047 [details] [diff] [review]
between-folders-copy-v2.diff

This seems reasonable. We've discussed the merit of setting the length to zero before. So this appears to be the more straight forward way without undesired side-effects.
Attachment #8952047 - Flags: feedback?(jorgk) → feedback+
(In reply to Jorg K (GMT+1) from comment #54)
> Gene, sadly there are heaps of maildir bugs, see meta-bug 845952. Maybe bug
> 1317117 covers part of what you're mentioning in comment #53.

Yes, bug 1317117 mentions my item 1. in comment 53 above. Still checking if there is mention of itme 2.
(In reply to Marco De Vitis from comment #49)
>
> Anyway I did more tests and I have new info: the issue only appears to
> happen if the filter is applied AFTER junk classification.
> If I set it to be applied BEFORE junk classification I cannot reproduce the
> issue.

Found what seems like a duplicate of this bug in bug 1182686. Also, in bug 1182686 comment 6 it confirms your observation. I will look at this again now that I have a maildir setup on my development machine.
(In reply to gene smith from comment #57)
> (In reply to Marco De Vitis from comment #49)
> 
> Found what seems like a duplicate of this bug in bug 1182686. Also, in bug
> 1182686 comment 6 it confirms your observation. I will look at this again
> now that I have a maildir setup on my development machine.

Still can't get it to fail. When filter moves the message *after* junk processing, the message in the destination folder displays all its attachments. Tested with several different messages. Also, the changes I made to fix the primary subject of this bug are not occurring and causing an accidental fix to the "move by filter" problem.

So I don't see this problem on linux running a development/daily tb version and I don't see it on windows running 52.6.0, both configured to use maildir. I will boot an old OSX and see if maybe it is OS dependent.
(In reply to gene smith from comment #58)
> (In reply to gene smith from comment #57)
> > (In reply to Marco De Vitis from comment #49)
>
> I will boot an old OSX and see if maybe it is OS dependent.

No luck. Still see attachments OK on move by filter after junk with or without maildir on Macbook AIR OSX running tb 52.6.0.
???
(In reply to Marco De Vitis from comment #51)
>
> I created in TB a subfolder of Inbox and a filter to move in there all
> messages containing "movee" in the subject, AFTER junk classification.
> :
> I then sent an email with a few words and a PNG attachments...

Marco, Could you send me a test message like the one you refer to that causes problems? I have 3 systems ready to copy the message to another folder in the account, *after* junk processing. Since I am unable to duplicate this with my own test messages, maybe there is something special about your message that triggers the problem. I have the filters also set to copy messages with subjects containing "movee". If this is OK, please send the test message to this address:
eugene_smith@charter.net

Thanks!
Flags: needinfo?(starless)
(In reply to gene smith from comment #60)

> Marco, Could you send me a test message like the one you refer to that
> causes problems? I have 3 systems ready to copy the message to another
> folder in the account, *after* junk processing. Since I am unable to

Hi gene,
I sent you two test mails, and I did put my address in Cc.

Strangely, the first mail was moved successfully in the subfolder, but the second one was not, and it was affected by the issue.
The only difference AFAIK is that the first one contained more text.

Thank you again for investigating!
I also hope to find the time to test TB on Windows.
Flags: needinfo?(starless)
Actually, I have tested on windows, linux and mac.
Anyhow, thanks for sending the test message. Like I said, I have 3 systems ready to receive and copy the message, each to a different folder in the same account. 

The first one I looked at was the macbook air. Both messages got copied correctly to the destination folder and were 100% readable.
Then I looked at linux and the messages were in Inbox but never got copied at all.
On windows, the same thing. No copy at all. The filter action on new messages probably depends on which systems sees the message first and marks the imap \Seen flag first.

On linux, I then did the "run filter now" on inbox folder and still nothing copied.
On windows, "run filter now" did copy the message, but the attachments are not accessible.

Actually, after I asked you send the test messages, with my own messages I noticed that I would only see the missing attachment problem when I did "run filter now". It never had the problem when copying the message when the email was first received. Also, some systems (not always linux) would not do the copy at all even when manually running the filter. 

So I need to look closer into this. It's looking more like a completely separate problem from what I have worked on and fixed in this bug so far. Thanks for helping!
(In reply to gene smith from comment #62)
> Also, some systems (not always linux) would not do
> the copy at all even when manually running the filter. 

Actually, was only on linux the filter didn't work manually. But it was a user (me) error. I had the filter set to subject "ends with movee" instead of "contains movee". However, when filter manually run on all systems, the messages copied to the destination folder have missing/empty attachments (although their encoded text appears with show source but there is no maildir file for the message).

Now, debugging this, I see the same problem as when messages are copied between accounts. The message is saved to cache but never saved to offline store (maildir/mbox file) because the ::Offline header flag for the message is set before the copy so offline storage is deemed to have already occurred and never really happens.

I can fix it by unconditionally not pre-setting the ::Offline flag regardless of whether the copy is to the same or different server. I will do some more tests to see if this has any side effects.
Gene,
I tested with Thunderbird 52.6.0 on Linux and I can confirm your findings:

* The filter is not automatically triggered when the message is received. I do not think it is due to other clients accessing the same mailbox because I did set offline my main TB on Mac. And I repeated the test more times, with the same result.

* When applying the filter manually the message is moved but then it has the empty attachments problem.

Anyway, did you experience the same issues if using mbox instead of maildir?
While searching around I discovered that maildir storage is still considered quite experimental and buggy in TB, even though the option is now officially available to be selected.
I prefer using it because mbox files can get huge and this can be an issue with online incremental backup services, where huge files have to be re-uploaded with each update.
But if maildir really is still so buggy, then maybe I should switch back to mbox...
(In reply to gene smith from comment #63)
> 
> Now, debugging this, I see the same problem as when messages are copied
> between accounts. The message is saved to cache but never saved to offline
> store (maildir/mbox file) because the ::Offline header flag for the message
> is set before the copy so offline storage is deemed to have already occurred
> and never really happens.
> 
> I can fix it by unconditionally not pre-setting the ::Offline flag
> regardless of whether the copy is to the same or different server. I will do
> some more tests to see if this has any side effects.

I've done a lot of testing using maildir and mbox and don't see any problem in always not doing the preset of the ::Offline flag in nsImapMailFolder.cpp function SetPendingAttributes(). My previous proposal was to not do the preset only when sameServer is false.

I did determine that when filter is set as "before junk processsing" that the messageSize variable is always zero. This causes none of the Offline attributes to be preset, including ::Offline flag. That's why it is observed in this bug and in other bugs that filtering "before junk processing" works OK. However, when done after processing, the value of messageSize is now correct and non-zero. Also, when filter is ran manually messageSize is correct and non-zero. In these cases ::Offline flag would be preset, and just like in the case where "sameServer" is false (copy/move between accounts), the move/copy is not completely successful in that attachments or inline graphics are not accessible.

I did have some trouble in always seeing a problem when the filter was in mode "after junk processing". For me, it would usually work OK when the filter did a simple copy from Inbox to another single folder, f1. I was only able to reproduce the problem consistently when I made multiple destination for the filter, e.g., copy from Inbox to f1, f2, f3, f4, f5. In this case, with maildir, a "complete" copy would occur to f1 but only a partial copy would occur to f2-f5. With mbox it was just the opposite: partial copy to f1 and full copy to f2-f5. 

I also tested the filters when copying to another account and it worked correctly.

Another test was to verify the filter copies OK in "before" and "after" junk mode when there is no offline store. In this case, the messages are typically stored to disk cache and this works OK too with preset of ::Offline is removed.

(In reply to Marco De Vitis from comment #64)
> Gene,
> I tested with Thunderbird 52.6.0 on Linux and I can confirm your findings:
> 
> * The filter is not automatically triggered when the message is received. I
> do not think it is due to other clients accessing the same mailbox because I
> did set offline my main TB on Mac. And I repeated the test more times, with
> the same result.

I am not seeing a problem anymore where the filter is not triggered when a matching message arrives into Inbox. But I now only have one tb client waiting for and processing the message.

> 
> * When applying the filter manually the message is moved but then it has the
> empty attachments problem.

Yes, this is true for mbox and maildir and before and after junk processing. Not presetting the ::Offline flag fixes this too. When run manually, the messageSize variable is always correct and non-zero so, with the release code, the ::Offline flag will be preset, which causes the problem.

> 
> Anyway, did you experience the same issues if using mbox instead of maildir?

Basically the same problem with both, once I was consistently able to duplicate the problem.

> While searching around I discovered that maildir storage is still considered
> quite experimental and buggy in TB, even though the option is now officially
> available to be selected.
> I prefer using it because mbox files can get huge and this can be an issue
> with online incremental backup services, where huge files have to be
> re-uploaded with each update.
> But if maildir really is still so buggy, then maybe I should switch back to
> mbox...

In Comment 53 I point out some things I've seen with maildir and Comment 54 points to some more. It would definitely be better for backup purposes. I looked at it quite a bit while using a gmail account while testing this bug and it seemed to work pretty good.
I didn't mention this before but whenever a copy between accounts or a filter induced copy occurs and there is problem displaying the attachments or inline mime content, this error appears in the console output for a debug build:

[24552, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /home/gene/comm-central/mailnews/base/util/nsMsgDBFolder.cpp, line 882

where NS_ERROR_FILE_NOT_FOUND = 0x80520012

This occurs when nsImapProtocol.cpp function nsImapMockChannel::ReadFromLocalCache() is called and the message content cannot be found in the local cache or offline store. The proposed fix for this bug (don't preset the message ::Offline flag) also prevents this error.
Assertion failure: !aNew (Logic error: Trying to read part from entire message which doesn't exist), at /home/gene/comm-central/mailnews/imap/src/nsImapProtocol.cpp:9195

Same as in comment 10 above. Copied to another server one of the large POSTA CERTIFICA test messages with attachments and when click on it I see the assertion. But this time the warning from comment 66 didn't occur. I am running with a pristine (no printfs or other debug code added) trunk version of daily that I just built. Don't have my fix in it right now.
Actually, the crash observeed in comment 10 and comment 67 can occur with or without the fix if the message in the destination folder is accessed quicky after the copy. However, if the assertion is ignored and just replaced with a printf (for test purposes) the message in the destination account displays OK when the printf occurs when the fix is applied. The MOZ_ASSERT allows a debugger to be attach within 300s before completely terminating tb. Also, when DEBUG not defined, MOZ_ASSERT macro becomes a noop. Possibly it could be replaced with NS_ASSERTION macro, also used in mailnews, that just prints a message with DEBUG defined and also is a noop when DEBUG not defined.
Attached patch 1430480-review-changes0.patch (obsolete) — — Splinter Review 

        Attachment #8952047 -
        Attachment is obsolete: true

        Attachment #8953801 -
        Flags: review?(jorgk)

    
    

    
  
Comment on attachment 8953801 [details] [diff] [review]
1430480-review-changes0.patch

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

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +7525,5 @@
>                                                    messageOffset);
> +        // No longer setting "flags" attribute to the Uint32 value
> +        // nsMsgMessageFlags::Offline here because it can cause missing
> +        // attachments or inline parts when messages are manually or by
> +        // filter action moved or copied.

I've followed this bug a bit and the discussion about setting or not setting the "flags" attribute. One version of the patch set it but only for "same server" moves.

What are the consequences of not setting the attribute? Is the message then fetched again from the server? Wouldn't that cause a performance issue, especially when moving many messages? Or is same-server move a different code path and this is for copy only?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +9190,5 @@
>      //    mTryingToReadPart==true.
>      if (mTryingToReadPart)
>      {
>        // We are here with the URI of the entire message which we know exists.
> +      NS_ASSERTION(!aNew,

MOZ_ASSERT() is the official macro to use. Of course that crashes a debug build, but it would be a serious logic error if this ever got triggered.

I wrote this code when we switched to cache2 about two years ago. I'm really worried that this can fail. Basically we tested that the entire message exists in the cache in OpenCacheEntry() (last few lines), and when we come to retrieve it from the cache, it's not there??

As you said, this fails with and without your patch so we should definitely look into it in a follow-up bug. I have a patch that adds debug to IMAP caching.

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #70)
> Comment on attachment 8953801 [details] [diff] [review]
> 1430480-review-changes0.patch
> 
> Review of attachment 8953801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/imap/src/nsImapMailFolder.cpp
> @@ +7525,5 @@
> >                                                    messageOffset);
> > +        // No longer setting "flags" attribute to the Uint32 value
> > +        // nsMsgMessageFlags::Offline here because it can cause missing
> > +        // attachments or inline parts when messages are manually or by
> > +        // filter action moved or copied.
> 
> I've followed this bug a bit and the discussion about setting or not setting
> the "flags" attribute. One version of the patch set it but only for "same
> server" moves.
> 
> What are the consequences of not setting the attribute? Is the message then
> fetched again from the server? Wouldn't that cause a performance issue,
> especially when moving many messages? Or is same-server move a different
> code path and this is for copy only?

This has no effect on the copy or move source mailbox. Just to make sure, I did some more test copies (with the patch applied) between servers and the source data was obtained from mbox files or cache and not by going out on the network and fetched again with imap. Of course, if "offline store" is not in use at the source mailbox and the email's url isn't in cache, it will have to go to the server to get the data.

Copies between mailboxes of the same account/server also don't cause unnecessary imap network accesses with the patch in place.

I removed the "same_server" conditional for setting the ::Offline flag and just never set it to fix the problem with copying with a filter as described in comment 65.

> 
> ::: mailnews/imap/src/nsImapProtocol.cpp
> @@ +9190,5 @@
> >      //    mTryingToReadPart==true.
> >      if (mTryingToReadPart)
> >      {
> >        // We are here with the URI of the entire message which we know exists.
> > +      NS_ASSERTION(!aNew,
> 
> MOZ_ASSERT() is the official macro to use. Of course that crashes a debug
> build, but it would be a serious logic error if this ever got triggered.

Yes, as I noted above, it it fairly easy to trigger. The newly copied-in large email with attachment or inline stuff may initially appear missing. But if you look at another email and then come back to it, it is then OK.

Anyhow, I have no problem with removing this part of the patch.
Assignee: nobody → gds

    
    

    
  
Comment on attachment 8953801 [details] [diff] [review]
1430480-review-changes0.patch

OK, I'll remove the second hunk and get this landed (with the comment tweaked a bit). I'll file another bug for the cache failure.

        Attachment #8953801 -
        Flags: review?(jorgk) → review+

    
    

    
  
Before landing this, I ran
  mozilla/mach xpcshell-test mailnews/imap/test/unit
and it turned out that test_offlineCopy.js no fails, so
  mozilla/mach xpcshell-test mailnews/imap/test/unit/test_offlineCopy.js.

Failures are:
[copyMessagesToSubfolder : 167] 0 == 6813
line 167: Assert.equal(message.messageOffset, parseInt(message.getStringProperty("storeToken")));
[test_headers : 176] 0 == true
line 176: Assert.ok(newMsgHdr.flags & Ci.nsMsgMessageFlags.Offline);

The second failure is easy to understand, the flag isn't set since we removed setting it.

These tests can be a pain, first we need to figure out that they are trying to do and then we need to decide whether to change the test or whether the test detected something we overlooked :-(

    
    

    
  
I managed to fix the unit test so it passes but with further testing I am seeing something that I thought was OK before. Not sure if it is related to this bug or is a separated bug or if it is just user confusion. I see this with and without the patch for this bug.

When testing before without offline store, it seemed like large and recently accessed messages were stored in cache so they were not fetched or downloaded via imap when re-selected. I originally thought I saw in the "advanced" options/preference setting that the large messages were stored and maybe 18MB of cache was being reported as used there. When I selected another recently selected message it would just be pulled from cache and not fetched on the network. When I "clear" the cache on the advanced page, then all selected messages were re-downloaded. 

But now the max cache usage I see used is 1.1MB which is much less the the message sizes I am testing. So the re-selected messages are always pulled from the network as I observe with wireshark and/or tb's imap.log.

I found a bug fix from last summer by Jorg where the memory cache size was increased in the default preferences. I tried increasing those values some more and it made no difference.

But maybe I am just don't understand tb "caches". Are there actually two caches: a memory cache and a disk cache? Maybe the disk cache is what shows the reported usage on the advanced preference page? For email data, maybe tb only uses the memory (ram, virtual memory?) cache? Maybe disk cache (files found in user profile) only contains html data that tb encounters? There is also a small "cache" defined in nsImapProtocol.cpp called kDownLoadCacheSize = 16000 for storing imap "line" data and pretty sure it is not where full emails are cached. I also sometimes see the offline store files referred to as offline cache.

    
    

    
  
Ok, regarding the always re-downloaded emails w/o a offline store in comment 76, I went back to my "shelved" changes (with lots of printf and other changes) while analyzing this bug and remembered that I made two changes to the code that fixes the problem for the most part. This needs to be discussed and fixed under another new bug report.

    
    

    
  
Let me try to clarify this.

(In reply to gene smith from comment #76)
> When testing before without offline store, it seemed like large and recently
> accessed messages were stored in cache so they were not fetched or
> downloaded via imap when re-selected. I originally thought I saw in the
> "advanced" options/preference setting that the large messages were stored
> and maybe 18MB of cache was being reported as used there. When I selected
> another recently selected message it would just be pulled from cache and not
> fetched on the network. When I "clear" the cache on the advanced page, then
> all selected messages were re-downloaded.
This is how it should work. Messages up to 25MB should be cached in memory. That's set in mailnews.js with
pref("browser.cache.memory.max_entry_size", 25000); //  25 MB
pref("browser.cache.memory.capacity",      200000); // 200 MB = 8*25 MB

> But now the max cache usage I see used is 1.1MB which is much less the the
> message sizes I am testing. So the re-selected messages are always pulled
> from the network as I observe with wireshark and/or tb's imap.log.
That's shouldn't be like that. You can also use my debug patch in attachment 8956770 [details] [diff] [review]. Is this really so? That would mean that IMAP caching is broken. I've just tried with two message, 10 MB and 2MB. At first they are downloaded which takes its time and I can see it in the progress bar ("Downloading message..."), when selected again, they load almost instantly ("Loading message..."). So as far as I can tell, this still works.

> I found a bug fix from last summer by Jorg where the memory cache size was
> increased in the default preferences. I tried increasing those values some
> more and it made no difference.
Yes, we increased those limits to 25MB.

> But maybe I am just don't understand tb "caches".
Neither do I, but I think the explanation given below seems correct.

> Are there actually two caches: a memory cache and a disk cache?
M-C's "cache2" can to lots of things and is also used when displaying HTML. I'm not sure whether that's moved to disk storage. Surely the "Clear Now" will clear all caches. To store entire IMAP message or message parts we use the memory cache only and I'm not sure whether that's ever paged out to disk.

> Maybe the disk cache is what shows
> the reported usage on the advanced preference page? For email data, maybe tb
> only uses the memory (ram, virtual memory?) cache? Maybe disk cache (files
> found in user profile) only contains html data that tb encounters?
I think so.

> There is
> also a small "cache" defined in nsImapProtocol.cpp called kDownLoadCacheSize
> = 16000 for storing imap "line" data and pretty sure it is not where full
> emails are cached.
Right, there is a class nsMsgImapLineDownloadCache for buffering lines being downloaded.

> I also sometimes see the offline store files referred to as offline cache.
Exactly.

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #78)
:
>You can also use my debug patch in attachment
> 8956770 [details] [diff] [review].

Thanks, I'll take a look at this.

> Is this really so? That would mean that
> IMAP caching is broken. I've just tried with two message, 10 MB and 2MB. At
> first they are downloaded which takes its time and I can see it in the
> progress bar ("Downloading message..."), when selected again, they load
> almost instantly ("Loading message..."). So as far as I can tell, this still
> works.

Just to make sure, does the folder you are testing have offline store disabled and there is no mbox file along with the *.msf file? I have seen times when I unchecked offline in the folder properties or in the "Items for offline use" dialog and it kept using and re-creating the mbox file even after I manually deleted it. And other times I had no problems in removing and disabling the offline store mbox file. This may be another bug.

Also, this may depend on the structure and size of the message. I need to test again, but smaller messages with simple attachments may work OK w/o offline store. But more complex messages with nested parts like the test message from the other bug about signatures with subject "POSTA CERTIFICATA: test large attachment" and the test message you provided with subject "Acuse..." were what I tested with and saw the problem.

> > But maybe I am just don't understand tb "caches".
> Neither do I, but I think the explanation given below seems correct.

I also see something called "necko" cache which I think is just another name for the disk cache (cache2?). Thanks for the clarifications!

    
    

    
  
Yes, I checked, no offline storage. You are correct, the caching depends on whether the message has inline attachments (images) or non-inline attachments (for example PDF files). Also, whether you're displaying (inline) attachments inline (View menu). Some message always get fetched, I think those with non-inline attachments or when attachments are not viewed inline. However, attachments should be cached in their own right.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1430480-review-changes1.patch (obsolete)
        — 
          — Splinter Review
      

    


  
This updates the patch to include a fix for the unittest so it passes. It's been a while since I updated the test but I don't think it is "papering over" anything. Some changes just make var names more consistent.
I wanted to go ahead and allow this patch to possibly be tested in beta. There has been recent activity on bug 1182686 regarding possible missing attachments after copies in maildir setups. Although user experiences vary when doing copies (manually or with filters, or mbox vs. maildir), I think this might fix the reported problems in that bug too.

(Other issues noticed and discussed in this bug have been or will be moved into new bugs.)

        Attachment #8953801 -
        Attachment is obsolete: true

        Attachment #8964659 -
        Flags: review?(jorgk)

    
    

    
  
Comment on attachment 8964659 [details] [diff] [review]
1430480-review-changes1.patch

See next comment.

        Attachment #8964659 -
        Flags: review?(jorgk)

    
    

    
  


  
I don't think setting Ci.nsMsgMessageFlags.Offline is the right way to go here.

The test deals with four messages and the first two already have the flag set as the debug I added shows:
INFO "=== count=1, is offline=128
INFO "=== count=2, is offline=128
INFO "=== count=3, is offline=0
INFO "=== count=4, is offline=0

The first two messages are created via addMessagesToServer(), three and four are created via CopyFileMessage().

I haven't found where this code lives for IMAP, it must be somewhere in nsImapService::AppendMessageFromFile() or similar.

Rather than setting the flag for messages three and four we should investigate why copying a message from a file to an IMAP store doesn't set that flag already as "adding messages to a server" does.

Do you agree?

    
    

    
  
Comment on attachment 8964659 [details] [diff] [review]
1430480-review-changes1.patch

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

::: mailnews/imap/test/unit/test_offlineCopy.js
@@ +123,5 @@
>    function copyMessagesToSubfolder() {
>      //  a message created from IMAP download
>      let db = IMAPPump.inbox.msgDatabase;
>      let msg1 = db.getMsgHdrForMessageID(gMsgId1);
> +    msg1.flags |= Ci.nsMsgMessageFlags.Offline;

Not necessary, that flag *is* already set.

    
    

    
  


  
I've looked at this for a long time now.

I see
 0:02.75 pid:9292 === nsImapMailFolder::CopyFileToOfflineStore 1
 0:02.75 pid:9292 === nsImapMailFolder::CopyFileToOfflineStore setting offline
but the flag is lost due to the code you removed.

Let me make another proposal. I briefly tested that at 1:35 AM with the test case from comment #16 and it still appears to be good.

        Attachment #8966383 -
        Attachment is obsolete: true

    
    

    
  


  
With this patch, the test still passes. I have the impression that it also fixed the message copy problem, but I'll leave the intensive testing to you.

        Attachment #8966426 -
        Flags: feedback?(gds)

    
    

    
  
Was working on another idea when I saw your email, but it doesn't seem to work. Will test your idea tomorrow.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1430480-review-changes2.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I was somewhat doubtful about your proposed fix since it looked like it still typically set the ::Offset flag when the source mailbox has offline store. When I tested it, it only worked when the source message is pulled from cache/imap (no offline store) so that the ::Offline flag is not set. I was also able to duplicate the message corruption, with your fix in place, after copying from a folder with offline store to a folder on another account with offline store.

I tried several other ways to set the offline flag but nothing fixed it so the unit test would pass. Then I looked closer at the unit test and where it was failing. Here's what it does:

0. Fake server starts out in mbox mode with a default profile at /tmp/firefox. Folder 1 is created in the profile.

1. Puts msg1 and msg2 into INBOX. When this happens there is no imap activity probably because autosync is switched off. But I can see that the messages get streamed to the INBOX file. Also, SetPendingAttributes() does not occur in this case. But as you pointed out, the offline flag for msg1 always tests good, so it is set somewhere.

2. Next a file copy of msg3 and msg4 to INBOX occurs. In this case imap activity does occur in that the files are transferred to the server using the imap "append" command to the INBOX. Also, before the transfer occurs, SetPendingAttributes() does occur and, with your fix, the header offline flag is seen as set (not sure why) so the pending ::Offline is then set and the tests pass.

3. Msg1, 3 and 4 are then copied from INBOX to folder 1. This causes no imap activity and SetPendingAttributes() is not called. Then in folder 1, the three messages are checked to make sure the offline flag is set. This is the point of failure with my original patch (msg3 and msg4 are missing the offline flag, msg1 is OK).

4. Msg1, 2, 3 and 4 are then moved from INBOX to folder 1. So folder 1 ends up with 7 messages and INBOX has 0. Again, this operation causes no imap activity and SetPendingAttributes() does not occur.

5. The profile that reside in /tmp/firefox is then deleted and the the server is reconfigured to maildir mode and the test steps repeat one time.

My new proposed fix is to only set the pending ::Offline flag in SetPendingAttributes() when it is needed to pass the unmodified unit test. This would be when it is called from CopyFileToOfflineStore() that occurs in step 2 above. So I added a new bool parameter, aSetOffline, to SetPendingAttributes() that defaults to false but is only set true when called from CopyFileToOfflineStore().

Dragging a file onto a folder (e.g., from desktop) causes the same functions as step 2 above. That was one activity that always worked fine when copying in from folders on another server caused corruption problems. So presetting the ::Offline flag in that case will cause no problems (but now allow the unit test to pass). In all other cases where messages are copied between servers or folders, CopyFileToOfflineStore() does not occur so pre-setting the ::Offline flag will not occur, keeping the fix for the original problem.

        Attachment #8967242 -
        Flags: review?(jorgk)

    
    

    
  
>1. Puts msg1 and msg2 into INBOX. When this happens there is no imap activity probably because autosync is switched off. But I can see that the messages get streamed to the INBOX file. Also, SetPendingAttributes() does not occur in this case. But as you pointed out, the offline flag for msg1 always tests good, so it is set somewhere.

Correction: Actually there is imap activity here in the form of a fetch to bring in the msg1 and 2 headers and bodies. This is were the data comes from that I see being streamed to INBOX. (I tried turning on autosync and it caused extra fetches.)

    
    

    
  
Comment on attachment 8967242 [details] [diff] [review]
1430480-review-changes2.patch

Thanks, let's go with this.

        Attachment #8967242 -
        Flags: review?(jorgk) → review+

        Attachment #8966426 -
        Attachment is obsolete: true

        Attachment #8966426 -
        Flags: feedback?(gds)

        Attachment #8964659 -
        Attachment is obsolete: true

        Attachment #8956761 -
        Attachment is obsolete: true

        Attachment #8966425 -
        Attachment is obsolete: true

        Attachment #8967242 -
        Flags: approval-comm-beta+

    
    

    
  
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/36a963a38529
Fix missing message parts when messages copied manually or filtered. r=jorgk
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED

          status-thunderbird60:
          --- → affected

          status-thunderbird61:
          --- → fixed
Target Milestone: --- → Thunderbird 61.0

    
    

    
  
Backout:
https://hg.mozilla.org/comm-central/rev/25d54026f87c9b351dbec342cbc339db9406c62e

Sorry, I had to back this out. It causes a test failure in test_index_messages_imap_offline.js, so
  mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_index_messages_imap_offline.js

These Gloda tests are quite painful :-(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 61.0 → ---

    
    

    
  
My tip would be: Add 'true' to the other callers of SetPendingAttributes() and see which will fix the test. Then see whether that still maintains the "fix" we want here.

    
    

    
  


  
Updated previous patch by added preset of ::Offset to when user copy occurs between folders of the same server/account (added true parameter to SetPendingAtributes() inside ReplayOfflineMoveCopy()). This is OK since I never see a problem when copying within the same server/account. 

This change now allows the gloda related unit test to pass that failed with the previous patch. 

The ::Offset preset is still skipped when copy occurs between folders on different servers and when doing copies via filters, maintaining the intended fix.

        Attachment #8967242 -
        Attachment is obsolete: true

        Attachment #8967647 -
        Flags: review?(jorgk)

    
    

    
  
Comment on attachment 8967647 [details] [diff] [review]
1430480-review-changes3.patch

I'm glad you get that sorted out easily!

        Attachment #8967647 -
        Flags: review?(jorgk) → review+

    
    

    
  
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/316b36c77bec
Fix missing message parts when messages copied manually or filtered. r=jorgk
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED

    
    

    
  
I've tweaked the comments a bit and made the aSetOffline parameter non-optional since we call the function four times, twice with 'false' and twice with 'true'.
Target Milestone: --- → Thunderbird 61.0

        Attachment #8967647 -
        Flags: approval-comm-beta+

    
    

    
  
Been tracking progress on this, reading the comments here and there, as much as my experience allowed. Glad it's gotten to a resolution. Outstanding! I'll fetch the Beta 3 if available and report status.

    
    

    
  
Thank you, gene, for this fix!
This fixed the bug for us.

    
    

    
  
I just discovered a special case where this doesn't quite fix it. If you take the sample email attachment from bug 647562 and edit out the empty attachment header pointed out in bug 647562 comment 24 and drag this email to a folder and then copy the contents of that folder containing several other emails to another folder on the same account, the sample email does not display when accessed in the destination folder. When you try to read that email, you see the wrong email. All the others are OK. The problem seems to be that the bad email is never fetched from the server, only its header info is fetched at the destination folder so the email never gets stored in offline storage mbox for the destination folder. 

In this case the ::Offline flag is still getting set but if I force to not be set, the sample email is readable at the destination. Of course, this causes unit test failure as was discovered above, so not a correct solution.

Even after removing the empty attachment header, that sample email actually contains two main headers for some reason. If you edit out the other header the copy works fine and the email is fetched from the server and stored into the mbox and displays properly after the copy.

A work-around for this problem is to "delete" the bad email and then immediately do ctrl-z to bring it back. This causes a proper fetch of just that email from the server and stores it to mbox without having to do a full folder repair that re-downloads the whole folder. (This may also be a general workaround for this bug that I had never tried before.)

At this time I have no idea why this problem occurs for this somewhat malformed email. Also, I think it works OK if the destination folder has no offline store and just stores to cache.

    
    

    
  
gene, is a bug filed for comment 102?
Flags: needinfo?(gds)

    
    

    
  
No bug report yet. I need to revisit that and try to duplicate it again. (Keeping NI set.)

    
  
Flags: needinfo?(gds)
Summary: Images not properly parsed/copied on mail copy/move from one mailbox to another (same IMAP server) → Images not properly parsed/copied on mail copy/move from one mailbox to another (between different accounts on same or different IMAP server)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: