Last Comment Bug 591884 - Migration wizard does not import favorites toolbar from IE7, IE8, and IE9
: Migration wizard does not import favorites toolbar from IE7, IE8, and IE9
Status: VERIFIED FIXED
[qa!]
: regression
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 2 votes (vote)
: Firefox 13
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on:
Blocks: iemigratewin 710895 735312
  Show dependency treegraph
 
Reported: 2010-08-30 01:26 PDT by ahoza
Modified: 2013-12-27 14:36 PST (History)
19 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
Checkpoint 1 (15.63 KB, patch)
2011-11-30 19:43 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
IE History reader (850 bytes, patch)
2011-12-07 17:43 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
IE History reader (12.50 KB, patch)
2011-12-07 17:45 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
patch v1.0 (9.21 KB, patch)
2012-03-08 16:08 PST, Marco Bonardo [::mak]
felipc: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description ahoza 2010-08-30 01:26:51 PDT
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.
Comment 1 Andreea Pod 2010-12-14 02:33:54 PST
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.
Comment 2 Henrik Skupin (:whimboo) 2011-04-21 05:09:11 PDT
(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?
Comment 3 Daifne 2011-04-22 20:00:15 PDT
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.
Comment 4 Henrik Skupin (:whimboo) 2011-04-26 01:56:30 PDT
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.
Comment 5 Henrik Skupin (:whimboo) 2011-04-26 02:00:38 PDT
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?
Comment 6 Asa Dotzler [:asa] 2011-08-25 13:06:06 PDT
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?
Comment 7 Marco Bonardo [::mak] 2011-08-25 13:12:37 PDT
If this is considered a priority I may work on it
Comment 8 Asa Dotzler [:asa] 2011-08-25 13:27:39 PDT
see also bug 682069 for password and cookie migration from IE failure.
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-07 06:43:16 PST
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.
Comment 10 Asa Dotzler [:asa] 2011-11-10 12:32:31 PST
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?
Comment 11 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-11-29 08:59:43 PST
Unfortunately, I do not know when I will find time to do that.
Comment 12 :Felipe Gomes (needinfo me!) 2011-11-30 01:10:31 PST
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
Comment 13 Marco Bonardo [::mak] 2011-11-30 02:49:38 PST
(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
Comment 14 Marco Bonardo [::mak] 2011-11-30 03:26:41 PST
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.
Comment 15 Marco Bonardo [::mak] 2011-11-30 03:28:04 PST
And the fallback is likely Favorites\Links
Comment 16 :Felipe Gomes (needinfo me!) 2011-11-30 16:32:51 PST
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.
Comment 17 :Felipe Gomes (needinfo me!) 2011-11-30 19:43:58 PST
Created attachment 578165 [details] [diff] [review]
Checkpoint 1

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.
Comment 18 :Felipe Gomes (needinfo me!) 2011-12-07 17:43:59 PST
Created attachment 579915 [details] [diff] [review]
IE History reader

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
Comment 19 :Felipe Gomes (needinfo me!) 2011-12-07 17:45:03 PST
Created attachment 579916 [details] [diff] [review]
IE History reader

(attached the wrong patch on the previous comment)
Comment 20 :Felipe Gomes (needinfo me!) 2011-12-14 15:05:38 PST
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.
Comment 21 :Felipe Gomes (needinfo me!) 2012-03-06 19:01:01 PST
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 ;)
Comment 22 :Felipe Gomes (needinfo me!) 2012-03-08 12:32:16 PST
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 :)
Comment 23 Marco Bonardo [::mak] 2012-03-08 13:00:25 PST
I can do that, or at least I suppose :)
Comment 24 Marco Bonardo [::mak] 2012-03-08 16:08:57 PST
Created attachment 604249 [details] [diff] [review]
patch v1.0

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.
Comment 25 Marco Bonardo [::mak] 2012-03-08 16:23:33 PST
https://tbpl.mozilla.org/?tree=Try&rev=d5fe8dd1a370
Comment 26 :Felipe Gomes (needinfo me!) 2012-03-08 16:26:03 PST
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
Comment 27 Marco Bonardo [::mak] 2012-03-09 02:16:33 PST
try builds were successful, so I proceded
https://hg.mozilla.org/integration/mozilla-inbound/rev/b33dc8921c00
Comment 28 Marco Bonardo [::mak] 2012-03-09 08:07:15 PST
https://hg.mozilla.org/mozilla-central/rev/b33dc8921c00
Comment 29 :Felipe Gomes (needinfo me!) 2012-03-09 12:03:06 PST
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.
Comment 30 Asa Dotzler [:asa] 2012-03-09 13:01:19 PST
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".
Comment 31 Alex Keybl [:akeybl] 2012-03-12 15:34:12 PDT
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.
Comment 32 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-12 15:53:22 PDT
@akeybl, what's the specific ask for qawanted here?
Comment 33 Alex Keybl [:akeybl] 2012-03-12 15:56:40 PDT
(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.
Comment 35 Vlad [QA] 2012-03-13 09:13:50 PDT
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.
Comment 36 Marco Bonardo [::mak] 2012-03-13 09:25:14 PDT
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?
Comment 37 Vlad [QA] 2012-03-13 09:30:12 PDT
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 .
Comment 38 Marco Bonardo [::mak] 2012-03-13 09:52:48 PDT
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.
Comment 39 Marco Bonardo [::mak] 2012-03-13 10:41:41 PDT
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.
Comment 40 Marco Bonardo [::mak] 2012-03-13 11:06:12 PDT
Vlad, just to clarify, did you use any command-line options to test, or a plain install in a clean box?
Comment 41 Marco Bonardo [::mak] 2012-03-13 12:40:20 PDT
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 :(
Comment 42 Vlad [QA] 2012-03-13 14:22:32 PDT
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.
Comment 43 Marco Bonardo [::mak] 2012-03-13 16:13:33 PDT
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 :(
Comment 44 Alex Keybl [:akeybl] 2012-03-19 17:15:03 PDT
(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.
Comment 45 Vlad [QA] 2012-03-22 08:40:56 PDT
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.
Comment 46 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-26 19:07:12 PDT
Removing qawanted based on comment 45. Vlad, please test to verify this is fixed in Firefox 13 as well.
Comment 47 Vlad [QA] 2012-04-18 06:42:34 PDT
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.
Comment 48 Marco Bonardo [::mak] 2012-04-18 06:50:57 PDT
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.
Comment 49 Vlad [QA] 2012-04-18 08:14:43 PDT
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.
Comment 50 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-18 08:19:17 PDT
Yeah, I think it can be marked VERIFIED.

Note You need to log in before you can comment on or make changes to this bug.