IE migration fails to import settings

RESOLVED FIXED

Status

()

Firefox
Migration
--
major
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Kevin Brosnan, Assigned: jimm)

Tracking

({regression})

Trunk
x86
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
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.
Asking QAWanted to get confirmation of this before determining if it blocks.
Keywords: qawanted
(Reporter)

Comment 3

8 years ago
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?
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?
blocking2.0: ? → betaN+
Keywords: regressionwindow-wanted

Comment 5

8 years ago
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.
(Reporter)

Comment 6

8 years ago
(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.
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?!
(Reporter)

Comment 8

7 years ago
Nothing useful in the error console as this only shows when run with no existing profile. File > import works fine.
(Assignee)

Comment 9

7 years ago
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?
(Assignee)

Updated

7 years ago
Blocks: 381459

Comment 10

7 years ago
(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).
(Assignee)

Comment 11

7 years ago
taking to sort this out.
Assignee: nobody → jmathies
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
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.
Blocks: 536650
(Assignee)

Updated

7 years ago
Keywords: qawanted, regressionwindow-wanted
(Assignee)

Updated

7 years ago
Keywords: regression
(Assignee)

Comment 14

7 years ago
(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

7 years ago
(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.
(Assignee)

Updated

7 years ago
Summary: Not asked to import IE settings on first run → IE migration fails to import settings
(Reporter)

Comment 16

7 years ago
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

7 years ago
(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.
(Assignee)

Comment 18

7 years ago
(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.
(Reporter)

Comment 19

7 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 476191
(Assignee)

Comment 20

7 years ago
Created attachment 488770 [details] [diff] [review]
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.
Attachment #488770 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #488770 - Flags: review? → review?(gavin.sharp)
(Assignee)

Comment 21

7 years ago
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.
(Assignee)

Comment 22

7 years ago
(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.
(Assignee)

Comment 23

7 years ago
Created attachment 488797 [details] [diff] [review]
don't prepend '.' for ip addresses v.2

better support using PR_StringToNetAddr.
Attachment #488779 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #488797 - Flags: review?(dwitte)
(Assignee)

Updated

7 years ago
Severity: blocker → major
(Assignee)

Comment 24

7 years ago
(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

7 years ago
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?
Attachment #488797 - Flags: review?(dwitte) → review-
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.
Attachment #488770 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 27

7 years ago
Created attachment 489151 [details] [diff] [review]
don't bail on failure, report instead v.2

updates plus I tested induced failure on each migration case.
Attachment #488770 - Attachment is obsolete: true
(Assignee)

Comment 28

7 years ago
(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

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

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

r=dwitte
Attachment #488797 - Flags: review- → review+
(Assignee)

Comment 30

7 years ago
http://hg.mozilla.org/mozilla-central/rev/1d21b07e4b88
http://hg.mozilla.org/mozilla-central/rev/26b26a93ab72
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

7 years ago
(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.
(Assignee)

Comment 32

7 years ago
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.
Attachment #488797 - Flags: approval1.9.1.16?
(Assignee)

Updated

7 years ago
Attachment #489151 - Flags: approval1.9.1.16?
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.
(Assignee)

Comment 34

7 years ago
(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.
(Assignee)

Updated

7 years ago
Attachment #488797 - Flags: approval1.9.2.13?
(Assignee)

Updated

7 years ago
Attachment #489151 - Flags: approval1.9.2.13?
(Assignee)

Comment 35

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #488797 - Flags: approval1.9.2.13?
Attachment #488797 - Flags: approval1.9.1.16?
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).
Attachment #489151 - Flags: approval1.9.2.13?
Attachment #489151 - Flags: approval1.9.2.13-
Attachment #489151 - Flags: approval1.9.1.16?
Attachment #489151 - Flags: approval1.9.1.16-
You need to log in before you can comment on or make changes to this bug.