Closed Bug 591884 Opened 14 years ago Closed 12 years ago

Migration wizard does not import favorites toolbar from IE7, IE8, and IE9

Categories

(Firefox :: Migration, defect)

Product:
Firefox
Component:
Migration
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 13
Tracking Flags:
Tracking Status
firefox12 + verified

People

(Reporter: ahoza, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [qa!])

Attachments

(1 file, 3 obsolete files)

patch v1.0
9.21 KB, patch
Felipe
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
BuildID: Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100829 Firefox/4.0b5pre
IE version: 8.0.7600.16385
 
Prerequisites:
Delete all existing FF profiles.

Steps to reproduce:

1. Start IE8 and bookmark some pages.
2. Start FF and when Migration dialog is displayed choose to import items from IE.
3. Check existing bookmarks in FF (in Bookmarks menu or Bookmarks sidebar or Library)

Expected results:
IE bookmarks were imported.

Actual results:
IE bookmarks were not imported.

Notes: Bookmarks are imported when using File->Import.
BuildID: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre)Gecko/20101213 firefox/4.0b8pre
Safari version: 5.0.2(6533.18.5)

This reproduces also on Mac OS.
(In reply to comment #0)
> 1. Start IE8 and bookmark some pages.
> 2. Start FF and when Migration dialog is displayed choose to import items from
> IE.
> 3. Check existing bookmarks in FF (in Bookmarks menu or Bookmarks sidebar or
> Library)

Does this problem with IE8 still persist with the final release of Firefox 4? What about IE9?
I am seeing this on any machine that comes into my shop with Firefox not previously installed. It does not seem to matter which version of IE is installed. I have seen it with version 7, 8 and 9. The favorites do not import as bookmarks during the initial setup. They import fine from File>Import after setup.
As tested it works fine in a Namoroka build. So this is a regression from Firefox 3.6.x. Can one of you please check between which beta releases of Firefox 4 it has been started? That would be a great help. Thanks.
Keywords: regression, regressionwindow-wanted
Summary: Bookmarks not imported from IE8 → Migration wizard does not import favorites from IE7, IE8, and IE9
There haven't been so many fixes to the migration wizard in the 4.0 time frame. I wonder if the fix for bug 548614 has been caused this regression. Dietrich, could you check this please?
I've just tested this with latest nightly build and Firefox fails consistently to migrate my IE bookmarks to Firefox. Tested with IE8 on winXP. 

This is a pretty serious regression and will mean fare fewer new users will have a comfortable experience converting from IE to Firefox. This is a product feature that cannot be allowed to regress like this. What can be done here? Who can help?
OS: Windows 7 → All
If this is considered a priority I may work on it
see also bug 682069 for password and cookie migration from IE failure.
As the current implementation of the IE migration wizard seems to be a blocker for bug 700250, I will start working on it. The general idea is to reimplement the IE migration wizard using async APIs, fixing issues along the way, and reusing some of the structure of bug 505192.
Blocks: 700250
Yoric, that's great news. Can you assign the bug to yourself and if you've got any scheduling or timing estimates, add those here?
Unfortunately, I do not know when I will find time to do that.
Assignee: nobody → felipc
I've got the basic JS migrator structure done (based on Makoto's awesome work from bug 505192), and looked into what will be necessary to port each of the existing migrator pieces from C++ to JS. Here's the digest:

- Misc settings and configuration data: these are mostly stored in the registry. Easily accessed from JS (nsIWindowsRegKey).

- Cookies: stored in files (some mix of text and binary blobs, it seems). It will require some parsing but definitely doable from JS.

