Soon there will be more than one implementation of nsIU2FToken, and for that situation to function properly, dom/u2f/U2F.cpp must interact with multiple tokens in parallel. In particular, the semantics necessary are: When calling Register(): * check isCompatible() across all nsIU2FTokens. * check isRegistered() for all RegisteredKeys across all compatible nsIU2FTokens. - If any return DEVICE_INELIGIBLE, stop and return that result. * call Register() for all RegisterRequests across all compatible nsIU2FTokens. - The first to return a non-error is the result. Cancel all others. A similar approach is needed for Sign(). This effort should also at least lay the groundwork for handling timeouts.

Comment on attachment 8789947 [details] Bug 1297552 - Perform U2F hash operations more efficiently https://reviewboard.mozilla.org/r/77972/#review77148 I think I get the general idea here, but there's a lot going on in this one patch. Maybe a good way to make progress would be to break this up into smaller patches that each do one item from the "does these things" list in the commit message. Overall, I think this is a good path forward if a) it gets us the timeout functionality fairly easily and b) we're sure that it's actually worth it (how many concurrent operations are we expecting sites to want to do? How often will this code actually be run?) I just want to make sure we're not spending a lot of effort optimizing something that doesn't need it. ::: dom/u2f/U2F.h:211 (Diff revision 1) > +}; > + > +// U2FThreads run to completion, performing a single U2F operation such as > +// registering, or signing. > +class U2FThread : public Runnable, > + public nsNSSShutDownObject Style comment: ` class U2FThread : public Runnable , public nsNSSShutDownObject ` ::: dom/u2f/U2F.cpp:78 (Diff revision 1) > +{} > + > +U2FStatus::~U2FStatus() > +{} > + > +void U2FStatus::WaitGroupAdd() style nit: return values should go on their own lines ::: dom/u2f/U2F.cpp:400 (Diff revision 1) > - ReturnError(ErrorCode::DEVICE_INELIGIBLE); > + mPromise.Reject(ErrorCode::DEVICE_INELIGIBLE, __func__); > - return NS_OK; > + return NS_OK; > - } > + } > + > + CryptoBuffer signatureData; > + uint8_t* buffer; Is there a utility class that encapsulates this paradigm of passing out a pointer to a pointer that will then have some memory allocated for it that we copy elsewhere and then free? I think we have a couple instances of this in this code (and elsewhere) - it might be nice to create such a thing that handles the memory freeing for us. ::: dom/u2f/U2F.cpp:434 (Diff revision 1) > - RegisterRequest request(mRegisterRequests[i]); > + response.mClientData.Construct(clientDataBase64); > + response.mSignatureData.Construct(signatureDataBase64); > + response.mErrorCode.Construct(static_cast<uint32_t>(ErrorCode::OK)); > + > + nsString responseStr; > + if (NS_WARN_IF(!response.ToJSON(responseStr))) { So this is main-thread-only, right? Let's assert that. ::: dom/u2f/U2F.cpp:1031 (Diff revision 1) > > - EvaluateAppIDAndRunTask(signTask); > + if (!mInitialized) { > + aRv.Throw(NS_ERROR_NOT_AVAILABLE); > + return; > + } > + RefPtr<U2FSignThread> thread = new U2FSignThread(mOrigin, aAppId, aChallenge, So, does this create a new thread each time? Or does it just assign one thread from the pool with some sort of limiting?

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3) > Comment on attachment 8789947 [details] > Bug 1297552 - [DO NOT LAND] Use MozPromise to run U2F operations in parallel. > > https://reviewboard.mozilla.org/r/77972/#review77148 > > I think I get the general idea here, but there's a lot going on in this one > patch. Maybe a good way to make progress would be to break this up into > smaller patches that each do one item from the "does these things" list in > the commit message. I'll see what I can do to pull the patch apart as you said, probably into: 1) Disable non-e10s 2) Moving the hash calculations to once-per call 3) Use MozPromises > > Overall, I think this is a good path forward if a) it gets us the timeout > functionality fairly easily and b) we're sure that it's actually worth it > (how many concurrent operations are we expecting sites to want to do? How > often will this code actually be run?) I just want to make sure we're not > spending a lot of effort optimizing something that doesn't need it. Fairly stated. The main issue is just: The Authenticators must run in parallel, and they must race. The first to complete (or timeout) must complete the callback, and the others should cancel as soon as they can. I believe this is the minimum complexity possible while using MozPromises - I'm not sure if it'd be simpler without. {snip the specific comments, which I'll address later} > ::: dom/u2f/U2F.cpp:1031 > (Diff revision 1) > > > > - EvaluateAppIDAndRunTask(signTask); > > + if (!mInitialized) { > > + aRv.Throw(NS_ERROR_NOT_AVAILABLE); > > + return; > > + } > > + RefPtr<U2FSignThread> thread = new U2FSignThread(mOrigin, aAppId, aChallenge, > > So, does this create a new thread each time? Or does it just assign one > thread from the pool with some sort of limiting? It queues it for execution in the `SharedThreadPool` named `mThreadPool`, which handles spinning up and down idle threads. In keeping with other usage in Gecko, U2FThread is _not_ a thread, it's a run-to-completion work unit that runs on a thread. Perhaps I should rename `U2FThread` to `U2FRunnable`?

