Closed Bug 190336 Opened 17 years ago Closed 17 years ago

[mach-o] unable to migrate 4.x profiles

Categories

(Core Graveyard :: Profile: Migration, defect)

PowerPC
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.4beta

People

(Reporter: bugzilla, Assigned: ccarlen)

References

Details

(Keywords: platform-parity, relnote)

Attachments

(4 files, 6 obsolete files)

spun off from bug 190174. according to conrad, the mach-o build cannot migrate
4.x profiles.

tentatively nominating. if minused, it prolly should be relnoted (combined with
bug 190174).
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.3final
QA Contact: ktrina → gbush
4.x profiles were migratable using the smi.bin file for Netscape 7.0x
see the document on early mac os x testing 
http://slip.mcom.com/projects/mojo/profile/MacOSXprofiletests.htm

I'm not sure how many users will have the dual boot machine and still be using
4.x up to now for migration but it is a change in behavior and I am SURE we will
get questions on it. 
-> 1.4b
Target Milestone: mozilla1.3final → mozilla1.4beta
there might be a dup of this, already nsbeta1+.

cavin and I were talking about it, and we definitely see places in
mozilla/profile and mozilla/profile/pref-migrator and mozilla/mailnews (and
ns/mailnews) places to worry about.

here's my initial guess at a todo list:

1) XP_MAC vs XP_MACOSX issues
2) file spec / file path issues
3) finding and reading the 4.x profile registry issues

I'm hoping that mach o builds already find and read mozilla 1.x profile registries?

cavin is already starting on some of this.  cavin, can you coordinate with
ccarlen, to avoid duplicating efforts?
I think this bug is only for not being able to see the 4.x profiles at all (and
so you can't migrate!) and bug 199565 is more for the migration problem itself.
Attached patch patch (obsolete) — Splinter Review
This fixes the profile mgr so that 4.x profiles can be read. There is some
overlap with Cavin's patch on bug 199565, but this has some additional profile
mgr cleanup:

(1) nsProfile::GetOldRegLocation started with an nsIFile, converted it to a
path, only to have the one method which used it turn it back into an nsIFile.
Now, it just stays as an nsIFile.
(2) There was code in ProfileStruct::InternalizeLocation which handled reading
a path from a mozilla5.0 profile as a Unicode HFS path:

-	 // For a brief time, this was a unicode path
-	 PRInt32 firstColon = regLocationData.FindChar(PRUnichar(':'));

It was a brief time - a few days back in the "M" milestone days when somebody
confused a persistent descriptor with a path. It's time that code path can be
gotten rid of.
Conrad and I decided to merge the patch of this bug and bug 199565 together and
post it here since there are overlaps in both patches.
Only did basic testing - migrating a 4.x profile with a POP account works.
Attachment #119771 - Attachment is obsolete: true
I tried the patch and it works for imap as well. Filter rules are migrated fine
too. We just need to remove two printf statements in the patch before getting
review.

So now we can see a list of 4x profiles when importing 4x mail (ie, from
Tools|Import and then select Mail radio button), but the import process does not
work due to some code was not compiled for OSX. A patch is coming.
Also, definition for "NewsFAT" needs to be corrected.
*** Bug 199565 has been marked as a duplicate of this bug. ***
Comment on attachment 119990 [details] [diff] [review]
merged with cavin's nsPrefMigration changes

Please ignore the two debugging printf calls for now as they will be removed
before checking in.
Attachment #119990 - Flags: review?(sfraser)
Attachment #120095 - Flags: review?(ssu)
Attachment #120097 - Flags: review?(ssu)
There is another addrbook sorting problem after 4.x migration is finished and
it's addressed in bug 201588 (since it's not a MacOS X specific bug).
Blocks: 183078
Comment on attachment 120095 [details] [diff] [review]
Fix 4x mail import problem.

r=ssu
Attachment #120095 - Flags: review?(ssu) → review+
Comment on attachment 120097 [details] [diff] [review]
Fix "NewsFAT" definition.

r=ssu
Attachment #120097 - Flags: review?(ssu) → review+
Comment on attachment 119990 [details] [diff] [review]
merged with cavin's nsPrefMigration changes

>Index: pref-migrator/src/nsPrefMigration.cpp
>===================================================================

>-#if defined(XP_UNIX)
>-#define IMAP_MAIL_FILTER_FILE_NAME_IN_4x "mailrule"
>+#if defined(XP_UNIX) && !defined(XP_MACOSX)
>+#define IMAP_MAIL_FILTER_FILE_NAME_IN_4x "maillrule"

"maillrule" looks like a typo.

>+#if defined(XP_MACOSX)
>+  // For Mac OS X, the input path string is base64 encoded so we need
>+  // to do something to get the slash-delimited pathname out of it.
>+  printf("*** In ConvertPersistentStringToFileSpec(), input str=%s\n", str);