- History: history is retrieved using win32 COM APIs. 
One idea was to use js-ctypes to call into these functions. This could work but it has two main problems:
  - there are many struct and interface types involved, declaring them all might be fragile and unnecessarily complex/unmaintainable
  - some of the APIs called involves memory management responsibilities delegated to the caller, which is bad to do from JS.

  The better model seems to be one where we have a few scriptable functions that will get this data from Win32 and give it to the JS migrator code, which would then feed the data to the Places async APIs. This will have the benefit of making it async more easily (since it's better to handle async apis from JS), while still keeping everything on its most natural place.
  I will still get some more opinions from folks in #windev about this

- Passwords and Form Fill: from previous investigations when this was first noticed as broken, it seems that it's no longer possible to retrieve this data because it's encrypted with the URLs as keys. For more info see bug 355538 comment 6, bug 399206, and bug 538654 with a proposed alternative.
It might be possible though that we could just simply dictionary-attack every URL imported at the history pass (except it sounds expensive, of course, specially on the first-run experience)

- Bookmarks: haven't looked yet into why they are broken
(In reply to Felipe Gomes (:felipe) from comment #12)
> - History: history is retrieved using win32 COM APIs. 

>   The better model seems to be one where we have a few scriptable functions
> that will get this data from Win32 and give it to the JS migrator code,
> which would then feed the data to the Places async APIs. This will have the
> benefit of making it async more easily (since it's better to handle async
> apis from JS), while still keeping everything on its most natural place.
>   I will still get some more opinions from folks in #windev about this

This was the same thought I had in how to make it, have a nsIIEHistoryReader (or whatever you want to call it) that hands history out to js. At that point is merely matter of passing the data to Places.

> - Passwords and Form Fill: from previous investigations when this was first
> noticed as broken, it seems that it's no longer possible to retrieve this
> data because it's encrypted with the URLs as keys. For more info see bug
> 355538 comment 6, bug 399206, and bug 538654 with a proposed alternative.
> It might be possible though that we could just simply dictionary-attack
> every URL imported at the history pass (except it sounds expensive, of
> course, specially on the first-run experience)

We can leave this to a follow-up, if it doesn't work already we are not breaking anything doing that.

> - Bookmarks: haven't looked yet into why they are broken

Bookmarks are just link files, shouldn't be problematic to do in js
btw, looking at our code we do this

1410     nsCOMPtr<nsIWindowsRegKey> regKey =
1411       do_CreateInstance("@mozilla.org/windows-registry-key;1");
1412     if (regKey &&
1413         NS_SUCCEEDED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_CURRENT_USER,
1414                                   REGISTRY_IE_TOOLBAR_KEY,
1415                                   nsIWindowsRegKey::ACCESS_READ))) {
1416       nsAutoString linksFolderName;
1417       if (NS_SUCCEEDED(regKey->ReadStringValue(
1418                          NS_LITERAL_STRING("LinksFolderName"),
1419                          linksFolderName)))
1420         personalToolbarFolderName = linksFolderName;
1421     }

If I check in my system, I have the REGISTRY_IE_TOOLBAR_KEY but inside it there is no LinksFolderName, though I have a bookmarks toolbar in ie. Maybe if the key does not exists it fallbacks to a default location.
And the fallback is likely Favorites\Links
So as far as I can tell, the bookmarks importing is not actually broken. What happens is just that it's not figuring out which is IE's Favorites Bar subfolder (due to what mak pointed out), so we're not adding the bookmarks to our toolbar. All bookmarks are still successfully imported to the Bookmarks menu (under "From Internet Explorer" if this is not the first run dialog)

I've tested this on Win7 with IE9. It'd be good if others who reported the same problem could verify if that is true for their cases too.
Attached patch Checkpoint 1 (obsolete) — — Splinter Review 
First checkpoint, this has all the migrator structure + the bookmarks importing code. It can read almost all of the necessary data from the Windows bookmarks folder but doesn't add anything to Places yet.
Attached patch IE History reader (obsolete) — — Splinter Review 
This is the C++ component that reads the history data from the system and hands it back to the migrator. It's almost ready for review, I just need to figure out some last details of the string handling in the C++/JS xpcom boundary
Attached patch IE History reader (obsolete) — — Splinter Review 
(attached the wrong patch on the previous comment)
Attachment #579915 - Attachment is obsolete: true
I've filed bug 710895 to do the actual work of migrating the IE profiler to JS which will also fix this bug, instead of hijacking and morphing this one. I marked this as depending on that, and then we can verify that the favorites import has been fixed once the bug lands.
Given the webapps work, I don't think I'll be able to get to the IE JS migrator before the next source migration. However, it's easy to fix this bug in the current migrator, independent of waiting for the new one.

The problem happens because in previous IE versions they stored the name of the Bookmarks folder in a regkey. Then on IE7 they dropped this key and went for the fixed name of "Links", but we didn't adjust our code to do so.

So what needs to be done is that if this RegKey reading fails: http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/src/nsIEProfileMigrator.cpp#1422
We just need to check for the existence of a folder called "Links", and if it exists, use that as the personalToolbarFolderName

Patches welcome ;)
Oh and actually we don't need to check for the folder existence, as that string is just compared to the name of the existing folders later.

So the fix should just be

} else {
  personalToolbarFolderName = NS_LITERAL_STRING("Links");
}

or whatever is the right way to assign to a nsAutoString

Now we just need to test if this works :)
I can do that, or at least I suppose :)
Assignee: felipc → mak77
Status: NEW → ASSIGNED
Blocks: 710895
No longer blocks: 700250
No longer depends on: 710895
OS: All → Windows 7
Attachment #578165 - Attachment is obsolete: true
Attachment #579916 - Attachment is obsolete: true
Attached patch patch v1.0 — — Splinter Review 
the test is not rocket science, but I verified it failed before and works now.

I will now push to try just to check it works in our tinderboxes.
Attachment #604249 - Flags: review?(felipc)
Comment on attachment 604249 [details] [diff] [review]
patch v1.0

Thanks Mak!

Hopefully the IE profile in the build machines will be sane in all Win versions and have at least the default bookmarks.