Sorry for the delay in getting this review updated with the split-out patch. This patch splits things apart, fixes the shutdown crash by using SharedThreadPool correctly, renames the U2FThread to U2FRunnable, and rebases on the Bug 1281932 intermittent fix. Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e875c5280889523c886d9f2c2a623416b1c5d23c&selectedJob=28931956

Comment on attachment 8789947 [details] Bug 1297552 - Perform U2F hash operations more efficiently https://reviewboard.mozilla.org/r/77972/#review83588 Sounds good. ::: dom/u2f/U2F.cpp:180 (Diff revision 2) > } > } > } > > + // Hash the AppID into the AppParam > + SECStatus srv; nit: declare this when it's used ::: dom/u2f/U2F.cpp:185 (Diff revision 2) > + SECStatus srv; > + nsCString cAppId = NS_ConvertUTF16toUTF8(mAppId); > + CryptoBuffer appParam; > + if (!appParam.SetLength(SHA256_LENGTH, fallible)) { > + ReturnError(ErrorCode::OTHER_ERROR); > + return NS_ERROR_FAILURE; This is OOM, right? ::: dom/u2f/U2F.cpp:334 (Diff revision 2) > ReturnError(ErrorCode::OTHER_ERROR); > return NS_ERROR_FAILURE; > } > > + // Hash the AppID into the AppParam > + SECStatus srv; nit: same here ::: dom/u2f/U2F.cpp:339 (Diff revision 2) > + SECStatus srv; > + nsCString cAppId = NS_ConvertUTF16toUTF8(mAppId); > + CryptoBuffer appParam; > + if (!appParam.SetLength(SHA256_LENGTH, fallible)) { > + ReturnError(ErrorCode::OTHER_ERROR); > + return NS_ERROR_FAILURE; OOM here

Comment on attachment 8799840 [details] Bug 1297552 - Only permit U2F operations in e10s mode https://reviewboard.mozilla.org/r/84932/#review83596 Being used to things that can only run in the parent process (and/or if e10s is disabled) this does seem a bit strange, but since we're shipping e10s and if this simplifies things, this sounds good. ::: dom/u2f/U2F.cpp:610 (Diff revision 1) > + } > + > + // This only functions in e10s mode > + if (XRE_IsParentProcess()) { > + MOZ_LOG(gWebauthLog, LogLevel::Debug, > + ("Is non-e10s Process, U2F not available")); nit: indentation

Comment on attachment 8799842 [details] Bug 1297552 - Reorder parts of U2F.cpp https://reviewboard.mozilla.org/r/84936/#review83598

Comment on attachment 8799843 [details] Bug 1297552 - Disable non-e10s tests https://reviewboard.mozilla.org/r/84938/#review83600 Might make sense to fold this in with part 2, but it's not a big deal.

