423460 - Saved Passwords are not imported from IE6 to Firefox on XP

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Pavlo]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; InfoPath.1)
Build Identifier: version 3.0b5pre

Saved Passwords and Home Page are not imported from IE6 to FireFox on XP

Reproducible: Always

Steps to Reproduce:
1. On XP open IE6 and navigate to bugzilla.flock.com 
2. Login to bugzilla and save password when IE prompts to save it. 
3. Make Bugzilla as Home Page in IE 
4. Open FireFox and Import settings from IE (File - Import) 
5. Verify that FireFox successfully finishes import the “Internet Settings”
and “Saved Passwords” 
6. Open Options of FireFox 

Actual Results:  
Home page from IE is not imported.
FireFox does not import the Saved Passwords from IE

Expected Results:  
FireFox should import the IE6 settings (“Home Page” and “Saved
Passwords”)

Comment 1

[Marcus Redivo]

Attachment: Fill in LoginInfo fields before calling AddLogin

This patch fills in the individual member fields of the LoginInfo structure to avoid the problem of unspecified fields on the Init() method.

Additionally, because the password field name is optional, I have updated the back end in toolkit/components/passwordmgr/src/storage-Legacy.js to permit this. Otherwise, a js error is thrown: "l.passwordField has no properties"

Comment 2

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

The home page not being migrated is expected, see bug 299060.

Comment 4

[Marcus Redivo]

Thanks for the reference. I knew that, but should have said so. At any rate, that's why there is no mention of a fix for the invalid part in my patch.

Sorry about attaching to the true dup, this is the one I was steered to.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 318700 [details] [diff] [review]
Fill in LoginInfo fields before calling AddLogin

>-                                        l.passwordField.indexOf(c) != -1);
>+                    (l.passwordField && l.passwordField.indexOf(c) != -1));

This storage-Legacy.js change shouldn't be made -- see bug 413314. Instead, the caller should have an aLogin->SetPasswordField() call. Unfortunately, we don't have a value for the field (at least with the current code?). We could set the field to empty-string (as HTTP logins do), but since it's a form login it seems like they should have *some* value. Maybe just "password". The login manager doesn't really use the field names anyway.

The rest of the change mostly looks good.

Except... *sigh*

1) It looks like I dropped the ball when fixing bug 396316. The login.hostname format changed, but it looks like the migration code was never changed to use the new format. :-( The code in storage-Legacy.js _upgrade_entry_to_2E() is what does this for existing Mozilla logins, but the IE migration code would need to convert from IE's format before calling addLogin(). Otherwise the login won't end up working anywhere. Blargh.

Marcus, do you want to take this on?

2) Setting formSubmitURL to the hostname won't work in some cases (sites that submit the login form to a different host -- eg www.site.com submitting to login.site.com)...

>+        aLogin->SetHostname(realm);
>+        aLogin->SetFormSubmitURL(realm);

This should just be |aLogin->SetFormSubmitURL(EmptyString());| But, surprise, addLogin() won't allow that. Bug 407567. Thankfully I already wrote a patch and tests, so Gavin just needs to review it. :)


Devil's Advocate: problem #1 is a bit of a pain to fix. It might we worth considering just WONTFIXing this bug and stop importing IE6 logins. That sounds harsh, but we already can't import IE7 logins, and I wonder if people still running IE6 are likely to be installing Firefox.

Comment 6

[Marcus Redivo]

