575591 - IE migration fails to import settings

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Kevin Brosnan]

Renamed my Mozilla folder in %userprofile%\appdata\roaming and %userprofile%\appdata\local

Ran Beta 1, should be offered to import my IE settings.

Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b1) Gecko/20100628 Firefox/4.0b1

Comment 1

[Kevin Brosnan]

I can't get the Settings Importer to trigger at all on my computer. Tried all the 1.9.3 Developer previews, some 3.6 builds. Would like some confirmation that this works on Windows 7 for other testers. 

I have tried a different Windows user and rebooting neither resolved the issue.

Comment 2

[Dietrich Ayala (:dietrich)]

Asking QAWanted to get confirmation of this before determining if it blocks.

Comment 3

[Kevin Brosnan]

This still fails for me using Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100801 Minefield/4.0b3pre.

I should have had this looked at during the summit. But seeing how that ship has sailed is there some logging or debugging info that would be helpful?

Comment 4

[Johnathan Nightingale [:johnath]]

I don't think we disabled this on purpose, and it's a regression. Blocking betaN to understand what's going on, here. If this is a deliberate product decision that got made when I wasn't around, re-nom, but I'd be surprised.

Kevin - any chance you can help us hunt down a regression window?

Comment 5

[aravindm]

Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b3) Gecko/20100805 Firefox/4.0b3

It works for me. After renaming my mozilla profile folder, It is offering me the Import IE settings. Also tried removing my profile folder, it works for me. Tested with the original environment and also with the latest trunk 4.0b4pre.

Comment 6

[Kevin Brosnan]