Given that the fix was simple and it has a test I think we should also request aurora approval once it goes green in the tree
Attachment #604249 - Flags: review?(felipc) → review+
try builds were successful, so I proceded
https://hg.mozilla.org/integration/mozilla-inbound/rev/b33dc8921c00
Flags: in-testsuite+
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/b33dc8921c00
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Summary: Migration wizard does not import favorites from IE7, IE8, and IE9 → Migration wizard does not import favorites toolbar from IE7, IE8, and IE9
Comment on attachment 604249 [details] [diff] [review]
patch v1.0

[Approval Request Comment]
This is a low-risk patch (w/ test) that fixes the bookmarks toolbar migrator that has been broken since forever. It landed on inbound yesterday and m-c today.

Since this was broken for a long time, not taking it to Aurora doesn't seem a big deal. However, for the first-run migration experience, every extra 6 weeks of fixed-ness seems worth it.
Attachment #604249 - Flags: approval-mozilla-aurora?
This is a regression, and a pretty important one that may be responsible for some of our much worse conversion rates over the last couple of years. I think we should get this into the hands of our users ASAP.  A 6 week wait may not seem like much, but in 6 weeks we get tens of millions of downloads from IE users and we simply fail to migrate their bookmarks toolbar bookmarks. That's a lot of potential missed opportunities at converting new downloaders from "looking at Firefox" to "becoming a Firefox user".
tracking-firefox12: --- → +
Keywords: qawanted
Comment on attachment 604249 [details] [diff] [review]
patch v1.0

[Triage Comment]
I'm OK with landing this early in FF12, since the extra bake time on Aurora likely would not shake any issues with migration out. Instead, we need to perform focused testing around migrations of the favorites toolbar from IE7, IE8, and IE9.
Attachment #604249 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
@akeybl, what's the specific ask for qawanted here?
Whiteboard: [qa+]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #32)
> @akeybl, what's the specific ask for qawanted here?

Start with the STR in comment#0, then do further exploratory testing to ensure that there aren't any regressions in bookmark/history migration from IE.
Hi guys.
I have tested this on the latest Nightly:
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120313 Firefox/13.0a1 

It seems that the bookmarks (favorites) from IE9 and also IE8 are not imported into Nightly via the migration wizard.
The only thing that is imported is the history under Bookmarks - Bookmarks Toolbar - Most Visited.
I have tried several times with the same result. The only way to import the bookmarks from IE is manually. In this case a new folder (From Internet Explorer) appears in Bookmarks menu that contains all the favorites from IE.
I just tried and everything was properly imported here, Win7x64 and IE9, checked both menu and toolbar.
Which steps did you follow exactly? Do you have Favorites in the Favorites and Favorites/Links folders?
I have followed the steps from the description on two separate test machines, one with Win 7 x 86 and the other with Win 7 x64.

No favorites were imported in Bookmarks menu. 
I don't have any favorites folder after the import, only the history .
interesting, with Nightly I can reproduce what you see, though with my latest build from inbound I couldn't and the test passes.  Will do a whole clobber and retest.
to keep things better separated I filed bug 735312 for the issue I'm tracking.
The patch in this bug was needed, but looks like in some case we fail elsewhere.
Vlad, just to clarify, did you use any command-line options to test, or a plain install in a clean box?
Vlad, nevermind, I think I identified an issue that happens with omni.ja, we were unable to reproduce cause our usual development builds don't use it :(
Hi Marco.

I performed a plain install of the Nightly build and as you saw, no bookmarks was imported.

If you want, I can further help in this matter.
I will surely need help in testing once bug 735312 is fixed (whether we port the fix to Aurora/Beta is another story, but I think should be feasible since the fix is simple and can't be more broken than it is).  I think we should move the QA to that bug, since here you can't really ensure anything using a release :(
(In reply to Marco Bonardo [:mak] from comment #43)
> I will surely need help in testing once bug 735312 is fixed (whether we port
> the fix to Aurora/Beta is another story, but I think should be feasible
> since the fix is simple and can't be more broken than it is).  I think we
> should move the QA to that bug, since here you can't really ensure anything
> using a release :(

Bug 735312 is now on Beta 12. Let's move forward with the testing.
I have verified this and the Favorites are imported from IE9 to:
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0 beta 2

I have followed the steps from the description.

Setting resolution to Verified Fixed on Firefox 12 beta.
status-firefox12: fixed → verified
Whiteboard: [qa+] → [qa+][qa!:12]
Removing qawanted based on comment 45. Vlad, please test to verify this is fixed in Firefox 13 as well.
Keywords: qawanted
I have tried this on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120417 Firefox/13.0a2

The Favorites from IE9 are imported along with the history in Firefox 13a2.
The only thing that does not get imported is the home page.
testing the homepage is sorta complicated, cause it works only for the official Firefox branding and only on initial migration.  We are looking into solutions to make it easier to test.
I understand...so based on comment45 and comment47, can I change the status of this  bug to Verified Fixed?

(In reply to Marco Bonardo [:mak] from comment #48)
> testing the homepage is sorta complicated, cause it works only for the
> official Firefox branding and only on initial migration.  We are looking
> into solutions to make it easier to test.
Yeah, I think it can be marked VERIFIED.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+][qa!:12] → [qa!]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: