Closed
Bug 575591
Opened 14 years ago
Closed 14 years ago
IE migration fails to import settings
Categories
(Firefox :: Migration, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: kbrosnan, Assigned: jimm)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
2.08 KB,
patch
|
dwitte
:
review+
|
Details | Diff | Splinter Review |
5.30 KB,
patch
|
dveditz
:
approval1.9.2.13-
dveditz
:
approval1.9.1.16-
|
Details | Diff | Splinter Review |
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•14 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.
Comment 2•14 years ago
|
||
Asking QAWanted to get confirmation of this before determining if it blocks.
Keywords: qawanted
Reporter | ||
Comment 3•14 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?
Comment 4•14 years ago
|
||
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
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•14 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.
Comment 7•14 years ago
|
||
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•14 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•14 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•14 years ago
|
Blocks: iemigratewin
Comment 10•14 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 12•14 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•14 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•14 years ago
|
Keywords: qawanted,
regressionwindow-wanted
Assignee | ||
Updated•14 years ago
|
Keywords: regression
Assignee | ||
Comment 14•14 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•14 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•14 years ago
|
Summary: Not asked to import IE settings on first run → IE migration fails to import settings
Reporter | ||
Comment 16•14 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•14 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•14 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•14 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 | ||
Comment 20•14 years ago
|
||
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•14 years ago
|
Attachment #488770 -
Flags: review? → review?(gavin.sharp)
Assignee | ||
Comment 21•14 years ago
|
||
This addresses the ip address cookies we're currently erroring out on.
Assignee | ||
Comment 22•14 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•14 years ago
|
||
better support using PR_StringToNetAddr.
Attachment #488779 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #488797 -
Flags: review?(dwitte)
Assignee | ||
Updated•14 years ago
|
Severity: blocker → major
Assignee | ||
Comment 24•14 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•14 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 26•14 years ago
|
||
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•14 years ago
|
||
updates plus I tested induced failure on each migration case.
Attachment #488770 -
Attachment is obsolete: true
Assignee | ||
Comment 28•14 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•14 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•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/1d21b07e4b88 http://hg.mozilla.org/mozilla-central/rev/26b26a93ab72
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 31•14 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•14 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•14 years ago
|
Attachment #489151 -
Flags: approval1.9.1.16?
Comment 33•14 years ago
|
||
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•14 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•14 years ago
|
Attachment #488797 -
Flags: approval1.9.2.13?
Assignee | ||
Updated•14 years ago
|
Attachment #489151 -
Flags: approval1.9.2.13?
Assignee | ||
Comment 35•14 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•14 years ago
|
Attachment #488797 -
Flags: approval1.9.2.13?
Attachment #488797 -
Flags: approval1.9.1.16?
Comment 36•14 years ago
|
||
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.
Description
•