Remove the printfs here and below.

>+  nsCAutoString pathStr(str);
>+  nsCOMPtr<nsILocalFile> localFile;
>+
>+  rv = NS_NewNativeLocalFile(nsCString(), PR_TRUE, getter_AddRefs(localFile));
>+  NS_ENSURE_SUCCESS(rv,rv);
>+  rv = localFile->SetPersistentDescriptor(pathStr);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+
>+  nsXPIDLCString nativePath;
>+  rv = localFile->GetNativePath(nativePath);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+  printf("*** In ConvertPersistentStringToFileSpec(), str=%s\n", nativePath.get());
>+  rv = path->SetPersistentDescriptorString(nativePath);
>+#else
> 	rv = path->SetPersistentDescriptorString(str);
>+#endif
> 	return rv;
> }


>+#if defined(XP_MACOSX)
>+    // For Mac OS X, make sure 'oldPath' contain slash-delimited pathname.
>+    nsXPIDLCString nativePath;
>+    rv = oldLocalFile->GetNativePath(nativePath);
>+    NS_ENSURE_SUCCESS(rv,rv);
>+    rv = oldPath->SetPersistentDescriptorString(nativePath);
>+    NS_ENSURE_SUCCESS(rv,rv);

I'm not sure I understand the comment, or the code. 'oldPath' is an nsIFile, it
doesn't "contain" a pathname. It seems wrong that oldLocalFile exists with
its native path set to a persistent descriptor. This should never have
been created this way.

>Index: src/nsProfile.cpp
>===================================================================

>-#if defined(XP_WIN) || defined(XP_OS2) || defined(XP_MAC)
>+#ifndef XP_BEOS
>+    nsCOMPtr<nsIFile> oldRegFile;
>+    rv = GetOldRegLocation(getter_AddRefs(oldRegFile));
>+    if (NS_SUCCEEDED(rv))
>+        rv = gProfileDataAccess->Get4xProfileInfo(oldRegFile, PR_TRUE);
>+#endif

Is it OK that that code didn't execute for XP_UNIX before, but does now?

>+#ifndef XP_BEOS
>+    nsCOMPtr<nsIFile> oldRegFile;
>+    rv = GetOldRegLocation(getter_AddRefs(oldRegFile));
>+    if (NS_SUCCEEDED(rv)) {
>+        rv = gProfileDataAccess->Get4xProfileInfo(oldRegFile, PR_FALSE);
>+        gProfileDataAccess->mProfileDataChanged = PR_TRUE;
>+        gProfileDataAccess->UpdateRegistry(nsnull);
>+    }
> #endif

Same here.
Attachment #119990 - Flags: review?(sfraser) → review-
I did the nsProfile/nsProfileAccess portion, so I'll speak for it ;-)

> Is it OK that that code didn't execute for XP_UNIX before, but does now?

Yes, the change makes it so that GetOldRegLocation returns null for XP_UNIX.
Get4xProfileInfo() takes nsnull for that param. That XP_UNIX uses nsnull as the
file param was a detail I wanted to hide at a lower level. Really, #ifndef
XP_BEOS could be changed to #ifdef SUPPORTS_4X_MIGRATION and it might read better.
>>+#if defined(XP_MACOSX)
>>+    // For Mac OS X, make sure 'oldPath' contain slash-delimited pathname.
>>+    nsXPIDLCString nativePath;
>>+    rv = oldLocalFile->GetNativePath(nativePath);
>>+    NS_ENSURE_SUCCESS(rv,rv);
>>+    rv = oldPath->SetPersistentDescriptorString(nativePath);
>>+    NS_ENSURE_SUCCESS(rv,rv);
>
>I'm not sure I understand the comment, or the code. 'oldPath' is an nsIFile, it
>doesn't "contain" a pathname. It seems wrong that oldLocalFile exists with
>its native path set to a persistent descriptor. This should never have
>been created this way.
>
I remember if we use the old code:

> rv = oldLocalFile->GetPersistentDescriptor(persistentDescriptor);
> if (NS_FAILED(rv)) return rv;
> rv = oldPath->SetPersistentDescriptorString(persistentDescriptor.get());

then the pathname in 'oldOath' is the base64 encoded stuff so that's the reason
for the change.
Can you supply enough context for that last change so that I can see what's
going on?
This patch fixes old-style nsPersistentFileDescriptor to use Base64-encoded
aliases, and not full paths, for Mac OS X.
Attachment #120767 - Flags: review?(ccarlen)
Comment on attachment 120767 [details] [diff] [review]
Patch to fix nsFileSpec persistent desciptors for XP_MACOSX