(In reply to comment #5)
> Marcus, do you want to take this on?

Yes, I can take it on. However, I am unlikely to complete it before leaving on vacation, so late June would be when it would be available for review.

I researched and understand all your points.

As for WONTFIXING because we can't import IE7 passwords, at Flock we have an interest in importing passwords for a specific list of sites, which IS possible in IE7. Therefore, we would still like to be able to use the general-case code for IE6, and are interested in fixing it.

(IE7 uses the URL to encrypt the login info. If you know the URL, you can get the password; we have some URLs of interest. At this point, there is no known general solution.)

Comment 7

[Justin Dolske [:Dolske]]

Requesting blocking to get this on drivers' radar.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #5)
> Devil's Advocate: problem #1 is a bit of a pain to fix. It might we worth
> considering just WONTFIXing this bug and stop importing IE6 logins. That sounds
> harsh, but we already can't import IE7 logins, and I wonder if people still
> running IE6 are likely to be installing Firefox.

Wasn't IE7 a forced update? Or were users able to avoid it? It would be good to consider the number of users who are affected by this patch in order to determine its blocking status.

Comment 9

[Marcus Redivo]

IE7 was not a forced update. I have a fully-up-to-date Windows XP Pro SP2 dev box with IE6 on it; that's how I can develop and test this.

Comment 10

[Justin Dolske [:Dolske]]

(In reply to comment #8)

> It would be good to
> consider the number of users who are affected by this patch in order to
> determine its blocking status.

http://www.w3schools.com/browsers/browsers_stats.asp and http://www.thecounter.com/stats/2008/March/browser.php both show basically an even split between IE6 and IE7. But the real question would be the rate of IE6->FF conversions... Same as IE7? Or is it skewed by people who don't want to upgrade (or can't)?

Paging Ken Kovash!

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

This affects migration & import, or just import?

Comment 12

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

Both.

Comment 13

[Justin Dolske [:Dolske]]

Taking, since this is now a blocker.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Thanks Justin - hate to make you work on a weekend, but any ETA to a fix?

Comment 15

[Justin Dolske [:Dolske]]

Should have something tomorrow. I think the fix itself will not be too terrible... It's basically the existing patch plus some glue to reuse the existing _upgrade_entry_to_2E code in storage-Legacy.js. The craptastic part is verifying that IE6 stores logings (in a variety of flavors) in the format we're expecting, and that we handle them correctly.

Comment 16

[Justin Dolske [:Dolske]]

Attachment: Test plan for verifying IE login format

I ran through a variety of logins for form and HTTP auth; saving them in IE and then importing in Firefox to see what we get. Generally looks consistent with what was previously expected.

One surprising result was that either IE doesn't save port numbers for form logins, or our migration code isn't pulling them out from wherever they're at. So, if you save a login on "http://example.com:999", it's just saved as "http://example.com", and thus Firefox will only use it on port 80 and not port 999.

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

* Fixes some bugs in nsIEProfileMigrator
* Adds a helper interface, which is basically a wrapper around the existing storage-Legacy.js upgrade code.
* Needs a little more cleanup of comments and a real UUID.
* Tested with saving and importing a bunch of logins from IE (see previous testplan attachment)
* Needs automated tests. These should be simple, basically clones of some of the existing tests. We can't really test the nsIEProfileMigrator code (other than manual Litmus testing), but we can automate testing of the helper interface by feeding it the same things the migrator should be feeding it.

Comment 18

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

Comment on attachment 319350 [details] [diff] [review]
Patch v.1

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp

>+  (void) pwmgr->InitHelper();

I hates the (void)s, and it isn't consistent with the rest of this file, so please remove them.

>+        // IE doesn't store the password field name either, so just set it
>+        // to "password". That should be fine, as login manager doesn't
>+        // really use the value anyway.

>+        aLogin->SetPasswordField(NS_LITERAL_STRING("password"));

Why not leave it blank, then? I'd rather not "pollute" this information lest we decide to start using it again for whatever reason.

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js

>+    initHelper : function () {

You could just put this stuff in the constructor, given that this shouldn't fail and you don't really care to propagate any errors beyond the getService() call failing.

>+    /*
>+     * migrateAndAddLogin
>+     *
>+     * Given a login with IE6-formatted fields, migrates it to the new format
>+     * and adds it to the login manger.

typo: manger. They're not specifically IE6-formatted, right? Just pre-2E, so potentially useful for other migrators?

>+     *   * Didn't have an easy way to test "https://example.com:666", but it
>+     *     is likely saved as "example.com:666". Thus, for non-standard ports
>+     *     we can't tell if it's SSL or not, so we create both.

You can use https://kuix.de:9443/misc/test35 to test that case, fwiw (u: test35 p: test35).

Comment 19

[Justin Dolske [:Dolske]]

(In reply to comment #18)

> >+        aLogin->SetPasswordField(NS_LITERAL_STRING("password"));
> 
> Why not leave it blank, then? I'd rather not "pollute" this information lest we
> decide to start using it again for whatever reason.

Yeah, that's a good point. The usernameField should always have a value, so that's good enough.

> >Index: toolkit/components/passwordmgr/src/storage-Legacy.js
> 
> >+    initHelper : function () {
> 
> You could just put this stuff in the constructor, given that this shouldn't
> fail and you don't really care to propagate any errors beyond the getService()
> call failing.

Well, I'd like to keep it as separated from the rest of storage-Legacy as possible. Although, I suppose I can just add a |if (!init)|  check in migrateAndAddLogin(), so make it self-initialize...


> >+    /*
> >+     * migrateAndAddLogin
> 
> They're not specifically IE6-formatted, right? Just pre-2E, so
> potentially useful for other migrators?

Hmm, I think I'd prefer to limit the scope of the API to just "internal helper method for whatever nsIEProfileMigrator feeds it" (and add IDL comments to that effect), and not make it a general utility API (since the specific storage format and version is basically opaque anyway).

Comment 20

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updated patch. Should just need tests now. Gavin, any more comments on this one?

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Same as v.2, with unit test.

Comment 22

[Justin Dolske [:Dolske]]

Checked in.

I'm being slightly naughty and checking in without approval1.9, but I'll just request retroactive approval because (1) mconnor knows this is coming (2) the code only changes stuff that doesn't work at all right now (so risk is very low) and (3) it's late and we want this landed ASAP.

Reed would probably chastise me, but this is a Windows bug so he's probably not even paying attention to it. :-P

Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
  new revision: 1.74; previous revision: 1.73
Checking in toolkit/components/passwordmgr/public/Makefile.in;
  new revision: 1.4; previous revision: 1.3
Checking in toolkit/components/passwordmgr/public/nsILoginManagerIEMigrationHelper.idl;
  initial revision: 1.1
Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
  new revision: 1.27; previous revision: 1.26
Checking in toolkit/components/passwordmgr/test/unit/test_storage_legacy_5.js;
  initial revision: 1.1

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 319522 [details] [diff] [review]
Patch v.3

a1.9=beltzner