[CrOS Cellular] Fix two OtaActivatorImpl issues.

(1) The class originally assumed that every Cellular-capable device must
    provide a MEID value. However, it turns out that this is not the
    case, so this check was removed.
(2) The class assumed that it would always notify its delegate via
    InvokeOnFinishedCallback() asynchronously. However, if the device
    has already been activated before the process starts, the reply
    comes synchronously. This CL starts the activation flow as part of a
    new task to ensure that the process is asynchronous.

Bug: 968858
Change-Id: Ie1d57a2a38465c6e2f1eb93632c6dbc7dcaf40bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726821
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Auto-Submit: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682847}

chromeos/services/cellular_setup/cellular_setup_impl_unittest.cc

@@ -42,11 +42,13 @@ class FakeOtaActivatorFactory : public OtaActivatorImpl::Factory {
       base::OnceClosure on_finished_callback,
       NetworkStateHandler* network_state_handler,
       NetworkConnectionHandler* network_connection_handler,
-      NetworkActivationHandler* network_activation_handler) override {
+      NetworkActivationHandler* network_activation_handler,
+      scoped_refptr<base::TaskRunner> task_runner) override {
     EXPECT_TRUE(activation_delegate);
     EXPECT_TRUE(network_state_handler);
     EXPECT_TRUE(network_connection_handler);
     EXPECT_TRUE(network_activation_handler);
+    EXPECT_TRUE(task_runner);
 
     auto fake_ota_activator =
         std::make_unique<FakeOtaActivator>(std::move(on_finished_callback));

chromeos/services/cellular_setup/ota_activator_impl.cc

@@ -51,18 +51,19 @@ std::unique_ptr<OtaActivator> OtaActivatorImpl::Factory::Create(
     base::OnceClosure on_finished_callback,
     NetworkStateHandler* network_state_handler,
     NetworkConnectionHandler* network_connection_handler,
-    NetworkActivationHandler* network_activation_handler) {
+    NetworkActivationHandler* network_activation_handler,
+    scoped_refptr<base::TaskRunner> task_runner) {
   if (g_test_factory) {
     return g_test_factory->BuildInstance(
         std::move(activation_delegate), std::move(on_finished_callback),
         network_state_handler, network_connection_handler,
-        network_activation_handler);
+        network_activation_handler, task_runner);
   }
 
   return base::WrapUnique(new OtaActivatorImpl(
       std::move(activation_delegate), std::move(on_finished_callback),
       network_state_handler, network_connection_handler,
-      network_activation_handler));
+      network_activation_handler, task_runner));
 }
 
 // static
@@ -77,22 +78,16 @@ OtaActivatorImpl::OtaActivatorImpl(
     base::OnceClosure on_finished_callback,
     NetworkStateHandler* network_state_handler,
     NetworkConnectionHandler* network_connection_handler,
-    NetworkActivationHandler* network_activation_handler)
+    NetworkActivationHandler* network_activation_handler,
+    scoped_refptr<base::TaskRunner> task_runner)
     : OtaActivator(std::move(on_finished_callback)),
       activation_delegate_(std::move(activation_delegate)),
       network_state_handler_(network_state_handler),
       network_connection_handler_(network_connection_handler),
