Closed Bug 381472 Opened 15 years ago Closed 13 years ago

copy of message(to local mail folder) from imap folder synced for offline(offline use=ON) gets corrupted headers(">From -" and duplicated X-Mozilla-Status:)

Categories

(MailNews Core :: Backend, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: tuukka.tolvanen, Assigned: Bienvenu)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss)

Attachments

(2 files, 1 obsolete file)

tbird trunk 20070515 and 2.0.0.x, linux

str
 1. have imap folder msgs downloaded for offline use (acct settings -> offline & disk space)
 2. copy single message from such imap folder (e.g. context menu, drag) to
     a) a local folder or
     b) another folder on the same imap account
 3. view source of message
 4. attempt to copy message to imap

expected:
 * valid headers at step 3
 * copy successful at step 4

actual:
 * bad headers at step 3
 * copy may fail at step 4 with "Message contains invalid header" depending on target server

Making the initial copy from an imap folder not selected for offline use did not result in corruption. Copying several messages at once did not result in corruption.

message source when imap folder not configured for offline:
Return-path: <...>

message source when imap folder configured for offline use:
From - Mon May 21 21:32:23 2007
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
Return-path: <...>

a) message source of corrupted local folder copy:
From - Mon May 21 21:33:25 2007
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
>From - Mon May 21 21:32:23 2007
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
Return-path: <...>

b) message source of corrupted copy to another imap folder on same acct:
 May 21 21:51:09 2007
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
Return-path: <...>

Message corruption in copies from offline-synced imap has been reproduced on exchange; cases (a) and (b) were reproduced on iPlanet Messaging Server 5.2 and Courier, and the dependency on offline sync was tested on iPlanet.