Comment on attachment 8799841 [details] Bug 1297552 - Use MozPromise to run U2F operations in parallel. https://reviewboard.mozilla.org/r/84934/#review83602 Well, this is a fairly large, somewhat complicated change, but I think I understand it, and it looks like it should work. I noted a few nits and some things we should probably address, but I don't really feel like I'll need to look at the interdiff unless you want me to double-check it, so r=me with comments addressed. ::: dom/u2f/U2F.h:211 (Diff revision 1) > +}; > + > +// U2FRunnables run to completion, performing a single U2F operation such as > +// registering, or signing. > +class U2FRunnable : public Runnable, > + public nsNSSShutDownObject nit: it can be nice stylistically to put the ',' from the previous line on this line below the ':' (although I see that this file already has instances where it doesn't follow this convention) ::: dom/u2f/U2F.h:215 (Diff revision 1) > +class U2FRunnable : public Runnable, > + public nsNSSShutDownObject > +{ > +public: > + U2FRunnable(const nsAString& aOrigin, > + const nsAString& aAppId); nit: let's not split lines where there's enough space to put everything on one line ::: dom/u2f/U2F.cpp:78 (Diff revision 1) > {} > > -U2FTask::~U2FTask() > +U2FStatus::~U2FStatus() > {} > > -U2FRegisterTask::U2FRegisterTask(const nsAString& aOrigin, > +void U2FStatus::WaitGroupAdd() nit: return type (such as it is) on its own line ::: dom/u2f/U2F.cpp:91 (Diff revision 1) > + > +void U2FStatus::WaitGroupDone() > { > - nsNSSShutDownPreventionLock locker; > + ReentrantMonitorAutoEnter mon(mReentrantMonitor); > > - if (isAlreadyShutDown()) { > + mCount -= 1; Should we assert that mCount >= 0 after this? ::: dom/u2f/U2F.cpp:113 (Diff revision 1) > + > + MOZ_ASSERT(mCount == 0); > + MOZ_LOG(gWebauthLog, LogLevel::Debug, > + ("U2FStatus::Wait completed, now count=%d stopped=%d", mCount, > + mIsStopped)); > - return; > + return; no need for an explicit return here :) ::: dom/u2f/U2F.cpp:277 (Diff revision 1) > - } > +} > > - // Hash the AppID into the AppParam > - SECStatus srv; > - nsCString cAppId = NS_ConvertUTF16toUTF8(mAppId); > - CryptoBuffer appParam; > +NS_IMETHODIMP > +U2FRegisterTask::Run() > +{ > + nsNSSShutDownPreventionLock locker; Does this need to hold the nsNSSShutDownPreventionLock directly? I'm not seeing any direct NSS usage... (along similar lines, does U2FRegisterTask need to be an nsNSSShutDownObject at all? (Same question with U2FSignTask, looks like)) ::: dom/u2f/U2F.cpp:310 (Diff revision 1) > + return NS_ERROR_FAILURE; > - } > + } > > - CryptoBuffer clientData; > - nsresult rv = AssembleClientData(mOrigin, kFinishEnrollment, > - request.mChallenge.Value(), > + MOZ_ASSERT(buffer); > + CryptoBuffer regData; > + regData.Assign(buffer, bufferlen); Hmmm - maybe this is a result of not being rebased on the most recent trunk, but we need to handle this not succeeding, right? ::: dom/u2f/U2F.cpp:314 (Diff revision 1) > - nsresult rv = AssembleClientData(mOrigin, kFinishEnrollment, > - request.mChallenge.Value(), > - clientData); > - if (NS_WARN_IF(NS_FAILED(rv))) { > - ReturnError(ErrorCode::OTHER_ERROR); > + CryptoBuffer regData; > + regData.Assign(buffer, bufferlen); > + free(buffer); > + > + // Assemble a response object to return > + nsString clientDataBase64, registrationDataBase64; nit: one declaration per line ::: dom/u2f/U2F.cpp:365 (Diff revision 1) > +} > + > +NS_IMETHODIMP > +U2FSignTask::Run() > +{ > + nsNSSShutDownPreventionLock locker; Same here with the NSS lock. ::: dom/u2f/U2F.cpp:409 (Diff revision 1) > - ReturnError(ErrorCode::OTHER_ERROR); > + mPromise.Reject(ErrorCode::OTHER_ERROR, __func__); > - return NS_ERROR_FAILURE; > + return NS_ERROR_FAILURE; > - } > + } > > - MOZ_ASSERT(buffer); > + MOZ_ASSERT(buffer); > - regData.Assign(buffer, bufferlen); > + signatureData.Assign(buffer, bufferlen); Same here with handling failure. ::: dom/u2f/U2F.cpp:413 (Diff revision 1) > - // Try another request > - continue; > - } > > - // Assemble a response object to return > + // Assemble a response object to return > - nsString clientDataBase64, registrationDataBase64; > + nsString clientDataBase64, signatureDataBase64, keyHandleBase64; nit: one declaration per line again ::: dom/u2f/U2F.cpp:487 (Diff revision 1) > + mRegisteredKeys.SetLength(aRegisteredKeys.Length()); > + for (size_t i = 0; i < aRegisteredKeys.Length(); ++i) { > + RegisteredKey key(aRegisteredKeys[i]); > + > + // Check for required attributes > + if (!key.mVersion.WasPassed() || !key.mKeyHandle.WasPassed()) { It seems like this could result in empty slots in mRegisteredKeys and it's not clear that the code that consumes these keys handles that properly. Maybe just use .AppendElement()? ::: dom/u2f/U2F.cpp:601 (Diff revision 1) > if (!appParam.SetLength(SHA256_LENGTH, fallible)) { > - ReturnError(ErrorCode::OTHER_ERROR); > return NS_ERROR_FAILURE; > } > > srv = PK11_HashBuf(SEC_OID_SHA256, appParam.Elements(), If we used nsICryptoHash, I don't think U2FRunnable would need to be an nsNSSShutDownObject, but maybe that's an improvement for another bug. ::: dom/u2f/U2F.cpp:699 (Diff revision 1) > + mRegisteredKeys.SetLength(aRegisteredKeys.Length()); > + for (size_t i = 0; i < aRegisteredKeys.Length(); ++i) { > + RegisteredKey key(aRegisteredKeys[i]); > + > + // Check for required attributes > + if (!key.mVersion.WasPassed() || !key.mKeyHandle.WasPassed()) { Same idea here with having gaps in mRegisteredKeys. ::: dom/u2f/U2F.cpp:779 (Diff revision 1) > - ReturnError(ErrorCode::OTHER_ERROR); > - return NS_ERROR_FAILURE; > + return NS_ERROR_FAILURE; > - } > + } > > + // Search the signing requests for one a token can fulfill > + for (size_t i = 0; i < mRegisteredKeys.Length(); i += 1) { For these iterator-like loops, can we just do `for (LocalRegistered key : mRegisteredKeys)` etc.?

