arcvm: Don't call Concierge's StopVm during browser shutdown

This is strictly unnecessary because either vm_concierge.conf job
(in case of user session termination) or session_manager (in case
of browser exit) stops all VMs including ARCVM.

BUG=b:143569709
TEST=arc.Boot
TEST=sign in, sign out, verify StopVm for ARCVM is not called
TEST=sign in, change chrome://flags and restart the browser,
 verify StopVm is not called

Change-Id: I4c3a410d76f43da7a9743d692380b5c86962479e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1889299Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711275}

components/arc/session/arc_client_adapter.h

@@ -40,8 +40,9 @@ class ArcClientAdapter {
   virtual void UpgradeArc(UpgradeParams params,
                           chromeos::VoidDBusMethodCallback callback) = 0;
 
-  // Asynchronously stops the ARC instance.
-  virtual void StopArcInstance() = 0;
+  // Asynchronously stops the ARC instance. |on_shutdown| is true if the method
+  // is called due to the browser being shut down.
+  virtual void StopArcInstance(bool on_shutdown) = 0;
 
   // Sets a hash string of the profile user ID.
   virtual void SetUserIdHashForProfile(const std::string& hash) = 0;

components/arc/session/arc_container_client_adapter.cc

@@ -124,7 +124,7 @@ class ArcContainerClientAdapter
         request, std::move(callback));
   }
 
-  void StopArcInstance() override {
+  void StopArcInstance(bool) override {
     // Since we have the ArcInstanceStopped() callback, we don't need to do
     // anything when StopArcInstance completes.
     chromeos::SessionManagerClient::Get()->StopArcInstance(

components/arc/session/arc_session_impl.cc

@@ -452,7 +452,7 @@ void ArcSessionImpl::OnMiniInstanceStarted(bool result) {
 
   if (stop_requested_) {
     // The ARC instance has started to run. Request to stop.
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
 
@@ -476,12 +476,12 @@ void ArcSessionImpl::OnFreeDiskSpace(int64_t space) {
   // Ensure there's sufficient space on disk for the container.
   if (space == -1) {
     LOG(ERROR) << "Could not determine free disk space";
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   } else if (space < kMinimumFreeDiskSpaceBytes) {
     VLOG(1) << "There is not enough disk space to start the ARC container";
     insufficient_disk_space_ = true;
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
 
@@ -496,13 +496,13 @@ void ArcSessionImpl::OnSocketCreated(base::ScopedFD socket_fd) {
   if (stop_requested_) {
     // The ARC instance has started to run. Request to stop.
     VLOG(1) << "Stop() called while creating socket";
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
 
   if (!socket_fd.is_valid()) {
     LOG(ERROR) << "ARC: Error creating socket";
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
 
@@ -526,7 +526,7 @@ void ArcSessionImpl::OnUpgraded(base::ScopedFD socket_fd, bool result) {
 
   if (stop_requested_) {
     // The ARC instance has started to run. Request to stop.
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
 
@@ -537,7 +537,7 @@ void ArcSessionImpl::OnUpgraded(base::ScopedFD socket_fd, bool result) {
                                            weak_factory_.GetWeakPtr()));
   if (!accept_cancel_pipe_.is_valid()) {
     // Failed to post a task to accept() the request.
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
 }
@@ -549,13 +549,13 @@ void ArcSessionImpl::OnMojoConnected(
   accept_cancel_pipe_.reset();
 
   if (stop_requested_) {
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
 
   if (!arc_bridge_host.get()) {
     LOG(ERROR) << "Invalid pipe.";
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/false);
     return;
   }
   arc_bridge_host_ = std::move(arc_bridge_host);
@@ -601,7 +601,7 @@ void ArcSessionImpl::Stop() {
     case State::RUNNING_MINI_INSTANCE:
     case State::RUNNING_FULL_INSTANCE:
       // An ARC {mini,full} instance is running. Request to stop it.
-      StopArcInstance();
+      StopArcInstance(/*on_shutdown=*/false);
       return;
 
     case State::CONNECTING_MOJO:
@@ -617,7 +617,7 @@ void ArcSessionImpl::Stop() {
   }
 }
 
-void ArcSessionImpl::StopArcInstance() {
+void ArcSessionImpl::StopArcInstance(bool on_shutdown) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(state_ == State::WAITING_FOR_LCD_DENSITY ||
          state_ == State::WAITING_FOR_NUM_CORES ||
@@ -631,7 +631,7 @@ void ArcSessionImpl::StopArcInstance() {
 
   // When the instance is full instance, change the |state_| in
   // ArcInstanceStopped().
-  client_->StopArcInstance();
+  client_->StopArcInstance(on_shutdown);
 }
 
 void ArcSessionImpl::ArcInstanceStopped() {
@@ -699,7 +699,7 @@ void ArcSessionImpl::OnShutdown() {
       state_ == State::STARTING_FULL_INSTANCE ||
       state_ == State::CONNECTING_MOJO ||
       state_ == State::RUNNING_FULL_INSTANCE) {
-    StopArcInstance();
+    StopArcInstance(/*on_shutdown=*/true);
   }
 
   // Directly set to the STOPPED state by OnStopped(). Note that calling

components/arc/session/arc_session_impl.h

@@ -219,7 +219,7 @@ class ArcSessionImpl
   void OnMojoConnected(std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host);
 
   // Request to stop ARC instance via DBus.
-  void StopArcInstance();
+  void StopArcInstance(bool on_shutdown);
 
   // ArcClientAdapter::Observer:
   void ArcInstanceStopped() override;

components/arc/session/arc_vm_client_adapter.cc

@@ -399,7 +399,17 @@ class ArcVmClientAdapter : public ArcClientAdapter,
                        std::move(callback)));
   }
 
-  void StopArcInstance() override {
+  void StopArcInstance(bool on_shutdown) override {
+    if (on_shutdown) {
+      // Do nothing when |on_shutdown| is true because either vm_concierge.conf
+      // job (in case of user session termination) or session_manager (in case
+      // of browser-initiated exit on e.g. chrome://flags or UI language change)
+      // will stop all VMs including ARCVM right after the browser exits.
+      VLOG(1)
+          << "StopArcInstance is called during browser shutdown. Do nothing.";
+      return;
+    }
+
     VLOG(1) << "Stopping arcvm";
     vm_tools::concierge::StopVmRequest request;
     request.set_name(kArcVmName);

components/arc/session/arc_vm_client_adapter_unittest.cc

@@ -232,7 +232,7 @@ TEST_F(ArcVmClientAdapterTest, StopArcInstance) {
   StartMiniArc();
   UpgradeArc(true);
 
-  adapter()->StopArcInstance();
+  adapter()->StopArcInstance(/*on_shutdown=*/false);
   run_loop()->RunUntilIdle();
   EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
   // The callback for StopVm D-Bus reply does NOT call ArcInstanceStopped when
@@ -247,6 +247,18 @@ TEST_F(ArcVmClientAdapterTest, StopArcInstance) {
   EXPECT_TRUE(arc_instance_stopped_called());
 }
 
+// Tests that StopArcInstance() called during shutdown doesn't do anything.
+TEST_F(ArcVmClientAdapterTest, StopArcInstance_OnShutdown) {
+  SetValidUserIdHash();
+  StartMiniArc();
+  UpgradeArc(true);
+
+  adapter()->StopArcInstance(/*on_shutdown=*/true);
+  run_loop()->RunUntilIdle();
+  EXPECT_FALSE(GetTestConciergeClient()->stop_vm_called());
+  EXPECT_FALSE(arc_instance_stopped_called());
+}
+
 // Tests that StopArcInstance() immediately notifies the observer on failure.
 TEST_F(ArcVmClientAdapterTest, StopArcInstance_Fail) {
   StartMiniArc();
@@ -256,7 +268,7 @@ TEST_F(ArcVmClientAdapterTest, StopArcInstance_Fail) {
   response.set_success(false);
   GetTestConciergeClient()->set_stop_vm_response(response);
 
-  adapter()->StopArcInstance();
+  adapter()->StopArcInstance(/*on_shutdown=*/false);
   run_loop()->Run();
   EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
   // The callback for StopVm D-Bus reply does call ArcInstanceStopped when
@@ -283,7 +295,7 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcVmProxyFailure) {
   vm_tools::concierge::StopVmResponse response;
   response.set_success(false);
   GetTestConciergeClient()->set_stop_vm_response(response);
-  adapter()->StopArcInstance();
+  adapter()->StopArcInstance(/*on_shutdown=*/false);
   run_loop()->Run();
   EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
@@ -305,7 +317,7 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartConciergeFailure) {
   vm_tools::concierge::StopVmResponse response;
   response.set_success(false);
   GetTestConciergeClient()->set_stop_vm_response(response);
-  adapter()->StopArcInstance();
+  adapter()->StopArcInstance(/*on_shutdown=*/false);
   run_loop()->Run();
   EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
@@ -327,7 +339,7 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_NoUserId) {
   vm_tools::concierge::StopVmResponse response;
   response.set_success(false);
   GetTestConciergeClient()->set_stop_vm_response(response);
-  adapter()->StopArcInstance();
+  adapter()->StopArcInstance(/*on_shutdown=*/false);
   run_loop()->Run();
   EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
@@ -352,7 +364,7 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_StartArcVmFailure) {
   vm_tools::concierge::StopVmResponse response;
   response.set_success(false);
   GetTestConciergeClient()->set_stop_vm_response(response);
-  adapter()->StopArcInstance();
+  adapter()->StopArcInstance(/*on_shutdown=*/false);
   run_loop()->Run();
   EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_TRUE(arc_instance_stopped_called());
@@ -368,7 +380,7 @@ TEST_F(ArcVmClientAdapterTest, UpgradeArc_Success) {
   EXPECT_FALSE(arc_instance_stopped_called());
 
   // Try to stop the VM.
-  adapter()->StopArcInstance();
+  adapter()->StopArcInstance(/*on_shutdown=*/false);
   run_loop()->RunUntilIdle();
   EXPECT_TRUE(GetTestConciergeClient()->stop_vm_called());
   EXPECT_FALSE(arc_instance_stopped_called());