421295 - Safari home page import failed

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P2)

Description

[Nicholas Shanks]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-GB; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-GB; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4

The Mac RC1 of beta 4 (en-GB locale) did not import my Safari home page "http://news.bbc.co.uk/" and instead set the home page to http://www.apple.com/startpage/

Reproducible: Didn't try

Steps to Reproduce:
1. Change your home page in Safari
2. Install FF 3.0b4rc1
3. Select to import home page from safari by the migration manager
4. Open new tab or check preferences
Actual Results:  
http://www.apple.com/startpage/ is loaded

Expected Results:  
http://news.bbc.co.uk/ would be loaded

I have FF 2.0.12 and 3.0 b3 installed, both have been run before.
I have Safari 3.0.4, a recent WebKit nightly build and my own WebKit subversion checkout/build also installed.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008030804 Minefield/3.0b5pre ID:2008030804

Can this be done by only choosing the Preferences option within the Profile Migrator? I did a complete import and the homepage isn't updated. Something broken by fetching the value via Internet Config?

Comment 2

[Michael Schonfeld]

What you must realize is that the home page only gets imported on the first migration wizard! This means that in order to check for this bug, you must completely delete/move/rename your Firefox folder in the Application Support folder, in order to make your Firefox think it is freshly installed. This will trigger the 'first run' migration wizard, which will also import your home page.

Never the less, there is a bug here which is caused by the fact that Apple had moved the home page key from the com.apple.internetconfig to com.apple.Safari in newer Safari versions (this is not related to Tiger/Leopard, but the version of Safari).

What I suggest, is to first check if a newer key exists in com.apple.safari, and only if none is found, look for one in the old location of com.apple.internetconfig.  This will ensure that the right home page is imported into Firefox.

Comment 3

[Michael Schonfeld]

Attachment: v1

This one is going to be a pain-in-the-ass to test!  The home page migration wizard is only invoked in official branding, which means that to test/verify this patch you will need to either: a. forge your branding.properties file; b. test this on an official beta release.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 308337 [details] [diff] [review]
v1

>Index: browser/components/migration/src/nsSafariProfileMigrator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp,v
>retrieving revision 1.48
>diff -u -p -8 -r1.48 nsSafariProfileMigrator.cpp
>--- browser/components/migration/src/nsSafariProfileMigrator.cpp	9 Jan 2008 03:54:37 -0000	1.48
>+++ browser/components/migration/src/nsSafariProfileMigrator.cpp	9 Mar 2008 23:22:27 -0000
>@@ -70,16 +70,17 @@
> 
> #define SAFARI_PREFERENCES_FILE_NAME      NS_LITERAL_STRING("com.apple.Safari.plist")
> #define SAFARI_BOOKMARKS_FILE_NAME        NS_LITERAL_STRING("Bookmarks.plist")
> #define SAFARI_HISTORY_FILE_NAME          NS_LITERAL_STRING("History.plist")
> #define SAFARI_COOKIES_FILE_NAME          NS_LITERAL_STRING("Cookies.plist")
> #define SAFARI_COOKIE_BEHAVIOR_FILE_NAME  NS_LITERAL_STRING("com.apple.WebFoundation.plist")
> #define SAFARI_DATE_OFFSET                978307200
> #define MIGRATION_BUNDLE                  "chrome://browser/locale/migration/migration.properties"
>+#define kSafariHomePage                   "HomePage"

nit: wrong case.

> 

>+NS_IMETHODIMP
>+nsSafariProfileMigrator::GetSourceHomePageURL(nsACString& aResult)
>+{
>+  aResult.Truncate();
>+  
>+  // Lets first check if theres a home page key in the com.apple.safari file...

nit: let's. nit: there's.

>+  char* safariHomePageValue;

leak: this is sring is never freed.
>+  CFStringRef safariHomePage = NULL;
>+  CFDictionaryRef safariPrefs = CopySafariPrefs();
>+  // To avoid unwanted hangs, we must first check whether this key even exists.
>+  Boolean hasValue = ::CFDictionaryContainsKey(safariPrefs,
>+                                               CFSTR(kSafariHomePage));
>+  if (hasValue) {
>+    safariHomePage = (CFStringRef)CFDictionaryGetValue(safariPrefs,
>+                                                       CFSTR(kSafariHomePage));
>+    safariHomePageValue = GetNullTerminatedString(safariHomePage);
>+    aResult.Assign(safariHomePageValue);

Why don't you just use GetDictionaryCStringValue?

>+  }
>+  ::CFRelease(safariPrefs);
>+
>+  // Couldn't find the home page in com.apple.safai, time to check
>+  // com.apple.internetconfig for this key!
>+  if (!hasValue) {

You can simply return early when you do find the entry in com.apple.safari.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> What you must realize is that the home page only gets imported on the first
> migration wizard! This means that in order to check for this bug, you must
> completely delete/move/rename your Firefox folder in the Application Support
> folder, in order to make your Firefox think it is freshly installed. This will
> trigger the 'first run' migration wizard, which will also import your home
> page.

Anyone knows why this is done in that way? IMO its a bit inconsistent. If I select to import all profile data from Safari, I suspect to really get all of them. There shouldn't be a difference between the really first start and a later import action. 

Comment 6

[Michael Schonfeld]

Attachment: v2

Comment 7

[Michael Schonfeld]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-GB;
> rv:1.9b4) Gecko/2008030317 Firefox/3.0b4
> Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-GB;
> rv:1.9b4) Gecko/2008030317 Firefox/3.0b4
> 
> The Mac RC1 of beta 4 (en-GB locale) did not import my Safari home page
> "http://news.bbc.co.uk/" and instead set the home page to
> http://www.apple.com/startpage/
> 
> Reproducible: Didn't try
> 
> Steps to Reproduce:
> 1. Change your home page in Safari
> 2. Install FF 3.0b4rc1
> 3. Select to import home page from safari by the migration manager
> 4. Open new tab or check preferences
> Actual Results:  
> http://www.apple.com/startpage/ is loaded
> 
> Expected Results:  
> http://news.bbc.co.uk/ would be loaded
> 
> I have FF 2.0.12 and 3.0 b3 installed, both have been run before.
> I have Safari 3.0.4, a recent WebKit nightly build and my own WebKit subversion
> checkout/build also installed.
> 

