Move FidoDiscovery::Start() invocation

Currently FidoDiscovery::Start() is called in the constructor of
FidoRequestHandlerBase class. This potentially leads to crash in
authenticator_impl_unittest.cc when FakeU2fDevice is used. When
FakeU2fDevice is used, the device is added in a synchronous manner as
soon as discovery starts. This leads to invocation of
FidoRequestHandlerBase::DeviceAdded() which then calls
CreateTaskForNewDevice() which is a pure abstract function. This has
been a non-issue until now as may unit tests does not actually call
AuthenticatorImpl::{MakeCredential, GetAssertion}() and because
AuthenticatorImpl constructor we used in unit tests has empty |protocols_|
field. This should be changed as we add more end to end tests in future.

Thus this CL moves invocation of FidoDiscovery::Start() from base class
constructor to constructor of implementing class and add hid transport to
AuthenticatorImpl::protocols_ in constructor.

Bug: 798573
Change-Id: Iaca4978dd06233f670b44b52a30ec192b5f2cb51
Reviewed-on: https://chromium-review.googlesource.com/1032258
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555939}

content/browser/webauth/authenticator_impl.cc

@@ -353,6 +353,11 @@ AuthenticatorImpl::AuthenticatorImpl(RenderFrameHost* render_frame_host,
       weak_factory_(this) {
   DCHECK(render_frame_host_);
   DCHECK(timer_);
+
+  protocols_.insert(device::FidoTransportProtocol::kUsbHumanInterfaceDevice);
+  if (base::FeatureList::IsEnabled(features::kWebAuthBle)) {
+    protocols_.insert(device::FidoTransportProtocol::kBluetoothLowEnergy);
+  }
 }
 
 AuthenticatorImpl::~AuthenticatorImpl() {}

device/fido/fido_request_handler_base.cc

@@ -24,7 +24,6 @@ FidoRequestHandlerBase::FidoRequestHandlerBase(
       continue;
     }
     discovery->set_observer(this);
-    discovery->Start();
     discoveries_.push_back(std::move(discovery));
   }
 }
@@ -46,6 +45,12 @@ void FidoRequestHandlerBase::CancelOngoingTasks(
   }
 }
 
+void FidoRequestHandlerBase::Start() {
+  for (const auto& discovery : discoveries_) {
+    discovery->Start();
+  }
+}
+
 void FidoRequestHandlerBase::DiscoveryStarted(FidoDiscovery* discovery,
                                               bool success) {}
 

device/fido/fido_request_handler_base.h

@@ -56,6 +56,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
 
  protected:
   virtual std::unique_ptr<FidoTask> CreateTaskForNewDevice(FidoDevice*) = 0;
+  void Start();
 
   TaskMap& ongoing_tasks() { return ongoing_tasks_; }
 

device/fido/fido_request_handler_unittest.cc

@@ -90,7 +90,9 @@ class FakeFidoRequestHandler : public FidoRequestHandler<std::vector<uint8_t>> {
       : FidoRequestHandler(nullptr /* connector */,
                            protocols,
                            std::move(callback)),
-        weak_factory_(this) {}
+        weak_factory_(this) {
+    Start();
+  }
   ~FakeFidoRequestHandler() override = default;
 
   std::unique_ptr<FidoTask> CreateTaskForNewDevice(

device/fido/get_assertion_request_handler.cc

@@ -20,7 +20,9 @@ GetAssertionRequestHandler::GetAssertionRequestHandler(
     SignResponseCallback completion_callback)
     : FidoRequestHandler(connector, protocols, std::move(completion_callback)),
       request_(std::move(request)),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  Start();
+}
 
 GetAssertionRequestHandler::~GetAssertionRequestHandler() = default;
 

device/fido/make_credential_request_handler.cc

@@ -24,7 +24,9 @@ MakeCredentialRequestHandler::MakeCredentialRequestHandler(
       request_parameter_(std::move(request_parameter)),
       authenticator_selection_criteria_(
           std::move(authenticator_selection_criteria)),
-      weak_factory_(this) {}
+      weak_factory_(this) {
+  Start();
+}
 
 MakeCredentialRequestHandler::~MakeCredentialRequestHandler() = default;