790912 - Message shows wrong content (body text) after move to other folder

Status: VERIFIED FIXED

(MailNews Core :: Database, defect)

Description

[Ben Bucksch (:BenB)]

Environment:
* Ubuntu 12.04 64bit
* Thunderbird trunk, 2012-07-23, self-built
* No "Conversations" ext
* IMAP server

Reproduction:
1. Open message, click "Reply", or "Write" new mail
2. Send mail
3. Go to Sent folder, and move the message to some other folder, e.g. "ProjectA"
4. Open folder "ProjectA"
5. Open the message you just moved

Actual result:
Often (not 100% reproducible, but maybe 50%):
The message shows some other, completely random content.
* Sometimes, it shows some other message with headers (as plaintext, unformatted, i.e. raw RFC822) in the body area, and the message content afterward.
* Sometimes, it shows some other message, headers not in body area, but in the header pane, but the headers are for that other message, not for the one I clicked on
* Sometimes, it shows some other message and the header pane fields are empty (blank From, blank Subject etc.)

Expected result:
* The message that I clicked on is shown
* From+Subject in header pane, not in body area

Diagnose:
- This is a regression
- protz said in bug 752768 comment 21 he "had consistently wrong results with streamHeaders ... starts streaming at a wrong offset"

Comment 1

[Ben Bucksch (:BenB)]

Note:
* If you open the message while it's still in the Sent folder, it displays fine. The bug appears after the move.
* A restart of Thunderbird doesn't fix it.

Comment 2

[Wayne Mery (:wsmwk)]

irving has a profile of mine where I've seen something like this

Comment 3

[Jonathan Protzenko [:protz]]

The offset is almost always wrong for "fake headers" (bug 574441). Fake headers are headers that are the result of a sent message; because it can take a while until autosync updates the "sent" folder, we speculatively write the message header locally, and then autosync is supposed to figure out afterwards what happens.

I suspect the offset we keep is the one of the "old" header, or that something is not updated, because the speculative message header has much less mime headers than the real "sent" message...

Comment 4

[Kent James (:rkent)]

I'm currently looking at this, as it is 100% reproducible on my main profile.

Comment 5

[Wayne Mery (:wsmwk)]

this doesn't fix with "repair folder", correct?

Comment 6

[Kent James (:rkent)]