Nicholas: I'm guessing you once used an older version of Safari before you upgraded to 3.0.4.  If you could maybe upload or just send me your com.apple.internetconfig.plist file so I could do some further testings and confirm some of my suspicions, I'd really appreciate it.

Comment 8

[Nicholas Shanks]

There is nothing of relavance in com.apple.internetconfig, but there is in internetconfigpriv:

nicholas$ defaults read com.apple.Safari
{
    ...
    HomePage = "http://news.bbc.co.uk/";
    ...
}
nicholas$ defaults read com.apple.internetconfigpriv
{
    ...
    WWWHomePage = "http://livepage.apple.com/";
}

Comment 9

[Michael Schonfeld]

Thanks Nicholas! That actually answered everything I needed from those files..

Okay, well then my patch would definitely fix your problem! It first checks for the home page key in com.apple.Safari and only then defaults back to internetconfig.

I do, however, need someone to check it on an older version of Safari.  I just wanna make sure it still defaults back to the right place in internetconfig and works all right.  I can't do it my self because I'm running Leopard, and it won't let me run anything older than Safari 3.0 for some reason... 

Comment 10

[Nicholas Shanks]

I can't provide that as I only have Leopard and Safari 3.0.4

Comment 11

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #9)
> Thanks Nicholas! That actually answered everything I needed from those files..
> 
> Okay, well then my patch would definitely fix your problem! It first checks for
> the home page key in com.apple.Safari and only then defaults back to
> internetconfig.
> 
> I do, however, need someone to check it on an older version of Safari.  I just
> wanna make sure it still defaults back to the right place in internetconfig and
> works all right.  I can't do it my self because I'm running Leopard, and it
> won't let me run anything older than Safari 3.0 for some reason... 

Michael - if you submit your patches to the tryserver and get a Mac build going, I can test on 10.4 with Safari 2.0.4; does that help/work?

Comment 12

[Michael Schonfeld]

Yea that would help a lot! Although we should probably wait for a review+ from Asaf, shouldn't we?

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

If your patch will build without errors you can immediately start. You don't need a review. It's just a TryServer...

Comment 14

[Michael Schonfeld]

Cool. Then, any chance I can get a BuildBot account?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I started the build process for you. Have a look here for the progress:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

Comment 16

[Michael Schonfeld]

The Mac OS build is done! This is exciting. My first tryserver build (even though technically it wasn't mine!)... Still exciting...

Stephen: If you could give it a try on your Mac with an older version of Safari (pre-v3) that'd be great. 

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just as an additionally note... Everybody who is interested in can grab it here:
https://build.mozilla.org/tryserver-builds/2008-03-11_04:[e-mail]-bug421295-SafariImport/

Comment 18

[Asaf Romano (gone)]

Comment on attachment 308586 [details] [diff] [review]
v2

+    return NS_OK;
+  }
+
+  ::CFRelease(safariPrefs);

Comment 19

[Michael Schonfeld]

Attachment: v2.1

Oops! I'm funny like that sometimes...

Comment 20

[Asaf Romano (gone)]

Comment on attachment 308604 [details] [diff] [review]
v2.1

