1426721 - Avoid blocking main thread when importing logins

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

[Alex Thayer [:alexical] (she/her)]

Right now, when importing logins from another browser, a huge chunk of time is spent on the main thread encrypting the logins (https://perfht.ml/2p7T4qj). I can think of 3 options for reducing the jank caused by this operation:

1. Dispatch to ourselves every N logins, to give the main thread some breathing room
2. Pass the plaintext logins to a background thread which will run the crypto and pass them back to us
3. Add a bulk encryption interface in order to optimize out any per-login crypto overhead

Option 1 is pretty cheap to implement, but if this is going on during startup it's really just going to replace a completely unresponsive browser for 500ms with a mostly unresponsive browser for 1000ms.

Option 3 might pay off, but I'm really not qualified to assess where that overhead is and how much it can be optimized. I know from the profile that we spend a lot of time in functions that have "init" in their name, but it's hard to tell if the "initialization" is redundant or not without knowing more about our crypto stack.

Accordingly, option 2 sounds to me like our best bet. I think it will be a decent amount of work getting this all to run in a background thread, but it's probably less than option 3 and it will probably have a bigger payoff.

Comment 1

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426721 - Encrypt Chrome logins in background thread

A significant chunk of migration jank that I observe locally
happens due to login encryption. This patch reduces the locally
observed jank (measured importing 100 logins) from 180ms to 25ms.

Try is green, and as far as I can tell I don't see any thread
safety issues, but I'm not 100% sure on that. I don't see any
red flags inside the SecretDecoderRing::Encrypt implementation.

I only moved Chrome logins over since I wanted to frontload any
potential issues with the whole approach. It shouldn't be too
hard to move the MSMigrationUtils and IEProfileMigrator uses
over though.

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

Comment 2

[:Gijs (he/him)]

I don't know enough about our password manager to review this, I'm afraid, and definitely not enough about SecretDecoderRing. Matt knows both migration and the password manager, and looks like Mark has reviewed stuff touching decoderring before, so I've redirected review to them.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review217950

Hi Doug, could you please split this into two separate commits since they require separate reviewers and it will make it easier to review:
1) security/manager/ + nsILoginManagerCrypto.idl + crypto-SDR.js
2) non-crypto changes

::: toolkit/components/passwordmgr/nsILoginManager.idl:55
(Diff revision 1)
> +   *
> +   * Default values for each login's nsILoginMetaInfo properties will be
> +   * created. However, if the caller specifies non-default values, they will
> +   * be used instead.
> +   */
> +  jsval addLogins(in jsval aLogins);

I would really rather not have to maintain more complexity in both storage backends when this component is basically unmaintained and since the changes aren't needed (AFAIK) for the mozStorage backend (only used on Android).

I believe a smaller change and the expense of less encapsulation of the encryption would be to add an optional 2nd argument to addLogin that indicated if aLogin was already encrypted, then making `let [encUsername, encPassword, encType] = this._encryptLogin(login);` conditional on that argument.

Thoughts?

Comment 4

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review217950

> I would really rather not have to maintain more complexity in both storage backends when this component is basically unmaintained and since the changes aren't needed (AFAIK) for the mozStorage backend (only used on Android).
> 
> I believe a smaller change and the expense of less encapsulation of the encryption would be to add an optional 2nd argument to addLogin that indicated if aLogin was already encrypted, then making `let [encUsername, encPassword, encType] = this._encryptLogin(login);` conditional on that argument.
> 
> Thoughts?

Hmm, the only problem with that we return all of these logins, and if we encrypt username and password on them then the bulk interface will return different values than the single interface. Given that, we could:
- Perform another clone beforehand (which already shows up in profiling)
- Reset username and password to the plaintext values after calling addLogin for each
- Add additional string arguments to addLogin so we don't have to mutate aLogin
- Just accept and document that nsILoginManager::addLogins mutates the logins, while nsILoginManager::addLogin doesn't

Which would you prefer?

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review217950

> Hmm, the only problem with that we return all of these logins, and if we encrypt username and password on them then the bulk interface will return different values than the single interface. Given that, we could:
> - Perform another clone beforehand (which already shows up in profiling)
> - Reset username and password to the plaintext values after calling addLogin for each
> - Add additional string arguments to addLogin so we don't have to mutate aLogin
> - Just accept and document that nsILoginManager::addLogins mutates the logins, while nsILoginManager::addLogin doesn't
> 
> Which would you prefer?

