fido: don't block the UI thread during IsUVPAA()

The implementation of IsUserVerifyingPlatformAuthenticatorAvailable()
was synchronous, but every OS's implementation relies on some form of
IPC and/or dynamic loading (macOS, CrOS, Windows). This worked fine for
the most part, except for macOS where the implementation talks to the
Security Framework which tends to stall or crash at random for a small
number of users.

This CL doesn't directly address the underlying hangs or crashes yet,
but makes the IsUVPAA() interface asynchronous, moves the
implementations off of the UI thread and makes call to external
libraries on Windows and macOS with the CONTINUE_ON_SHUTDOWN trait.
This should ensure that if these calls do hang, the browser stays
responsive and they don't block browser shutdown.

Change-Id: I84fcff3331830f842d92468decd713a7fd57aaf1
Bug: 1168157, 1154063
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2683862
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#856961}

chrome/browser/metrics/authenticator_utility.cc

@@ -38,8 +38,12 @@ void ReportAvailability(bool available) {
 #if defined(OS_MAC)
 void ReportUVPlatformAuthenticatorAvailabilityWithConfig(
     base::Optional<device::fido::mac::AuthenticatorConfig> config) {
-  ReportAvailability(config &&
-                     content::IsUVPlatformAuthenticatorAvailable(*config));
+  if (!config) {
+    ReportAvailability(false);
+    return;
+  }
+  content::IsUVPlatformAuthenticatorAvailable(
+      *config, base::BindOnce(&ReportAvailability));
 }
 
 void ReportUVPlatformAuthenticatorAvailabilityMainThreadMac() {
@@ -91,13 +95,9 @@ void ReportUVPlatformAuthenticatorAvailability() {
                  base::BindOnce(
                      &ReportUVPlatformAuthenticatorAvailabilityMainThreadMac));
 #elif defined(OS_WIN)
-  std::unique_ptr<content::AuthenticatorRequestClientDelegate> client_delegate =
-      std::make_unique<content::AuthenticatorRequestClientDelegate>();
-  device::WinWebAuthnApi* win_webauthn_api =
-      device::WinWebAuthnApi::GetDefault();
-  ReportAvailability(
-      win_webauthn_api &&
-      content::IsUVPlatformAuthenticatorAvailable(win_webauthn_api));
+  content::IsUVPlatformAuthenticatorAvailable(
+      device::WinWebAuthnApi::GetDefault(),
+      base::BindOnce(&ReportAvailability));
 #elif BUILDFLAG(IS_CHROMEOS_ASH)
   // TODO(crbug.com/1181426): Reenable the IsUVPAA() startup metric on CrOS.
 #endif

chromeos/dbus/u2f/fake_u2f_client.cc

@@ -22,11 +22,12 @@ void FakeU2FClient::WaitForServiceToBeAvailable(
       FROM_HERE, base::BindOnce(std::move(callback), true));
 }
 
-base::Optional<u2f::IsUvpaaResponse> FakeU2FClient::IsUvpaaBlocking(
-    const u2f::IsUvpaaRequest& request) {
+void FakeU2FClient::IsUvpaa(const u2f::IsUvpaaRequest& request,
+                            DBusMethodCallback<u2f::IsUvpaaResponse> callback) {
   u2f::IsUvpaaResponse response;
   response.set_available(false);
-  return response;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), std::move(response)));
 }
 
 void FakeU2FClient::IsU2FEnabled(

chromeos/dbus/u2f/fake_u2f_client.h

@@ -24,8 +24,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_U2F) FakeU2FClient : public U2FClient {
   // U2FClient:
   void WaitForServiceToBeAvailable(
       WaitForServiceToBeAvailableCallback callback) override;
-  base::Optional<u2f::IsUvpaaResponse> IsUvpaaBlocking(
-      const u2f::IsUvpaaRequest& request) override;
+  void IsUvpaa(const u2f::IsUvpaaRequest& request,
+               DBusMethodCallback<u2f::IsUvpaaResponse> callback) override;
   void IsU2FEnabled(const u2f::IsUvpaaRequest& request,
                     DBusMethodCallback<u2f::IsUvpaaResponse> callback) override;
   void MakeCredential(

chromeos/dbus/u2f/u2f_client.cc

@@ -60,8 +60,8 @@ class U2FClientImpl : public U2FClient {
   // U2FClient:
   void WaitForServiceToBeAvailable(
       WaitForServiceToBeAvailableCallback callback) override;
-  base::Optional<u2f::IsUvpaaResponse> IsUvpaaBlocking(
-      const u2f::IsUvpaaRequest& request) override;
+  void IsUvpaa(const u2f::IsUvpaaRequest& request,
+               DBusMethodCallback<u2f::IsUvpaaResponse> callback) override;
   void IsU2FEnabled(const u2f::IsUvpaaRequest& request,
                     DBusMethodCallback<u2f::IsUvpaaResponse> callback) override;
   void MakeCredential(
@@ -106,26 +106,15 @@ void U2FClientImpl::WaitForServiceToBeAvailable(
   proxy_->WaitForServiceToBeAvailable(std::move(callback));
 }
 
-base::Optional<u2f::IsUvpaaResponse> U2FClientImpl::IsUvpaaBlocking(
-    const u2f::IsUvpaaRequest& request) {
+void U2FClientImpl::IsUvpaa(const u2f::IsUvpaaRequest& request,
+                            DBusMethodCallback<u2f::IsUvpaaResponse> callback) {
   dbus::MethodCall method_call(u2f::kU2FInterface, u2f::kU2FIsUvpaa);
   dbus::MessageWriter writer(&method_call);
   writer.AppendProtoAsArrayOfBytes(request);
-
-  std::unique_ptr<dbus::Response> dbus_response =
-      proxy_->CallMethodAndBlock(&method_call, kU2FShortTimeout);
-
-  if (!dbus_response) {
-    return base::nullopt;
-  }
-
-  dbus::MessageReader reader(dbus_response.get());
-  u2f::IsUvpaaResponse response;
-  if (!reader.PopArrayOfBytesAsProto(&response)) {
-    return base::nullopt;
-  }
-
-  return response;
+  proxy_->CallMethod(
+      &method_call, kU2FShortTimeout,
+      base::BindOnce(&U2FClientImpl::HandleResponse<u2f::IsUvpaaResponse>,
+                     weak_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void U2FClientImpl::IsU2FEnabled(

chromeos/dbus/u2f/u2f_client.h

@@ -44,9 +44,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_U2F) U2FClient {
   // or fingerprint is enrolled). The name is short for
   // IsUserVerifyingPlatformAuthenticatorAvailable(), which is a method defined
   // in the WebAuthn spec.
-  // TODO(martinkr): Change to take a callback.
-  virtual base::Optional<u2f::IsUvpaaResponse> IsUvpaaBlocking(
-      const u2f::IsUvpaaRequest& request) = 0;
+  virtual void IsUvpaa(const u2f::IsUvpaaRequest& request,
+                       DBusMethodCallback<u2f::IsUvpaaResponse> callback) = 0;
 
   // Returns whether the legacy enterprise policy to enable a U2F authenticator
   // that requires a power button press to register or sign with a credential is

content/browser/webauth/authenticator_common.cc

@@ -19,8 +19,6 @@
 #include "base/rand_util.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversion_utils.h"
-#include "base/task/task_traits.h"
-#include "base/task/thread_pool.h"
 #include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
@@ -512,9 +510,11 @@ void IsUserVerifyingPlatformAuthenticatorAvailableImpl(
 #if defined(OS_MAC)
   const base::Optional<device::fido::mac::AuthenticatorConfig> config =
       delegate->GetTouchIdAuthenticatorConfig();
-  std::move(callback).Run(config &&
-                          IsUVPlatformAuthenticatorAvailable(*config));
-  return;
+  if (!config) {
+    std::move(callback).Run(false);
+    return;
+  }
+  IsUVPlatformAuthenticatorAvailable(*config, std::move(callback));
 #elif defined(OS_WIN)
   // TODO(crbug.com/908622): Enable platform authenticators in Incognito on
   // Windows once the API allows triggering an adequate warning dialog.
@@ -523,19 +523,12 @@ void IsUserVerifyingPlatformAuthenticatorAvailableImpl(
     return;
   }
 
-  std::move(callback).Run(IsUVPlatformAuthenticatorAvailable(
-      discovery_factory->win_webauthn_api()));
-  return;
+  IsUVPlatformAuthenticatorAvailable(discovery_factory->win_webauthn_api(),
+                                     std::move(callback));
 #elif BUILDFLAG(IS_CHROMEOS_ASH)
-  // ChromeOS needs to do a dbus call to determine platform authenticator
-  // availability. The call is fast in practice, but nonetheless may
-  // theoretically block.
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::TaskPriority::USER_BLOCKING, base::MayBlock()},
-      base::BindOnce(&IsUVPlatformAuthenticatorAvailable), std::move(callback));
+  IsUVPlatformAuthenticatorAvailable(std::move(callback));
 #else
   std::move(callback).Run(false);
-  return;
 #endif
 }
 

content/browser/webauth/authenticator_impl_unittest.cc

@@ -857,11 +857,10 @@ TEST(ClientDataSerializationTest, Sign) {
 }
 
 TEST_F(AuthenticatorImplTest, TestMakeCredentialTimeout) {
-  // The VirtualFidoAuthenticator simulates a tap immediately after it gets the
-  // request. Replace by the real discovery that will wait until timeout.
-  AuthenticatorEnvironmentImpl::GetInstance()
-      ->ReplaceDefaultDiscoveryFactoryForTesting(
-          std::make_unique<device::FidoDiscoveryFactory>());
+  // Don't provide an authenticator tap so the request times out.
+  virtual_device_factory_->mutable_state()->simulate_press_callback =
+      base::BindLambdaForTesting(
+          [&](device::VirtualFidoDevice* device) { return false; });
   NavigateAndCommit(GURL(kTestOrigin1));
 
   PublicKeyCredentialCreationOptionsPtr options =

content/browser/webauth/is_uvpaa.cc

@@ -25,31 +25,39 @@
 namespace content {
 
 #if defined(OS_MAC)
-bool IsUVPlatformAuthenticatorAvailable(
+void IsUVPlatformAuthenticatorAvailable(
     const content::AuthenticatorRequestClientDelegate::
-        TouchIdAuthenticatorConfig& config) {
-  return device::fido::mac::TouchIdAuthenticator::IsAvailable(config);
+        TouchIdAuthenticatorConfig& config,
+    IsUVPlatformAuthenticatorAvailableCallback callback) {
+  device::fido::mac::TouchIdAuthenticator::IsAvailable(config,
+                                                       std::move(callback));
 }
 
 #elif defined(OS_WIN)
-bool IsUVPlatformAuthenticatorAvailable(
-    device::WinWebAuthnApi* win_webauthn_api) {
-  return base::FeatureList::IsEnabled(device::kWebAuthUseNativeWinApi) &&
-         device::WinWebAuthnApiAuthenticator::
-             IsUserVerifyingPlatformAuthenticatorAvailable(win_webauthn_api);
+void IsUVPlatformAuthenticatorAvailable(
+    device::WinWebAuthnApi* win_webauthn_api,
+    IsUVPlatformAuthenticatorAvailableCallback callback) {
+  device::WinWebAuthnApiAuthenticator::
+      IsUserVerifyingPlatformAuthenticatorAvailable(win_webauthn_api,
+                                                    std::move(callback));
 }
 
 #elif BUILDFLAG(IS_CHROMEOS_ASH)
-bool IsUVPlatformAuthenticatorAvailable() {
-  return base::FeatureList::IsEnabled(
-             device::kWebAuthCrosPlatformAuthenticator) &&
-         device::ChromeOSAuthenticator::
-             IsUVPlatformAuthenticatorAvailableBlocking();
+void IsUVPlatformAuthenticatorAvailable(
+    IsUVPlatformAuthenticatorAvailableCallback callback) {
+  if (!base::FeatureList::IsEnabled(
+          device::kWebAuthCrosPlatformAuthenticator)) {
+    std::move(callback).Run(false);
+    return;
+  }
+  device::ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable(
+      std::move(callback));
 }
 
 #else
-bool IsUVPlatformAuthenticatorAvailable() {
-  return false;
+void IsUVPlatformAuthenticatorAvailable(
+    IsUVPlatformAuthenticatorAvailableCallback callback) {
+  std::move(callback).Run(false);
 }
 #endif
 

content/public/browser/is_uvpaa.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_PUBLIC_BROWSER_IS_UVPAA_H_
 #define CONTENT_PUBLIC_BROWSER_IS_UVPAA_H_
 
+#include "base/callback.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
 #include "content/common/content_export.h"
@@ -23,23 +24,31 @@ namespace content {
 // OS. This is exposed through the content public API for the purpose of
 // reporting startup metrics.
 
+using IsUVPlatformAuthenticatorAvailableCallback =
+    base::OnceCallback<void(bool is_available)>;
+
 #if defined(OS_MAC)
-CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable(
+CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable(
     const content::AuthenticatorRequestClientDelegate::
-        TouchIdAuthenticatorConfig&);
+        TouchIdAuthenticatorConfig&,
+    IsUVPlatformAuthenticatorAvailableCallback);
 
 #elif defined(OS_WIN)
-CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable(device::WinWebAuthnApi*);
+CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable(
+    device::WinWebAuthnApi*,
+    IsUVPlatformAuthenticatorAvailableCallback);
 
 #elif BUILDFLAG(IS_CHROMEOS_ASH)
-CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable();
+CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable(
+    IsUVPlatformAuthenticatorAvailableCallback);
 
 #else
 // Always returns false. On Android IsUVPlatformAuthenticatorAvailable() is
 // called on GMSCore from Java and is not proxied from here because there could
 // be performance costs to calling it outside of an actual WebAuthn API
 // invocation.
-CONTENT_EXPORT bool IsUVPlatformAuthenticatorAvailable();
+CONTENT_EXPORT void IsUVPlatformAuthenticatorAvailable(
+    IsUVPlatformAuthenticatorAvailableCallback);
 #endif
 
 }  // namespace content

device/fido/cros/authenticator.cc

@@ -331,14 +331,18 @@ void ChromeOSAuthenticator::OnCancelResponse(
   }
 }
 
-// static
-bool ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailableBlocking() {
-  base::Optional<u2f::IsUvpaaResponse> response =
-      chromeos::U2FClient::Get()->IsUvpaaBlocking(u2f::IsUvpaaRequest());
-  return response && response->available();
+void ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable(
+    base::OnceCallback<void(bool is_available)> callback) {
+  chromeos::U2FClient::Get()->IsUvpaa(
+      u2f::IsUvpaaRequest(),
+      base::BindOnce(
+          [](base::OnceCallback<void(bool is_available)> callback,
+             base::Optional<u2f::IsUvpaaResponse> response) {
+            std::move(callback).Run(response && response->available());
+          },
+          std::move(callback)));
 }
 
-// static
 void ChromeOSAuthenticator::IsPowerButtonModeEnabled(
     base::OnceCallback<void(bool is_enabled)> callback) {
   chromeos::U2FClient::Get()->IsU2FEnabled(

device/fido/cros/authenticator.h

@@ -36,14 +36,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) ChromeOSAuthenticator
       const CtapGetAssertionRequest& request,
       base::OnceCallback<void(bool has_credential)> callback);
 
-  // Returns whether the platform authenticator is available, which is true if
-  // the current user has a PIN set up or biometrics enrolled.
-  //
-  // Since this call makes a (quick) dbus call, it is potentially blocking and
-  // should not run on the main thread/sequence.
-  //
-  // TODO(crbug.com/1154063): Refactor IsUVPAA() to be async.
-  static bool IsUVPlatformAuthenticatorAvailableBlocking();
+  // Invokes |callback| with a bool indicating  whether the platform
+  // authenticator is available, which is true if the current user has a PIN set
+  // up or biometrics enrolled.
+  static void IsUVPlatformAuthenticatorAvailable(
+      base::OnceCallback<void(bool is_available)> callback);
 
   // Invokes |callback| with a bool indicating whether the legacy U2F
   // authenticator, which uses the power button for user presence checking, is

device/fido/cros/discovery.cc

@@ -5,8 +5,7 @@
 #include "device/fido/cros/discovery.h"
 
 #include "base/bind.h"
-#include "base/task/task_traits.h"
-#include "base/task/thread_pool.h"
+#include "base/logging.h"
 #include "chromeos/dbus/u2f/u2f_client.h"
 
 namespace device {
@@ -57,14 +56,7 @@ void FidoChromeOSDiscovery::OnU2FServiceAvailable(bool u2f_service_available) {
     return;
   }
 
-  AddAuthenticatorIfIsUVPAA();
-}
-
-void FidoChromeOSDiscovery::AddAuthenticatorIfIsUVPAA() {
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::TaskPriority::USER_BLOCKING, base::MayBlock()},
-      base::BindOnce(
-          &ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailableBlocking),
+  ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable(
       base::BindOnce(&FidoChromeOSDiscovery::MaybeAddAuthenticator,
                      weak_factory_.GetWeakPtr()));
 }
@@ -82,7 +74,9 @@ void FidoChromeOSDiscovery::MaybeAddAuthenticator(bool is_available) {
 void FidoChromeOSDiscovery::OnHasLegacyU2fCredential(bool has_credential) {
   DCHECK(!authenticator_);
   if (!has_credential) {
-    AddAuthenticatorIfIsUVPAA();
+    ChromeOSAuthenticator::IsUVPlatformAuthenticatorAvailable(
+        base::BindOnce(&FidoChromeOSDiscovery::MaybeAddAuthenticator,
+                       weak_factory_.GetWeakPtr()));
     return;
   }
 

device/fido/cros/discovery.h

@@ -7,9 +7,12 @@
 
 #include <memory>
 
+#include "base/callback.h"
 #include "base/component_export.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "device/fido/cros/authenticator.h"
+#include "device/fido/ctap_get_assertion_request.h"
 #include "device/fido/fido_discovery_base.h"
 
 namespace device {
@@ -29,7 +32,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoChromeOSDiscovery
 
  private:
   void OnU2FServiceAvailable(bool u2f_service_available);
-  void AddAuthenticatorIfIsUVPAA();
   void MaybeAddAuthenticator(bool is_available);
   void OnHasLegacyU2fCredential(bool has_credential);