Comment on attachment 8799841 [details] Bug 1297552 - Use MozPromise to run U2F operations in parallel. https://reviewboard.mozilla.org/r/84934/#review83602 OK, thanks Keeler! I'm going to ask you to double-check it since I needed to move the `nsNSSShutDownPreventionLock` logic out of U2F*Task and into U2F*Runnable; always good to make sure I didn't do it wrong. > Does this need to hold the nsNSSShutDownPreventionLock directly? I'm not seeing any direct NSS usage... (along similar lines, does U2FRegisterTask need to be an nsNSSShutDownObject at all? (Same question with U2FSignTask, looks like)) You're right; I left that structure in there from when these actually did the hashes. Thank you. > Hmmm - maybe this is a result of not being rebased on the most recent trunk, but we need to handle this not succeeding, right? Yeah, this is fixed in the rebase. > It seems like this could result in empty slots in mRegisteredKeys and it's not clear that the code that consumes these keys handles that properly. Maybe just use .AppendElement()? Very, very good catch. Thank you. Did as you recommended. > If we used nsICryptoHash, I don't think U2FRunnable would need to be an nsNSSShutDownObject, but maybe that's an improvement for another bug. Cool; added a note, but not opening a bug at this time. > For these iterator-like loops, can we just do `for (LocalRegistered key : mRegisteredKeys)` etc.? Hmmm, I guess we can now!

Comment on attachment 8799841 [details] Bug 1297552 - Use MozPromise to run U2F operations in parallel. https://reviewboard.mozilla.org/r/84934/#review83994

Comment on attachment 8799841 [details] Bug 1297552 - Use MozPromise to run U2F operations in parallel. https://reviewboard.mozilla.org/r/84934/#review84220 LGTM! ::: dom/u2f/U2F.cpp:456 (Diff revisions 1 - 3) > MOZ_ASSERT(NS_IsMainThread()); > > // The WebIDL dictionary types RegisterRequest and RegisteredKey cannot > // be copied to this thread, so store them serialized. > - mRegisterRequests.SetLength(aRegisterRequests.Length()); > for (size_t i = 0; i < aRegisterRequests.Length(); ++i) { I think there are a few more places where we could use the loop iterator style rather than loop index style, but it's not a big deal. ::: dom/u2f/U2F.cpp:498 (Diff revisions 1 - 3) > } > } > > U2FRegisterRunnable::~U2FRegisterRunnable() > -{} > +{ > + nsNSSShutDownPreventionLock locker; Hmmm - we should be able to put this in ~U2FRunnable and share the code, right? (Of course, this only works as long as the subclasses never have to directly release NSS resources, but if we're eventually going to have these not be nsNSSShutDownObjects anyway, I think it's fine for now.)

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f824c01ff5ca Perform U2F hash operations more efficiently r=keeler https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9ddecd3f45 Only permit U2F operations in e10s mode r=keeler https://hg.mozilla.org/integration/mozilla-inbound/rev/8ffab2bebce5 Use MozPromise to run U2F operations in parallel. r=keeler https://hg.mozilla.org/integration/mozilla-inbound/rev/14d6e492e2bb Reorder parts of U2F.cpp r=keeler

https://hg.mozilla.org/mozilla-central/rev/f824c01ff5ca https://hg.mozilla.org/mozilla-central/rev/bf9ddecd3f45 https://hg.mozilla.org/mozilla-central/rev/8ffab2bebce5 https://hg.mozilla.org/mozilla-central/rev/14d6e492e2bb