Oh, sorry, I think there is a misunderstanding. Part of my suggestion is that we don't introduce a new `addLogins` API, only add an `addLogin` argument. Does that make sense? The return value difference would be that `username` and `password` remain encrypted whereas they would have remained unencrypted when the new argument isn't used?

Comment 6

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review217950

> Oh, sorry, I think there is a misunderstanding. Part of my suggestion is that we don't introduce a new `addLogins` API, only add an `addLogin` argument. Does that make sense? The return value difference would be that `username` and `password` remain encrypted whereas they would have remained unencrypted when the new argument isn't used?

I don't think I was quite clear. To be clear now: you're referring to avoiding adding the addLogins method to nsILoginManagerStorage, but keeping the addLogins method on nsILoginManager, correct? I would like to keep the addLogins method on nsILoginManager to avoid redundant and expensive checks that addLogin makes. The issue that I was raising is just that I'll have to do something like this inside nsLoginManager.js if we want to remove addLogins from nsILoginManagerStorage:

```
  // (inside nsLoginManager.js)

  async addLogins(logins) {
    let crypto = Cc["@mozilla.org/login-manager/crypto/SDR;1"].
                 getService(Ci.nsILoginManagerCrypto);
    let plaintexts = logins.map(l => l.username).concat(logins.map(l => l.password));
    let ciphertexts = await crypto.encryptMany(plaintexts);
    let usernames = ciphertexts.slice(0, logins.length);
    let passwords = ciphertexts.slice(logins.length);
    let encType = crypto.defaultEncType;
    for (let i = 0; i < logins.length; i++) {
      let plaintextUsername = logins[i].username;
      let plaintextPassword = logins[i].password;
      logins[i].username = usernames[i];
      logins[i].password = passwords[i];
      this._storage.addLogin(logins[i], true);
      // Reset the username and password to keep the same guarantees as addLogin
      logins[i].username = plaintextUsername;
      logins[i].password = plaintextPassword;
    }
  },
```

Does that make sense?

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review217950

> I don't think I was quite clear. To be clear now: you're referring to avoiding adding the addLogins method to nsILoginManagerStorage, but keeping the addLogins method on nsILoginManager, correct? I would like to keep the addLogins method on nsILoginManager to avoid redundant and expensive checks that addLogin makes. The issue that I was raising is just that I'll have to do something like this inside nsLoginManager.js if we want to remove addLogins from nsILoginManagerStorage:
> 
> ```
>   // (inside nsLoginManager.js)
> 
>   async addLogins(logins) {
>     let crypto = Cc["@mozilla.org/login-manager/crypto/SDR;1"].
>                  getService(Ci.nsILoginManagerCrypto);
>     let plaintexts = logins.map(l => l.username).concat(logins.map(l => l.password));
>     let ciphertexts = await crypto.encryptMany(plaintexts);
>     let usernames = ciphertexts.slice(0, logins.length);
>     let passwords = ciphertexts.slice(logins.length);
>     let encType = crypto.defaultEncType;
>     for (let i = 0; i < logins.length; i++) {
>       let plaintextUsername = logins[i].username;
>       let plaintextPassword = logins[i].password;
>       logins[i].username = usernames[i];
>       logins[i].password = passwords[i];
>       this._storage.addLogin(logins[i], true);
>       // Reset the username and password to keep the same guarantees as addLogin
>       logins[i].username = plaintextUsername;
>       logins[i].password = plaintextPassword;
>     }
>   },
> ```
> 
> Does that make sense?

I guess the "Look for an existing entry" part of `addLogin` is the expensive part you want to avoid? The other parts don't seem that expensive.

Comment 8

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review217950

> I guess the "Look for an existing entry" part of `addLogin` is the expensive part you want to avoid? The other parts don't seem that expensive.

Correct, and we could excuse that check based on an isEncrypted argument, but we'd just be pushing up the above snippet of code into LoginHelper's maybeImportLogins. I'm fine with doing that, I was only clarifying since it seems separate from the initial desire to just avoid adding complexity to the two storage implementations.

Comment 9

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211414/#review219324

This looks reasonable to me - but I'd really rather have :keeler's eyes on this too.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review219408

This looks good. I believe the NSS API is thread-safe (it does have some global data, but it protects it with locks). Please add a simple API unit test to security/manager/ssl/tests/unit/test_sdr.js. The other comments are mostly style nits.