The patch looks good, but be careful to check all usage of nsFileSpec
persistent descs.
Attachment #120767 - Flags: review?(ccarlen) → review+
Simon's patch to nsFileSpec persistent desciptors for XP_MACOSX does work so
removing code to get native path from nsILocalFile and put it into nsFileSpec
code.
Attachment #119990 - Attachment is obsolete: true
Attachment #120782 - Flags: review?(sfraser)
The patch description was set wrong, this is a better one.
Attachment #120782 - Attachment is obsolete: true
Attachment #120782 - Flags: review?(sfraser)
Attachment #120783 - Flags: review?(sfraser)
You still have a typo here:
+#if defined(XP_UNIX) && !defined(XP_MACOSX)
+#define IMAP_MAIL_FILTER_FILE_NAME_IN_4x "maillrule"
sfraser is right, if you don't fix that, you'll break UNIX/Linux profile migration.
Corrected "maillrule".
Attachment #120783 - Attachment is obsolete: true
Attachment #120783 - Flags: review?(sfraser)
Attachment #120851 - Flags: review?(sfraser)
Attachment #120851 - Flags: review?(sfraser) → review+
Attachment #120851 - Flags: superreview?(sspitzer)
Attachment #120095 - Flags: superreview?(sspitzer)
Attachment #120097 - Flags: superreview?(sspitzer)
Comment on attachment 120767 [details] [diff] [review]
Patch to fix nsFileSpec persistent desciptors for XP_MACOSX

Seth, do you know of places in the mailnews code where, under XP_UNIX, the
format of a persistent desc is assumed to be a native path? Hopefully not - if
that was the case, we would have broken when we started reading CFM profiles
with the Mach-O build (6 months ago).
Attachment #120767 - Flags: superreview?(sspitzer)
Comment on attachment 120095 [details] [diff] [review]
Fix 4x mail import problem.

sr=sspitzer.
Attachment #120095 - Flags: superreview?(sspitzer) → superreview+
Comment on attachment 120097 [details] [diff] [review]
Fix "NewsFAT" definition.

sr=sspitzer.
Attachment #120097 - Flags: superreview?(sspitzer) → superreview+
Comment on attachment 120767 [details] [diff] [review]
Patch to fix nsFileSpec persistent desciptors for XP_MACOSX

sr=sspitzer.
Attachment #120767 - Flags: superreview?(sspitzer) → superreview+
Comment on attachment 120851 [details] [diff] [review]
merged with cavin's nsPrefMigration changes , v3

sr=sspitzer.
Attachment #120851 - Flags: superreview?(sspitzer) → superreview+
Need to add check for imap type before copying msg filter rules.  Everything
else stays the same.
Attachment #120851 - Attachment is obsolete: true
Comment on attachment 120922 [details] [diff] [review]
120851: merged with cavin's nsPrefMigration changes , v4

carry over r=sfraser and sr=sspitzer (from v3).
Attachment #120922 - Flags: superreview?
Attachment #120922 - Flags: superreview?
All patches, by a cast of many, are checked in. Thanks for the help.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
verified -build 20030421
Status: RESOLVED → VERIFIED
spoke too soon- not getting mail settings/ bookmarks
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
On a tree I pulled last night, I did both debug and opt (-O) builds from the
same src. Debug works, opt doesn't. nsPrefMigration.cpp is a challenge for
optimizers, and it might be the problem.  See:
http://lxr.mozilla.org/seamonkey/source/profile/pref-migrator/src/nsPrefMigration.cpp#78
We have run into this same bug before - with the very same code even :-/
Quicker to explain than find that bug: You can't take the address of a cast.
gcc compiles it and it works in debug build. In opt build, it doesn't complain
when compiling it - it just silently fails at runtime. Patch fixes the cast and
combines more of the XP_MAC and XP_MACOSX code.
Attachment #120767 - Attachment is obsolete: true
Attachment #121578 - Flags: superreview?(sfraser)
Attachment #121578 - Flags: review?(pinkerton)
Attachment #121578 - Flags: superreview?(sfraser) → superreview+
Comment on attachment 121578 [details] [diff] [review]
patch to fix opt build

r=pink
Attachment #121578 - Flags: review?(pinkerton) → review+
Request is only for attachment 121578 [details] [diff] [review]. That fixes a bug which prevents the whole
rest of the patch from working on opt builds. Low risk since this is to fix code
which was not previously enabled.
Flags: blocking1.4b?
Flags: blocking1.4b? → blocking1.4b+
Comment on attachment 121578 [details] [diff] [review]
patch to fix opt build

a=asa (on behalf of drivers) for checkin to 1.4b
Attachment #121578 - Flags: approval1.4b+
This was checked in on 04/25/2003 12:29. Thought I marked this fixed :-/ 
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
verified with build 2003042903
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.