Closed Bug 676916 Opened 8 years ago Closed 8 years ago

Thunderbird 5 not threading email correctly when message-id spans to continuation line (not fixed by Repair Folder)

Categories

(Thunderbird :: Folder and Message Lists, defect)

x86
Windows XP
defect
Not set

Tracking

(thunderbird7+ fixed)

RESOLVED FIXED
Thunderbird 8.0
Tracking Status
thunderbird7 + fixed

People

(Reporter: stubbsj, Assigned: Bienvenu)

Details

Attachments

(6 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

I have an email chain that includes responses from Outlook.  These were not threading properly.

As a test I created a thread between two of my accounts.  First account is IMAP access through Thunderbird.  Second account is IMAP to what is eventually an Exchange Server accessed through Thunderbird.  I also responded to the thread from Outlook which is the same Second account but with Exchange Server directly.

The responses following the first email from Outlook do not thread properly.

This only seems affects one account that is IMAP access to what is an Exchange Server.

Threading settings are
mail.correct_threading - True
mail.strict_threading - True
mail.thread_without_re - False




Actual results:

The response after any response made using Outlook does not thread properly in Thunderbird accessing the account via IMAP to the Exchange Server.  The threading appears as the following

Original
  Response
    Response - From Outlook
    Response

From account One, the threading is 

Original
  Response
    Response - From Outlook
      Response

Comparing Message-ID, References, and In-Reply-To header fields does not show any problems.

However noticed that Outlook 

Compacting the IMAP Inbox folder of Second account has no impact
Moving the thread to a Local folder has no impact
Moving the thread back to the IMAP Inbox folder of the Second account changes the threading.

Original
  Response
    Response - From Outlook
      Response

Moving new threading back to Local folder keeps proper threading


Expected results:

The responses should be properly threaded always

Original
  Response
    Response - From Outlook
      Response
Captured the MSF file for the Local Folder with incorrect threading before moving back to IMAP Inbox.  Then captured again with correct threading after moving back from IMAP Inbox.

Appears that the file update includes how the threading was fixed?
Perhaps this includes updates to the Hash / Hash tables used for threading?
With this 2nd Example attachment I have shown another example of improper threading.  In this case the original email is started in Outlook.  A response is then made from the Thunderbird IMAP account to the same Exchange Server.  A response is then made to that response from a different IMAP account (not associated with Exchange Server).

The result is that the Thunderbird IMAP account of the Exchange Server does not thread the original and the replies together.

The Message-ID, References, In-Reply-To fields again seem to be correct.

Moving the emails to a Local Folder and back to the IMAP account of the Exchange Server causes proper threading.
A work around that would force the re-evaluation of the threading (as the move to local folders and back to IMAP Inbox of Exchange Server does) would be appreciated.

The moves are time consuming to perform.  Hoping that such a work around would work on Local Folders as I have had this problem propogate across my filing until I found the current move "solution".
Repairing the folder from the properties dialog works for both imap and local folders, though it does force a redownload of all the imap headers and imap message bodies for offline use.

Does this only happen with messages from Outlook?
David-

Thanks for the suggestion.

Repairing folder does not fix the threading.  Tried this with the IMAP account attached to the Exchange Server (witnessed the redownload, thanks for the warning) and in a Local folder.

Similarly I have tried deleting MSF file and allowing it to be rebuilt (not sure if this is the same as the repair)


I only see the issue with the account that is IMAP access to what is also an Exchange Server and only once a message is sent from Outlook (my account or someone else's).

Access from a different IMAP account for the same thread does not exhibit the issue.
An imap protocol log of a session where you repair the folder and still see the bad threading would be helpful - log module would be IMAP in these instructions - https://wiki.mozilla.org/MailNews:Logging - even better would be to include the case where threading works with a different server and the same messages.

If you send the log directly to me, then you don't have to sanitize it (I'd rather you didn't sanitize it because there's a chance that relevant info might be sanitized away)

(In reply to David :Bienvenu from comment #8)
> An imap protocol log of a session where you repair the folder and still see
> the bad threading would be helpful - log module would be IMAP in these
> instructions - https://wiki.mozilla.org/MailNews:Logging - even better would
> be to include the case where threading works with a different server and the
> same messages.
> 
> If you send the log directly to me, then you don't have to sanitize it (I'd
> rather you didn't sanitize it because there's a chance that relevant info
> might be sanitized away)

Logs collected and sent directly to you David.
I haven't received them - I check the server side spam filter and it wasn't there either.
OK, I see the issue here. I'll try to come up with a patch along with a unit test.
Assignee: nobody → dbienvenu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to David :Bienvenu from comment #11)
> OK, I see the issue here. I'll try to come up with a patch along with a unit
> test.

Couple of question.

Will I need to pick up a nightly to get this fix?
Will running repair on folders cause the headers to be re-evaluated and fix the threading once I have a build with the patch?

Thank you so much for looking into this and finding a fix so quickly!
(In reply to stubbsj from comment #12)

> Will I need to pick up a nightly to get this fix?
> Will running repair on folders cause the headers to be re-evaluated and fix
> the threading once I have a build with the patch?
>
Yes and yes, once I get a fix coded, reviewed and landed.

Thx again for your persistence in helping me track this down.
Attached patch proposed fix with unit test (obsolete) — Splinter Review
The unit test shows the failing case, and the nsParseMailbox.cpp change shows the fix, which is to advance past whitespace before '<' when figuring out the message-id.
Attachment #552145 - Flags: review?(neil)
I think we want this for TB 7 - not sure if it's a regression, or how common it is, but the fix is simple.
Comment on attachment 552145 [details] [diff] [review]
proposed fix with unit test

So the bug here is that something else normally strips whitespace but doesn't if the header is immediately put on a continuation line?
The continuation line parsing folds whitespace into a single space,

      // put a single folded space character
      *(foldedSpace - writeOffset) = ' ';
      writeOffset += (buf - foldedSpace);


but the messageID parsing code expects that all whitespace after the MessageID: has already been eaten by this code:

    buf = colon + 1;
    // eliminate trailing blanks after the colon
    while (*buf == ' ' || *buf == '\t')
      buf++;
I suppose you could argue that the folding code should not put a single folded space if the previous&first line of the header didn't have any data after the ':'. That would make some already messy code a bit messier.
(In reply to David :Bienvenu from comment #17)
> The continuation line parsing folds whitespace into a single space,
> 
>       // put a single folded space character
>       *(foldedSpace - writeOffset) = ' ';
>       writeOffset += (buf - foldedSpace);
> 
> 
> but the messageID parsing code expects that all whitespace after the
> MessageID: has already been eaten by this code:
> 
>     buf = colon + 1;
>     // eliminate trailing blanks after the colon
>     while (*buf == ' ' || *buf == '\t')
>       buf++;

Actually that code is buggy - it finds foldedSpace by searching backwards from buf, and could therefore end up overwriting in the wrong place. For instance, suppose the header contains
Message-ID:[SP][SP][SP][CR][LF][SP][SP][SP]<foo@bar>
This gets rewritten to
Message-ID:[SP]<foo@bar>
Sadly the value still points at the place the CR was.

There are at least two options that might fix this.
1. Move the value whitespace skipping code until after the space folding code.
2. Don't eliminate leading whitespace if we haven't advanced the value yet.
Attachment #552145 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk from comment #19)
> Actually that code is buggy - it finds foldedSpace by searching backwards
> from buf, and could therefore end up overwriting in the wrong place. For
> instance, suppose the header contains
> Message-ID:[SP][SP][SP][CR][LF][SP][SP][SP]<foo@bar>
> This gets rewritten to
> Message-ID:[SP]<foo@bar>
> Sadly the value still points at the place the CR was.
> 
> There are at least two options that might fix this.
> 1. Move the value whitespace skipping code until after the space folding
> code.
> 2. Don't eliminate leading whitespace if we haven't advanced the value yet.

Ugh, thx, yeah, that case is broken. I've updated the unit test to make sure we handle that case.  The whitespace folding code seems fairly broken if it doesn't adjust value when it invalidates it.  1) seems like least invasive change. 2) I don't quite understand because in the example case, the first thing we do is advance value. But if 2) means make the whitespace skipping code deal with value correctly, that makes sense too.
I actually thought of case 2) first, but then thought of case 1) as an alternative, so I jotted them both down in case you found it useful.
Whiteboard: duptome
I'd be very careful of duping to this bug, since this bug is not the same as the random threading issues that are fixed by repairing the folder. This bug does not get fixed when you repair the folder. I've removed the duptome notification for that reason.
Whiteboard: duptome
This figures out header->value after the whitespace folding. W/ this fix, I don't need to other code to get rid of whitespace before setting the msg id.
Attachment #552145 - Attachment is obsolete: true
Attachment #552484 - Flags: review?(neil)
Comment on attachment 552484 [details] [diff] [review]
don't advance buf until whitespace folding is done