::: security/manager/ssl/SecretDecoderRing.cpp:38
(Diff revision 1)
> +                                 RefPtr<Promise>& aPromise) {
> +  nsCOMPtr<nsISecretDecoderRing> sdrService =
> +    do_GetService(NS_SECRETDECODERRING_CONTRACTID);
> +  InfallibleTArray<nsString> cipherTexts(plaintexts.Length());
> +
> +  nsresult rv;

Let's initialize this to something (`NS_ERROR_FAILURE`?) just so we don't use it uninitialized (I realize you ensure the array has at least one element at a higher level, but still).

::: security/manager/ssl/SecretDecoderRing.cpp:43
(Diff revision 1)
> +  nsresult rv;
> +  for (uint32_t i = 0; i < plaintexts.Length(); ++i) {
> +    const nsCString& plaintext = plaintexts[i];
> +    nsCString cipherText;
> +    rv = sdrService->EncryptString(plaintext, cipherText);
> +    cipherTexts.AppendElement(NS_ConvertASCIItoUTF16(cipherText));

So if `rv` is a failure, we probably don't want to append `cipherText` to the list in the first place.

::: security/manager/ssl/SecretDecoderRing.cpp:172
(Diff revision 1)
>  NS_IMETHODIMP
> +SecretDecoderRing::AsyncEncryptStrings(uint32_t plaintextsCount,
> +                                       const char16_t** plaintexts,
> +                                       JSContext* aCx,
> +                                       nsISupports** aPromise) {
> +  MOZ_ASSERT(NS_IsMainThread());

If this is essential, let's also enforce this in optimized code.

::: security/manager/ssl/SecretDecoderRing.cpp:180
(Diff revision 1)
> +  NS_ENSURE_STATE(aCx);
> +
> +  nsIGlobalObject* globalObject =
> +    xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));
> +
> +  NS_ENSURE_STATE(globalObject);

nit: for new code, I think it's best to avoid `NS_ENSURE_*` macros other than at the beginning of functions because they hide returns

Comment 11

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1426721 - Add async/bulk encryption interface to SDR

Since encryption can be somewhat CPU intensive, if we're encrypting
a large number of strings we want to be able to do so in a background
thread. This will be consumed by the profile migrators when importing
logins.

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

Comment 12

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

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

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8944906 [details]
Bug 1426721 - Add async/bulk encryption interface to SDR

https://reviewboard.mozilla.org/r/215052/#review220710

Looks good. For the record, I only reviewed the security/ parts, not the toolkit/ parts.

Comment 14

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8944906 [details]
Bug 1426721 - Add async/bulk encryption interface to SDR

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

Comment 15

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

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

Comment 16

[Alex Thayer [:alexical] (she/her)]

(In reply to David Keeler [:keeler] (use needinfo) from comment #13)
> Looks good. For the record, I only reviewed the security/ parts, not the
> toolkit/ parts.

Justin, since yours is the only name I can find on the crypto-SDR.js code, do you know who would be a good fit to review it / are you taking review requests currently?

Comment 17

[Matthew N. [:MattN]]

(In reply to Doug Thayer [:dthayer] from comment #16)
> (In reply to David Keeler [:keeler] (use needinfo) from comment #13)
> > Looks good. For the record, I only reviewed the security/ parts, not the
> > toolkit/ parts.
> 
> Justin, since yours is the only name I can find on the crypto-SDR.js code,
> do you know who would be a good fit to review it / are you taking review
> requests currently?

I can review the toolkit usage.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8944906 [details]
Bug 1426721 - Add async/bulk encryption interface to SDR

https://reviewboard.mozilla.org/r/215052/#review221008

::: toolkit/components/passwordmgr/crypto-SDR.js:102
(Diff revision 2)
>    /*
> +   * encryptMany
> +   *

Not that this isn't a style to be follow in other code and this file is just doing the doc comments "wrong" since it's old. The method name doesn't need to be repeated in new code and it should use `/**` with @param and @returns. You can leave it matching the other bad style though but just wanted to mention it.

::: toolkit/components/passwordmgr/crypto-SDR.js:137
(Diff revision 2)
> +      if (!wasLoggedIn && this.isLoggedIn)
> +        this._notifyObservers("passwordmgr-crypto-login");
> +      else if (canceledMP)
> +        this._notifyObservers("passwordmgr-crypto-loginCanceled");

Nit: always brace if/else, even with single-line bodies, in new code. (I realize you copied this from above)

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review221010

I still need to review LoginHelper.jsm but here's some comments in the meantime.

::: browser/components/migration/MigrationUtils.jsm:1011
(Diff revision 3)
>    insertLoginWrapper(login) {
>      this._importQuantities.logins++;

Reminding myself that this is remaining for the other browser migrators which will be converted later.

::: browser/components/migration/MigrationUtils.jsm:1012
(Diff revision 3)
>      }
>      return PlacesUtils.asyncHistory.updatePlaces(places, options, true);
>    },
>  
>    insertLoginWrapper(login) {
>      this._importQuantities.logins++;

Side note: seems like we probably should only increment this if we're successful here and in the new method (but I didn't look closely at the other counters).

::: toolkit/components/passwordmgr/nsILoginManagerStorage.idl:61
(Diff revision 3)
>     *
>     * Default values for the login's nsILoginMetaInfo properties will be
>     * created. However, if the caller specifies non-default values, they will
>     * be used instead.
>     */
> -  nsILoginInfo addLogin(in nsILoginInfo aLogin);
> +  nsILoginInfo addLogin(in nsILoginInfo aLogin, in boolean aPreEncrypted);

I believe this should be `[optional]` for clarity:
```idl
[optional] in boolean aPreEncrypted
```

::: toolkit/components/passwordmgr/storage-json.js:102
(Diff revision 3)
>    terminate() {
>      this._store._saver.disarm();
>      return this._store._save();
>    },
>  
> -  addLogin(login) {
> +  addLogin(login, preEncrypted) {

Nit: make it more obvious that the argument is optional and defaults to false:
`…, preEncrypted = false) {`

(same for storage-mozStorage please)

Comment 20

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review221010

> Side note: seems like we probably should only increment this if we're successful here and in the new method (but I didn't look closely at the other counters).

Yeah, honestly it's hard to say, since it's for telemetry, and really we'd probably want to know both how many we attempted and how many failed. The only usage for the `_QUANTITY` probes that I am aware of is for correlating them with the `_IMPORT_MS` and `_JANK_MS` probes, in which case both the attempted number and the successful number are relevant. In either case, the counts are not all consistent with each other (`insertManyBookmarksWrapper` counts successful imports, whereas `insertBookmarkWrapper` counts attempted), so I can file a follow-up to just make the counts consistent.

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review221546

Sorry for the delay, the diff of LoginHelper.jsm was hard to review because of the duplication. Hopefully we can remove `maybeImportLogin` soon. Maybe we could even remove it in this bug by changing consumers of `maybeImportLogin` to call `maybeImportLogins` with a new 1-item array: `maybeImportLogins([login])`? Changing `maybeImportLogin` to `maybeImportLogins` would make the diff clearer too btw. With `maybeImportLogin` removed, checkForDuplicatesAndMaybeUpdate would have only one caller and arguably wouldn't be needed. Up to you how to proceed, I'm just weary about this code be a bit fragile already and not properly maintained.

Thanks

::: toolkit/components/passwordmgr/LoginHelper.jsm:627
(Diff revision 3)
> -    // Now check for a login with the same username, where it may be that we have an
> -    // updated password.
> -    let foundMatchingLogin = false;
> -    for (let existingLogin of existingLogins) {
> -      if (login.username == existingLogin.username) {
> -        foundMatchingLogin = true;
> +    return Services.logins.addLogin(login);
> +  },
> +
> +  /**
> +   * Equivalent to maybeImportLogin, except asynchronous and for multiple logins.
> +   * @param {Object} loginDatas - For each login, the data that needs to be added.

Shouldn't this be `{Object[]}`?

::: toolkit/components/passwordmgr/LoginHelper.jsm:660
(Diff revision 3)
> +        Cu.reportError(e);
> +        continue;
> +      }
> +
> +      // First, we need to check the logins that we've already decided to add, to
> +      // see if this is a duplicate. This should mirror the login inside

s/login/logic/ ?

Comment 22

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review221546

I would like to, the problem is that maybeImportLogins is async, while maybeImportLogin isn't, which itself isn't a huge issue, but I suspect the overhead of encrypting each login individually in a background thread would be high. I suppose we could special-case one-item lists and not use a background thread, but I'd rather not complicate it since I do intend on following through shortly with converting the other profile migrators.

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

https://reviewboard.mozilla.org/r/211416/#review221546

ok, wfm

Comment 24

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8944906 [details]
Bug 1426721 - Add async/bulk encryption interface to SDR

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

Comment 25

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8941139 [details]
Bug 1426721 - Encrypt Chrome logins in background thread

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df447e1b1a38
Add async/bulk encryption interface to SDR r=keeler,MattN
https://hg.mozilla.org/integration/autoland/rev/edd801d576da
Encrypt Chrome logins in background thread r=keeler,MattN

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/df447e1b1a38
https://hg.mozilla.org/mozilla-central/rev/edd801d576da