(In reply to Wayne Mery (:wsmwk) from comment #5)
> this doesn't fix with "repair folder", correct?

I don't want to try that, because I might lose my reproducibility.

Comment 7

[Ben Bucksch (:BenB)]

> this doesn't fix with "repair folder", correct?

It does fix it.

Comment 8

[Kent James (:rkent)]

Attachment: use storeToken or messageOffset

So this I believe fixes the issue that I am seeing. Let me describe the issues as I understand them. This is quite complex unfortunately as the method calls have so many side effects where a lot of the real action is happening.

Previously, the messageOffset was used to set an integer offset from the beginning of an mbox file where a message is stored. With the advent of pluggableStores, that is being replaced by a string storeToken, which in the case of mbox is just a string representation of the messageOffset.

When a message is copied to an offline store (for example the Send folder), CopyFileToOfflineStore does the work. That uses a message parser to parse the file, and at the end of parsing, the messageOffset of the header is set using m_envelope_pos in nsParseMailMessageState::FinishHeader(); So the parser must have the value of m_envelope_pos to correctly set that. Pre-pluggableStores that was set. Post pluggableStores it was supposedly set only in fakeHdr as a side effect of GetOfflineStoreOutputStream, but that value is being reset in nsParseMailMessageState::FinishHeader() But the correct storeToken survived, which is why you can open a message in the Sent folder correctly. The fix is to simply set m_envelope_pos again in the parser. This does not really work with non-mbox stores, but there are other reasons why the offline store is currently forced to be an mbox anyway.

So the above fix starts to get the correct messageOffset set for offline messages, which allows offline copies of those messages to start working correctly. (Note also that the items in the Sent folder due to this problem cannot be correctly viewed now on pre-pluggable store versions of TB, but after the fix this issue will go away for new moves.)

But I would also like to allow post-pluggableStores versions to correctly copy any messages from offline stores that have storeToken but not messageOffset due to the earlier issues. That is what the second change in CopyOfflineMsgBody does.

I can probably do a unit test for this as well.

Comment 9

[Jonathan Protzenko [:protz]]

Wow, thanks for the work. Do you think it will fix streamHeaders not working correctly as well?

Comment 10

[Ludovic Hirlimann [:Usul]]

(In reply to Kent James (:rkent) from comment #8)
 
> I can probably do a unit test for this as well.

Yes please !

Comment 11

[Kent James (:rkent)]

(In reply to Jonathan Protzenko [:protz] from comment #9)
> Wow, thanks for the work. Do you think it will fix streamHeaders not working
> correctly as well?

Well clearly if the mbox store is completely messed up (as in the message copied from the Send folder), then nothing is going to work on the message body, including streamHeaders. But that shows in more than just streamHeaders.

The more subtle case, where storeToken is set but messageOffset=0 in the header, could be an issue in other places as well, but in briefly looking at the streamHeaders code it is not apparent to me that there is an issue there.

A test case would be useful of course.

Since streamHeaders was introduced in TB 10, and TB 10 is completely unable  to read the messages stored in a post-TB12 Imap Sent folder (without the current fix), one test you could do would be to save send some messages in current TB, load TB 10esr, and confirm that the messages in the Sent folder are unreadable. Are those the same messages where streamHeaders fails, when streamHeaders is run with current TB? (TB 10 esr here is just used to confirm that the Sent messages have incorrect messageOffset, as TB 10 absolutely requires that for offline stores to display messages correctly).

Comment 12

[Kent James (:rkent)]

Once this gets all sorted, we should really push this into TB 17.

Comment 13

[Kent James (:rkent)]

Attachment: with unit test

Comment 14

[Kent James (:rkent)]

This adds a unit test.

In addition to the two changes mentioned in comment 8, there are a couple of additional fixes to core code that were added to make the unit test work more reliably.

+  if (m_playbackTimer)
+    m_playbackTimer->Cancel();

I was getting an assertion at shutdown from the PlaybackTimerCallback with invalid parameters. That timer should really be cancelled at shutdown, so I did that.

+    if (NS_SUCCEEDED(rv))
+      seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, *offset);

This is a fix to streamHeaders (which uses nsMsgDBFolder::GetOfflineFileStream) That function promises in its comments:

  // We'll also advance the offset past the envelope header and
  // X-Mozilla-Status lines.

I interpreted that to mean that it would advance the stream pointer to that location. Previously, it was advancing the stream pointer by 200, the length of startOfMsg. Though it returns the offset, the GetStreamHeaders users were ignoring that, which meant that the first 200 characters of the header stream were being ignored.

Comment 15

[Jonathan Protzenko [:protz]]

> which meant that the first 200 characters of the header stream were being ignored.

I think you found out why streamHeaders would fail. Yay!

Comment 16

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 665164 [details] [diff] [review]
with unit test

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

r=me with nits either fixed or explained away

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +828,5 @@
>          }
>      if (NS_FAILED(rv) && mDatabase)
>        mDatabase->MarkOffline(msgKey, false, nullptr);
> +    if (NS_SUCCEEDED(rv))
> +      seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, *offset);

I'd prefer logic like:

if (NS_SUCCEEDED(rv))
  seek()
else if (mDatabase)
  MarkOffline()

but i'm not going to r- over something this small

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +2597,5 @@
>    mPath = nullptr;
>    NS_IF_RELEASE(m_moveCoalescer);
>    m_msgParser = nullptr;
> +  if (m_playbackTimer)
> +    m_playbackTimer->Cancel();

Should we also null this?

m_playbackTimer = nullptr;

@@ +6958,5 @@
>    origHdr->GetMessageOffset(&messageOffset);
> +  if (!messageOffset)
> +  {
> +    // Some offline stores may contain a bug where the storeToken is set but the messageOffset is zero.
> +    // Detect this and use. Note that offline stores at least for now do not fully support

missing text from comment? "Detect this and use."

Comment 17

[Kent James (:rkent)]

Attachment: Patch as landed, r=irving

Carrying over r=irving

Landed as http://hg.mozilla.org/comm-central/rev/1809ea45723b

Comment 18

[Ben Bucksch (:BenB)]

Thanks, Kent!

Comment 19

[Ben Bucksch (:BenB)]

After updating, I don't see the bug anymore.

Comment 20

[Kent James (:rkent)]

Comment on attachment 665941 [details] [diff] [review]
Patch as landed, r=irving

[Approval Request Comment]
Regression caused by (bug #): 402392

User impact if declined: Sent emails that are moved do not appear correctly in message lists; older TB versions cannot see emails in the Sent folder at all.

Testing completed (on c-c, etc.): Unit test and user test in Daily builds.

Risk to taking this patch (and alternatives if risky): Although the patch should be low risk, it does touch areas that could have unforeseen uses in edge cases. Still, the risk is small compared to the benefit, so this should really be in Thunderbird 17, which means getting it into aurora-17 would be beneficial rather than waiting until beta-17.

Comment 21

[Ben Bucksch (:BenB)]

I support the backport. This is one of the most serious regressions I've seen in years.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 665941 [details] [diff] [review]
Patch as landed, r=irving

Ok, lets try it. If we find problems in beta, we can always back it out.

Comment 23

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/comm-aurora/rev/1a977dfd006a