>-        *buf++;
>+        buf++;
This had me scared for a second but then I realised that the * did nothing.

>+     char *startBuf = colon + 1;
>+    // eliminate trailing blanks after the colon
>+    while (*startBuf == ' ' || *startBuf == '\t')
>+      startBuf++;
>+
>+    value = startBuf;
Could we just not use value throughout?
Also, incorrect indentation on the first line.
yeah, that's nicer.

*buf++ scared me when I realized it didn't do anything :-)
Attachment #552484 - Attachment is obsolete: true
Attachment #552484 - Flags: review?(neil)
Attachment #552544 - Flags: review?(neil)
Comment on attachment 552544 [details] [diff] [review]
address prev comments

I don't think you meant to attach this.
Attachment #552544 - Flags: review?(neil)
Attached patch just the mailnews/local diffs. (obsolete) — Splinter Review
Attachment #552662 - Flags: review?(neil)
Comment on attachment 552662 [details] [diff] [review]
just the mailnews/local diffs.

>+    value = colon + 1;
>+    // eliminate trailing blanks after the colon
>+    while (*value == ' ' || *value == '\t')
>+      value++;
>+
>+    if (header)
>+      header->value = value;
>+
>     if (header)
>       header->length = buf - header->value - writeOffset;
[I don't know why I didn't spot this before, but we could probably have done something about those if (header) statements. In fact, if I've read the code correctly, we could have put this entire code fragment inside if (header).]
Attachment #552662 - Flags: review?(neil) → review+
this is what I'll land.
Attachment #552662 - Attachment is obsolete: true
Attachment #552756 - Flags: review+
Comment on attachment 552756 [details] [diff] [review]
get rid of extra check for null header.

I think we should take this for TB 7
Attachment #552756 - Flags: approval-comm-aurora?
Attachment #552756 - Flags: approval-comm-aurora? → approval-comm-aurora+
This already landed:

http://hg.mozilla.org/comm-central/rev/1ba91121b8d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 8.0
Summary: Thunderbird 5 not threading email correctly → Thunderbird 5 not threading email correctly when message-id spans to continuation line (not fixed by Repair Folder)
You need to log in before you can comment on or make changes to this bug.