1333734 - Firefox gets stuck when importing passwords from Chrome

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Ghader Ahmadi]

Attachment: bug.PNG

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170118123726

Steps to reproduce:

I get import data from another browser to my firefox version 51 and when i select what data i want to import from google chrome browser version 55 and i just unselect bookmarks and selet passwords and history and cookies to import from google chrome.


Actual results:

after i click in next to import data its just freezing in password importing for long time  i just wait for an hour but its not finished just freez in importing 


Expected results:

its have to be import in moxmimum 1 minitus but its take too long and not finished

Comment 1

[:Gijs (he/him)]

If you follow the following steps:

1. open the browser console (ctrl-shift-j)
2. click the trashcan button on the top left
3. migrate only the passwords (or, if that doesn't produce the problem, migrate both the passwords and history)

Do any errors show up in the browser console when we get stuck?

Comment 2

[Ghader Ahmadi]

i did that and follow that steps and get this error :

--------------------------------------------------

          This is a browser feature intended for developers. Do not paste any code here given to you by someone else. It may compromise your account or have other negative side effects.  root_library.js.d21b70aeb39d100314ecd053146251cb.js:15
Error detecting Chrome profiles: Error: Chrome's 'Local State' file does not exist.  ChromeProfileMigrator.js:190
instagramstatic-a.akamaihd.net : server does not support RFC 5746, see CVE-2009-3555 (unknown)
[Exception... "Must have a non-null string spec or nsIFile object"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://app/components/ChromeProfileMigrator.js :: GetWindowsPasswordsResource/<.migrate< :: line 468"  data: no]  (unknown)
Login data scheme type not supported: 3 ChromeProfileMigrator.js:477
Invalid chrome URI: /
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Jan 26 2017 15:03:12 GMT+0330 (Iran Standard Time)
Full Message: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIOService.newURI]
Full Stack: JS frame :: resource://gre/modules/NetUtil.jsm :: NetUtil_newURI :: line 206
JS frame :: resource://app/components/ChromeProfileMigrator.js :: GetWindowsPasswordsResource/<.migrate< :: line 456
JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 319
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 937
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 816
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.scheduleWalkerLoop/< :: line 750
JS frame :: resource:///modules/MigrationUtils.jsm :: MU_showMigrationWizard :: line 841
JS frame :: chrome://browser/content/places/places.js :: PO_importFromBrowser :: line 366
JS frame :: chrome://browser/content/places/places.xul :: oncommand :: line 1  NetUtil.jsm:206
fb-s-c-a.akamaihd.net : server does not support RFC 5746, see CVE-2009-3555  (unknown)
fb-s-d-a.akamaihd.net : server does not support RFC 5746, see CVE-2009-3555  (unknown)
fb-s-b-a.akamaihd.net : server does not support RFC 5746, see CVE-2009-3555  (unknown)
"


                                            
 .d8888b.  888                       888    
d88P  Y88b 888                       888    
Y88b.      888                       888    This is a browser feature intended for 
 "Y888b.   888888  .d88b.  88888b.   888    developers. If someone told you to copy-paste 
    "Y88b. 888    d88""88b 888 "88b  888    something here to enable a Facebook feature 
      "888 888    888  888 888  888  Y8P    or "hack" someone's account, it is a 
Y88b  d88P Y88b.  Y88..88P 888 d88P         scam and will give them access to your 
 "Y8888P"   "Y888  "Y88P"  88888P"   888    Facebook account.
                           888              
                           888              
                           888              

See https://www.facebook.com/selfxss for more information.
"  XJScVoRCySs.js:23
Content Security Policy: Directive ‘frame-src’ has been deprecated. Please use directive ‘child-src’ instead. (unknown)
JQMIGRATE: Migrate is installed with logging active, version 1.4.1  init.en.299aa73216febc81ea11.js:20
JQMIGRATE: jQuery.browser is deprecated  init.en.299aa73216febc81ea11.js:20
console.trace():
------------------------------------

Comment 3

[:Gijs (he/him)]

(In reply to Ghader Ahmadi from comment #2)
> i did that and follow that steps and get this error :

Thanks, this is really helpful!


> Error detecting Chrome profiles: Error: Chrome's 'Local State' file does not
> exist.  ChromeProfileMigrator.js:190

This is not fatal.

> [Exception... "Must have a non-null string spec or nsIFile object" 
> nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame ::
> resource://app/components/ChromeProfileMigrator.js ::
> GetWindowsPasswordsResource/<.migrate< :: line 468"  data: no]  (unknown)
> Login data scheme type not supported: 3 ChromeProfileMigrator.js:477

These are also not fatal ( it's this try...catch block: https://dxr.mozilla.org/mozilla-release/source/browser/components/migration/ChromeProfileMigrator.js#465-483 ).

> Invalid chrome URI: /
> A promise chain failed to handle a rejection. Did you forget to '.catch', or
> did you forget to 'return'?
> See
> https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/
> Promise
> 
> Date: Thu Jan 26 2017 15:03:12 GMT+0330 (Iran Standard Time)
> Full Message: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
> [nsIIOService.newURI]
> Full Stack: JS frame :: resource://gre/modules/NetUtil.jsm :: NetUtil_newURI
> :: line 206
> JS frame :: resource://app/components/ChromeProfileMigrator.js ::
> GetWindowsPasswordsResource/<.migrate< :: line 456
> JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 319
> JS frame :: resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js :: Handler.prototype.process ::
> line 937
> JS frame :: resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop
> :: line 816
> JS frame :: resource://gre/modules/Promise.jsm ->
> resource://gre/modules/Promise-backend.js ::
> this.PromiseWalker.scheduleWalkerLoop/< :: line 750
> JS frame :: resource:///modules/MigrationUtils.jsm :: MU_showMigrationWizard
> :: line 841
> JS frame :: chrome://browser/content/places/places.js ::
> PO_importFromBrowser :: line 366
> JS frame :: chrome://browser/content/places/places.xul :: oncommand :: line
> 1  NetUtil.jsm:206

But this is (it's broken on several levels, all of which we should fix).

Comment 4

[Ghader Ahmadi]

ok great ,its nicee to me to cou;d help

Comment 5

[Marco Bonardo [:mak]]

uh-oh, try/catch doesn't cover the full for loop :(

Comment 6

[Ghader Ahmadi]

what is going on ? did you resolve that ? any news ?

Comment 7

[:Gijs (he/him)]

(In reply to Ghader Ahmadi from comment #6)
> what is going on ? did you resolve that ? any news ?

Any news will appear on the bug. Someone needs to write a patch (that will likely be me), which then needs to land on the current development branch (54) and then be uplifted to beta (52). 51 is shipped, so we will only do extra releases on it for very serious security/stability-related fixes, which this isn't.

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1333734 - fix chrome password import if it contains items we can't read as a URI,

Review commit: https://reviewboard.mozilla.org/r/108174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108174/

Comment 9

[:Gijs (he/him)]

I would have liked to add test coverage for this, but I'm not near my Windows machine right now. In any case, the patch is very straightforward. There is a little bit of existing test coverage, and I pushed to try to make sure I'm not breaking that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99fd1a72ef79

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8831623 [details]
Bug 1333734 - fix chrome password import if it contains items we can't read as a URI,

https://reviewboard.mozilla.org/r/108174/#review109216

::: browser/components/migration/ChromeProfileMigrator.js:448
(Diff revision 1)
> +        try {
> +          let origin_url = row.getResultByName("origin_url");
> +          let action_url = row.getResultByName("action_url");
> +          // Ignore entries for non-http(s)/ftp URLs because we likely can't
> +          // use them anyway.
> +          if ((origin_url && !(/https?|ftp/i).test(origin_url)) ||

since later we NetUtil.newURI both of them, wouldn't be nicer to do that here instead, and use .scheme?

Comment 11

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #10)
> Comment on attachment 8831623 [details]
> Bug 1333734 - fix chrome password import if it contains items we can't read
> as a URI,
> 
> https://reviewboard.mozilla.org/r/108174/#review109216
> 
> ::: browser/components/migration/ChromeProfileMigrator.js:448
> (Diff revision 1)
> > +        try {
> > +          let origin_url = row.getResultByName("origin_url");
> > +          let action_url = row.getResultByName("action_url");
> > +          // Ignore entries for non-http(s)/ftp URLs because we likely can't
> > +          // use them anyway.
> > +          if ((origin_url && !(/https?|ftp/i).test(origin_url)) ||
> 
> since later we NetUtil.newURI both of them, wouldn't be nicer to do that
> here instead, and use .scheme?

I'm trying to avoid the console warning and/or exception that is automatically generated when creating chrome:// URIs that don't match our format (which Chrome generates and uses). I'll update the comment to make this more obvious.

Comment 12

[Marco Bonardo [:mak]]

(In reply to :Gijs from comment #11)
> I'm trying to avoid the console warning and/or exception that is
> automatically generated when creating chrome:// URIs that don't match our
> format 

If it's just warnings in the shell, this is such an edge case that we should probably not care.
If it's exceptions, we are indeed catching them here, isn't it?

Comment 13

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #12)
> (In reply to :Gijs from comment #11)
> > I'm trying to avoid the console warning and/or exception that is
> > automatically generated when creating chrome:// URIs that don't match our
> > format 
> 
> If it's just warnings in the shell, this is such an edge case that we should
> probably not care.
> If it's exceptions, we are indeed catching them here, isn't it?

Yes, and reporting them. I'm not sure if it's an edgecase, but if you think so, I'll update the patch accordingly.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8831623 [details]
Bug 1333734 - fix chrome password import if it contains items we can't read as a URI,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108174/diff/1-2/

Comment 15

[:Gijs (he/him)]

Marco, can you confirm the current version is what you had in mind? I think it's possible for there to be valid logins that don't have an action_url, so this version also feels kludgy...

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8831623 [details]
Bug 1333734 - fix chrome password import if it contains items we can't read as a URI,

https://reviewboard.mozilla.org/r/108174/#review109268

::: browser/components/migration/ChromeProfileMigrator.js:461
(Diff revision 2)
> -        let loginInfo = {
> +          let loginInfo = {
> -          username: row.getResultByName("username_value"),
> +            username: row.getResultByName("username_value"),
> -          password: crypto.
> +            password: crypto.
> -                    decryptData(crypto.arrayToString(row.getResultByName("password_value")),
> +                      decryptData(crypto.arrayToString(row.getResultByName("password_value")),
> -                                                     null),
> +                                                       null),
> -          hostname: NetUtil.newURI(row.getResultByName("origin_url")).prePath,
> +            hostname: NetUtil.newURI(origin_url).prePath,

well, now this looks wrong, newURI of an nsIURI?

Comment 17

[Marco Bonardo [:mak]]

I missed the fact action_url may be empty.
Is there a reason to have both checks at the top? What about we only handle action_url in the only code point where it's used?

So at the beginning we do:
const kValidSchemes = new Set(["https", "http", "ftp"]);
let origin_url = NetUtil.newURI(row.getResultByName("origin_url"));
if (!kValidSchemes.has(origin_url.scheme)) {
  continue;
}

and later:

case AUTH_TYPE.SCHEME_HTML:
  let action_url = NetUtil.newURI(row.getResultByName("action_url"));
  if (!kValidSchemes.has(action_url.scheme)) {
    continue;
  }
  loginInfo.formSubmitURL = action_url.prePath;

That said, at this point sounds like a yellow bikeshed, just do what you feel better about, provided it's not bogus :)

Comment 18

[:Gijs (he/him)]

Comment on attachment 8831623 [details]
Bug 1333734 - fix chrome password import if it contains items we can't read as a URI,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108174/diff/2-3/

Comment 19

[Marco Bonardo [:mak]]

LGTM, thanks!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/104219e32a5c
fix chrome password import if it contains items we can't read as a URI, r=mak

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/104219e32a5c

Comment 22

[:Gijs (he/him)]

Comment on attachment 8831623 [details]
Bug 1333734 - fix chrome password import if it contains items we can't read as a URI,

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: on some chrome profiles, it's impossible to successfully import Chrome logins (either manually or automatically)
[Is this code covered by automated tests?]: yes, though the tests could do with expanding.
[Has the fix been verified in Nightly?]: via the automated tests, yes, but not manually.
[Needs manual test from QE? If yes, steps to reproduce]: maybe - one of the patch iterations had a mistake, and the automated tests caught it, so I'm reasonably confident that the tests are sufficient to avoid complete breakage.  To verify for sure that the issue is fixed, though, we'd need steps of how you save a password for a non-http/https/ftp form or domain in Chrome, steps that I don't have...
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: relatively small changes, has automated tests
[String changes made/needed]: nope

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8831623 [details]
Bug 1333734 - fix chrome password import if it contains items we can't read as a URI,

chrome password import fix, aurora53+, beta52+

Should be in 52.0b3.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b67691e8a2e

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f13c827cdc49