Closed Bug 531245 Opened 15 years ago Closed 13 years ago

Import from Apple Mail.app breaks messages and causes data loss.

Categories

(Thunderbird :: Migration, defect)

x86_64
macOS
defect
Not set
critical

Tracking

(blocking-thunderbird3.1 -)

RESOLVED FIXED
Thunderbird 3.3a2
Tracking Status
blocking-thunderbird3.1 --- -

People

(Reporter: david.rekowski, Assigned: rolnas)

Details

(Keywords: student-project, testcase, Whiteboard: [has protocol logs][good first bug])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_2; de-de) AppleWebKit/531.21.8 (KHTML, like Gecko) Version/4.0.4 Safari/531.21.10
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; de; rv:1.9.1.5) Gecko/20091121 Thunderbird/3.0

I imported my emails from apple mail. Most folders now contain broken messages. Broken means, no sensible header information exist. 
Many messages contain parts of Apple PLists. 
Some contain a section of a mail header. 
Some show the bottom part of  a message and a PList (sometimes with another message appended). 
Sometimes there's binary data included. 
Some are completely empty.
They appear as dated from 1970-01-01. 

The number of messages is correct, i.e. for every message in Apple Mail there is _either_ a correct or a broken message in TB.

Reproducible: Didn't try

Steps to Reproduce:
1. Start Thunderbird 3rc1
2. Import emails from Apple Mail
3. Examine messages dated 1970-01-01
Actual Results:  
The result are broken messages without sender and subject information. 

Expected Results:  
All messages should have been imported correctly.