-      network_activation_handler_(network_activation_handler),
-      weak_ptr_factory_(this) {
-  network_state_handler_->AddObserver(this, FROM_HERE);
-
-  // If |activation_delegate_| becomes disconnected, the activation request is
-  // considered canceled.
-  activation_delegate_.set_connection_error_handler(base::BindOnce(
-      &OtaActivatorImpl::FinishActivationAttempt, base::Unretained(this),
-      mojom::ActivationResult::kFailedToActivate));
-
-  ChangeStateAndAttemptNextStep(State::kWaitingForValidSimToBecomePresent);
+      network_activation_handler_(network_activation_handler) {
+  task_runner->PostTask(FROM_HERE,
+                        base::BindOnce(&OtaActivatorImpl::StartActivation,
+                                       weak_ptr_factory_.GetWeakPtr()));
 }
 
 OtaActivatorImpl::~OtaActivatorImpl() {
@@ -151,6 +146,18 @@ const NetworkState* OtaActivatorImpl::GetCellularNetworkState() const {
       NetworkTypePattern::Cellular());
 }
 
+void OtaActivatorImpl::StartActivation() {
+  network_state_handler_->AddObserver(this, FROM_HERE);
+
+  // If |activation_delegate_| becomes disconnected, the activation request is
+  // considered canceled.
+  activation_delegate_.set_connection_error_handler(base::BindOnce(
+      &OtaActivatorImpl::FinishActivationAttempt, base::Unretained(this),
+      mojom::ActivationResult::kFailedToActivate));
+
+  ChangeStateAndAttemptNextStep(State::kWaitingForValidSimToBecomePresent);
+}
+
 void OtaActivatorImpl::ChangeStateAndAttemptNextStep(State state) {
   DCHECK_NE(state, state_);
   NET_LOG(DEBUG) << "OtaActivatorImpl: " << state_ << " => " << state << ".";
@@ -188,6 +195,7 @@ void OtaActivatorImpl::FinishActivationAttempt(
   network_state_handler_ = nullptr;
 
   NET_LOG(EVENT) << "Finished attempt with result " << activation_result << ".";
+
   if (activation_delegate_)
     activation_delegate_->OnActivationFinished(activation_result);
 
@@ -220,16 +228,14 @@ void OtaActivatorImpl::AttemptToDiscoverSim() {
   }
 
   // The device must have the properties required for the activation flow;
-  // namely, the operator name, MEID, IMEI, and MDN must be available. Return
+  // namely, the operator name, IMEI, and MDN must be available. Return
   // and wait to see if DevicePropertiesUpdated() is invoked with valid
   // properties.
   if (cellular_device->operator_name().empty() ||
-      cellular_device->meid().empty() || cellular_device->imei().empty() ||
-      cellular_device->mdn().empty()) {
+      cellular_device->imei().empty() || cellular_device->mdn().empty()) {
     NET_LOG(DEBUG) << "Insufficient activation data: "
                    << "Operator name: " << cellular_device->operator_name()
-                   << ", MEID: " << cellular_device->meid() << ", "
-                   << "IMEI: " << cellular_device->imei() << ", "
+                   << ", IMEI: " << cellular_device->imei() << ", "
                    << "MDN: " << cellular_device->mdn();
     return;
   }

chromeos/services/cellular_setup/ota_activator_impl.h

@@ -11,6 +11,7 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/network/network_state_handler_observer.h"
 #include "chromeos/services/cellular_setup/ota_activator.h"
 #include "chromeos/services/cellular_setup/public/mojom/cellular_setup.mojom.h"
@@ -51,7 +52,9 @@ class OtaActivatorImpl : public OtaActivator,
         base::OnceClosure on_finished_callback,
         NetworkStateHandler* network_state_handler,
         NetworkConnectionHandler* network_connection_handler,
-        NetworkActivationHandler* network_activation_handler);
+        NetworkActivationHandler* network_activation_handler,
+        scoped_refptr<base::TaskRunner> task_runner =
+            base::ThreadTaskRunnerHandle::Get());
     static void SetFactoryForTesting(Factory* test_factory);
     virtual ~Factory();
     virtual std::unique_ptr<OtaActivator> BuildInstance(
@@ -59,7 +62,8 @@ class OtaActivatorImpl : public OtaActivator,
         base::OnceClosure on_finished_callback,
         NetworkStateHandler* network_state_handler,
         NetworkConnectionHandler* network_connection_handler,
-        NetworkActivationHandler* network_activation_handler) = 0;
+        NetworkActivationHandler* network_activation_handler,
+        scoped_refptr<base::TaskRunner> task_runner) = 0;
   };
 
   ~OtaActivatorImpl() override;
@@ -81,7 +85,8 @@ class OtaActivatorImpl : public OtaActivator,
                    base::OnceClosure on_finished_callback,
                    NetworkStateHandler* network_state_handler,
                    NetworkConnectionHandler* network_connection_handler,
-                   NetworkActivationHandler* network_activation_handler);
+                   NetworkActivationHandler* network_activation_handler,
+                   scoped_refptr<base::TaskRunner> task_runner);
 
   // mojom::CarrierPortalHandler:
   void OnCarrierPortalStatusChange(mojom::CarrierPortalStatus status) override;
@@ -96,6 +101,7 @@ class OtaActivatorImpl : public OtaActivator,
   const DeviceState* GetCellularDeviceState() const;
   const NetworkState* GetCellularNetworkState() const;
 
+  void StartActivation();
   void ChangeStateAndAttemptNextStep(State state);
   void AttemptNextActivationStep();
   void FinishActivationAttempt(mojom::ActivationResult activation_result);
@@ -121,7 +127,7 @@ class OtaActivatorImpl : public OtaActivator,
   bool has_sent_metadata_ = false;
   bool has_called_complete_activation_ = false;
 
-  base::WeakPtrFactory<OtaActivatorImpl> weak_ptr_factory_;
+  base::WeakPtrFactory<OtaActivatorImpl> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(OtaActivatorImpl);
 };

chromeos/services/cellular_setup/ota_activator_impl_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
+#include "base/test/test_simple_task_runner.h"
 #include "chromeos/network/fake_network_activation_handler.h"
 #include "chromeos/network/fake_network_connection_handler.h"
 #include "chromeos/network/network_state_handler.h"
@@ -57,13 +58,15 @@ class CellularSetupOtaActivatorImplTest : public testing::Test {
   }
 
   void BuildOtaActivator() {
+    auto test_task_runner = base::MakeRefCounted<base::TestSimpleTaskRunner>();
     ota_activator_ = OtaActivatorImpl::Factory::Create(
         fake_activation_delegate_->GenerateInterfacePtr(),
         base::BindOnce(&CellularSetupOtaActivatorImplTest::OnFinished,
                        base::Unretained(this)),
         test_helper_.network_state_handler(),
         fake_network_connection_handler_.get(),
-        fake_network_activation_handler_.get());
+        fake_network_activation_handler_.get(), test_task_runner);
+    test_task_runner->RunUntilIdle();
     carrier_portal_handler_ptr_ = ota_activator_->GenerateInterfacePtr();
   }