707044 - Import login data from Google Chrome on Windows

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Makoto Kato [:m_kato]]

Attachment: wip

password encryption on Chrome depends OS.  Windows seems to use crypt32 API.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: fix v1

Comment 2

[Makoto Kato [:m_kato]]

Attachment: fix v1.1

Comment 3

[Marco Bonardo [:mak]]

So, to add some background (since I needed to document myself on the thing to review), Chrome uses CryptProtectData:
http://msdn.microsoft.com/en-us/library/aa380261.aspx

The particularity of this API is that (from MSDN):

"Typically, only a user with the same logon credential as the user who encrypted the data can decrypt the data. In addition, the encryption and decryption usually must be done on the same computer."

With a couple exceptions:
- "a user with a roaming profile can decrypt the data from another computer on the network."
- "If the CRYPTPROTECT_LOCAL_MACHINE flag is set when the data is encrypted, any user on the computer where the encryption was done can decrypt the data."

The former is unlikely being hit by our users, the latter is untrue in Chrome.
So we are always in the stricter case where only the same user on the same computer can migrate.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 580321 [details] [diff] [review]
fix v1.1

Review of attachment 580321 [details] [diff] [review]:
-----------------------------------------------------------------

It looks globally OK, though I have some questions on our login types support, since this handles BASIC HTTP AUTH one way, and all the rest another, and I'm not sure the latter is correct for all the types supported by Chrome and us. We should limit import to known working types.
Plus I think we should not ignore the blacklisted_by_user field.

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +393,5 @@
> +   *
> +   * @param   aSrcData
> +   *          Encrytped password.  This is array type.
> +   */
> +  _decryptedData: function(aSrcData) {

please rename to _decryptLoginData, see below. Name the function too.

@@ +394,5 @@
> +   * @param   aSrcData
> +   *          Encrytped password.  This is array type.
> +   */
> +  _decryptedData: function(aSrcData) {
> +    // password is encrypted.  Chrome uses OS function for it if available.

merge this into the javadoc of the method.

@@ +418,5 @@
> +                                                 ctypes.voidptr_t,
> +                                                 ctypes.voidptr_t,
> +                                                 ctypes.voidptr_t,
> +                                                 ctypes.uint32_t,
> +                                                 this._DATA_BLOB.ptr);

afaik, declare()d objects keep alive the library. I think once the passwords migration is complete we should free this._DATA_BLOB, this._CryptUnprotectData and this._LocalFree. Sure when the script is unloaded gc may do that, but I think we should try to cleanup after ourselves when possible.

@@ +422,5 @@
> +                                                 this._DATA_BLOB.ptr);
> +    }
> +
> +    if (this._LocalFree == null) {
> +      kernel32 = ctypes.open("kernel32.dll");

undeclared, add let

@@ +435,5 @@
> +    let outData = new this._DATA_BLOB;
> +    let ret = this._CryptUnprotectData(inData.address(), null, null, null,
> +                                       null, 0, outData.address());
> +    if (ret == 0)
> +      throw "Cannot decrypt password :" + ret;

please throw new Components.Exception(msg, errorcode);

@@ +443,5 @@
> +    let decrypted = ctypes.cast(outData.pbData,
> +                                ctypes.uint8_t.array(len).ptr).contents;
> +    let expanded = "";
> +    for (let i = 0; i < len; i++)
> +      expanded += String.fromCharCode(decrypted[i]);

please always brace loops

@@ +447,5 @@
> +      expanded += String.fromCharCode(decrypted[i]);
> +    this._LocalFree(outData.pbData);
> +    return expanded;
> +#else
> +    throw "decrypting password isn't supported";

ditto on Components.exception

@@ +460,5 @@
> +      file.initWithPath(this._paths.loginData);
> +
> +      let dbConn = Services.storage.openUnsharedDatabase(file);
> +      let stmt = dbConn.createAsyncStatement(
> +                   "SELECT origin_url, action_url, username_element, username_value, password_element, password_value, signon_realm, scheme FROM logins");

this is crying to get some better indentation and cropping to 80 chars :)

Looks like you are ignoring the blacklisted_by_user column. That is the equivalent to "Never remember this password". We may import those using nsILoginManager.setLoginSavingEnabled, but surely we should not import those as common login data.
So either you filter on blacklisted_by_user = 0 or you properly differentiate the two cases.

