arc: simplify ARC session launching code

Since we will only support the mini-container start up flow, we can
simplify the code by eliminating code paths which required to support
launching the full instance directly.

Bug: 65548422
Test: Ensure container boots and upgrades successfully.
Change-Id: Ibb1c2f7af66e3f1ea7f3495f26f34b5e47a4270a
Reviewed-on: https://chromium-review.googlesource.com/821650
Commit-Queue: Christopher Morin <cmtm@google.com>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531818}

components/arc/arc_session.h

@@ -9,8 +9,6 @@
 
 #include "base/macros.h"
 #include "base/observer_list.h"
-#include "base/optional.h"
-#include "components/arc/arc_instance_mode.h"
 #include "components/arc/arc_stop_reason.h"
 
 namespace arc {
@@ -20,20 +18,21 @@ class ArcBridgeService;
 // Starts the ARC instance and bootstraps the bridge connection.
 // Clients should implement the Delegate to be notified upon communications
 // being available.
-// The instance can be safely removed 1) before Start() is called, or 2) after
-// OnSessionStopped() is called.
-// The number of instances must be at most one. Otherwise, ARC instances will
-// conflict.
+// The instance can be safely removed before StartMiniInstance() is called, or
+// after OnSessionStopped() is called.  The number of instances must be at most
+// one. Otherwise, ARC instances will conflict.
 class ArcSession {
  public:
   // Observer to notify events corresponding to one ARC session run.
   class Observer {
    public:
-    // Called when ARC instance is stopped. This is called exactly once
-    // per instance which is Start()ed.
-    // |was_running| is true, if the stopped instance was fully set up
-    // and running.
-    virtual void OnSessionStopped(ArcStopReason reason, bool was_running) = 0;
+    // Called when ARC instance is stopped. This is called exactly once per
+    // instance.  |was_running| is true if the stopped instance was fully set up
+    // and running. |full_requested| is true if the full container was
+    // requested.
+    virtual void OnSessionStopped(ArcStopReason reason,
+                                  bool was_running,
+                                  bool full_requested) = 0;
 
    protected:
     virtual ~Observer() = default;
@@ -44,21 +43,18 @@ class ArcSession {
       ArcBridgeService* arc_bridge_service);
   virtual ~ArcSession();
 
-  // Starts an instance in the |request_mode|. Start(FULL_INSTANCE) should
-  // not be called twice or more. When Start(MINI_INSTANCE) was called then
-  // Start(FULL_INSTANCE) is called, it upgrades the mini instance to a full
-  // instance.
-  virtual void Start(ArcInstanceMode request_mode) = 0;
+  // Sends D-Bus message to start a mini-container.
+  virtual void StartMiniInstance() = 0;
+
+  // Sends a D-Bus message to upgrade to a full instance if
+  // possible. This might be done asynchronously; the message might only be sent
+  // after other operations have completed.
+  virtual void RequestUpgrade() = 0;
 
   // Requests to stop the currently-running instance regardless of its mode.
   // The completion is notified via OnSessionStopped() of the Observer.
   virtual void Stop() = 0;
 
-  // Returns the current target mode, in which eventually this instance is
-  // running.
-  // If the instance is not yet started, this returns nullopt.
-  virtual base::Optional<ArcInstanceMode> GetTargetMode() = 0;
-
   // Returns true if Stop() has been called already.
   virtual bool IsStopRequested() = 0;
 

components/arc/arc_session_impl.h

@@ -13,10 +13,8 @@
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/optional.h"
 #include "base/threading/thread_checker.h"
 #include "chromeos/dbus/session_manager_client.h"
-#include "components/arc/arc_instance_mode.h"
 #include "components/arc/arc_session.h"
 
 namespace arc {
@@ -30,29 +28,19 @@ class ArcSessionImpl : public ArcSession,
  public:
   // The possible states of the session. Expected state changes are as follows.
   //
-  // 1) Starting MINI_INSTANCE.
   // NOT_STARTED
-  //   -> StartMiniInstance() ->
+  // -> StartMiniInstance() ->
   // STARTING_MINI_INSTANCE
   //   -> OnMiniInstanceStarted() ->
   // RUNNING_MINI_INSTANCE.
-  //
-  // 2) Starting FULL_INSTANCE.
-  // NOT_STARTED
-  //   -> StartFullInstance() ->
+  //   -> RequestUpgrade() ->
   // STARTING_FULL_INSTANCE
-  //   -> OnFullInstanceStarted() ->
+  //   -> OnUpgraded() ->
   // CONNECTING_MOJO
   //   -> OnMojoConnected() ->
   // RUNNING_FULL_INSTANCE
   //
-  // 3) Upgrading from a MINI_INSTANCE to FULL_INSTANCE.
-  // RUNNING_MINI_INSTANCE
-  //   -> StartFullInstance() ->
-  // STARTING_FULL_INSTANCE
-  //   -> ... (remaining is as same as (2)).
-  //
-  // Note that, if Start(FULL_INSTANCE) is called during STARTING_MINI_INSTANCE
+  // Note that, if RequestUpgrade() is called during STARTING_MINI_INSTANCE
   // state, the state change to STARTING_FULL_INSTANCE is suspended until
   // the state becomes RUNNING_MINI_INSTANCE.
   //
@@ -108,7 +96,7 @@ class ArcSessionImpl : public ArcSession,
     // arcbridgeservice (i.e. mojo endpoint) are running.
     RUNNING_MINI_INSTANCE,
 
-    // The request to start or upgrade to a full instance has been sent.
+    // The request to upgrade to a full instance has been sent.
     STARTING_FULL_INSTANCE,
 
     // The instance has started. Waiting for it to connect to the IPC bridge.
@@ -150,16 +138,13 @@ class ArcSessionImpl : public ArcSession,
   State GetStateForTesting() { return state_; }
 
   // ArcSession overrides:
-  void Start(ArcInstanceMode request_mode) override;
+  void StartMiniInstance() override;
+  void RequestUpgrade() override;
   void Stop() override;
-  base::Optional<ArcInstanceMode> GetTargetMode() override;
   bool IsStopRequested() override;
   void OnShutdown() override;
 
  private:
-  // Sends a D-Bus message to start a mini instance.
-  void StartMiniInstance();
-
   // D-Bus callback for StartArcInstance() for a mini instance.
   // In case of success, |container_instance_id| must not be empty, and
   // |socket_fd| is /dev/null.
@@ -169,19 +154,15 @@ class ArcSessionImpl : public ArcSession,
       const std::string& container_instance_id,
       base::ScopedFD socket_fd);
 
-  // Sends a D-Bus message to start or to upgrade to a full instance.
-  void StartFullInstance();
+  // Sends a D-Bus message to upgrade to a full instance.
+  void DoUpgrade();
 
-  // D-Bus callback for StartArcInstance() for a full instance.
-  // In case of success, |container_instance_id| must not be empty, if this is
-  // actually starting an instance, or empty if this is upgrade from a mini
-  // instance to a full instance.
-  // In either start or upgrade case, |socket_fd| should be a socket which
-  // shold be accept(2)ed to connect ArcBridgeService Mojo channel.
-  void OnFullInstanceStarted(
-      chromeos::SessionManagerClient::StartArcInstanceResult result,
-      const std::string& container_instance_id,
-      base::ScopedFD socket_fd);
+  // D-Bus callback for StartArcInstance() for an upgrade. |socket_fd| should be
+  // a socket which should be accept(2)ed to connect ArcBridgeService Mojo
+  // channel.
+  void OnUpgraded(chromeos::SessionManagerClient::StartArcInstanceResult result,
+                  const std::string& container_instance_id,  // unused
+                  base::ScopedFD socket_fd);
 
   // Called when Mojo connection is established (or canceled during the
   // connect.)
@@ -198,11 +179,6 @@ class ArcSessionImpl : public ArcSession,
   // deleting |this| because the function calls observers' OnSessionStopped().
   void OnStopped(ArcStopReason reason);
 
-  // Sends a StartArcInstance D-Bus request to session_manager.
-  static void SendStartArcInstanceDBusMessage(
-      ArcInstanceMode target_mode,
-      chromeos::SessionManagerClient::StartArcInstanceCallback callback);
-
   // Checks whether a function runs on the thread where the instance is
   // created.
   THREAD_CHECKER(thread_checker_);
@@ -216,9 +192,8 @@ class ArcSessionImpl : public ArcSession,
   // When Stop() is called, this flag is set.
   bool stop_requested_ = false;
 
-  // In which mode this instance should be running eventually.
-  // Initialized in nullopt.
-  base::Optional<ArcInstanceMode> target_mode_;
+  // Whether the full container has been requested
+  bool upgrade_requested_ = false;
 
   // Container instance id passed from session_manager.
   // Should be available only after On{Mini,Full}InstanceStarted().

components/arc/arc_session_runner.cc

@@ -112,23 +112,6 @@ bool IsRequestAllowed(const base::Optional<ArcInstanceMode>& current_mode,
   return false;
 }
 
-// Returns true if OnSessionStopped() should be called to notify observers.
-bool ShouldNotifyOnSessionStopped(
-    const base::Optional<ArcInstanceMode>& target_mode) {
-  DCHECK(target_mode.has_value());
-
-  switch (target_mode.value()) {
-    case ArcInstanceMode::MINI_INSTANCE:
-      return false;
-    case ArcInstanceMode::FULL_INSTANCE:
-      return true;
-  }
-
-  NOTREACHED() << "Unexpeceted |target_mode|: "
-               << static_cast<int>(target_mode.value());
-  return false;
-}
-
 }  // namespace
 
 ArcSessionRunner::ArcSessionRunner(const ArcSessionFactory& factory)
@@ -242,15 +225,15 @@ void ArcSessionRunner::StartArcSession() {
   if (!arc_session_) {
     arc_session_ = factory_.Run();
     arc_session_->AddObserver(this);
+    arc_session_->StartMiniInstance();
     // Record the UMA only when |restart_after_crash_count_| is zero to avoid
     // recording an auto-restart-then-crash loop. Such a crash loop is recorded
     // separately with RecordInstanceRestartAfterCrashUma().
     if (!restart_after_crash_count_)
       RecordInstanceCrashUma(ArcContainerLifetimeEvent::CONTAINER_STARTING);
-  } else {
-    DCHECK_EQ(ArcInstanceMode::MINI_INSTANCE, arc_session_->GetTargetMode());
   }
-  arc_session_->Start(target_mode_.value());
+  if (target_mode_ == ArcInstanceMode::FULL_INSTANCE)
+    arc_session_->RequestUpgrade();
 }
 
 void ArcSessionRunner::RestartArcSession() {
@@ -262,18 +245,14 @@ void ArcSessionRunner::RestartArcSession() {
 }
 
 void ArcSessionRunner::OnSessionStopped(ArcStopReason stop_reason,
-                                        bool was_running) {
+                                        bool was_running,
+                                        bool full_requested) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(arc_session_);
   DCHECK(!restart_timer_.IsRunning());
 
   VLOG(0) << "ARC stopped: " << stop_reason;
 
-  // The observers should be agnostic to the existence of the limited-purpose
-  // instance.
-  const bool notify_observers =
-      ShouldNotifyOnSessionStopped(arc_session_->GetTargetMode());
-
   arc_session_->RemoveObserver(this);
   arc_session_.reset();
 
@@ -305,7 +284,9 @@ void ArcSessionRunner::OnSessionStopped(ArcStopReason stop_reason,
                                     weak_ptr_factory_.GetWeakPtr()));
   }
 
-  if (notify_observers) {
+  // The observers should be agnostic to the existence of the limited-purpose
+  // instance.
+  if (full_requested) {
     for (auto& observer : observer_list_)
       observer.OnSessionStopped(stop_reason, restarting);
   }

components/arc/arc_session_runner.h

@@ -102,7 +102,9 @@ class ArcSessionRunner : public ArcSession::Observer {
   void RestartArcSession();
 
   // ArcSession::Observer:
-  void OnSessionStopped(ArcStopReason reason, bool was_running) override;
+  void OnSessionStopped(ArcStopReason reason,
+                        bool was_running,
+                        bool full_requested) override;
 
   THREAD_CHECKER(thread_checker_);
 

components/arc/arc_session_runner_unittest.cc

@@ -233,6 +233,7 @@ TEST_F(ArcSessionRunnerTest, BootFailure_MiniInstance) {
   // If starting the mini instance fails, arc_session_runner()'s state goes back
   // to STOPPED, but its observers won't be notified.
   arc_session_runner()->RequestStart(ArcInstanceMode::MINI_INSTANCE);
+  arc_session()->EmulateMiniContainerStart();
   EXPECT_FALSE(arc_session());
   EXPECT_FALSE(stopped_called());
 
@@ -255,6 +256,7 @@ TEST_F(ArcSessionRunnerTest, Crash_MiniInstance) {
   // If starting the mini instance fails, arc_session_runner()'s state goes back
   // to STOPPED, but its observers won't be notified.
   arc_session_runner()->RequestStart(ArcInstanceMode::MINI_INSTANCE);
+  arc_session()->EmulateMiniContainerStart();
   EXPECT_FALSE(arc_session());
   EXPECT_FALSE(stopped_called());
 }

components/arc/test/fake_arc_session.cc

@@ -14,13 +14,14 @@ FakeArcSession::FakeArcSession() = default;
 
 FakeArcSession::~FakeArcSession() = default;
 
-void FakeArcSession::Start(ArcInstanceMode request_mode) {
-  target_mode_ = request_mode;
+void FakeArcSession::StartMiniInstance() {}
+
+void FakeArcSession::RequestUpgrade() {
+  upgrade_requested_ = true;
   if (boot_failure_emulation_enabled_) {
     for (auto& observer : observer_list_)
-      observer.OnSessionStopped(boot_failure_reason_, false);
-  } else if (!boot_suspended_ &&
-             target_mode_ == ArcInstanceMode::FULL_INSTANCE) {
+      observer.OnSessionStopped(boot_failure_reason_, false, true);
+  } else if (!boot_suspended_) {
     running_ = true;
   }
 }
@@ -30,10 +31,6 @@ void FakeArcSession::Stop() {
   StopWithReason(ArcStopReason::SHUTDOWN);
 }
 
-base::Optional<ArcInstanceMode> FakeArcSession::GetTargetMode() {
-  return target_mode_;
-}
-
 bool FakeArcSession::IsStopRequested() {
   return stop_requested_;
 }
@@ -46,7 +43,7 @@ void FakeArcSession::StopWithReason(ArcStopReason reason) {
   bool was_mojo_connected = running_;
   running_ = false;
   for (auto& observer : observer_list_)
-    observer.OnSessionStopped(reason, was_mojo_connected);
+    observer.OnSessionStopped(reason, was_mojo_connected, upgrade_requested_);
 }
 
 void FakeArcSession::EnableBootFailureEmulation(ArcStopReason reason) {
@@ -64,6 +61,17 @@ void FakeArcSession::SuspendBoot() {
   boot_suspended_ = true;
 }
 
+// TODO(cmtm): Change this function so that it emulates both mini-container
+// startup and upgrade. With this change, we should also be able to get rid of
+// SuspendBoot since RequestUpgrade will no longer notify observers.
+void FakeArcSession::EmulateMiniContainerStart() {
+  if (boot_failure_emulation_enabled_) {
+    for (auto& observer : observer_list_)
+      observer.OnSessionStopped(boot_failure_reason_, false,
+                                upgrade_requested_);
+  }
+}
+
 // static
 std::unique_ptr<ArcSession> FakeArcSession::Create() {
   return std::make_unique<FakeArcSession>();

components/arc/test/fake_arc_session.h

@@ -8,8 +8,6 @@
 #include <memory>
 
 #include "base/macros.h"
-#include "base/optional.h"
-#include "components/arc/arc_instance_mode.h"
 #include "components/arc/arc_session.h"
 #include "components/arc/arc_stop_reason.h"
 
@@ -22,9 +20,9 @@ class FakeArcSession : public ArcSession {
   ~FakeArcSession() override;
 
   // ArcSession overrides:
-  void Start(ArcInstanceMode request_mode) override;
+  void StartMiniInstance() override;
+  void RequestUpgrade() override;
   void Stop() override;
-  base::Optional<ArcInstanceMode> GetTargetMode() override;
   bool IsStopRequested() override;
   void OnShutdown() override;
 
@@ -40,6 +38,10 @@ class FakeArcSession : public ArcSession {
   // Emulate Start() is suspended at some phase.
   void SuspendBoot();
 
+  // To emulate the mini-container starting. This can cause a failure if
+  // EnableBootFailureEmulation was called on this instance
+  void EmulateMiniContainerStart();
+
   // Returns true if the session is considered as running.
   bool is_running() const { return running_; }
 
@@ -52,7 +54,7 @@ class FakeArcSession : public ArcSession {
   ArcStopReason boot_failure_reason_;
 
   bool boot_suspended_ = false;
-  base::Optional<ArcInstanceMode> target_mode_;
+  bool upgrade_requested_ = false;
   bool running_ = false;
   bool stop_requested_ = false;