This was with a fresh installation without any add-ons.
Version: unspecified → 3.0
is import log what we need here? https://wiki.mozilla.org/MailNews:Logging
I tried again, with no change in the result. Did I miss something? The log was empty.
$ env
NSPR_LOG_MODULES=import:5,appleimportlog:5
NSPR_LOG_FILE=tb-import.log
(In reply to comment #2)
> I tried again, with no change in the result. Did I miss something? The log was
> empty.
> $ env
> NSPR_LOG_MODULES=import:5,appleimportlog:5
> NSPR_LOG_FILE=tb-import.log

That probably requires a special build with import being enabled, David could you provide a try build ?
import log should be working fine in a release build, but it's tricky to get working on the mac. For one thing, the applemail import log module is "APPLEMAILIMPORTLOG"
Could not get a log using

NSPR_LOG_MODULES=APPLEMAILIMPORTLOG:5
NSPR_LOG_MODULES=applemailimportlog:5
Are you setting the env vars using export? I verified that this works for me with a release build.
Yes, these lines above are copied from evn output. This is what I enter:

$ export NSPR_LOG_MODULES=applemailimportlog:5
$ export NSPR_LOG_FILE=tb-import.log
$ /Applications/Thunderbird.app/Contents/MacOS/thunderbird-bin
s/evn/env/ , sorry
Where are you looking for the log? I've been cd'ing into the MacOS directory and running thunderbird-bin from there, and the log shows up there.
This worked. Maybe missed something else before. However, the log is full of these (middle part) messages:

-1600793344[140b860]: nsAppleMailImportModule Created
-1600793344[140b860]: nsAppleMailImportModule Deleted
-1600793344[140b860]: nsAppleMailImportModule Created
-1600793344[140b860]: nsAppleMailImportMail created
-1600793344[140b860]: FindMailboxes for Apple mail invoked
-1600793344[140b860]: Found account: myemail@example.com
...
-1600793344[140b860]: Adding .mbox dir: spam.mbox
-1600793344[140b860]: Will import spam with approx 311 messages, depth is 2
-1600793344[140b860]: trying to locate a '/Users/myusername/Library/Mail/Mailboxes/website/spam'
...
-1600793344[140b860]: nsAppleMailImportModule Deleted
-1600793344[140b860]: nsAppleMailImportMail destroyed

No hints on any problems.
With *working* I was refering to creating the log. The messagers are still broken as before.
Whiteboard: [has protocol logs]
I think I found the source for this behaviour: I could confirm that there is a wrong header in a message that broke at import:

>From - Mon Dec 10 10:09:13 2007

Removing this line causes the import to work as expected. Since it is not very practicable to remove those lines by hand I strongly recommend to find a way to deal with those broken header lines, i.e. remove them on import or even better remove them whenever they are encountered. Since the format is very clear, there should be a safe way to do it.

Note: There is another bug report regarding this issue, though in a different context: https://bugzilla.mozilla.org/show_bug.cgi?id=310583
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-thunderbird3.1?
David Rekowski, if you could create a tiny Mail.app test case that could be used by whoever wants to take that on, it'd be super helpful.
Keywords: student-project
Whiteboard: [has protocol logs] → [has protocol logs][good first bug]
David R, was that line in the middle of a message? And was it really escaped with > in the original, or did it just appear as "From - ..." in the original mail.app message.
I obfuscated header and content
As you can see in the attachment, the line is within the header section and it's escaped. Looking at the source in Mail.app does not show the line, it seems to be removed by Mail.app.
Keywords: testcase
It rather looks like mail.app imported those messages from Thunderbird to start with, since they have x-mozilla-status headers. Two sets of them, even. That makes me disinclined to block on this, though I'd love to see a fix.
blocking-thunderbird3.1: --- → -
Flags: blocking-thunderbird3.1?
Yes, it was previously imported from TB, but the incorrect line is still in the message file. Only mail.app ignores it, whereas the TB importer does not.
Yeah, I just meant that's going to be a relatively rare round trip. I would love to see this fixed, and it shouldn't be too hard. The importer could just ignore lines that aren't continuation lines and don't have a ':' in them in the header part of the message, before the first blank line.
Is there anything I can do to help this get fixed? I might even get down to the code if someone points me to where I have to look.
(In reply to comment #20)
> Is there anything I can do to help this get fixed? I might even get down to the
> code if someone points me to where I have to look.

http://mxr.mozilla.org/comm-central/source/mailnews/import/applemail/src/

And you might want to read https://developer.mozilla.org/en/comm-central#Requirements too .
Well, this won't do for me, I lack the required skills and knowledge of the codebase. Please can someone else pick that up. If you're familiar with the code, I suppose it's quite straightforward.
Flags: wanted-thunderbird+
Will something be done in regard to this problem? Or can someone explain to me, why this is difficult to implement for TB developers, so I can understand? Thank you.
By looking to mail import code I found at least one place where the source looks suspicious. In nsEmlxHelperUtils.mm there are lines
...
  // go through foundFromLines
  if (foundFromLines.Length()) {
    // pre-grow the string to the right size
    aOutBuffer.SetLength((aMessageBufferEnd-aMessageBufferStart) + foundFromLines.Length());

    const char *chunkStart = aMessageBufferStart;
    for (unsigned i=0; i<foundFromLines.Length(); ++i) {
      aOutBuffer.Append(chunkStart, (foundFromLines[i]-chunkStart));
      aOutBuffer.Append(NS_LITERAL_CSTRING(">"));

      chunkStart = foundFromLines[i];
    }
  }
...
I think there is missing aOutBuffer.Append(chunkStart, (aMessageBufferEnd-chunkStart)); after loop "for" to append the rest of message to aOutBuffer.

Probably that could lead to random memory content at the end of aOutBuffer written to output mbox.
(In reply to comment #24)
> By looking to mail import code I found at least one place where the source
> looks suspicious. In nsEmlxHelperUtils.mm there are lines

Feel like making a patch ?
Thx for looking into this. Rolandas, if you're not set up to build Thunderbird, we could probably generate a try server build with your suggested change and you could download that and try it. But if you are setup to build Thunderbird, then the best thing would be for you to make the change, test it, and then propose a patch.
Attached patch proposed patch to this problem (obsolete) — Splinter Review
Submitting patch, but I cannot to test it. If developpers could provide new build, I could test it if this problem is solved.
I've requested a try server build with this patch - it should be available in a couple hours. I'll add a link here once its finished.
I have built and debug this problem and found several more problems.
With this patch I tried and it works much better.
aOutbuffer.Append appends after mark set by SetLength and there was a mistake in foundFromLines.AppendElement.
Attachment #486485 - Attachment is obsolete: true
That corrects situation when "From " appears at the begining.
That should be all for the moment.
Attachment #486622 - Attachment is obsolete: true
Comment on attachment 486623 [details] [diff] [review]
third proposed patch to this problem

OK, cool, so you don't need a try server build, which is good because of course it failed for me, as it nearly always does.

Thx very much for looking at this. We're going to want the fromLineStart++ on its own line, and spaces around the - in aMessageBufferEnd-chunkStart.  Requesting review from Standard8.
Attachment #486623 - Flags: review?(bugzilla)
OK for cosmetic changes. My idea was about shortest patch possible. About spaces - there also you can look to foundFromLines[i]-chunkStart also.
Comment on attachment 486623 [details] [diff] [review]
third proposed patch to this problem

I've just been trying this and it seemed to fix at least one issue I saw from my Mac import, which is excellent!

r=Standard8 with David's comments addressed. Thanks for the patch.
Attachment #486623 - Flags: review?(bugzilla) → review+
Rolandas, if you're able to attach a new patch, when you do so add checkin-needed to the list of keywords and it'll then get checked in to the tree for you soon after. Thanks again.
Assignee: nobody → rolnas
Keywords: checkin-needed
(In reply to comment #34)
> Rolandas, if you're able to attach a new patch, when you do so add

Mark wants a patch were david's issues are addressed.
Keywords: checkin-needed
My patch solves already most (if not all) problems discussed in this bug report.
(In reply to comment #36)
> My patch solves already most (if not all) problems discussed in this bug
> report.

It doesn't fix the style issues asked for in comment 31. We generally prefer to have complete patches for check in as it makes it easier all round.
I found time to address the style issues in comment 31 and checked this in:

http://hg.mozilla.org/comm-central/rev/c43894b3547a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: