Reland "arc: Only call StopVm() if ARCVM is running"

This is a reland of 4bf05b95 with
updated ArcVmClientAdapterTest unitests.

Original change's description:
> arc: Only call StopVm() if ARCVM is running
>
> This change adds a check in ArcVmClientAdapter::StopArcInstance() to see
> if a VM is running (current_cid_ is set). If not, skip calling
> ConciergeClient::StopVm() and notify observers that ARCVM is stopped
> right away.
>
> BUG=b:169893632
> TEST=arc.AuthPerf.unmanaged_vm
>
> Change-Id: Ic927f122cacea0e89cacffbbcd67a217772dba43
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2462573
> Commit-Queue: Willie Koomson <wvk@google.com>
> Reviewed-by: Yusuke Sato <yusukes@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#816221}

BUG=b:169893632
TEST=components_unittests --gtest_filter="ArcVmClientAdapterTest.*"

Change-Id: I3f9440333f7339687e3a4bd36799de06376b9351
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2466559Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Commit-Queue: Willie Koomson <wvk@google.com>
Cr-Commit-Position: refs/heads/master@{#816649}

components/arc/session/arc_vm_client_adapter.cc

@@ -547,6 +547,13 @@ class ArcVmClientAdapter : public ArcClientAdapter,
           << "StopArcInstance is called during browser shutdown. Do nothing.";
       return;
     }
+    if (current_cid_ == kInvalidCid) {
+      // No VM is currently running, avoid calling ConciergeClient::StopVm().
+      // TODO(wvk): Once StartMiniArc() is implemented, use a DCHECK here
+      // instead.
+      OnArcInstanceStopped();
+      return;
+    }
 
     if (should_backup_log) {
       GetDebugDaemonClient()->BackupArcBugReport(

components/arc/session/arc_vm_client_adapter_unittest.cc

@@ -710,7 +710,9 @@ TEST_F(ArcVmClientAdapterTest, StopArcInstance_OnShutdown) {
 
 // Tests that StopArcInstance() immediately notifies the observer on failure.
 TEST_F(ArcVmClientAdapterTest, StopArcInstance_Fail) {
+  SetValidUserInfo();
   StartMiniArc();
+  UpgradeArc(true);
 
   // Inject failure.
   vm_tools::concierge::StopVmResponse response;
@@ -738,15 +740,11 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcVmProxyFailure) {
   EXPECT_FALSE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
@@ -802,15 +800,11 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcCreateDataFailure) {
   EXPECT_FALSE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
@@ -827,15 +821,11 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcVmMountMyFilesJobFail) {
   EXPECT_FALSE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
@@ -853,15 +843,11 @@ TEST_F(ArcVmClientAdapterTest,
   EXPECT_FALSE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
@@ -876,15 +862,11 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_NoUserId) {
   EXPECT_FALSE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
@@ -899,15 +881,11 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_NoSerial) {
   EXPECT_FALSE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
@@ -924,15 +902,11 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcVmFailure) {
   EXPECT_TRUE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
@@ -946,15 +920,11 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcVmFailureEmptyReply) {
   EXPECT_TRUE(GetTestConciergeClient()->start_arc_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());
 
-  // Try to stop the VM. StopVm will fail in this case because
-  // no VM is running.
-  vm_tools::concierge::StopVmResponse response;
-  response.set_success(false);
-  GetTestConciergeClient()->set_stop_vm_response(response);
+  // Try to stop the VM. No VM is running so StopVm() shouldn't be called.
   adapter()->StopArcInstance(/*on_shutdown=*/false,
                              /*should_backup_log=*/false);
   run_loop()->Run();
-  EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
 }