537872 - Import Eudora mail code isn't 64-bit compatible

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Makoto Kato [:m_kato]]

The code to importing Eudora mail uses old APIs.  So this isn't 64-bit compatible.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

cc'ing a bunch of Eudora folks..

Comment 2

[Jim]

FYI,
I imported my email from Eudora 7.1 using Eudora 8.0b7 running under Windows 7 64bit. I had 96,464 messages. I imported address book, mail, settings, and filters in one fell swoop. The first try it failed almost immediately because of some problem in my address book. I went into Eudora 7 and deleted a bunch of old mailing lists I no longer needed and tried it again. The second try worked like a champ and as far as I can tell it imported everything.

Comment 3

[Jim]

are you running Mac or WinDoze?

Comment 4

[Nomis101]

(In reply to comment #2)
> FYI,
> I imported my email from Eudora 7.1 using Eudora 8.0b7 running under Windows 7
> 64bit.

Yes, on Windows. But the problem in this Bug is, it doesn't build on Mac OS X in x86_64:

/src/mailnews/import/eudora/src/nsEudoraMac.cpp:444: error: ‘FSpOpenResFile’ was not declared in this scope
/src/mailnews/import/eudora/src/nsEudoraMac.cpp:582: error: ‘FSpOpenResFile’ was not declared in this scope
/src/mailnews/import/eudora/src/nsEudoraMac.cpp:601: error: ‘GetIndString’ was not declared in this scope
/src/mailnews/import/eudora/src/nsEudoraMac.cpp:1052: error: ‘::FSpMakeFSRef’ has not been declared

Comment 5

[Mark Banner (:standard8)]

The changes in bug 558246 may help to indicate what should be done here.

Comment 6

[Ben Frisch]

I'll take this.  I think I got a start to this that I'll post this weekend, but I need to do more testing before it  is ready for review.

Comment 7

[Ben Frisch]

Attachment: W.I.P.

Here is my current work in progress.  My next step is to write 64-bit safe code to parse the string property list, since unfortunately there is no 64-bit safe system call to do that.  Hopefully, I will have something in a couple of weeks (after finals).

Comment 8

[Mark Banner (:standard8)]

Ben, this is a good start, do you have time to move the patch forward?

Comment 9

[Ben Frisch]

Yes.  I will try to get the patch down this weekend.

Comment 10

[Mark Banner (:standard8)]

This will block the next trunk release as we're going to need 32/64 bit support there as ppc/32 is not longer supported in core.

Comment 11

[Ben Frisch]

Attachment: W.I.P. 2.0

This is my second work in progress patch with an initial version of a function to parse the string list.  Currently an error occurs while attempting an import, but it compiles.  I will be able to resume work on it this evening, but as I realize this is holding up the alpha 1 release, anyone is free to take this bug from me.

Comment 12

[Mark Banner (:standard8)]

I've taken the decision that for the first few alphas we can live without Eudora import, and I've therefore filed bug 612286 to temporarily disable it on Mac 64-bit builds. This bug will therefore block one of the latest releases but not sure which one yet.

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #11)
> Created attachment 489803 [details] [diff] [review]
> W.I.P. 2.0
> 
> This is my second work in progress patch with an initial version of a function
> to parse the string list.  Currently an error occurs while attempting an
> import, but it compiles.  I will be able to resume work on it this evening, but
> as I realize this is holding up the alpha 1 release, anyone is free to take
> this bug from me.

Ben, have you tried looking at CFBundleCopyLocalizedString ? I saw a reference somewhere that it was a possible replacement for GetIndString.

Comment 14

[Makoto Kato [:m_kato]]

(In reply to comment #13)
> (In reply to comment #11)
> > Created attachment 489803 [details] [diff] [review] [details]
> > W.I.P. 2.0
> > 
> > This is my second work in progress patch with an initial version of a function
> > to parse the string list.  Currently an error occurs while attempting an
> > import, but it compiles.  I will be able to resume work on it this evening, but
> > as I realize this is holding up the alpha 1 release, anyone is free to take
> > this bug from me.
> 
> Ben, have you tried looking at CFBundleCopyLocalizedString ? I saw a reference
> somewhere that it was a possible replacement for GetIndString.

CFBundleCopyLocalizedString reads from bundle string file, not resource fork.  To read Eudora settings, I believe that Ben's way is correct.   But we have to analyze resource fork format to support 64-bit.

Comment 15

[Makoto Kato [:m_kato]]

Attachment: WIP v3

This patch works well on x86_64.

Comment 16

[Makoto Kato [:m_kato]]

Attachment: fix v1

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 493622 [details] [diff] [review]
fix v1

This is looking good, but I'm struggling to understand what's happening in this function:

>+// GetIndString isn't supported on 64-bit Mac OS X
>+// This code is emulation for GetIndString.
>+static PRBool GetStringFromHandle(Handle aResource, StringPtr aString,
>+                                  ResourceIndex aId)
>+{
>+  if (!aResource)
>+    return PR_FALSE;
>+
>+  // Resource file format is the following URL.
>+  // http://developer.apple.com/legacy/mac/library/documentation/mac/MoreToolbox/MoreToolbox-99.html

So this document has several sections, which bit of the resource structure are we actually looking at?

>+  PRUint8 *data = *((PRUint8**)aResource);
>+  //PRUint8 length;

We can drop the unused commented-out variable.

>+  // XXX for PowerPC support, we may need endian swap.
>+  if (*(PRUint16*)data < aId)
>+    return PR_FALSE;
>+
>+  data += 2;

What is this skipping?

>+
>+  while (--aId > 0) {
>+    //length = (*(PRUint8*)data) + 1;
>+    data = data + (*(PRUint8*)data) + 1;
>+  }

I guess this assumes that the Ids are stored in order?

>+  //length = (*(PRUint8*)data + 1);
>+  memcpy (aString, data, (*(PRUint8*)data) + 1);

We should limit this memcpy to the length of aString - just in case.

So I think we should consider returning data + 1, or documenting that StringPtr doesn't just return the string, it returns an offset in the first char (to be discarded) and the string itself.


I think this is almost there just needs a few tweaks.

Comment 18

[Makoto Kato [:m_kato]]

sorry for delay.

(In reply to comment #17)
> Comment on attachment 493622 [details] [diff] [review]
> fix v1
> 
> This is looking good, but I'm struggling to understand what's happening in this
> function:
> 
> >+// GetIndString isn't supported on 64-bit Mac OS X
> >+// This code is emulation for GetIndString.
> >+static PRBool GetStringFromHandle(Handle aResource, StringPtr aString,
> >+                                  ResourceIndex aId)
> >+{
> >+  if (!aResource)
> >+    return PR_FALSE;
> >+
> >+  // Resource file format is the following URL.
> >+  // http://developer.apple.com/legacy/mac/library/documentation/mac/MoreToolbox/MoreToolbox-99.html
> 
> So this document has several sections, which bit of the resource structure are
> we actually looking at?

No, I have analyzed the resource using derez command.  I should remove this comment.


> >+  PRUint8 *data = *((PRUint8**)aResource);
> >+  //PRUint8 length;
> 
> We can drop the unused commented-out variable.

OK.

> >+  // XXX for PowerPC support, we may need endian swap.
> >+  if (*(PRUint16*)data < aId)
> >+    return PR_FALSE;
> >+
> >+  data += 2;
> 
> What is this skipping?

First 2 bytes is count that this resource has.  Also I have to consider byte order.

resource format is

0000: count (2 bytes)
0002: data 1 (n bytes)
... : data 2
... : data 3
 :
 :
... : data N (repeating)

data format is 

0000: length (1 byte)
0001: data... (n bytes)

> >+
> >+  while (--aId > 0) {
> >+    //length = (*(PRUint8*)data) + 1;
> >+    data = data + (*(PRUint8*)data) + 1;
> >+  }
> 
> I guess this assumes that the Ids are stored in order?

IDs are always stored in order.

 
> >+  //length = (*(PRUint8*)data + 1);
> >+  memcpy (aString, data, (*(PRUint8*)data) + 1);
> 
> We should limit this memcpy to the length of aString - just in case.
> 
> So I think we should consider returning data + 1, or documenting that StringPtr
> doesn't just return the string, it returns an offset in the first char (to be
> discarded) and the string itself.

I agree.


Also, I will attach unit test for this.

Comment 19

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 20

[Makoto Kato [:m_kato]]

Attachment: unit test

Although this is unit test, it is failed on tinderbox.  But It works fine on local MacBook Pro.  I don't know why this doesn't work.

Also,
- Mercurial doesn't support resource fork.
- This works on HFS+ only because this is using resource fork.

Comment 21

[Mark Banner (:standard8)]

I'll see if I can find someone to drive this in.

Comment 22

[Makoto Kato [:m_kato]]

Attachment: unit test

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 517690 [details] [diff] [review]
fix v2

Thanks for the updated patch. r=Standard8

Comment 24

[Mark Banner (:standard8)]

On the test front, I tried this on try server, and couldn't understand the issue. As it works locally, and I want to get this moving forward, I've elected for committing the code and we'll see if we can work the unit test out later, hence leaving in-testsuite?

Comment 25

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/c2d8974cb702

Thanks to Ben and Makato for all their work on this bug.