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

Status: RESOLVED FIXED

(Thunderbird :: Migration, defect)

Description

[David Rekowski]

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.

Comment 1

[Wayne Mery (:wsmwk)]

is import log what we need here? https://wiki.mozilla.org/MailNews:Logging

Comment 2

[David Rekowski]

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

Comment 3

[Ludovic Hirlimann [:Usul]]

(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 ?

Comment 4

[David :Bienvenu]

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"

Comment 5

[David Rekowski]

Could not get a log using

NSPR_LOG_MODULES=APPLEMAILIMPORTLOG:5
NSPR_LOG_MODULES=applemailimportlog:5

Comment 6

[David :Bienvenu]

Are you setting the env vars using export? I verified that this works for me with a release build.

Comment 7

[David Rekowski]

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

Comment 8

[David Rekowski]

s/evn/env/ , sorry

Comment 9

[David :Bienvenu]

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.

Comment 10

[David Rekowski]

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: [e-mail]
...
-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.

Comment 11

[David Rekowski]

With *working* I was refering to creating the log. The messagers are still broken as before.

Comment 12

[David Rekowski]

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

Comment 13

[David Ascher (:davida)]

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.

Comment 14

[David :Bienvenu]

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.

Comment 15

[David Rekowski]

Attachment: example Mail.app message file

I obfuscated header and content

Comment 16

[David Rekowski]

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.

Comment 17

[David :Bienvenu]

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.

Comment 18

[David Rekowski]

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.

Comment 19

[David :Bienvenu]

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.

Comment 20

[David Rekowski]

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.

Comment 21

[Ludovic Hirlimann [:Usul]]

(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 .

Comment 22

[David Rekowski]

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.

Comment 23

[David Rekowski]

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.

Comment 24

[Rolandas Naujikas]

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.

Comment 25

[Ludovic Hirlimann [:Usul]]

(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 ?

Comment 26

[David :Bienvenu]

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.

Comment 27

[Rolandas Naujikas]

Attachment: proposed patch to this problem

Submitting patch, but I cannot to test it. If developpers could provide new build, I could test it if this problem is solved.

Comment 28

[David :Bienvenu]

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.

Comment 29

[Rolandas Naujikas]

Attachment: second proposed patch to this problem

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.

Comment 30

[Rolandas Naujikas]

Attachment: third proposed patch to this problem

That corrects situation when "From " appears at the begining.
That should be all for the moment.

Comment 31

[David :Bienvenu]

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.

Comment 32

[Rolandas Naujikas]

OK for cosmetic changes. My idea was about shortest patch possible. About spaces - there also you can look to foundFromLines[i]-chunkStart also.

Comment 33

[Mark Banner (:standard8)]

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.

Comment 34

[Mark Banner (:standard8)]

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.

Comment 35

[Ludovic Hirlimann [:Usul]]

(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.

Comment 36

[Rolandas Naujikas]

My patch solves already most (if not all) problems discussed in this bug report.

Comment 37

[Mark Banner (:standard8)]

(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.

Comment 38

[Mark Banner (:standard8)]

I found time to address the style issues in comment 31 and checked this in:

http://hg.mozilla.org/comm-central/rev/c43894b3547a