fido: avoid hairpinning of FidoRequestHandlerBase callbacks

Currently, the |FidoRequestHandlerBase::AddAuthenticator| method
synchronously invokes the virtual |DispatchRequest| method, which
subclasses implement to process the request and invoke the response
handling callback. Most authenticators are instantiated by a
|FidoDiscovery| which does some asynchronous discovery work and then
calls |AddAuthenticator|.

Platform authenticators, on the other hand, are instantiated by
|MaybeAddPlatformAuthenticator|, which gets invoked synchronously with
request handler instantiation. This makes it possible to have a
synchronous code path from request handler instantation to response
callback invocation (i.e.  hairpinning). In those cases,
|AuthenticatorImpl| is unable to handle the response (see e.g.
https://cs.chromium.org/chromium/src/content/browser/webauth/authenticator_impl.cc?sq=package:chromium&dr&g=0&l=749).

This change makes |DispatchRequest| invocation asynchronous, which
eliminates any possibility of response callback hairpinning.

Bug: 678128
Change-Id: I6bf273775885abac1fa38fe12c964e930407cc7e
Reviewed-on: https://chromium-review.googlesource.com/1172917
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583051}

device/fido/fido_request_handler_base.cc

@@ -7,8 +7,11 @@
 #include <utility>
 
 #include "base/barrier_closure.h"
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
 #include "device/fido/fido_device.h"
 #include "device/fido/fido_task.h"
@@ -118,7 +121,6 @@ void FidoRequestHandlerBase::Start() {
   MaybeAddPlatformAuthenticator();
 }
 
-
 void FidoRequestHandlerBase::DiscoveryStarted(FidoDiscovery* discovery,
                                               bool success) {
   if (discovery->transport() == FidoTransportProtocol::kBluetoothLowEnergy) {
@@ -175,8 +177,13 @@ void FidoRequestHandlerBase::AddAuthenticator(
   FidoAuthenticator* authenticator_ptr = authenticator.get();
   active_authenticators_.emplace(authenticator->GetId(),
                                  std::move(authenticator));
-  if (!ShouldDeferRequestDispatchToUi(*authenticator_ptr))
-    DispatchRequest(authenticator_ptr);
+  if (!ShouldDeferRequestDispatchToUi(*authenticator_ptr)) {
+    // Post |DispatchRequest| into its own task. This avoids hairpinning, even
+    // if the authenticator immediately invokes the request callback.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&FidoRequestHandlerBase::DispatchRequest,
+                                  GetWeakPtr(), authenticator_ptr));
+  }
 
   if (observer_)
     observer_->FidoAuthenticatorAdded(*authenticator_ptr);

device/fido/fido_request_handler_base.h

@@ -16,6 +16,7 @@
 #include "base/component_export.h"
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string_piece_forward.h"
 #include "device/fido/fido_device_authenticator.h"
 #include "device/fido/fido_discovery.h"
@@ -106,6 +107,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
   }
 
  protected:
+  virtual base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() = 0;
+
   // Subclasses implement this method to dispatch their request onto the given
   // FidoAuthenticator. The FidoAuthenticator is owned by this
   // FidoRequestHandler and stored in active_authenticators().

device/fido/fido_request_handler_unittest.cc

@@ -110,6 +110,10 @@ class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
   }
   ~FakeFidoRequestHandler() override = default;
 
+  base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() override {
+    return weak_factory_.GetWeakPtr();
+  }
+
   void DispatchRequest(FidoAuthenticator* authenticator) override {
     static_cast<FakeFidoAuthenticator*>(authenticator)
         ->RunFakeTask(

device/fido/get_assertion_request_handler.cc

@@ -179,6 +179,10 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
 
 GetAssertionRequestHandler::~GetAssertionRequestHandler() = default;
 
+base::WeakPtr<FidoRequestHandlerBase> GetAssertionRequestHandler::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 void GetAssertionRequestHandler::DispatchRequest(
     FidoAuthenticator* authenticator) {
   // The user verification field of the request may be adjusted to the

device/fido/get_assertion_request_handler.h

@@ -47,6 +47,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) GetAssertionRequestHandler
 
  private:
   // FidoRequestHandlerBase:
+  base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() final;
   void DispatchRequest(FidoAuthenticator* authenticator) override;
 
   void HandleResponse(

device/fido/make_credential_request_handler.cc

@@ -127,6 +127,11 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
 
 MakeCredentialRequestHandler::~MakeCredentialRequestHandler() = default;
 
+base::WeakPtr<FidoRequestHandlerBase>
+MakeCredentialRequestHandler::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 void MakeCredentialRequestHandler::DispatchRequest(
     FidoAuthenticator* authenticator) {
   // The user verification field of the request may be adjusted to the

device/fido/make_credential_request_handler.h

@@ -50,6 +50,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) MakeCredentialRequestHandler
 
  private:
   // FidoRequestHandlerBase:
+  base::WeakPtr<FidoRequestHandlerBase> GetWeakPtr() final;
   void DispatchRequest(FidoAuthenticator* authenticator) final;
 
   void HandleResponse(