>Index: browser/components/migration/src/nsSafariProfileMigrator.cpp
>===================================================================
>+nsSafariProfileMigrator::GetSourceHomePageURL(nsACString& aResult)
>+{
>+  aResult.Truncate();
>+
>+  // Let's first check if there's a home page key in the com.apple.safari file...
>+  CFDictionaryRef safariPrefs = CopySafariPrefs();
>+  nsCAutoString safariHomePage;
>+  if (GetDictionaryCStringValue(safariPrefs,
>+                                CFSTR(SAFARI_HOME_PAGE_PREF),
>+                                safariHomePage, kCFStringEncodingUTF8)) {
>+    aResult.Assign(safariHomePage);
>+    ::CFRelease(safariPrefs);
>+    return NS_OK;
>+  }

you can get rid of safariHomePage, and just do:
>+  if (GetDictionaryCStringValue(safariPrefs,
>+                                CFSTR(SAFARI_HOME_PAGE_PREF),
>+                                aResult, kCFStringEncodingUTF8)) {
>+    ::CFRelease(safariPrefs);
>+    return NS_OK;
>+  }

r=mano otherwise.

Comment 21

[Michael Schonfeld]

Attachment: For checkin

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Michael, you fixed Manos review comments? Than carrying over his r+ and ask for approval1.9 for the latest patch.

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

I've deleted my stock Firefox profile, run 2.0.4 on OS X 10.4, changed from the Apple start page to http://www.netscape.com, shut down, and have run the tryserver build (where I get the migration dialog), however, I still see the Minefield start page after import and subsequent restarts.

Does Safari have to be the default before we try to import?

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Stephen, you have deleted the whole Firefox folder under Application Support?

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #24)
> Stephen, you have deleted the whole Firefox folder under Application Support?

Yeah, I rm rf'd /Firefox, got the import wizard, clicked through; no dice.

Comment 26

[Michael Schonfeld]

Stephen: I need to ask you for 3 things, please: a. Please check and see if you have a "HomePage" key in your com.apple.Safari.plist file; b. Please check if you have a "WWWHomePage" key in your com.apple.internetconfig.plist file; c. Please check if you a "WWWHomePage" key in your com.apple.internetconfigpriv.plist file;

I need to know if you have any of those keys, and if so, what data they contain.

Sorry to bother you so much...
Thanks!!

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

a) nothing
b) ic-data, string, value == http://www.netscape.com
c) string, value == http://www.netscape.com

Comment 28

[Michael Schonfeld]

Stephen: Thanks! that's great, but... One last thing that I wanted to ask but forgot... If you can download and run FF3 b3/b4 (whichever, you just need the official branding for this work), and see if this will do the older Safari home page import, that will answer all my questions. I am trying to find out if this is a bug created by my patch, or something that was there all along, and somehow no one had stumbled upon it...

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

Sorry for the delay, Michael; testing shows that Firefox 3 beta 4 imports my Safari homepage after an rm -rf of the "Profiles" directory and the profiles.ini file.

Comment 30

[Asaf Romano (gone)]

mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp 1.49

Comment 31

[Michael Schonfeld]

Attachment: For Tryserver

Okay so as far as I can tell, this bug has been fixed. I simulated both Safari 3 and older versions of Safari, and was successfully able to import my homepage.

What I think had happened, is that the tryserver build we did a few days ago didn't contain the official branding strings, thus causing the home page migrator not to work (it will not start without the strings in branding.properties). So, I created another patch and changed migration.js to include the needed strings to run the home page migration wizard just for testing purposes.

Henrik: If you could maybe upload this patch to the tryserver, that be great. It's gonna make testing for this bug much easier...

Henrik, Stephen: Thanks for all your help guys!!!

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #31)
> branding.properties). So, I created another patch and changed migration.js to
> include the needed strings to run the home page migration wizard just for
> testing purposes.

So, my question again: Why this only works with offical branding? Is there any reason behind?

> Henrik: If you could maybe upload this patch to the tryserver, that be great.
> It's gonna make testing for this bug much easier...

I did that. Follow the status here:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

The TryServer also accepts an alternate .mozconfig. So using one with offical branding enabled should hopefully also work. But haven't tried yet.

Comment 33

[Michael Schonfeld]

Attachment: For Tryserver

Forgot to remove some printf commands!

Comment 34

[Michael Schonfeld]

I'm thinking the reason behind this is that we cannot offer our Google search page as a home page on non-official releases, And the home page migration wizard is set no to run if it has only one option to import.  

Having that said, I'm guessing we can open a new bug, to make the home page wizard run in non-official branding, and also offer to import even if it finds only one import source.  Though, I'm not entirely sure this is something worth working on.

Comment 35

[Carsten Book [:Tomcat]]

verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032615 Firefox/3.0b5pre and a test homepage in Safari -> works as expected and Homepage is imported into the Build.

--> Verified fixed

Comment 36

[Marcia Knous [:marcia]]

I modified the existing testcase https://litmus.mozilla.org/show_test.cgi?id=4072 to cover this and made a notation about the branding issue. I also added this tes to the Leopard specific test suite.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Michael, I stumbled over this issue again while running smoketests today for the upcoming 3.0.6 build. The question I've asked in comment 5 was never answered.

Can someone please tell me why the import of the home page only works for the initial import and not for subsequent imports? Thanks.