@@ +470,5 @@
> +                        getService(Ci.nsILoginManager),
> +        handleResult : function(aResults) {
> +          for (let row = aResults.getNextRow(); row; row = aResults.getNextRow()) {
> +
> +            try {

please remove extraneous newline

@@ +474,5 @@
> +            try {
> +              let login = Cc["@mozilla.org/login-manager/loginInfo;1"].
> +                          createInstance(Ci.nsILoginInfo);
> +              let password =
> +                this._self._decryptedData(row.getResultByName("password_value"));

to me looks like the method is actually decrypting data and returning the value, so should probably be named _decryptData(), BUT, since there may be multiple crypting systems used for different profile parts, it should probably have a name related to the kind of data it's handling.
I'd suggest _decryptLoginData()

@@ +480,5 @@
> +              let httpRealm = null;
> +              let submitURL = null;
> +              if (row.getResultByName("scheme") == 1) {
> +                 // this is http authentication.
> +                // signon_realm format is URI+realm, so we need remove URI

please put that magic 1 into a const, then you could avoid the clarifying "this is http authentication" comment.
If I read Chrome sources correctly, scheme may be any of
  enum Scheme {
    SCHEME_HTML,
    SCHEME_BASIC,
    SCHEME_DIGEST,
    SCHEME_OTHER
  } scheme;
and the default is 0, even if I see most pages use basic auth that is 1.
Btw, we should have a fake-enum mimicking this (let AUTH = {SCHEME_HTML: 0, ...}), and ensure what we support, there is no point in importing data for auth types we don't support (like _OTHER).
I'm not sure how the login manager supports DIGEST as well, I couldn't find docs on that. Maybe ask Dolske?

@@ +484,5 @@
> +                // signon_realm format is URI+realm, so we need remove URI
> +                httpRealm = row.getResultByName("signon_realm");
> +                httpRealm = httpRealm.substring(hostName.length + 1);
> +              } else {
> +                submitURL = NetUtil.newURI(row.getResultByName("action_url")).prePath;

And related to the question above, did you test this with digest and html form authentications? I couldn't find easy test pages for these online to check db values. Surely we should add a Login Data file with one login per each combination, and check they are correctly imported or ignored.

@@ +515,5 @@
> +        handleCompletion : function(aReason) {
> +          this._db.asyncClose();
> +          this._self._notifyCompleted(Ci.nsIBrowserProfileMigrator.PASSWORDS);
> +        },
> +      });

looks like you forgot to finalize() the statement

Comment 5

[Makoto Kato [:m_kato]]

Attachment: v2

Comment 6

[Makoto Kato [:m_kato]]

- digest authentication can import using same way of http authentication
- tested on HTTP, Form and Digest.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 617404 [details] [diff] [review]
v2

Review of attachment 617404 [details] [diff] [review]:
-----------------------------------------------------------------

I will do another pass at this after a bit of reorganization of the code, though not before next week cause I'm in Toronto atm and don't have Windows here.

::: browser/components/migration/src/ChromeProfileMigrator.js
@@ +19,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/ctypes.jsm");

defineLazyModule Getter this

@@ +340,5 @@
>      }
>    }
>  }
>  
> +function GetSignonResource(aProfileFolder) {

don't have to do now, though we should probably change the structure of this migrator to be more similar to the IE and Safari ones at some point, to get some more consistency.  So rather have actual instanceable resource objects with an exists getter.

@@ +343,5 @@
>  
> +function GetSignonResource(aProfileFolder) {
> +  let loginFile = aProfileFolder.clone();
> +  loginFile.append("Login Data");
> +  if (!loginFile.exists())

may be worth to also check isReadable()

@@ +347,5 @@
> +  if (!loginFile.exists())
> +    return null;
> +
> +  return {
> +    type: Ci.nsIBrowserProfileMigrator.PASSWORDS,

Use MigrationUtils.resourceTypes.PASSWORDS

@@ +364,5 @@
> +        _CryptUnprotectData: null,
> +        _LocalFree: null,
> +        _DATA_BLOB: null,
> +
> +        _setupOSDecrypter: function() {

We should move all of this stuff to a separate object with an initialize() and finalize() methods, we have clear entry and exit points so that's easily feasible.  See the CtypesHelper and its usage in IEProfileMigrator, for example.
The resource migrator here should really be simply reading entries, passing them to an helper for conversion to our format and store them.  Then you could just have a different helper per platform (ifdefed), with the same name and exposed methods. That would make this much more readable, it's a bit hard to read this patch now.

@@ +412,5 @@
> +          const DATA_BLOB_SIZE = 12;
> +#else
> +          const DATA_BLOB_SIZE = 8;
> +#endif
> +          let encryptedData = new ctypes.ArrayType(ctypes.uint8_t)(aSrcData);

hm I was actually hoping bug 710830 was already fixed :(

@@ +419,5 @@
> +          let ret = this._CryptUnprotectData(inData.address(), null, null,
> +                                             null, null, 0, outData.address());
> +          if (ret == 0) {
> +            Components.Exception("Cannot decrypt password :" + ret,
> +                                 Cr.NS_ERROR_FAILURE);

did you actually mean to throw this?

@@ +429,5 @@
> +                                      ctypes.uint8_t.array(len).ptr).contents;
> +          let expanded = "";
> +          for (let i = 0; i < len; i++) {
> +            expanded += String.fromCharCode(decrypted[i]);
> +          }

I think something like this may work (though please double check):
let expanded = [String.fromCharCode(c) for each (c in decrypted)].join("")

@@ +434,5 @@
> +          this._LocalFree(outData.pbData);
> +          return expanded;
> +#else
> +          Components.Exception("decrypting password isn't supported",
> +                               Cr.NS_ERROR_NOT_IMPLEMENTED);

ditto on missing throw.

Though, I'm not sure why all of this is ifdefed, we can't enter this code due to ifdef in getResources

@@ +439,5 @@
> +#endif
> +        },
> +
> +        handleResult: function(aResults) {
> +          this._setupOSDecrypter();

why do we have to do this each time we get results rather than just initially? perf is not much of a problem here, if it's to lazily initialize it only if there are results, you may just go the simpler way.

@@ +446,5 @@
> +            SCHEME_HTML: 0,
> +            SCHEME_BASIC: 1,
> +            SCHEME_DIGEST: 2,
> +            SCHEME_OTHER: 3
> +          };

may be a const and moved to the global scope

@@ +479,5 @@
> +
> +              default:
> +                // not support type (SCHEME_OTHER)
> +                throw "We don't support this login data this scheme type: " +
> +                      row.getResultByName("scheme");

throw new Error

@@ +488,5 @@
> +                         httpRealm,
> +                         row.getResultByName("username_value"),
> +                         password,
> +                         row.getResultByName("username_element"),
> +                         row.getResultByName("password_element"));

may be handy to have a this._rowToLoginInfo method instead of putting everything here

Comment 8

[x.redir]

Dear Mozilla,

I don't see how this issue is not a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=589589 ? Anyway:

The day my HDD crashed a couple of years ago, I lost all my Firefox data, including bookmarks, history, passwords, form fills, settings, addons, etc. I lost YEARS of browsing data.

When I got a new HDD the day after and re-installed my system, I looked for a backup solution to avoid this kind of situation in the future. Firefox Sync didn't exist at the time. Chrome was here, offering full cloud backup and sync, and also offered me to import all my data from Firefox, including passwords (even though my Firefox profile was empty because of HDD crash).

It's been now a couple of years that I'm using Chrome, and I have cumulated hundreds of passwords saved in Chrome, including credit card numbers, passport number, addresses, extensive form fill data, usernames and passwords for PayPal, banking sites, eBay, Skype, etc. literally HUNDREDS of logins all different from another that no human being could remember.

Today I can see that Firefox has implemented a similar Sync feature as Chrome, and I would like to switch back to Firefox. How could I do that without loosing the past 2 years of browsing data?

I have found this tutorial suggesting to do the import manually by visiting each site separately on Firefox and login-in on each site and then clicking "Save password" for EACH SINGLE SITE: http://techlogon.com/2011/12/02/how-to-import-google-chrome-data-into-firefox/

This operation would take me a week of work, therefore I will remain on Chrome for the time being.

It's 2014, it's been 4 years since you have this issue open, and you just lost another user.

Here is a screenshot of the Chrome Import tool:
http://i.imgur.com/TAOW4xf.png

Comment 9

[Marco Bonardo [:mak]]

I already replied to you in bug 589589, please don't crosspost (also because this is not a message board...)

Comment 10

[Edouard Oger [:eoger]]

Attachment: bug-707044.patch

Rebased and refactored the previous patch . Also unassigning myself (I just heard :rchtara is gonna start working on this tomorrow).

Comment 11

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows

Bug 707044 - Import login data from Google Chrome on Windows

by [e-mail]
refactored rebased by [e-mail]

Comment 12

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows
Fixing decrypting password issue

Comment 13

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13627/#review12265

::: browser/components/migration/ChromeProfileMigrator.js:37
(Diff revision 1)
> +#ifdef XP_WIN
> +let CtypesHelpers = {
> +  _structs: {},
> +  _functions: {},
> +  _libs: {},
> +
> +  /**
> +   * Must be invoked once before first use of any of the provided helpers.
> +   */
> +  initialize: function CH_initialize() {
> +    this._structs.DATA_BLOB = new ctypes.StructType('DATA_BLOB', [

Can you move the ctype crypto stuff to a new module in the password manager directory (e.g. OSCrypto_win.jsm) and implement the subset of the os_crypt API[1] from chromium. Then we can have a module named OSCrypto.jsm which exports the implementation for the proper OS like how osfile.jsm[2] does. I think this will make this file easier to maintain as we won't need as many ifdef in this file.

[1] https://mxr.mozilla.org/chromium/source/src/components/os_crypt/os_crypt.h
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/osfile.jsm

::: browser/components/migration/ChromeProfileMigrator.js:446
(Diff revision 1)
> +function GetSignonResource(aProfileFolder) {

Nit: "Signon" isn't a name we or Chrome seems to use reguarly. I think we should we should use a name which aligns with the `MigrationUtils.resourceTypes` which in this case is `PASSWORDS`. I also think that as-is, it's not ideal that the function will exist on non-Windows. For now I think we can leave it here but call it `GetWindowsPasswordsResource`.

::: browser/components/migration/ChromeProfileMigrator.js:455
(Diff revision 1)
> +    migrate: function(aCallback) {

Nit: use newer method syntax without `function`

::: browser/components/migration/ChromeProfileMigrator.js:463
(Diff revision 1)
> +        _loginManager: Cc["@mozilla.org/login-manager;1"].
> +                       getService(Ci.nsILoginManager),

This is available as Services.logins

::: browser/components/migration/ChromeProfileMigrator.js:458
(Diff revision 1)
> +        SELECT origin_url, action_url, username_element, username_value,
> +        password_element, password_value, signon_realm, scheme
> +        FROM logins WHERE blacklisted_by_user = 0`);

Did you audit the other columns and the chromium code to ensure this is the correct query to use?
It seems like this is missing the date columns.

https://mxr.mozilla.org/chromium/source/src/components/password_manager/core/browser/login_database.cc#221

::: browser/components/migration/ChromeProfileMigrator.js:511
(Diff revision 1)
> +              if (!logins.some(l => login.matches(l, true))) {
> +                this._loginManager.addLogin(login);
> +              }

I think this means that we won't propagate password-only changes from Chrome to Firefox? We can handle that in a follow-up bug as we'll likely want to consider the timestamps to do proper merging and I don't want to complicate this bug more.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

https://reviewboard.mozilla.org/r/13631/#review12267

I think you should just fold this into the first patch and leave the blame with one of the previous authors. You can get the blame on the tests you write.

::: browser/components/migration/ChromeProfileMigrator.js:107
(Diff revision 1)
> +    for(let i = 0; i < decrypted.length; i++) {

Nit: missing space after `for`

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8636110 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows

https://reviewboard.mozilla.org/r/13629/#review12269

Comment 16

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: add test

add test

Comment 17

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows
tests and refactoring

Comment 18

[Riadh Chtara[:rchtara]]

Comment on attachment 8637704 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 19

[Riadh Chtara[:rchtara]]

Comment on attachment 8637703 [details]
MozReview Request: add test

add test

Comment 20

[Riadh Chtara[:rchtara]]

Comment on attachment 8637704 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 21

[Riadh Chtara[:rchtara]]

Comment on attachment 8636110 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows

Bug 707044 - Import login data from Google Chrome on Windows

Comment 22

[Riadh Chtara[:rchtara]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

https://reviewboard.mozilla.org/r/13631/#review12497

Awesome, this is very close!

::: browser/components/migration/ChromeProfileMigrator.js:36
(Diff revision 2)
> -  /**
> +Cu.import("resource://gre/modules/OSCrypto.jsm");

Nit: make this use XPCOMUtils.defineLazyModuleGetter so it doesn't get loaded if the user unchecks importing passwords.

::: browser/components/migration/ChromeProfileMigrator.js:387
(Diff revision 2)
> -        _loginManager: Cc["@mozilla.org/login-manager;1"].
> +        _loginManager: Services.logins,

We don't need an alias for one consumer :)

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:3
(Diff revision 2)
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +Cu.import("resource://gre/modules/OSCrypto.jsm");
> +let Crypto = new OS.Crypto();

Nit: We normally put the Cu.import together with a new line after instead.

::: toolkit/components/passwordmgr/OSCrypto.jsm:1
(Diff revision 2)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
> + * vim: sw=2 ts=2 sts=2 et */

Nit: don't bother with mode lines in new JS files.

::: toolkit/components/passwordmgr/OSCrypto.jsm:13
(Diff revision 2)
> +if (typeof Components != "undefined") {
> +  this.EXPORTED_SYMBOLS = ["OS"];
> +  Components.utils.import("resource://gre/modules/OSCrypto_win.jsm", this);
> +} else {

If this to handle xpcshell tests or what's the `typeof Components` stuff for?

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:1
(Diff revision 2)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
> + * vim: sw=2 ts=2 sts=2 et */

Nit: Not worth the screen real estate anymore.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:9
(Diff revision 2)
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;

Nit: use the shorter object destructuring single-line syntax.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:29
(Diff revision 2)
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");

Remove any modules you're not actually using e.g. PlacesUtils

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:16
(Diff revision 2)
> +const AUTH_TYPE = {
> +  SCHEME_HTML: 0,
> +  SCHEME_BASIC: 1,
> +  SCHEME_DIGEST: 2
> +};

I don't think this belongs here.

::: toolkit/components/passwordmgr/OSCrypto.jsm:14
(Diff revision 2)
> +  this.EXPORTED_SYMBOLS = ["OS"];

I actually wasn't thinking about using the "OS." namespace, I was thinking just "OSCrypto" to indicate that operating system crypto wrappers are here. I guess it's fine if it works but what happens if a file imports both osfile.jsm and OSCrypto.jsm? Do they conflict? I don't know what happens when trying to import a symbol that is already defined.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:46
(Diff revision 2)
> +/**
> + * Must be invoked once before first use of any of the provided helpers.
> + */
> +Crypto.prototype.initialize = function() {

What's the reason for not putting this in the constructor? Are you thinking there are times when OSCrypto is constructed but the developer doesn't want the work to initialize to be done immediately?

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:87
(Diff revision 2)
> +  } catch (ex) {
> +    this.finalize();

Add a `Cu.reportError(ex);` too so this gets logged.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:109
(Diff revision 2)
> +  decryptedData = "";

You already initialized this at declaration time.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:110
(Diff revision 2)
> +  for (let i = 0; i < decrypted.length; i++) {
> +    decryptedData += String.fromCharCode(decrypted[i]);

Can you use a for…of loop here instead?

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:151
(Diff revision 2)
> +    for each (let lib in this._libs) {
> +        try {
> +          lib.close();
> +      } catch (ex) {}

`for each` is deprecated.

Indentation also seems off.

::: toolkit/components/passwordmgr/moz.build:38
(Diff revision 2)
>  EXTRA_PP_JS_MODULES += [
>      'LoginManagerParent.jsm',
> +    'OSCrypto.jsm',
> +    'OSCrypto_win.jsm',

The "PP" here referes to the preprocessor but OSCrypto_win.jsm doesn't use preprocessor instructions so this should move to EXTRA_JS_MODULES for faster builds since symlinks can be used instead.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:50
(Diff revision 2)
> +  login1 = {

Please explcitly declare all variables and move these logins to the top as `const` so it can be used for other task functions in the file.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:77
(Diff revision 2)
> +  let loginFile = Cc["@mozilla.org/file/local;1"]
> +                  .createInstance(Ci.nsILocalFile);

dead code

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:79
(Diff revision 2)
> +  dbConn = Services.storage.openUnsharedDatabase(do_get_file("Library/Google/Chrome/User Data/Default/Login Data"));

Move this inside the test function below and explicitly declare it with `let`.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:9
(Diff revision 2)
> +function promiseSetPassword(login) {
> +  return new Promise((resolve, reject) => {
> +
> +  let stmt = dbConn.createAsyncStatement(`UPDATE logins SET password_value = :password_value WHERE rowid = :rowid`);

Indentation is incorrect here.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:8
(Diff revision 2)
> +
> +function promiseSetPassword(login) {

Maybe pass the dbConn as an argument so this function doesn't reply on globals.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:117
(Diff revision 2)
> +  do_register_cleanup(() => {
> +    Services.logins.removeAllLogins();
> +    dbConn.asyncClose();
> +    Crypto.finalize();
> +  });

The cleanup function should be registered earlier in the test (as soon as possible) so it will be registered before any uncaught exception happens.

Comment 24

[Riadh Chtara[:rchtara]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 25

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/13631/#review12497

> Can you use a for…of loop here instead?

decrypted is not really an array. so it didnt work

> I actually wasn't thinking about using the "OS." namespace, I was thinking just "OSCrypto" to indicate that operating system crypto wrappers are here. I guess it's fine if it works but what happens if a file imports both osfile.jsm and OSCrypto.jsm? Do they conflict? I don't know what happens when trying to import a symbol that is already defined.

I think it worked, I try firefox with it and even import passwords without issues.But  to be safe, i changed.

> If this to handle xpcshell tests or what's the `typeof Components` stuff for?

I thought its a magical thing that needs to be done (it s iin osfile.jsm) and didnt want to removed because I was afraid it s imporatant.
but it s probabaly not.

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

https://reviewboard.mozilla.org/r/13631/#review12589

::: browser/components/migration/ChromeProfileMigrator.js:430
(Diff revisions 2 - 3)
> +              login.timeCreated = loginInfo.timeCreated;

AFAICT, date_created is also updated in Chromium's UpdateLogin so I think it also makes sense to assign this to login.timePasswordChanged.

::: browser/components/migration/ChromeProfileMigrator.js:433
(Diff revisions 2 - 3)
>                // Add the login only if there's not existing entry

Nit: "not an existing entry"

::: browser/components/migration/ChromeProfileMigrator.js:437
(Diff revisions 2 - 3)
>                if (!logins.some(l => login.matches(l, true))) {
> -                this._loginManager.addLogin(login);
> +                Services.logins.addLogin(login);
>                }

Please remember to file the updateLogin bug and make it block this one.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:1
(Diff revisions 2 - 3)
> -let { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +Cu.import("resource://gre/modules/OSCrypto.jsm");
> -
>  Cu.import("resource://gre/modules/Services.jsm");

Nit: alphabetical

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:90
(Diff revisions 2 - 3)
> +function run_test() {
> +  Crypto = new OSCrypto();
>    run_next_test();
> -
>  }

None of this is needed. You already have `let Crypto = new OSCrypto();` above which is fine.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:95
(Diff revisions 2 - 3)
>  add_task(function* () {

You still need to name this function to describe the test e.g. test_…

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:7
(Diff revisions 2 - 3)
> -const Cc = Components.classes;
> +let { utils: Cu } = Components;

Include all 4 aliases anyways just to make it easier to update. Use the most common ordering.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:60
(Diff revisions 2 - 3)
>    } catch (ex) {
> +    Cu.reportError(ex);
>      this.finalize();
>    }

Actually, I think you should re-throw after finalizing instead of Cu.reportError now.

`throw ex;`

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:129
(Diff revisions 2 - 3)
>  });

Can you add a test that existing logins for the same primary key don't get overwritten?

::: toolkit/components/passwordmgr/OSCrypto.jsm:14
(Diff revisions 2 - 3)
> +#else
> +let OSCrypto = {};
>  #endif

Maybe `throw new Error("OSCrypto hasn't been implemented for this operating system");` instead?

Comment 27

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12595

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:23
(Diff revision 3)
> +    this._structs.DATA_BLOB = new ctypes.StructType('DATA_BLOB', [

Nit: wrong indentation and double-quotes please.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:32
(Diff revision 3)
> +      this._libs.crypt32.declare("CryptProtectData",
> +                            ctypes.winapi_abi,
> +                            ctypes.uint32_t,
> +                            this._structs.DATA_BLOB.ptr,
> +                            ctypes.voidptr_t,
> +                            ctypes.voidptr_t,
> +                            ctypes.voidptr_t,
> +                            ctypes.voidptr_t,
> +                            ctypes.uint32_t,
> +                            this._structs.DATA_BLOB.ptr);

Nit: indentation

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:20
(Diff revision 3)
> +  this._structs = {};
> +  this._functions = {};
> +  this._libs = {};

Could you also switch these to `Map`s? We generally use `Map`s instead of object for mapping names to values now.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:127
(Diff revision 3)
> +    for (let lib in this._libs) {
> +      try {
> +        lib.close();
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +      }
> +    }

This loop doesn't work but it will be easier with `for…of` after the `Map` conversion.

Comment 28

[Riadh Chtara[:rchtara]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 29

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/13631/#review12589

> Maybe `throw new Error("OSCrypto hasn't been implemented for this operating system");` instead?

in the current situation, doing that is going to throw an exception on linux

Comment 30

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/13631/#review12595

> Could you also switch these to `Map`s? We generally use `Map`s instead of object for mapping names to values now.

its not possible to put a contractor in  a map, so for _structs I will keep it as it was

Comment 31

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12589

> in the current situation, doing that is going to throw an exception on linux

That's good. Any code that is currently using OSCrypto outside of Windows should be changed to not use it.

Comment 32

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12595

> its not possible to put a contractor in  a map, so for _structs I will keep it as it was

Where do you get the error? "Any value (both objects and primitive values) may be used as either a key or a value." https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

https://reviewboard.mozilla.org/r/13631/#review12703

::: browser/components/migration/ChromeProfileMigrator.js:397
(Diff revisions 3 - 4)
> +            timePasswordChanged: row.getResultByName("date_created") + 0,

Remove this since it adds confusion by mislabelling a value.

::: browser/components/migration/ChromeProfileMigrator.js:431
(Diff revisions 3 - 4)
>                login.timeCreated = loginInfo.timeCreated;
> +              login.timePasswordChanged = loginInfo.timePasswordChanged;
>                login.timesUsed = loginInfo.timesUsed;

What happens if timeLastUsed isn't specified and the user looks in the password manager list? If we don't already have a fallback I think we may want to use timeCreated as well.
login.timePasswordChanged = loginInfo.timeCreated;
login.timeLastUsed = loginInfo.timeCreated;

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:82
(Diff revisions 3 - 4)
>    Assert.equal(passwordManagerLogin.hostname, chromeLogin.hostname,
> -               "The two logins N " + chromeLogin.id + " have the same hostname");
> +               "The two logins N " + id + " have the same hostname");

Test that the submitURL is properly converted to an origin

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:97
(Diff revisions 3 - 4)
> +function generateDiffrentLogin(login) {

Typo in "Different"

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:149
(Diff revisions 3 - 4)
> +  // Test that existing logins for the same primary key don't get overwritten
> +  Services.logins.removeAllLogins();
> +  logins = Services.logins.getAllLogins({});
> +  Assert.equal(logins.length, 0,
> +               "There are no logins after removing all of them");
> +
> +  let newLogins = [];

This should start a new test task.

`add_task(function* test_importExistingLogin() {`

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:114
(Diff revisions 3 - 4)
> -add_task(function* () {
> +add_task(function* test_Chrome_passwords() {

Other test functions should be added for specific tasks so the task should describe at a high-level what is being tested. The file name already indicates the information you specified.

`add_task(function* test_importIntoEmptyDB() {`

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:110
(Diff revisions 3 - 4)
>  function run_test() {
> -  Crypto = new OSCrypto();
>    run_next_test();
>  }

Please delete this whole function like I said last time. It shouldn't be necessary but if you think it is, please explain why and leave the issue open.

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:162
(Diff revisions 3 - 4)
> +  for (let i = 0; i < newLogins.length; i++) {
> +    Services.logins.addLogin(newLogins[i]);

Nit: for…of

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:157
(Diff revisions 3 - 4)
> +  // Create a new logins that are different but where the key properties are still the same

Nit: "Create new logins" (remove " a". Also add a period to the comment.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:128
(Diff revisions 3 - 4)
> -    for (let lib in this._libs) {
> +    for (let lib in this._libs.values()) {

replace "in" with "of"

Comment 34

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12707

::: toolkit/components/passwordmgr/OSCrypto.jsm:13
(Diff revision 4)
> +Components.utils.import("resource://gre/modules/OSCrypto_win.jsm", this);

Why did you change this from importScript…? This way uses more memory IIUC since it create a separate compartment.

Comment 35

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12711

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:25
(Diff revision 4)
> +  id: 2,// id of the row in the chrome login db

Nit: space space after comma

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:38
(Diff revision 4)
> +  id: 3,// id of the row in the chrome login db

Nit: space space after comma

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:52
(Diff revision 4)
> +    let stmt = dbConn.createAsyncStatement(`UPDATE logins SET password_value = :password_value WHERE rowid = :rowid`);

This line is over 100 characters. Please wrap it:
…(`
UPDATE logins
SET password_value = :password_value
WHERE rowid = :rowid`);

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:4
(Diff revision 4)
> +let Crypto = new OSCrypto();

Nit: Crypto should be camel case: crypto

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:66
(Diff revision 4)
> +          reject("satement has failed: " + aReason);

"Query has failed: "

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:59
(Diff revision 4)
> +        reject("error in the statement: "  + aError.message);

"Error with the query: "

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:74
(Diff revision 4)
> +function checkLoginsAreEqual(passwordManagerLogin, chromeLogin, id) {
> +
> +  passwordManagerLogin.QueryInterface(Ci.nsILoginMetaInfo);

Nit: delete the extra new line

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:93
(Diff revision 4)
> +               "The two logins N " + id + " have the same timesUsed");
> +
> +}

Nit: delete the extra new line

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:79
(Diff revision 4)
> +               "The two logins N " + id + " have the same username");
> +  Assert.equal(passwordManagerLogin.password, chromeLogin.password,
> +               "The two logins N " + id + " have the same password");

s/ N / with ID /

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:11
(Diff revision 4)
> +const login1 = {
> +  id: 1, // id of the row in the chrome login db
> +  username: "username",

You should have test logins for HTTP authentication (both basic and digest).

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:115
(Diff revision 4)
> +  let dbConn = Services.storage.openUnsharedDatabase(do_get_file("Library/Google/Chrome/User Data/Default/Login Data"));

Move this file to "AppData\Local\Google\Chrome\User Data\Default\Login Data"

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:127
(Diff revision 4)
> +  registerFakePath("LocalAppData",  do_get_file("Library/"));

This is mixing Windows with OS X conventions. Use "AppData/Local",

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:117
(Diff revision 4)
> +  do_register_cleanup(() => {
> +    Services.logins.removeAllLogins();
> +    dbConn.asyncClose();
> +    Crypto.finalize();
> +  });

Move this along with the line `registerFakePath("LocalAppData",  do_get_file("Library/"));` to a new task that comes before this test task which does the setup. That keeps the generic chrome migrator password setup separate from the subtests.

`add_task(function* setup() {
…
})`

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:182
(Diff revision 4)
> +  }
> +
> +});

Nit: extra new line

::: browser/components/migration/tests/unit/test_Chrome_passwords.js:58
(Diff revision 4)
> +      handleError: function(aError) {
> +        reject("error in the statement: "  + aError.message);
> +      },
> +
> +      handleCompletion: function(aReason) {

Nit: new method syntax

Comment 36

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12719

Almost done my review. I just need to audit all of the system calls now.

::: toolkit/components/passwordmgr/OSCrypto.jsm:12
(Diff revision 4)
> +#ifdef XP_WIN
> +Components.utils.import("resource://gre/modules/OSCrypto_win.jsm", this);
> +#else
> +let OSCrypto = {};
> +#endif

I forgot about this before but we're trying to avoid pre-processing for simple if-defs nowadsys since the preprocessor slows down builds. Can you replace the ifdefs with checks using AppConstants.jsm (you will need to import it).

::: toolkit/components/passwordmgr/moz.build:39
(Diff revision 4)
>      'LoginManagerParent.jsm',
> +    'OSCrypto.jsm',
>  ]

Once you switch to AppConstants.jsm, you can move this to EXTRA_JS_MODULES below.

::: toolkit/components/passwordmgr/moz.build:49
(Diff revision 4)
> +    'OSCrypto_win.jsm',

This should move to a separate EXTRA_JS_MODULES section so it only gets packaged on Windows:
`
if CONFIG['OS_TARGET'] == 'WINNT':
    EXTRA_JS_MODULES += [
        'OSCrypto_win.jsm',
    ]
`

Put that by the similar check for Android.

::: toolkit/components/passwordmgr/moz.build:45
(Diff revision 4)
>      'LoginDoorhangers.jsm',

While you're here, could you move the `LoginDoorhangers.jsm` line to the else section of the Android condition so it doesn't get packaged on Android.

::: browser/components/migration/ChromeProfileMigrator.js:37
(Diff revision 4)
> -#ifdef XP_WIN
> +let Crypto;

Why is this a global? Move it into the password function

::: browser/components/migration/ChromeProfileMigrator.js:419
(Diff revision 4)
> -        handleResult: function(aResults) {
> -          CtypesHelpers.initialize();
> +        handleResult(aResults) {
> +          Crypto = new OSCrypto();
>  

handleResult: "Generally, this method will be called several times, each time providing one or more results"
We should finalize in handleCompletion and construct this before executeAsync to avoid repeated construction.

See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIStorageStatementCallback#eatCookie.28.29

::: browser/components/migration/ChromeProfileMigrator.js:436
(Diff revision 4)
> -              let logins = this._loginManager.findLogins({}, login.hostname,
> +              let logins = Services.logins.findLogins({}, login.hostname,
>                                                           login.formSubmitURL,
>                                                           login.httpRealm);

Fix indentation

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:12
(Diff revision 4)
> +XPCOMUtils.defineLazyModuleGetter(this, "ctypes",
> +                                  "resource://gre/modules/ctypes.jsm");

Nit: This fits on one line  < 100 characters

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:125
(Diff revision 4)
> + OSCrypto.prototype.finalize = function() {

Nit: extra space at the beginning of the line

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:68
(Diff revision 4)
> +OSCrypto.prototype.decryptData = function(data) {

Add JSDoc for the encrypt and decrypt methods with the arguments and return values including types (in {…}). See http://usejsdoc.org/tags-param.html#names-types-and-descriptions

Comment 37

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12721

More style nits as I review the ctypes & library usage.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:30
(Diff revision 4)
> +    this._libs.set("kernel32", ctypes.open("Kernel32"));

Move this line just before the `this._functions.set("_LocalFree",` line to keep the libraries together.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:125
(Diff revision 4)
> + OSCrypto.prototype.finalize = function() {
> +    this._structs = {};
> +    this._functions.clear();
> +    for (let lib in this._libs.values()) {
> +      try {
> +        lib.close();
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +      }
> +    }
> +    this._libs.clear();
> +};

Indentation is incorrect.

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:120
(Diff revision 4)
> +}

Nit: missing semicolon

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:90
(Diff revision 4)
> +}

Nit: missing semicolon

Comment 38

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13631/#review12731

I've thoroughly reviewed everything now so I don't expect much more after this :). Please include a try push with your updated patch:
try: -b do -p linux,linux64,linux64-asan,linux64-valgrind,linux64-br-haz,macosx64,win32 -u xpcshell,mochitest-1,mochitest-bc,mochitest-o,mochitest-e10s-2,mochitest-e10s-browser-chrome-1 -t none

Thank you!

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:76
(Diff revision 4)
> +  if (status === 0) {
> +    return decryptedData;
> +  }

It seems like a bad idea to silently fail like this. I think we should throw and include the status in the message:
`
throw new Error("decryptData failed: " + status);
`

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:68
(Diff revision 4)
> +OSCrypto.prototype.decryptData = function(data) {

Why didn't use the decryptString and encryptString names?

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:105
(Diff revision 4)
> +  if (status === 0) {
> +    return encryptedData;
> +  }

throw here too

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:74
(Diff revision 4)
> +                                                          null,null, null, 0,

space between arguments

::: toolkit/components/passwordmgr/OSCrypto_win.jsm:32
(Diff revision 4)
> +    this._functions.set("_CryptProtectData",

There's no need for the underscore prefix on the functions in the Map.

Comment 39

[Riadh Chtara[:rchtara]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f2ddd70f50a

Comment 40

[Riadh Chtara[:rchtara]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 41

[Riadh Chtara[:rchtara]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 42

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/13631/#review12707

importscripts is not defined now.
so i think i will use the other one

> Why did you change this from importScript…? This way uses more memory IIUC since it create a separate compartment.

I thought that Components.utils.import is faster, because importScript could import many script.

Comment 43

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/13631/#review12497

> Maybe pass the dbConn as an argument so this function doesn't reply on globals.

dbConn needed in multiple task  now

Comment 44

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/13631/#review12497

Comment 45

[Riadh Chtara[:rchtara]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

Comment 46

[Matthew N. [:MattN]]

Comment on attachment 8636111 [details]
MozReview Request: Bug 707044 - Import login data from Google Chrome on Windows: tests and refactoring

https://reviewboard.mozilla.org/r/13631/#review12853

Great job! Thank you for your patience on your first large patch.

I manually tested with form, basic auth, & digest auth ensured existing logins don't get overwritten or cause errors.

I split the patch like I mentioned in earlier review commetns so Makoto keeps the blame for the migrator and you get the blame for OSCrypto and tests.

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4bf5a310be1c
https://hg.mozilla.org/integration/fx-team/rev/f3d5da775d2b
https://hg.mozilla.org/integration/fx-team/rev/3e589989f51a

Comment 48

[Riadh Chtara[:rchtara]]

Great
thanks a lot matt for your patience  doing the reviews, providing me with an access to your windows pc and setting up everything there for me, answering my questions, doing the manual tests..

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bf5a310be1c
https://hg.mozilla.org/mozilla-central/rev/f3d5da775d2b
https://hg.mozilla.org/mozilla-central/rev/3e589989f51a

Comment 50

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 42.0a1 (2015-07-01) under Windows 7 64-bit.

Verified on Firefox 42 beta 1 (20150921151815) on Windows 7 64-bit, navigating to several popular webpages like: Facebook, Twitter, Gmail, Yahoo, Amazon, Ebay, LinkedIn, Instagram, Pinterest.

I have noticed that for some of the above mentioned sites the login credentials are not imported. Encountered this on Gmail, Twitter, Amazon, Instagram and Ebay.

Is this an issue that need to be filed or the login data of this sites can not be imported for other reasons?

Comment 51

[Vasilica Tamas, QA [:vtamas]]

Tested again on Firefox 42 beta 7 (20151015151621) under Windows 10 64-bit and Windows 7 64-bit. All saved password were successfully imported this time.

Based on this testing I am marking this bug as Verified.