1192035 - Import passwords from Microsoft Edge / Windows 8+

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P1)

Description

[Justin Dolske [:Dolske]]

We need a migrator that can import a user's existing Edge passwords.

Comment 1

[Matthew N. [:MattN]]

On my Windows 8 and Windows 10 machine, web credentials are now stored in the system Credential Manager (found in Control Panel).
It looks like we need to use CredEnumerate[1] and CredRead[2] to access these credentials. The enumerator is nice in that we no longer have to brute force hashes using history entries like in bug 682069.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa374794%28v=vs.85%29.aspx
[2] https://msdn.microsoft.com/en-us/library/windows/desktop/aa374804%28v=vs.85%29.aspx

Comment 2

[Edwin Wong [:edwong]]

From speaking with Sheila (Dolske/Mattn indirectly) this is a high priority for Win10 work, can we set this as high priority for 43.1

Comment 3

[Riadh Chtara[:rchtara]]

The passwords are stored for 8.1 in Windows.Security.Credentials.PasswordVault. They could be access via
RetrieveAll (it's probably the same for win 8/10, but didnt test it)
The  CredEnumerate and CredRead dont work.

Comment 4

[Riadh Chtara[:rchtara]]

Attachment: import8.js

Comment 5

[Riadh Chtara[:rchtara]]

Hi Edwin,
I have added the code that needs to be tested.
To do tests save some passwords with ie/edge on windows 8, 8.1, 10  (8 is important, I dont have it)
open the scratchpad:
if it's the first time you use the browser tools:
* right click in any part of the current page, click on inspect element.
* click on toolbox options icons
* activate enable remote debugging/enable browser chrome and addon debugging

Then open the browser toolbox:
* hamburger menu -> developer -> browser toolbox

Then click on the scratchpad, open the file, on click on run 
Go to the console and check if there is all the passwords/usernames/urls there.

if there is an issue please let me know.
thanks

Comment 6

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 7

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 8

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 9

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 10

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

https://reviewboard.mozilla.org/r/17063/#review15509

Alright! This is impressive - I had to deal with some native APIs for the Edge reading list documentation, and I ended up just writing a native component. When I got stuck and tried ctypes, I found it tricky to get right. This looks neat!

I do have a number of comments. Can we maybe meet sometime tomorrow morning (US time) on vidyo to discuss the patch?

::: browser/components/migration/IEProfileMigrator.js:31
(Diff revision 3)
> +let CtypesHelpers = MSMigrationUtils.CtypesHelpers;
>  
> -let CtypesHelpers = {

Sorry, I landed before you here (with bug 1192036) and you will need to rebase this - the ctypes helpers are already in MSMigrationUtils.jsm by now, though they are still a singleton, so you'll need to update that still.

::: browser/components/migration/IEProfileMigrator.js:422
(Diff revision 3)
> +  , MSMigrationUtils.getWin8PasswordsMigrator()

The code in the migrator implies that this should work for Edge, but you haven't added the migrator to the Edge migrator file. Is there a particular reason for this?

::: browser/components/migration/MSMigrationUtils.jsm:29
(Diff revision 3)
> +const INTERNET_EXPLORER_EDEGE_GUID = [1020089497,

Nit: typo in the variable name

::: browser/components/migration/MSMigrationUtils.jsm:29
(Diff revision 3)
> +const INTERNET_EXPLORER_EDEGE_GUID = [1020089497,
> +                                      1259374504,
> +                                      2287998370,
> +                                      1429986696];

Have you verified this has remained the same across different versions of IE / Edge / Windows in any way? Or have you only tested on Windows 8?

::: browser/components/migration/MSMigrationUtils.jsm:112
(Diff revision 3)
> +    this._libs.vaultcli = ctypes.open("vaultcli.dll");
> +    this._libs.kernel32 = ctypes.open("Kernel32");

Maybe we should let people constructing instances specify which libraries they need? That way we can avoid the cookie migrator loading vaultcli.dll and declaring all these functions that it'll never use. Either that, or make all of these lazy, which would be neat, but might be harder to implement.

::: browser/components/migration/MSMigrationUtils.jsm:121
(Diff revision 3)
> +    this._functions.VaultEnumerateVaults =
> +      this._libs.vaultcli.declare("VaultEnumerateVaults",
> +                                  ctypes.winapi_abi,
> +                                  wintypes.DWORD,
> +                                  wintypes.DWORD,
> +                                  wintypes.PDWORD,
> +                                 this._structs.GUID.ptr.ptr);

Considering there is no documentation for this on MSDN that I can find, it seems like it would be useful to add some here - what are the names/meanings of these arguments?

::: browser/components/migration/MSMigrationUtils.jsm:229
(Diff revision 3)
> +  addLogin(login, existingLogins) {

I would just add this on Win8Passwords. Alternatively, because you foresee reuse (and so do I), you could stick it in MigrationUtils.jsm . Then we can reuse it from the Chrome and other migrators if/where appropriate - it's not tied to the migrated browser being made by Microsoft. :-)

::: browser/components/migration/MSMigrationUtils.jsm:230
(Diff revision 3)
> +    let isNew = true; // the login is newer then the existing one

You could get rid of this boolean tracker and instead return early as soon as you find an existing login that has a different password and whose password was changed at a later time than the one you're adding.

::: browser/components/migration/MSMigrationUtils.jsm:232
(Diff revision 3)
> +      if (login.username == existingLogin.username && login.password != existing.password) {
> +        // if a login with the same username and different password already exists and it's older
> +        // than the current one, that login needs to be removed and the current one needs to be
> +        // added

Seems like we might need to update some of the metadata for the login even if the passwords are the same, right?

::: browser/components/migration/MSMigrationUtils.jsm:236
(Diff revision 3)
> +        if (login.timePasswordChanged > existingLogin.timePasswordChanged) {
> +          // the current login timestamps should be updated
> +          login.timeCreated = existingLogin.timeCreated;
> +          login.timeLastUsed = existingLogin.timeLastUsed;
> +          Services.logins.removeLogin(existingLogin);

This is confusing. It's essentially saying that if the new login had its password changed after the old one (so the old one needs to be updated), then... we update the timeCreated and timeLastUsed on the new one (that we'll add instead of the old one. That really only makes sense if the former (timeCreated) is older (on existingLogin) and the latter (timeLastUsed) is newer.

::: browser/components/migration/MSMigrationUtils.jsm:390
(Diff revision 3)
> -        Components.utils.reportError("Unable to import " + this.importedAppLabel + " favorite (" + entry.leafName + "): " + ex);
> +        Components.utils.reportError("Unable to import " + this.impoApprtedAppLabel + " favorite (" + entry.leafName + "): " + ex);

What happened here?

::: browser/components/migration/MSMigrationUtils.jsm:404
(Diff revision 3)
> +    if (AppConstants.isPlatformAndVersionAtLeast("win", "6.1")) {
> +      return true;

Generally, migrators are supposed to indicate whether they have data available as well. So the other migrators return false here if e.g. there isn't a favorites or cookie folder.

Is there another check we can use here to check if the vault even exists and/or is in use, rather than always returning true when we're on the right version of Windows?


Also note that this check will return true on Windows 7 (6.1) as well as Windows 8 (6.2), 8.1 (6.3) and 10 (10.0, but 6.4 on older insider/beta builds, IIRC) -- so it's not quite right as written.

::: browser/components/migration/MSMigrationUtils.jsm:412
(Diff revision 3)
> +    function _isIEOrEdgePassowd(id) {
> +      return id[0] == INTERNET_EXPLORER_EDEGE_GUID[0] &&
> +             id[1] == INTERNET_EXPLORER_EDEGE_GUID[1] &&
> +             id[2] == INTERNET_EXPLORER_EDEGE_GUID[2] &&
> +             id[3] == INTERNET_EXPLORER_EDEGE_GUID[3];
> +    }

So, I take it there is no possibility to distinguish between IE and Edge passwords? Are those currently shared between the two browsers on Windows 10 as well? That is, if I save a password in Edge, I can use it in IE and vice versa? That's quite interesting if so... if not, we should make sure passwords from either IE or Edge pass this check.

::: browser/components/migration/MSMigrationUtils.jsm:431
(Diff revision 3)
> +        throw "Unable to enumerate Vaults: " + error;

Nit: please use new Error(...);

::: browser/components/migration/MSMigrationUtils.jsm:439
(Diff revision 3)
> +        if (error != ERROR_SUCCESS) {
> +          throw "Unable to open Vault: " + error;

Do we need to bail out (and claim the migration has failed) if any of the vaults fail to open? That doesn't seem right.

::: browser/components/migration/MSMigrationUtils.jsm:458
(Diff revision 3)
> +          let url = item.contents.pResourceElement.contents.itemValue.readString();

Can any of these ever be null (ISTR .contents means we're reading the contents of a pointer, is that right?) ? If so, what happens? I'm kinda hoping ctypes makes a nullptr read less broken, but it still doesn't sound like fun...

::: browser/components/migration/MSMigrationUtils.jsm:468
(Diff revision 3)
> +          if (error != ERROR_SUCCESS) {
> +            throw "Unable to get item: " + error;
> +          }

Likewise, failing to read 1 item doesn't seem like it should break off the migration entirely.

I'm also wondering if we know anything about the errors this returns? It seems like none of these APIs are documented on MSDN, which is sad, but means we're a little in the dark about how this is going to behave. I think ideally we would like to import as much as possible, without stumbling as soon as something breaks. On the other hand, if the error indicates some fundamental issue with the call we're making, maybe continuing with the next item would make things worse? Have you actually encountered non-success status codes while testing?

::: browser/components/migration/MSMigrationUtils.jsm:471
(Diff revision 3)
> +          // the current password

Nit: this comment, and some of the previous one ("the username", "url where the login has occur") don't seem super-useful.

::: browser/components/migration/MSMigrationUtils.jsm:485
(Diff revision 3)
> +          let logins = Services.logins.findLogins({}, login.hostname,
> +                                                  login.formSubmitURL,
> +                                                  login.httpRealm);
> +          // Add the login only if it doesn't already exist
> +          // if the login is not already available, it s going to be added or merged with other
> +          // logins
> +          if (!logins.some(l => login.matches(l, true))) {
> +            let existingLogins = Services.logins.findLogins({}, login.hostname, "", null);

I'm not sure why you need two separate calls to findLogins here - isn't the first set of logins going to be the relevant subset of the former? Or are we on-purpose ignoring the httpRealm and formSubmitURL in the second call? If so, the reason for that should be documented.

Code-structure-wise, it seems all of this information could live in the addLogin method.

::: browser/components/migration/MSMigrationUtils.jsm:495
(Diff revision 3)
> +          // close current item
> +          if (!ctypesHelpers._functions.VaultFree(credential)) {
> +            throw "Unable to free item: " + error;

This doesn't work - you're not updating |error|, so the error message will be wrong.

::: browser/components/migration/MSMigrationUtils.jsm:503
(Diff revision 3)
> +        if (!ctypesHelpers._functions.VaultCloseVault(vault)) {
> +          throw "Unable to close vault: " + error;
> +        }

The same.

::: browser/components/migration/MSMigrationUtils.jsm:510
(Diff revision 3)
> +      Cu.reportError(e);
> +      ctypesHelpers.finalize();
> +      aCallback(false);
> +      return;
> +    }
> +    ctypesHelpers.finalize();
> +    aCallback(true);

This seems like code duplication that could be cleaned up with a |finally| clause.

::: browser/components/migration/MSMigrationUtils.jsm:531
(Diff revision 3)
> +  addLogin: PasswordHelpers.addLogin,

I don't follow why you're forwarding this onto MSMigrationUtils. It doesn't seem to be used?

Comment 12

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review15509

> Sorry, I landed before you here (with bug 1192036) and you will need to rebase this - the ctypes helpers are already in MSMigrationUtils.jsm by now, though they are still a singleton, so you'll need to update that still.

it's my fault I should have seen it and update my repo :)

> The code in the migrator implies that this should work for Edge, but you haven't added the migrator to the Edge migrator file. Is there a particular reason for this?

Yes, because they store there passwords in the same way, so the the win8 migrator can get edge password. but I will rename to something more clear

> Have you verified this has remained the same across different versions of IE / Edge / Windows in any way? Or have you only tested on Windows 8?

Yes, I tried that with windows 8, different 8.1, 10 and it s the same.

> Considering there is no documentation for this on MSDN that I can find, it seems like it would be useful to add some here - what are the names/meanings of these arguments?

unfortunately, for most of the arguments I don't know exactly what are they doing . As you said there is no documentation from msdn for that.
There is only one good resource about them:
The c++ example proposed here  http://insecurety.net/?p=933
It's reverse engineering  not a full documentation

> I would just add this on Win8Passwords. Alternatively, because you foresee reuse (and so do I), you could stick it in MigrationUtils.jsm . Then we can reuse it from the Chrome and other migrators if/where appropriate - it's not tied to the migrated browser being made by Microsoft. :-)

yes, you are right, it's going to be used in chrome too, I didnt know where to add it, but MigrationUtils.jsm is the prefect place for it

> Seems like we might need to update some of the metadata for the login even if the passwords are the same, right?

You are right, we have a bug  (Bug 1187190)
So this function is trying to partially fix that.
The main purpose of this function is to fix the issue when two logins have the same host and different urls (which can happens as in IE the logins is identified with  the whole address while in firefox we use only the hostname)
So in this case: if there two logins with same user name and different passwords we should keep the most recent one.

> So, I take it there is no possibility to distinguish between IE and Edge passwords? Are those currently shared between the two browsers on Windows 10 as well? That is, if I save a password in Edge, I can use it in IE and vice versa? That's quite interesting if so... if not, we should make sure passwords from either IE or Edge pass this check.

Yes you are right about out that.There are no difference between how ie in windows 8 and edge stores there passwords.
I'm not sure about the ability to use edge passwords in ie and ie passwords in edge, but I tried to import passwords from ie and from edge and both of them worked fine.

> I don't follow why you're forwarding this onto MSMigrationUtils. It doesn't seem to be used?

it s going to be used there
I expect that the following patch
https://reviewboard.mozilla.org/r/15073 to land before this one, but it didn't as I m still waiting for reviews.

Comment 13

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review15509

Thanks a lot
I totally agree, ctypes made me crazy too, every function or structure declaration and function call was a fight :)
Yes of course, we can meet.

Comment 14

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 15

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: rebase

rebase

Comment 16

[Riadh Chtara[:rchtara]]

Attachment: MozReview Request: fix issues 0

fix issues 0

Comment 17

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review15509

> Do we need to bail out (and claim the migration has failed) if any of the vaults fail to open? That doesn't seem right.

dummmy comment

Comment 18

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review15821

::: browser/components/migration/MSMigrationUtils.jsm:236
(Diff revision 3)
> +        if (login.timePasswordChanged > existingLogin.timePasswordChanged) {
> +          // the current login timestamps should be updated
> +          login.timeCreated = existingLogin.timeCreated;
> +          login.timeLastUsed = existingLogin.timeLastUsed;
> +          Services.logins.removeLogin(existingLogin);

I think there are many time stamps and many right ways to implement addLogin using these timestamps:
If the new login was changed after the old one, it means that the new one has the most recent password and which needs to be kept. So we have to update the other one  (which is done by removing and adding)

Comment 19

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review15509

> This is confusing. It's essentially saying that if the new login had its password changed after the old one (so the old one needs to be updated), then... we update the timeCreated and timeLastUsed on the new one (that we'll add instead of the old one. That really only makes sense if the former (timeCreated) is older (on existingLogin) and the latter (timeLastUsed) is newer.

I think there are many time stamps and many right ways to implement addLogin using these timestamps:
If the new login was changed after the old one, it means that the new one has the most recent password and which needs to be kept. So we have to update the other one  (which is done by removing and adding)

> Can any of these ever be null (ISTR .contents means we're reading the contents of a pointer, is that right?) ? If so, what happens? I'm kinda hoping ctypes makes a nullptr read less broken, but it still doesn't sound like fun...

good catch :), there is just an exception thrown and it's going to be reported by Cu.reportError(e);
So I dont think we should worry about it

> Likewise, failing to read 1 item doesn't seem like it should break off the migration entirely.
> 
> I'm also wondering if we know anything about the errors this returns? It seems like none of these APIs are documented on MSDN, which is sad, but means we're a little in the dark about how this is going to behave. I think ideally we would like to import as much as possible, without stumbling as soon as something breaks. On the other hand, if the error indicates some fundamental issue with the call we're making, maybe continuing with the next item would make things worse? Have you actually encountered non-success status codes while testing?

unfortunately the returning codes are not documented.

> I'm not sure why you need two separate calls to findLogins here - isn't the first set of logins going to be the relevant subset of the former? Or are we on-purpose ignoring the httpRealm and formSubmitURL in the second call? If so, the reason for that should be documented.
> 
> Code-structure-wise, it seems all of this information could live in the addLogin method.

there the same good catch

Comment 20

[Riadh Chtara[:rchtara]]

Comment on attachment 8654174 [details]
MozReview Request: fix issues 0

fix issues 0

Comment 21

[Justin Dolske [:Dolske]]

https://reviewboard.mozilla.org/r/17545/#review15833

Quick driveby review... But is there a way to get a single, complete diff of all 3 changesets you've got listed in this bug? Maybe I'm missing some reviewboard feature, but it's hard to review when code is spread and changed across 3 places.

::: browser/components/migration/EdgeProfileMigrator.js:26
(Diff revision 2)
> +    resources.push(MSMigrationUtils.getWin8EdgePasswordsMigrator());

It's confusing to have something named "getWin8" in code that's conditional on being Win10+...

getPasswordVaultMigrator()?

::: browser/components/migration/MSMigrationUtils.jsm:37
(Diff revision 2)
> +                                  685460848];

Nit: it's generally better form to express these as hex values instead of decimal.

::: browser/components/migration/MSMigrationUtils.jsm:640
(Diff revision 2)
>      function _isIEOrEdgePassowd(id) {

s/Passowd/Password/

::: browser/components/migration/MSMigrationUtils.jsm
(Diff revision 2)
> -          let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);

Why are you removing this?

Comment 22

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 23

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17545/#review15873

::: browser/components/migration/MSMigrationUtils.jsm
(Diff revision 2)
> -          let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);

I just move it to addLogins function

Comment 24

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17545/#review15833

> It's confusing to have something named "getWin8" in code that's conditional on being Win10+...
> 
> getPasswordVaultMigrator()?

This is a miagrator for both ie on windows8+ and edge for windows 10, so I will add a comment to explain

Comment 25

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17545/#review15833

> It's confusing to have something named "getWin8" in code that's conditional on being Win10+...
> 
> getPasswordVaultMigrator()?

This is a miagrator for both ie on windows8+ and edge for windows 10, so I will add a comment to explain

Comment 26

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17545/#review15833

> Why are you removing this?

I just move it to addLogins function

Comment 27

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17545/#review15833

I think it s possible to do that, but you know review board ui is not the best.
I wanted just that you understand the difference between what I have done and the rebase, but sorry because I think I made things more confusing.

Comment 28

[Sheila Mooney]

[Tracking Requested - why for this release]: We are trying to uplift a number of Win 10 items to Fx 41. Ritu asked me to nom for tracking.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked. I hope we can land the fixes soon because in ~10 days we will be entering RC phase where uplifting these patches would be very hard.

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

https://reviewboard.mozilla.org/r/17063/#review16073

::: browser/components/migration/EdgeProfileMigrator.js:20
(Diff revision 5)
>    let resources = [
>      MSMigrationUtils.getBookmarksMigrator(MSMigrationUtils.MIGRATION_TYPE_EDGE),
>      MSMigrationUtils.getCookiesMigrator(MSMigrationUtils.MIGRATION_TYPE_EDGE),
>    ];
> +  // activate the password migrator for edge only for win10
> +  if (AppConstants.isPlatformAndVersionAtLeast("win", "10")) {
> +    resources.push(MSMigrationUtils.getWin8EdgePasswordsMigrator());
> +  }

I'm not sure why only the password migrator belongs in this condition?

I think a better solution may be to implement sourceProfiles and have it return an empty array outside of Windows 10.

::: browser/components/migration/MSMigrationUtils.jsm:28
(Diff revision 5)
> +const ERROR_SUCCESS = 0

Nit: missing semicolon. Is ERROR_SUCCESS a name you got from somewhere else? It seems like a contradiction to me since it's both an error and success. How about RESULT_SUCCESS?

::: browser/components/migration/MSMigrationUtils.jsm:28
(Diff revision 5)
> +const ERROR_SUCCESS = 0
> +const VAULT_ENUMERATE_ALL_ITEMS = 512;
> +const INTERNET_EXPLORER_EDGE_GUID = [0x3CCD5499,
> +                                     0x4B1087A8,
> +                                     0x886015A2,
> +                                     0x553BDD88];
> +const WEB_CREDENTIALS_VAULT_ID = [0x4BF4C442,
> +                                  0x41A09B8A,
> +                                  0x4ADD80B3,
> +                                  0x28DB4D70];

Please keep these sorted alphabetically.

::: browser/components/migration/MSMigrationUtils.jsm:142
(Diff revision 5)
> +  this._structs.GUID = new ctypes.StructType('GUID', [
> +    {id: wintypes.DWORD.array(4)},
> +  ]);
> +
> +  this._structs.VAULT_ITEM_ELEMENT = new ctypes.StructType('VAULT_ITEM_ELEMENT', [

Why are you using single quotes?

::: browser/components/migration/MSMigrationUtils.jsm:161
(Diff revision 5)
> +  this._structs.VAULT_ELEMENT = new ctypes.StructType('VAULT_ELEMENT', [

Here too

::: browser/components/migration/MSMigrationUtils.jsm:617
(Diff revision 5)
> +// password migrator for IE on windows 8+ and Edge on windows 10
> +function Win8EdgePasswords () {

This is just form passwords, right?
I personally would have called this `WindowsVaultFormPasswords` with your comment but without the browser references since the APIs are for Windows, not the browsers:

"// Migrator for form passwords on Windows 8 and higher."

::: browser/components/migration/MSMigrationUtils.jsm:628
(Diff revision 5)
> +      return this.migrate(() => {} , true);

Nit: extra space

::: browser/components/migration/MSMigrationUtils.jsm:634
(Diff revision 5)
> +   * Migrate Win8Edge passwords.

This line isn't very informative.

::: browser/components/migration/MSMigrationUtils.jsm:635
(Diff revision 5)
> +   * @param {function} aCallback - a callback called when the migration is done.
> +   * @param {bool} exist - if exists is true, just check if there are some passwords to migrate and

`exists` isn't very descriptive. How about `aOnlyCheckExists`?

::: browser/components/migration/MSMigrationUtils.jsm:636
(Diff revision 5)
> +   * @param {bool} exist - if exists is true, just check if there are some passwords to migrate and
> +   * return true in that case without adding the data.

…or calling the callback

::: browser/components/migration/MSMigrationUtils.jsm:637
(Diff revision 5)
> +   * return true in that case without adding the data.
> +   */

Use @return and define the other cases too.

::: browser/components/migration/MSMigrationUtils.jsm:719
(Diff revision 5)
> +          // close current item
> +          error = ctypesVaultHelpers._functions.VaultFree(credential);
> +          if (!error) {
> +            throw new Error("Unable to free item: " + error);

This doesn't read correctly: "If there is no error, throw an Error." Can you compare with the const here instead?

::: browser/components/migration/MSMigrationUtils.jsm:733
(Diff revision 5)
> +      error = ctypesVaultHelpers._functions.VaultCloseVault(vault);
> +      if (!error) {
> +        throw new Error("Unable to close vault: " + error);
> +      }

This doesn't read correctly: "If there is no error, throw an Error." Can you compare with the const here instead?

::: browser/components/migration/MSMigrationUtils.jsm:758
(Diff revision 5)
> +  getWin8EdgePasswordsMigrator() {

Rename this too

::: browser/components/migration/MigrationUtils.jsm:651
(Diff revision 5)
> +   * Add the login to the password manager and merge it with the existing ones.
> +   * @param {Object} loginData - the data about the login that needs to be added.
> +   */
> +  addLogin(loginData) {

I think this helper should be in LoginHelper.jsm since it can be useful in other places. I wouldn't be surprised if we have some of this logic implemented already. I would call it `maybeImportLogin` or `maybeAddLogin` to indicate that no login will be added in some cases.

::: browser/components/migration/MigrationUtils.jsm:694
(Diff revision 5)
> +    if (isNew) {

Use an early return !isNew instead

Comment 31

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #21)
> Quick driveby review... But is there a way to get a single, complete diff of
> all 3 changesets you've got listed in this bug? 

You can click "Complete Diff" in reviewboard for this.

Comment 32

[Justin Dolske [:Dolske]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #31)
> (In reply to Justin Dolske [:Dolske] from comment #21)
> > Quick driveby review... But is there a way to get a single, complete diff of
> > all 3 changesets you've got listed in this bug? 
> 
> You can click "Complete Diff" in reviewboard for this.

That didn't work at the time, because there were 3 separate mozreview requests. It's just 1 now, so it's moot. :)

Comment 33

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 34

[Edwin Wong [:edwong]]

This is high priority for the Win10 team. We want to uplift once we get the patch landed and tested.

Comment 35

[Justin Dolske [:Dolske]]

https://reviewboard.mozilla.org/r/17063/#review16577

Tiny driveby...

::: browser/components/migration/MSMigrationUtils.jsm:770
(Diff revision 6)
> +            return true;

Should you be closing the Valut here?

::: browser/components/migration/MSMigrationUtils.jsm:818
(Diff revision 6)
> +      error = ctypesVaultHelpers._functions.VaultCloseVault(vault);

Maybe move your VaultClose into the finally with other cleanup, so it always executes even if an exception is thrown? (Conditional on a VaultOpen having actually succeeded, of course.)

Nice to report a VaultClose failure, but I probably wouldn't make it fatal.

Comment 36

[:Gijs (out for now; he/him)]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

https://reviewboard.mozilla.org/r/17063/#review16585

Close, but still some issues. Did you trypush this yet? :-)

::: browser/components/migration/EdgeProfileMigrator.js:18
(Diff revision 6)
> +Object.defineProperty(EdgeProfileMigrator.prototype, "sourceProfiles", {

I landed something similar to this as part of bug 1200598, so this will need rebasing. Sorry. :-(

::: browser/components/migration/EdgeProfileMigrator.js:31
(Diff revision 6)
> +    MSMigrationUtils.getWindowsVaultFormPasswordsMigrator(),

This is going to break the Edge test here: https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/unit/test_Edge_availability.js
Please make sure to fix that.

::: browser/components/migration/IEProfileMigrator.js:133
(Diff revision 6)
> +  this.name = "IE7FormPasswords";

The edge/win8 password migrator you're adding should also get a name.

::: browser/components/migration/IEProfileMigrator.js:262
(Diff revision 6)
> -        login.init(ieLogin.url, "", null,
> -                   ieLogin.username, ieLogin.password, "", "");
> -        login.QueryInterface(Ci.nsILoginMetaInfo);
> -        login.timeCreated = ieLogin.creation;
> -        login.timeLastUsed = ieLogin.creation;
> -        login.timePasswordChanged = ieLogin.creation;
> -        // login.timesUsed is going to set to the default value 1
> -        // Add the login only if there's not an existing entry
> -        let existingLogins = Services.logins.findLogins({}, login.hostname, "", null);
> -        addLogin(login, existingLogins);
> +          hostname: ieLogin.url,
> +          submitURL: "",
> +          httpRealm: null,
> +          username: ieLogin.username,
> +          password: ieLogin.password,
> +          usernameElement: "",
> +          passwordElement: "",
> +          timeCreated: ieLogin.creation,
> +          timeLastUsed: ieLogin.creation,
> +          timePasswordChanged: ieLogin.creation,
> +          timesUsed: 1,

See the comment below. Don't set all the values on the object if they're just the default.

The comment in the existing code here says that setting timesUsed to 1 is not necessary. Is the comment wrong?

::: browser/components/migration/IEProfileMigrator.js:504
(Diff revision 6)
>    , new IE7FormPasswords()
>    , new Settings()
> +  , MSMigrationUtils.getWindowsVaultFormPasswordsMigrator()

Are we *sure* these two password migrators are mutually exclusive? What if I have a machine with win7 credentials that gets upgraded to win8 or win10 - I wonder if the registry (which is how the IE7 migrator checks if it needs to do something) will get wiped (Murphy's law says it won't). I don't think (but haven't checked) that our migration code will like having two migratable resources of the same type (at best, I think it'll show that resource type twice in the UI, at worst it will only run one or die trying to do the migration).

I would suggest returning false early in the exists() getter of IE7FormPasswords iff we're on win8+ to make sure we don't run into this case.

::: browser/components/migration/MSMigrationUtils.jsm:143
(Diff revision 6)
> +  this._libs = {};

Nit: this is only going to have a single property so the nested object doesn't serve much purpose.

Can you file a followup bug to tidy up the two different ctypes helpers? I'd really rather merge them into one object and have callers request which libraries (and thereby methods) they want loaded + defined - but it's not necessary to do that here.

::: browser/components/migration/MSMigrationUtils.jsm:758
(Diff revision 6)
> +      if (error != RESULT_SUCCESS) {
> +        throw new Error("Unable to enumerate Vault items: " + error);
> +      }

What happens if the table is empty, ie no passwords are saved? That probably shouldn't throw a fatal error here. I expect MS will use a specific error code for that. (or maybe it uses RESULT_SUCCESS and sets itemCount to 0? Not sure.)

::: browser/components/migration/MSMigrationUtils.jsm:761
(Diff revision 6)
> +      for (let j = 0; j < itemCount.value; j++) {

I am still curious about performance here, though I guess chunking this can be a followup issue if we find out it's slow with many passwords.

::: browser/components/migration/MSMigrationUtils.jsm:791
(Diff revision 6)
> +            hostname: NetUtil.newURI(url).prePath,
> +            submitURL: "",
> +            httpRealm: null,
> +            username: username,
> +            password: password,
> +            usernameElement: "",
> +            passwordElement: "",
> +            timeCreated: creation,
> +            timeLastUsed: creation,
> +            timePasswordChanged: creation,
> +            timesUsed: 1,

Oof. May I suggest:

let login = {
  username, password,
  hostname: NetUtil.newURI(url).prePath,
  timesUsed: 1,
};
login.timeCreated = login.timeLastUsed = login.timePasswordChanged = creation;

::: browser/components/migration/MSMigrationUtils.jsm:830
(Diff revision 6)
> +    }

Please explicitly return false if aOnlyCheckExists is true.

::: browser/components/migration/MSMigrationUtils.jsm:844
(Diff revision 6)
> +  getWindowsVaultFormPasswordsMigrator() {
> +    return new WindowsVaultFormPasswords();
> +  },

The other two methods here return different things if you pass a different migration type. This one doesn't, it'll just return two instances of the same thing.

I think this should just be a property - I don't see any reason why the two migrators having a handle on the same object would be a bad thing - in any case, only one of them will ever call migrate() on it at a time, and the object doesn't store any kind of state. Matt, can you sanity-check if that's a dumb idea for some reason? I guess it *will* persist for the lifetime of the process, so if we ever start storing state it'll be an issue, that's not immediately obvious because it has a constructor+prototype structure.

Maybe it should just be a gWindowsVaultFormPasswords object without a prototype? That makes more sense to me, but a sanity-check would still be nice.

::: toolkit/components/passwordmgr/LoginHelper.jsm:354
(Diff revision 6)
> +    login.init(loginData.hostname, loginData.submitURL, loginData.httpRealm, loginData.username,
> +               loginData.password, loginData.usernameElement, loginData.passwordElement);

If you omit the relevant things off loginData, this call should use || <defaultValue> to ensure the default null/"" value gets used if the property is not defined on loginData.

I should note that both the httpRealm and submitURL docs suggest that the default is NULL, and both are AStrings - right now your code uses "" for one and null for the other.

::: toolkit/components/passwordmgr/LoginHelper.jsm:359
(Diff revision 6)
> +    login.timeLastUsed = loginData.timeLastUsed;
> +    login.timePasswordChanged = loginData.timePasswordChanged;

In fact, this could set these to timeCreated if timeLastUsed/timePasswordChanged are not present.

::: toolkit/components/passwordmgr/LoginHelper.jsm:361
(Diff revision 6)
> +    login.timesUsed = loginData.timesUsed;

Likewise, this could probably set it to 1 if undefined/falsy.

::: toolkit/components/passwordmgr/LoginHelper.jsm:374
(Diff revision 6)
> +      if (login.username == existingLogin.username && login.password != existingLogin.password) {
> +        // if a login with the same username and different password already exists and it's older
> +        // than the current one, that login needs to be updated using the current one details
> +        if (login.timePasswordChanged > existingLogin.timePasswordChanged) {

Two nested ifs without elses or code outside the inner conditional - you can just merge the conditions, and move the comment before the outer if() statement.

... except... I think that if the new login's change time is *before* the old one, we shouldn't be adding the login at all, right? In which case we should return early rather than adding the outdated login information after the end of the loop.

::: toolkit/components/passwordmgr/LoginHelper.jsm:391
(Diff revision 6)
> +    // if the new login is an update, don't add it.
> +    if (isUpdate) {
> +      return;
> +    }

Get rid of the bool, and instead of setting isUpdate to true, return immediately from within the loop, because at that point there is no point continuing in that loop - unless there are multiple duplicate outdated logins in the database already, which seems unlikely. (and maybe impossible? I don't know the implementation of the login service, maybe dolske or Matt can clarify. It also seems I suggested this before, so maybe someone told you out-of-band not to do this?)

Comment 37

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 38

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review16649

::: browser/components/migration/MSMigrationUtils.jsm:758
(Diff revision 6)
> +      if (error != RESULT_SUCCESS) {
> +        throw new Error("Unable to enumerate Vault items: " + error);
> +      }

Good remark, The VaultEnumerateItems will return itemCount.value
So, in this case, we are going to get 0 for itemCount. We are going to do 0 iteration in the for loop.

Comment 39

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review16585

> See the comment below. Don't set all the values on the object if they're just the default.
> 
> The comment in the existing code here says that setting timesUsed to 1 is not necessary. Is the comment wrong?

No it's true, but I said when login was nsILoginInfo object, but now it's just a normal javascript object, so it has changed a  bit.

> What happens if the table is empty, ie no passwords are saved? That probably shouldn't throw a fatal error here. I expect MS will use a specific error code for that. (or maybe it uses RESULT_SUCCESS and sets itemCount to 0? Not sure.)

Good remark, The VaultEnumerateItems will return itemCount.value
So, in this case, we are going to get 0 for itemCount. We are going to do 0 iteration in the for loop.

> I am still curious about performance here, though I guess chunking this can be a followup issue if we find out it's slow with many passwords.

cool let s wait and see

> Oof. May I suggest:
> 
> let login = {
>   username, password,
>   hostname: NetUtil.newURI(url).prePath,
>   timesUsed: 1,
> };
> login.timeCreated = login.timeLastUsed = login.timePasswordChanged = creation;

the httpRealm or formSubmitURL have to be not null, otherwise the login cannot be added

> The other two methods here return different things if you pass a different migration type. This one doesn't, it'll just return two instances of the same thing.
> 
> I think this should just be a property - I don't see any reason why the two migrators having a handle on the same object would be a bad thing - in any case, only one of them will ever call migrate() on it at a time, and the object doesn't store any kind of state. Matt, can you sanity-check if that's a dumb idea for some reason? I guess it *will* persist for the lifetime of the process, so if we ever start storing state it'll be an issue, that's not immediately obvious because it has a constructor+prototype structure.
> 
> Maybe it should just be a gWindowsVaultFormPasswords object without a prototype? That makes more sense to me, but a sanity-check would still be nice.

I discussed with matt about this and you are right we don't keep state here. But matt and I think that it's probably better to follow exactly the same patterns with other migrators

> If you omit the relevant things off loginData, this call should use || <defaultValue> to ensure the default null/"" value gets used if the property is not defined on loginData.
> 
> I should note that both the httpRealm and submitURL docs suggest that the default is NULL, and both are AStrings - right now your code uses "" for one and null for the other.

the httpRealm or formSubmitURL have to be not null, otherwise the login cannot be added
So because the value I will use for submitURL is "" which is falsy, so if I do "" || null I will get null.
So the best solution is to just check if submitURL/httpRealm are string, otherwise  there replaced witht there default value

> Two nested ifs without elses or code outside the inner conditional - you can just merge the conditions, and move the comment before the outer if() statement.
> 
> ... except... I think that if the new login's change time is *before* the old one, we shouldn't be adding the login at all, right? In which case we should return early rather than adding the outdated login information after the end of the loop.

good catch
according to MattN, it's possible to have multiple logins with the same hostname/username and different formSubmit urls)
So to fix that, I introduced a new bool value isold (which is true is the new login is older than the existing one, and thus this login is not going to be added.

> Get rid of the bool, and instead of setting isUpdate to true, return immediately from within the loop, because at that point there is no point continuing in that loop - unless there are multiple duplicate outdated logins in the database already, which seems unlikely. (and maybe impossible? I don't know the implementation of the login service, maybe dolske or Matt can clarify. It also seems I suggested this before, so maybe someone told you out-of-band not to do this?)

I asked matt and he told me its possible to have multiple logins with same hostname and username (they have to just have a diffrent form submiturl)
So, I think It's  safer to keep it like that

Comment 40

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review16585

thanks.
Yes I have try push
I will fix almost all the issues (I still just have to fix the tests on win10 (it takes aitme , a need to buiold firefox there.)
But as we are running out of time could you have a quik look to this patch
thanks

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

seems too late to take a fix for this in 41. Let's target 42.

Comment 42

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 43

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 44

[:Gijs (out for now; he/him)]

(In reply to Riadh Chtara[:rchtara] from comment #39)
> > Oof. May I suggest:
> > 
> > let login = {
> >   username, password,
> >   hostname: NetUtil.newURI(url).prePath,
> >   timesUsed: 1,
> > };
> > login.timeCreated = login.timeLastUsed = login.timePasswordChanged = creation;
> 
> the httpRealm or formSubmitURL have to be not null, otherwise the login
> cannot be added

That's fine, but in that case the LoginHelper method can force-set formSubmitURL to "" instead of null. I'm trying to make sure that callers don't all have to duplicate the same code.

> > Two nested ifs without elses or code outside the inner conditional - you can just merge the conditions, and move the comment before the outer if() statement.
> > 
> > ... except... I think that if the new login's change time is *before* the old one, we shouldn't be adding the login at all, right? In which case we should return early rather than adding the outdated login information after the end of the loop.
> 
> good catch
> according to MattN, it's possible to have multiple logins with the same
> hostname/username and different formSubmit urls)

How is this possible, considering we call findLogins providing both formSubmitURL and httpRealm? IOW, won't we get exactly 1 login with that username anyway? If that parameter gets ignored if it's the empty string/null, let's add a comment in the code explaining that while we're passing formSubmitURL and httpRealm, they could be empty/null and get ignored in that case, leading to multiple logins for the same username.

> So to fix that, I introduced a new bool value isold (which is true is the
> new login is older than the existing one, and thus this login is not going
> to be added.

You now essentially have:

let isA = false;
let isB = false;
for ...
  if (x) {
    ...
    isA = true
  } else {
    isB = true
  }
}
...

if (isA || isB) {
  return;
}

which could be improved. In fact, there's a subtle bug with the code in that it will call addLogin if there is a pre-existing login with the same username *and* the same password - both isUpdate and isOld only get set if there is a login with a *different* password. We should fix that.



All of this is a little messy because we don't seem to have the realm/formsubmiturl information from IE. Is that info not there?

There's also the fact that if I have the following logins:

A: 2015-08-01
C: 2015-08-05

and import B from 2015-08-03, we will update A and then leave C alone. If we don't know that they are for the same form/realm, I wonder if that even makes sense. :-\


It's probably not useful to figure all this out for this bug, but maybe we can file a followup bug to improve the merge/updating behaviour here.

Comment 45

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 46

[Riadh Chtara[:rchtara]]

(In reply to :Gijs Kruitbosch from comment #44)
> (In reply to Riadh Chtara[:rchtara] from comment #39)
> > > Oof. May I suggest:
> > > 
> > > let login = {
> > >   username, password,
> > >   hostname: NetUtil.newURI(url).prePath,
> > >   timesUsed: 1,
> > > };
> > > login.timeCreated = login.timeLastUsed = login.timePasswordChanged = creation;
> > 
> > the httpRealm or formSubmitURL have to be not null, otherwise the login
> > cannot be added
> 
> That's fine, but in that case the LoginHelper method can force-set
> formSubmitURL to "" instead of null. I'm trying to make sure that callers
> don't all have to duplicate the same code.
> 
OK, I have fixed that.
> > > Two nested ifs without elses or code outside the inner conditional - you can just merge the conditions, and move the comment before the outer if() statement.
> > > 
> > > ... except... I think that if the new login's change time is *before* the old one, we shouldn't be adding the login at all, right? In which case we should return early rather than adding the outdated login information after the end of the loop.
> > 
> > good catch
> > according to MattN, it's possible to have multiple logins with the same
> > hostname/username and different formSubmit urls)
> 
> How is this possible, considering we call findLogins providing both
> formSubmitURL and httpRealm? IOW, won't we get exactly 1 login with that
> username anyway? If that parameter gets ignored if it's the empty
> string/null, let's add a comment in the code explaining that while we're
> passing formSubmitURL and httpRealm, they could be empty/null and get
> ignored in that case, leading to multiple logins for the same username.
> 
You right, and yes , it's a bit weird.
According to here:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsILoginManager.idl#177
An empty string ("") will match any value (except null).
> > So to fix that, I introduced a new bool value isold (which is true is the
> > new login is older than the existing one, and thus this login is not going
> > to be added.
> 
> You now essentially have:
> 
> let isA = false;
> let isB = false;
> for ...
>   if (x) {
>     ...
>     isA = true
>   } else {
>     isB = true
>   }
> }
> ...
> 
> if (isA || isB) {
>   return;
> }
>
The structure is a bit different  
let isA = false;
let isB = false;
for ...
  if (y) {
    if (x) {
      ...
      isA = true
    } else {
     isB = true
    }
  }
}
...
I kept the two if
(y : login.username == existingLogin.username && login.password != existingLogin.password)
which mean if all the logins have different username, both isUpdate and isOld are false,
If there is at least on logibs   
if (isA || isB) {
  return;
}
> which could be improved. In fact, there's a subtle bug with the code in that
> it will call addLogin if there is a pre-existing login with the same
> username *and* the same password - both isUpdate and isOld only get set if
> there is a login with a *different* password. We should fix that.
I thought I removed that case wit
    if (existingLogins.some(l => login.matches(l, true))) {
      return;
    }
but you are right, it's not enough (they could have same passwords usernames and just different timestamps
So I fixed that
> 
> All of this is a little messy because we don't seem to have the
> realm/formsubmiturl information from IE. Is that info not there?
no it's not, as far as I know

> There's also the fact that if I have the following logins:
> 
> A: 2015-08-01
> C: 2015-08-05
> 
> and import B from 2015-08-03, we will update A and then leave C alone. If we
> don't know that they are for the same form/realm, I wonder if that even
> makes sense. :-\
> 
> 
> It's probably not useful to figure all this out for this bug, but maybe we
> can file a followup bug to improve the merge/updating behaviour here.
Yes it s confusing, but there is already a bug for that:
Bug 1187190: Password changes should be propagated depending on time stamps.
So lets not worry about that a lot, and fix time stamps issues in the new bug

Comment 47

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 48

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/17063/#review16819

::: toolkit/components/passwordmgr/LoginHelper.jsm:364
(Diff revision 11)
> +               loginData.submitURL || "",
> +               isString(loginData.httpRealm) ? loginData.httpRealm : null,

Nit: loginData.submitURL || (isString(loginData.httpRealm) ? null : "")

because submitURL should still be null (instead of "") if loginData.httpRealm is passed.

::: toolkit/components/passwordmgr/LoginHelper.jsm:383
(Diff revision 11)
> +    // if the login is not already available, it s going to be added or merged with other

nit: it's

::: toolkit/components/passwordmgr/LoginHelper.jsm:389
(Diff revision 11)
> +    let isUpdateOrOld = false;

Instead of "isUpdateOrOld", how about "foundMatchingLogin" ?

::: toolkit/components/passwordmgr/LoginHelper.jsm:391
(Diff revision 11)
> +      if (login.username == existingLogin.username && login.password != existingLogin.password) {

You can eliminate the duplication (of the username check and the bool assignment) here by doing:

if (login.username == existingLogin.username) {
  foundMatchingLogin = true;
  if (login.password != existingLogin.password &&
      login.timePasswordChanged > existingLogin.timePasswordChanged) {
    // update.
  }
}

Comment 49

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 50

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 51

[Riadh Chtara[:rchtara]]

Tests are has now succeeded
https://reviewboard.mozilla.org/r/17061/

Comment 52

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 53

[:Gijs (out for now; he/him)]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

https://reviewboard.mozilla.org/r/17063/#review17223

Looks OK to me, with the nit below fixed, but I think Matt should still have a look, too.

::: browser/components/migration/tests/unit/test_IE7_passwords.js:270
(Diff revision 13)
> -function getFirstResourceOfType(type) {
> +function getFirstResourceOfType(name, type) {

Considering this is a test for IE7 passwords, I guess you could just hardcode the name check instead of passing the parameter from all the callsites.

Comment 54

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 55

[:Gijs (out for now; he/him)]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

https://reviewboard.mozilla.org/r/17063/#review17261

Comment 56

[Matthew N. [:MattN]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

https://reviewboard.mozilla.org/r/17063/#review17307

::: browser/components/migration/MSMigrationUtils.jsm:26
(Diff revision 14)
>  const EDGE_COOKIE_PATH_OPTIONS = ["", "#!001\\", "#!002\\"];
>  const EDGE_COOKIES_SUFFIX = "MicrosoftEdge\\Cookies";
>  const EDGE_FAVORITES = "AC\\MicrosoftEdge\\User\\Default\\Favorites";
>  const EDGE_READINGLIST = "AC\\MicrosoftEdge\\User\\Default\\DataStore\\Data\\";
> +const INTERNET_EXPLORER_EDGE_GUID = [0x3CCD5499,
> +                                     0x4B1087A8,
> +                                     0x886015A2,
> +                                     0x553BDD88];
> +const FREE_CLOSE_FAILED = 0;
> +const RESULT_SUCCESS = 0;
> +const VAULT_ENUMERATE_ALL_ITEMS = 512;
> +const WEB_CREDENTIALS_VAULT_ID = [0x4BF4C442,
> +                                  0x41A09B8A,
> +                                  0x4ADD80B3,
> +                                  0x28DB4D70];

Nit: Sort these alphabetically

::: browser/components/migration/MSMigrationUtils.jsm:146
(Diff revision 14)
> +  // the size of the vault handle in 32 bits version is 32 and 64 in 64 bits version
> +  if (wintypes.VOIDP.size == 4) {
> +    this._vaultHandleType = wintypes.DWORD;
> +  } else {
> +    this._vaultHandleType = wintypes.DWORDLONG;
> +  }

Dolske, is there a better way to handle this?

::: browser/components/migration/MSMigrationUtils.jsm:724
(Diff revision 14)
> +   * @param {boolean?} aOnlyCheckExists - if aOnlyCheckExists is true, just check if there are some

Use the square bracket syntax to indicate this is optional and include the default value:

@param {boolean} [aOnlyCheckExists=false]…

::: toolkit/components/passwordmgr/LoginHelper.jsm:32
(Diff revision 14)
> +function isString(aValue) {
> +  // We cannot use the "instanceof" operator reliably across module boundaries.
> +  return (typeof aValue == "string") ||
> +         (typeof aValue == "object" && "charAt" in aValue);

Do we actually need to care about String objects? We generally try to avoid them. If not, just inline the typeof check twice.

How about just checking that aValue.constructor.name == "String" instead of checking if charAt exists?

::: toolkit/components/passwordmgr/LoginHelper.jsm:363
(Diff revision 14)
> +    login.init(loginData.hostname || "",
> +               loginData.submitURL || (isString(loginData.httpRealm) ? null : ""),
> +               isString(loginData.httpRealm) ? loginData.httpRealm : null,
> +               loginData.username || "",
> +               loginData.password || "",

Don't use to default values on required fields (e.g. hostname, username, password, httpRealm) as we want to catch those bugs instead of paving over them.

I also think we should get rid of isString and be stricter about submitURL and httpRealm by checking for null instead (not just falsy) otherwise use what was passed.

::: toolkit/components/passwordmgr/LoginHelper.jsm:376
(Diff revision 14)
> +    // the logins that already exist for the same host
> +    // while here we're passing formSubmitURL and httpRealm, they could be empty/null and get
> +    // ignored in that case, leading to multiple logins for the same username.

This seems like an incomplete sentence. Can you rewrite this, fix wrapping and please use full sentences, capitals and punctuation.

::: toolkit/components/passwordmgr/LoginHelper.jsm:392
(Diff revision 14)
> +        // Bug 1187190: Password changes should be propagated depending on timestamps.
> +        // this an old login or a just an update, so make sure not to add it
> +        foundMatchingLogin = true;
> +        if(login.password != existingLogin.password &

What is left to be done in bug 1187190?

Comment 57

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #56)
> ::: browser/components/migration/MSMigrationUtils.jsm:146
> (Diff revision 14)
> > +  // the size of the vault handle in 32 bits version is 32 and 64 in 64 bits version
> > +  if (wintypes.VOIDP.size == 4) {
> > +    this._vaultHandleType = wintypes.DWORD;
> > +  } else {
> > +    this._vaultHandleType = wintypes.DWORDLONG;
> > +  }
> 
> Dolske, is there a better way to handle this?

Comment 58

[Riadh Chtara[:rchtara]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Comment 59

[Matthew N. [:MattN]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

https://reviewboard.mozilla.org/r/17063/#review17473

Comment 60

[Riadh Chtara[:rchtara]]

https://reviewboard.mozilla.org/r/17063/#review17307

> Dolske, is there a better way to handle this?

I'm not sure but here they are following the same pattern: 
(Example of StructType() on Windows)
https://developer.mozilla.org/en-US/docs/Mozilla/js-ctypes/js-ctypes_reference/ctypes

Comment 61

[Justin Dolske [:Dolske]]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #57)

> > > +  // the size of the vault handle in 32 bits version is 32 and 64 in 64 bits version
> > > +  if (wintypes.VOIDP.size == 4) {
> > > +    this._vaultHandleType = wintypes.DWORD;
> > > +  } else {
> > > +    this._vaultHandleType = wintypes.DWORDLONG;
> > > +  }
> > 
> > Dolske, is there a better way to handle this?

Hmm, .DWORD is defined as a ctypes.uint32_t, but then that's what DWORD is supposed to me.

Do we have actual headers for these files to check against?

I'm used to int being 32 bits and long being 64 in the Unix C world, but looks like on Windows long is always 32 bits, even on 64 bit systems. So presumably ctypes.long won't do the right thing thing (by being the right size for the binary). (long long is 64 bits, but then it appears to always be 64 bits). So I sorta wonder how MS formally defines this argument.

In lieu if that, this seems like an acceptable workaround.

Comment 62

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/888d001bcc3c

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/888d001bcc3c

Comment 64

[Riadh Chtara[:rchtara]]

It's defined as  void *

Thanks a lot guys for the review.

Comment 65

[Justin Dolske [:Dolske]]

(In reply to Riadh Chtara[:rchtara] from comment #64)
> It's defined as  void *

Then why are you using DWORD/DWORDLONG? ctypes.voidptr_t would be the right thing to use, and that would also be the right size by default.

Comment 66

[Riadh Chtara[:rchtara]]

yes, you are right. 
Sorry for that, but it's probably  too late to fix that

Comment 67

[Matthew N. [:MattN]]

I updated https://wiki.mozilla.org/QA/Firefox_migrators#Supported_data_types

Comment 68

[Matthew N. [:MattN]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

Approval Request Comment
[Feature/regressing bug #]: Migrators for new Edge users who may want to switch to Firefox
[User impact if declined]: No ability to import passwords from Edge or IE on Win8+
[Describe test coverage new/current, TreeHerder]: Simple sanity checks only since it relies on undocumented APIs so it's hard to do automated tests. QA does manual verification in Beta.
[Risks and why]: Medium risk due to new complicated code using undocumented APIs but I think it's worth uplifting to help new Fx users since this code doesn't get much manual baking/testing on Nightly/Aurora anyways since most people only use this feature upon first installation.
[String/UUID change made/needed]: None. Strings were pre-landed.

Comment 69

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652058 [details]
MozReview Request: Bug 1192035 - Import passwords from Microsoft Edge / Windows 8+

It is already in 43. Let's take in 42, this will be appreciated by our users.

Comment 70

[Carsten Book [:Tomcat]]

Hi, when uplifting to beta this cause conflicts

merging browser/components/migration/EdgeProfileMigrator.js failed!
merging browser/components/migration/IEProfileMigrator.js                                                                                                                                                 
3 files to edit
merging browser/components/migration/IEProfileMigrator.js failed!                                                                                                                                         
merging browser/components/migration/MSMigrationUtils.jsm                                                                                                                                                 
3 files to edit
merging browser/components/migration/MSMigrationUtils.jsm failed!                                                                                                                                         
merging toolkit/components/passwordmgr/LoginHelper.jsm                                                                                                                                                    
3 files to edit
merging toolkit/components/passwordmgr/LoginHelper.jsm failed!  

could you take a look, thanks!

Comment 71

[Bill Maggs (bmaggs)]

NI to MattN because riadh may be back at school and busy.

Comment 72

[Justin Dolske [:Dolske]]

The conflicts were because bug 1193387 ran into an L10N hiuccup for landing, I think we've sorted that out which can unblock this from cleanly landing.

Comment 73

[Carsten Book [:Tomcat]]

(In reply to Justin Dolske [:Dolske] from comment #72)
> The conflicts were because bug 1193387 ran into an L10N hiuccup for landing,
> I think we've sorted that out which can unblock this from cleanly landing.

hey dolske i still get merging browser/components/migration/EdgeProfileMigrator.js
warning: conflicts during merge.
merging browser/components/migration/EdgeProfileMigrator.js incomplete! (edit conflicts, then use 'hg resolve --mark')
merging browser/components/migration/IEProfileMigrator.js
warning: conflicts during merge.
merging browser/components/migration/IEProfileMigrator.js incomplete! (edit conflicts, then use 'hg resolve --mark')
merging browser/components/migration/MSMigrationUtils.jsm
warning: conflicts during merge.
merging browser/components/migration/MSMigrationUtils.jsm incomplete! (edit conflicts, then use 'hg resolve --mark')
merging toolkit/components/passwordmgr/LoginHelper.jsm
warning: conflicts during merge.
merging toolkit/components/passwordmgr/LoginHelper.jsm incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
 when trying to uplift

Comment 74

[:Gijs (out for now; he/him)]

(In reply to Carsten Book [:Tomcat] from comment #73)
> (In reply to Justin Dolske [:Dolske] from comment #72)
> > The conflicts were because bug 1193387 ran into an L10N hiuccup for landing,
> > I think we've sorted that out which can unblock this from cleanly landing.
> 
> hey dolske i still get merging
> browser/components/migration/EdgeProfileMigrator.js
> warning: conflicts during merge.
> merging browser/components/migration/EdgeProfileMigrator.js incomplete!
> (edit conflicts, then use 'hg resolve --mark')
> merging browser/components/migration/IEProfileMigrator.js
> warning: conflicts during merge.
> merging browser/components/migration/IEProfileMigrator.js incomplete! (edit
> conflicts, then use 'hg resolve --mark')
> merging browser/components/migration/MSMigrationUtils.jsm
> warning: conflicts during merge.
> merging browser/components/migration/MSMigrationUtils.jsm incomplete! (edit
> conflicts, then use 'hg resolve --mark')
> merging toolkit/components/passwordmgr/LoginHelper.jsm
> warning: conflicts during merge.
> merging toolkit/components/passwordmgr/LoginHelper.jsm incomplete! (edit
> conflicts, then use 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
>  when trying to uplift

Because bug 1193387 hasn't landed yet...

Comment 75

[Sylvestre Ledru [:Sylvestre]]

Gijs, I refused the uplift of bug 1193387. Should we wontfix this one or rebase it?

Comment 76

[Sylvestre Ledru [:Sylvestre]]

I changed my mind in the bug 1193387. We can take it.

Comment 77

[:Gijs (out for now; he/him)]

http://hg.mozilla.org/releases/mozilla-beta/rev/22a32d0004c2

Comment 78

[Vasilica Tamas, QA [:vtamas]]

Verified on Firefox 42 beta 6 (20151012151721) and Firefox 43.0a2 (2015-10-13) under Windows 8 32-bit (importing from IE) and Windows 10 64-bit (importing from Egde and IE).

Besides the issue that is already known Bug 1191175 I’ve encountered another one: Bug 1214573

Based on the above mentions, I am marking this bug as Verified since the other issues are tracked separately.