(don't dup this to the other similar bugs, please; this one is trying to be specific.)
>  * copy may fail at step 4 with "Message contains invalid header" depending on
> target server

fwiw, iPlanet shrieked, Courier took the corrupt message without complaints.
(In reply to comment #0)
> tbird trunk 20070515 and 2.0.0.x, linux
> message source when imap folder not configured for offline:
> Return-path: <...>

Bug 382836 reported no "From -" line in local mail folder file when "not offline use" with trunk build. If this occurs on release builds of Tb too, Bug 295672 (even when "not offline use" case) and Bug 218190 can be explained ;
 - Compact folder after local copy => Bug 295672 (same result as this bug)
   (See Bug 377986 for actions taken by Tb when compact folder) 
 - Rebuild index after local copy  => Bug 218190 (mail disappears)
To Tuukka Tolvanen:
Can you check local mail folder file content copied from IMAP folder with "no offline use"?
Bug 373941 is an older bug report for phenomenon of comment #0.
Depends on: 373941
> Can you check local mail folder file content copied from IMAP folder with "no
> offline use"?

From - Thu Aug 23 23:26:06 2007
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
Return-path: <...>
Blocks: 373941
No longer depends on: 373941
Summary: copy of single message from imap folder synced for offline gets corrupted headers → copy of single message from imap folder synced for offline gets corrupted headers(">From -" and duplicated X-Mozilla-Status:)
Problem looks to be;
 When offline use, "From -" line(Unix Mbox separator) and X-Mozilla-Status:/2:
 are already written in local version of mail folder.
 Even though already exist, "copy to local mail folder" adds these tree headers,
 as done when copy from non-offline-use folder.
 And second "From -" line is escaped. (formerly added one)
   a) message source of corrupted local folder copy:
      From - Mon May 21 21:33:25 2007  (<= added when copy to local)
      >From - Mon May 21 21:32:23 2007 (<= added becasue offline use)
Severity: normal → critical
Keywords: dataloss
OS: Linux → All
Hardware: PC → All
I've acquired Gmail accounts and tested with Gmail IMAP/Seamonkey 1.1.8.
aI could observe phenomenon/problem explained in comment #0 & comment #5.
Summary: copy of single message from imap folder synced for offline gets corrupted headers(">From -" and duplicated X-Mozilla-Status:) → copy of message(to local mail folder) from imap folder synced for offline(offline use=ON) gets corrupted headers(">From -" and duplicated X-Mozilla-Status:)
Flags: blocking-thunderbird3?
Closing this bug will require careful look at the dependent bugs.
blocking‑thunderbird3+; this is very reproducible (just copy an imap msg to local and look at the headers of the local copy) and annoying
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Duplicate of this bug: 196839
Product: Core → MailNews Core
hmm, I can look into this. I'm not sure if the right way to fix this would be to check if the message already has x-mozilla-status lines before adding them (might be hard, since we want to add them at the beginning, and we may not have all the data yet), or to strip out the headers when streaming from the offline store...
If the offline store is already in mbox format, what about treating it as such (i.e., the same way mboxes for "Local Folders" or for POP accounts are treated)?
taking.
Assignee: nobody → bienvenu
Status: NEW → ASSIGNED
my current thinking is to make nsMsgDBFolder::GetOfflineFileStream advance past the From envelope and the x-mozilla-status lines, if any, and return that offset.
(In reply to comment #13)
> my current thinking is to make nsMsgDBFolder::GetOfflineFileStream advance past
> the From envelope and the x-mozilla-status lines, if any, and return that
> offset.

It occurred to me that this would break compaction of offline imap/news stores, in the sense that we would lose the original envelope line and x-mozilla-status headers. But maybe that doesn't matter...
Attached patch wip (obsolete) — Splinter Review
this fixes the problem for me.  But as I investigate this, I'm seeing some strange things with the offline stores, where we don't have the correct offsets into the store, which makes us go back and get messages from the server. I'm trying to make sure this patch doesn't break anything, but I got sucked into figuring out if things were broken before.

Oh, and I need to change nsMsgFolderCompactor.cpp to use the new MsgAdvanceToNextLine method, which was stolen from it.
Attached patch proposed fixSplinter Review
this fixes the problem, and also adds a unit test for compacting offline stores. I changed GetOfflineFileStream to skip over the envelope and x-mozilla-status headers, since we won't see those from an imap server. I also had to fix the offline store compaction code to add back an envelope. 

I also had to fix the line endings for bugmail11 - I'll attach a copy of that in a second...
Attachment #365564 - Attachment is obsolete: true
Attachment #366949 - Flags: superreview?(neil)
Attachment #366949 - Flags: review?(bugzilla)
Attachment #366949 - Flags: superreview?(neil) → superreview+
Comment on attachment 366949 [details] [diff] [review]
proposed fix

>+    // Ugly hack: make sure we don't get stuck in a JS->C++->JS->C++... call stack
>+    // This can happen with a bunch of synchronous functions grouped together, and
>+    // can even cause tests to fail because they're still waiting for the listener
>+    // to return
>+    do_timeout(0, "doTest(++gCurTestNum)");
I seem to recall that there's a function called executeSoon for that sort of thing; maybe that would be neater than do_timeout.
(In reply to comment #18)
> (From update of attachment 366949 [details] [diff] [review])
> >+    // Ugly hack: make sure we don't get stuck in a JS->C++->JS->C++... call stack
> >+    // This can happen with a bunch of synchronous functions grouped together, and
> >+    // can even cause tests to fail because they're still waiting for the listener
> >+    // to return
> >+    do_timeout(0, "doTest(++gCurTestNum)");
> I seem to recall that there's a function called executeSoon for that sort of
> thing; maybe that would be neater than do_timeout.

A quick look at mxr suggests executeSoon is only available for mochitests.
Attachment #366949 - Flags: review?(bugzilla) → review+
Comment on attachment 366949 [details] [diff] [review]
proposed fix

I'm assuming that we are expecting to have just one:

From - Mon Mar 16 11:06:16 2009
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000

in view source for messages in local folders. If so, r=me.
Whiteboard: [has reviewed patch][bienvenu to check in]
Target Milestone: --- → Thunderbird 3.0b3
yes, right. fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has reviewed patch][bienvenu to check in]
bienvenu: _tsk_ - sorry, that was bug 381472, but I marked it fixed already
Flags: in-testsuite+
reporter reported this bug with 2.0.0.x
Will be this bug fixed also in next 2.0.0.y?
PM-
(In reply to comment #23)
> reporter reported this bug with 2.0.0.x
> Will be this bug fixed also in next 2.0.0.y?

2.x series ply get security related fixes. so the answer is this is fixed in upcoming 3.0 thunderbird (you can get them at http://www.mozillamessaging.com/en-US/thunderbird/early_releases/downloads/)
so bug classified as "dataloss" is not security bug. Am I right?

How can I find out if bug in bugzilla is security bug?
I've searched keywords https://bugzilla.mozilla.org/describekeywords.cgi but there is no which says that this is security bug.
Depends on: 469222
Duplicate of this bug: 509728
You need to log in before you can comment on or make changes to this bug.