(In reply to comment #4)
That is the problem it seems that on my computer importing settings on first run just fails. See comment 1, I can't get the import to trigger on 3.6. I don't think this should be blocking but I am interested in sorting out where the failure is.

To do: check if file > import or the bookmarks importer works at all.

Comment 7

[(mostly gone) XtC4UaLL [:xtc4uall]]

Kevin, you didn't have a look at Error Console by any Chance?

When i tried to reproduce the Import from MSIE 8 (WinXP) with all Options checked (=default), only the Cookies got imported (using a Build built of http://hg.mozilla.org/mozilla-central/rev/42270894db65).

Error Console Output:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIBrowserProfileMigrator.migrate]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/migration/migration.js :: anonymous :: line 367"  data: no]

That one got mentioned in Bug 434911 what states to be fixed by Bug 433070 what landed for 3.6a1 but seems to be a Seamonkey-Import only fix, no?

Maybe the same Fix has to be done for MSIE Import?!

Comment 8

[Kevin Brosnan]

Nothing useful in the error console as this only shows when run with no existing profile. File > import works fine.

Comment 9

[Jim Mathies [:jimm]]

Note certain parts of the importer don't run if you're running a non officially branded build. Could this be part of the problem?

Comment 10

[Alan Thomas]

(In reply to comment #7)
> When i tried to reproduce the Import from MSIE 8 (WinXP) with all Options
> checked (=default), only the Cookies got imported (using a Build built of
> http://hg.mozilla.org/mozilla-central/rev/42270894db65).

Still happens in current nightlies. No first-run ask (although according to comment 9, maybe this is intended), and import still fails (except cookies).

Comment 11

[Jim Mathies [:jimm]]

taking to sort this out.

Comment 12

[Jim Mathies [:jimm]]

Hmm appears we fail trying to migrate some cookies, and the macro we use to manage the migration calls is designed to opt out after a single failure.

http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/nsBrowserProfileMigratorUtils.h#49

That's probably not the best way to handle things.

Comment 13

[Jim Mathies [:jimm]]

The reason we're failing seems to do with a bit of code in the migrator which adds a leading '.' to domains and ip addresses:

http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/nsIEProfileMigrator.cpp#1903

Bug 536650 changed things so we reject ip addresses with this format when adding cookies.

Between the two problems, we end up with situations (which are probably pretty common) where the migrator exits out without doing much of anything. I guess we don't warn the user either when this happens. This looks to be a problem on branch as well.

I'll work up some fixes.

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #0)
> Renamed my Mozilla folder in %userprofile%\appdata\roaming and
> %userprofile%\appdata\local
> 
> Ran Beta 1, should be offered to import my IE settings.
> 
> Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:2.0b1) Gecko/20100628
> Firefox/4.0b1

Kevin, you should still see the dialog. If you're not seeing that at all then we may have two bugs here. To bring it up reliably, I just rename:

C:\Users\(username)\AppData\Roaming\Mozilla\Firefox

to something like 

C:\Users\(username)\AppData\Roaming\Mozilla\Firefox Save

and launch a nightly or a beta. The migration wizard comes up, but fails to import. Can you confirm this is what you're seeing?

Comment 15

[Alan Thomas]

(In reply to comment #10)
> No first-run ask (although according to comment 9, maybe this is intended)

My apologies, this was incorrect. The first-run ask does appear in latest nightly, and it claims to have been successful, but in reality it is broken. 

The title of this bug should be changed to reflect this failure to import correctly.

Comment 16

[Kevin Brosnan]

As I've stated several times, I get no dialog at all. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101105 Firefox/4.0b8pre

Comment 17

[Alan Thomas]

(In reply to comment #16)
> As I've stated several times, I get no dialog at all. Mozilla/5.0 (Windows NT
> 6.1; WOW64; rv:2.0b8pre) Gecko/20101105 Firefox/4.0b8pre

Is this perhaps something to do with x64 vs. x86? I think IE8 comes in a native 64-bit flavor on x64 systems, so perhaps we're failing to detect it properly. The code in nsProfileMigrator::GetDefaultBrowserMigratorKey seems to delve around in Win32 resources to detect browsers, which might be different on x64. I don't know enough about x64 Windows to confirm this, though.

I am on a 32-bit system (Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101105 Firefox/4.0b8pre). This bug's platform is set to x86; I don't understand why, since the only report of problem has been on an x64 system.

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #16)
> As I've stated several times, I get no dialog at all. Mozilla/5.0 (Windows NT
> 6.1; WOW64; rv:2.0b8pre) Gecko/20101105 Firefox/4.0b8pre

Have you tried the step I mentioned in comment 14? I can trigger the wizard with that, on 64-bit windows 7 with the latest nightly I already have installed.

Comment 19

[Kevin Brosnan]

Yes, I've tried renaming the Mozilla %appdata% folder. I've also tried a new user account. I still have this issue, comment 0, comment 1, comment 8.

Comment 20

[Jim Mathies [:jimm]]

Attachment: don't bail on failure, report instead v.1

First patch: provide better reporting on failures, and don't bail on the entire migration when something doesn't migrate successfully. This should also address bug 476191.

Comment 21

[Jim Mathies [:jimm]]

Attachment: don't prepend '.' for ip addresses v.1

This addresses the ip address cookies we're currently erroring out on.

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #21)
> Created attachment 488779 [details] [diff] [review]
> don't prepend '.' for ip addresses v.1
> 
> This addresses the ip address cookies we're currently erroring out on.

Hmm, I suppose this won't work for IPv6. It doesn't look like our current migration code is compatible with that standard anyway. The alternative here would be to fully parse the domain using something like nsIEffectiveTLDService. I'll work an alternative patch that does that.

Comment 23

[Jim Mathies [:jimm]]

Attachment: don't prepend '.' for ip addresses v.2

better support using PR_StringToNetAddr.

Comment 24

[Jim Mathies [:jimm]]

(In reply to comment #19)
> Yes, I've tried renaming the Mozilla %appdata% folder. I've also tried a new
> user account. I still have this issue, comment 0, comment 1, comment 8.

Kevin, is there anything you can think of that's unique to your system? Have you disabled any os security features?

Comment 25

[[e-mail]]

Comment on attachment 488797 [details] [diff] [review]
don't prepend '.' for ip addresses v.2

>diff --git a/browser/components/migration/src/nsIEProfileMigrator.cpp b/browser/components/migration/src/nsIEProfileMigrator.cpp
>-    // delete any possible extant matching host cookie
>-    if (hostCopy[0] == '.')
>+    // delete any possible extant matching host cookie and
>+    // check if we're dealing with an IPv4/IPv6 hostname.
>+    PRBool isIPAddress = PR_FALSE;
>+    if (hostCopy[0] == '.') {
>       aCookieManager->Remove(nsDependentCString(hostCopy+1),
>                              stringName, stringPath, PR_FALSE);
>+      PRNetAddr addr;
>+      if (PR_StringToNetAddr(hostCopy+1, &addr) == PR_SUCCESS)
>+        isIPAddress = PR_TRUE;
>+    }

I think there's a simpler way. Right above this section:

1903     // first, with a non-null domain, assume it's what Mozilla considers
1904     // a domain cookie. see bug 222343.
1905     if (*host && *host != '.' && *host != '/')
1906       *hostCopyConstructor++ = '.';

I think you can just expand the 'if' condition to include '&& PR_StringToNetAddr(host, &addr) != PR_SUCCESS'. Then you don't have to worry about doing this:

>-    onerv = aCookieManager->Add(nsDependentCString(hostCopy),
>+    onerv = aCookieManager->Add(nsDependentCString(hostCopy + (isIPAddress ? 1 : 0)),

You should also update the comment above that 'if' check to reflect reality.

Sound right?

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 488770 [details] [diff] [review]
don't bail on failure, report instead v.1

>diff --git a/browser/components/migration/content/migration.js b/browser/components/migration/content/migration.js

>+    case "Migration:ItemError":
>+      var type = "undefined";
>+      switch (aData) {

>+      case "10":
>+      case "20":
>+      case "40":

Won't these be in decimal? So 16/32/64... Probably easier to just use parseInt(aData) and the Ci.nsIBrowserProfileMigrator.BOOKMARKS etc. constants. Would be good to test this reporting by making all the methods fail.

r=me with that fixed.

Comment 27

[Jim Mathies [:jimm]]

Attachment: don't bail on failure, report instead v.2

updates plus I tested induced failure on each migration case.

Comment 28

[Jim Mathies [:jimm]]

(In reply to comment #25)
> Comment on attachment 488797 [details] [diff] [review]
> don't prepend '.' for ip addresses v.2
> 
> >diff --git a/browser/components/migration/src/nsIEProfileMigrator.cpp b/browser/components/migration/src/nsIEProfileMigrator.cpp
> >-    // delete any possible extant matching host cookie
> >-    if (hostCopy[0] == '.')
> >+    // delete any possible extant matching host cookie and
> >+    // check if we're dealing with an IPv4/IPv6 hostname.
> >+    PRBool isIPAddress = PR_FALSE;
> >+    if (hostCopy[0] == '.') {
> >       aCookieManager->Remove(nsDependentCString(hostCopy+1),
> >                              stringName, stringPath, PR_FALSE);
> >+      PRNetAddr addr;
> >+      if (PR_StringToNetAddr(hostCopy+1, &addr) == PR_SUCCESS)
> >+        isIPAddress = PR_TRUE;
> >+    }
> 
> I think there's a simpler way. Right above this section:
> 
..
> 
> I think you can just expand the 'if' condition to include '&&
> PR_StringToNetAddr(host, &addr) != PR_SUCCESS'. Then you don't have to worry
> about doing this:
> 
> Sound right?

'host' includes the path at this point so the conversion in PR_StringToNetAddr will always fail, even for ip addresses. Which is why I put all this logic down below the construction of hostCopy.

I considered just blowing all this crazy string manipulation away and using normal string classes, but decided against it.

Comment 29

[[e-mail]]

Comment on attachment 488797 [details] [diff] [review]
don't prepend '.' for ip addresses v.2

Er, right you are. This code sucks. :(

r=dwitte

Comment 30

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/1d21b07e4b88
http://hg.mozilla.org/mozilla-central/rev/26b26a93ab72

Comment 31

[Jim Mathies [:jimm]]

(In reply to comment #19)
> Yes, I've tried renaming the Mozilla %appdata% folder. I've also tried a new
> user account. I still have this issue, comment 0, comment 1, comment 8.

Kevin, if you're still seeing the display problem in the next nightly or beta, lets file off a new bug that isn't blocking and investigate further.

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 488797 [details] [diff] [review]
don't prepend '.' for ip addresses v.2

requesting 1.9.1 approval, this problem is on branch as well.

Comment 33

[Daniel Veditz [:dveditz]]

only the 1.9.1 branch and not the 1.9.2 branch? 1.9.2.x is what you get when you go to www.mozilla.com to get Firefox, people have to really dig to install a 1.9.1-based Firefox as a new user.

Comment 34

[Jim Mathies [:jimm]]

(In reply to comment #33)
> only the 1.9.1 branch and not the 1.9.2 branch? 1.9.2.x is what you get when
> you go to www.mozilla.com to get Firefox, people have to really dig to install
> a 1.9.1-based Firefox as a new user.

ah, sorry, both actually.

Comment 35

[Jim Mathies [:jimm]]

The changes that rejected '.' on IPs landed on trunk only back on 1/2010, so we might not gain anything. Even before we rejected that scenario, those addresses probably didn't work, this patch may address that. I'll have to test on that branch.

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 489151 [details] [diff] [review]
don't bail on failure, report instead v.2

It's too late in this cycle to take more non-blockers. If fixing this in trunk isn't enough we can look at it again next time (but it seems like if it were a terrible problem we'd have heard a lot more complaints about it).