[CrOS PhoneHub] Add cleanup timeout for Nearby process

Allowing a short timeout before shutting down the process works around
race conditions that occur during shutdown, allowing the Nearby utility
process to shut down without any errors.

Bug: 1152609
Change-Id: I56124135cbd67a3256bc37a384ec55f40f583869
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2561541
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#831181}

chrome/browser/chromeos/nearby/nearby_process_manager_impl.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/time/time.h"
 #include "chrome/browser/chromeos/nearby/nearby_connections_dependencies_provider.h"
 #include "chrome/browser/nearby_sharing/logging/logging.h"
 #include "chrome/browser/sharing/webrtc/sharing_mojo_service.h"
@@ -17,6 +18,9 @@ namespace {
 
 NearbyProcessManagerImpl::Factory* g_test_factory = nullptr;
 
+constexpr base::TimeDelta kProcessCleanupTimeout =
+    base::TimeDelta::FromSeconds(5);
+
 void OnSharingShutDownComplete(
     mojo::Remote<sharing::mojom::Sharing> sharing,
     mojo::SharedRemote<location::nearby::connections::mojom::NearbyConnections>
@@ -31,14 +35,15 @@ void OnSharingShutDownComplete(
 // static
 std::unique_ptr<NearbyProcessManager> NearbyProcessManagerImpl::Factory::Create(
     NearbyConnectionsDependenciesProvider*
-        nearby_connections_dependencies_provider) {
+        nearby_connections_dependencies_provider,
+    std::unique_ptr<base::OneShotTimer> timer) {
   if (g_test_factory) {
     return g_test_factory->BuildInstance(
-        nearby_connections_dependencies_provider);
+        nearby_connections_dependencies_provider, std::move(timer));
   }
 
   return base::WrapUnique(new NearbyProcessManagerImpl(
-      nearby_connections_dependencies_provider,
+      nearby_connections_dependencies_provider, std::move(timer),
       base::BindRepeating(&sharing::LaunchSharing)));
 }
 
@@ -80,10 +85,12 @@ NearbyProcessManagerImpl::NearbyReferenceImpl::GetNearbySharingDecoder() const {
 NearbyProcessManagerImpl::NearbyProcessManagerImpl(
     NearbyConnectionsDependenciesProvider*
         nearby_connections_dependencies_provider,
+    std::unique_ptr<base::OneShotTimer> timer,
     const base::RepeatingCallback<
         mojo::PendingRemote<sharing::mojom::Sharing>()>& sharing_binder)
     : nearby_connections_dependencies_provider_(
           nearby_connections_dependencies_provider),
+      shutdown_debounce_timer_(std::move(timer)),
       sharing_binder_(sharing_binder) {}
 
 NearbyProcessManagerImpl::~NearbyProcessManagerImpl() = default;
@@ -103,10 +110,16 @@ NearbyProcessManagerImpl::GetNearbyProcessReference(
     }
   }
 
+  NS_LOG(VERBOSE) << "New Nearby process reference requested.";
   auto reference_id = base::UnguessableToken::Create();
   id_to_process_stopped_callback_map_.emplace(
       reference_id, std::move(on_process_stopped_callback));
 
+  // If we were waiting to shut down the process but a new client was added,
+  // stop the timer since the process needs to be kept alive for this new
+  // client.
+  shutdown_debounce_timer_->Stop();
+
   return std::make_unique<NearbyReferenceImpl>(
       connections_, decoder_,
       base::BindOnce(&NearbyProcessManagerImpl::OnReferenceDeleted,
@@ -195,10 +208,24 @@ void NearbyProcessManagerImpl::OnReferenceDeleted(
   if (!id_to_process_stopped_callback_map_.empty())
     return;
 
-  ShutDownProcess();
+  NS_LOG(VERBOSE) << "All Nearby references have been released; will shut down "
+                  << "process in " << kProcessCleanupTimeout << " unless a new "
+                  << "reference is obtained.";
+
+  // Stop the process, but wait |kProcessCleanupTimeout| before doing so. Adding
+  // this additional timeout works around issues during Nearby shutdown
+  // (see https://crbug.com/1152609).
+  // TODO(https://crbug.com/1152892): Remove this timeout.
+  shutdown_debounce_timer_->Start(
+      FROM_HERE, kProcessCleanupTimeout,
+      base::BindOnce(&NearbyProcessManagerImpl::ShutDownProcess,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void NearbyProcessManagerImpl::ShutDownProcess() {
+  // Ensure that we don't try to stop the process again.
+  shutdown_debounce_timer_->Stop();
+
   NS_LOG(INFO) << "Shutting down Nearby utility process.";
 
   // Ensure that any in-progress CreateNearbyConnections() or

chrome/browser/chromeos/nearby/nearby_process_manager_impl.h

@@ -5,9 +5,12 @@
 #ifndef CHROME_BROWSER_CHROMEOS_NEARBY_NEARBY_PROCESS_MANAGER_IMPL_H_
 #define CHROME_BROWSER_CHROMEOS_NEARBY_NEARBY_PROCESS_MANAGER_IMPL_H_
 
+#include <memory>
+
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
+#include "base/timer/timer.h"
 #include "base/unguessable_token.h"
 #include "chromeos/services/nearby/public/cpp/nearby_process_manager.h"
 #include "mojo/public/cpp/bindings/shared_remote.h"
@@ -32,14 +35,17 @@ class NearbyProcessManagerImpl : public NearbyProcessManager {
    public:
     static std::unique_ptr<NearbyProcessManager> Create(
         NearbyConnectionsDependenciesProvider*
-            nearby_connections_dependencies_provider);
+            nearby_connections_dependencies_provider,
+        std::unique_ptr<base::OneShotTimer> timer =
+            std::make_unique<base::OneShotTimer>());
     static void SetFactoryForTesting(Factory* factory);
     virtual ~Factory() = default;
 
    private:
     virtual std::unique_ptr<NearbyProcessManager> BuildInstance(
         NearbyConnectionsDependenciesProvider*
-            nearby_connections_dependencies_provider) = 0;
+            nearby_connections_dependencies_provider,
+        std::unique_ptr<base::OneShotTimer> timer) = 0;
   };
 
   ~NearbyProcessManagerImpl() override;
@@ -75,6 +81,7 @@ class NearbyProcessManagerImpl : public NearbyProcessManager {
   NearbyProcessManagerImpl(
       NearbyConnectionsDependenciesProvider*
           nearby_connections_dependencies_provider,
+      std::unique_ptr<base::OneShotTimer> timer,
       const base::RepeatingCallback<
           mojo::PendingRemote<sharing::mojom::Sharing>()>& sharing_binder);
 
@@ -94,6 +101,7 @@ class NearbyProcessManagerImpl : public NearbyProcessManager {
 
   NearbyConnectionsDependenciesProvider*
       nearby_connections_dependencies_provider_;
+  std::unique_ptr<base::OneShotTimer> shutdown_debounce_timer_;
   base::RepeatingCallback<mojo::PendingRemote<sharing::mojom::Sharing>()>
       sharing_binder_;
 

chrome/browser/chromeos/nearby/nearby_process_manager_impl_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind.h"
 #include "base/test/task_environment.h"
+#include "base/timer/mock_timer.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/nearby/nearby_connections_dependencies_provider.h"
 #include "chrome/browser/chromeos/nearby/nearby_process_manager_factory.h"
@@ -130,8 +131,11 @@ class NearbyProcessManagerImplTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
+    auto mock_timer = std::make_unique<base::MockOneShotTimer>();
+    mock_timer_ = mock_timer.get();
+
     nearby_process_manager_ = base::WrapUnique(new NearbyProcessManagerImpl(
-        &fake_deps_provider_,
+        &fake_deps_provider_, std::move(mock_timer),
         base::BindRepeating(&FakeSharingMojoService::BindSharingService,
                             base::Unretained(&fake_sharing_mojo_service_))));
   }
@@ -166,6 +170,10 @@ class NearbyProcessManagerImplTest : public testing::Test {
     EXPECT_FALSE(fake_sharing_mojo_service_.AreMocksSet());
   }
 
+  bool IsTimerRunning() const { return mock_timer_->IsRunning(); }
+
+  void FireTimer() { mock_timer_->Fire(); }
+
  private:
   NearbyProcessManagerImpl* GetImpl() {
     return static_cast<NearbyProcessManagerImpl*>(
@@ -181,6 +189,8 @@ class NearbyProcessManagerImplTest : public testing::Test {
   FakeNearbyConnectionsDependenciesProvider fake_deps_provider_;
 
   std::unique_ptr<NearbyProcessManager> nearby_process_manager_;
+
+  base::MockOneShotTimer* mock_timer_ = nullptr;
 };
 
 TEST_F(NearbyProcessManagerImplTest, StartAndStop) {
@@ -189,6 +199,7 @@ TEST_F(NearbyProcessManagerImplTest, StartAndStop) {
   VerifyBound(reference.get());
 
   reference.reset();
+  FireTimer();
   base::RunLoop().RunUntilIdle();
   VerifyNotBound();
   EXPECT_EQ(0u, num_process_stopped_calls());
@@ -200,6 +211,7 @@ TEST_F(NearbyProcessManagerImplTest, StartAndStop) {
   VerifyBound(reference.get());
 
   reference.reset();
+  FireTimer();
   base::RunLoop().RunUntilIdle();
   VerifyNotBound();
   EXPECT_EQ(0u, num_process_stopped_calls());
@@ -215,10 +227,12 @@ TEST_F(NearbyProcessManagerImplTest, MultipleReferences) {
 
   // Deleting one reference should still keep the other reference bound.
   reference1.reset();
+  EXPECT_FALSE(IsTimerRunning());
   base::RunLoop().RunUntilIdle();
   VerifyBound(reference2.get());
 
   reference2.reset();
+  FireTimer();
   base::RunLoop().RunUntilIdle();
   VerifyNotBound();
   EXPECT_EQ(0u, num_process_stopped_calls());
@@ -235,6 +249,31 @@ TEST_F(NearbyProcessManagerImplTest, ProcessStopped) {
 
   VerifyNotBound();
   EXPECT_EQ(1u, num_process_stopped_calls());
+  EXPECT_FALSE(IsTimerRunning());
+}
+
+TEST_F(NearbyProcessManagerImplTest,
+       NewReferenceObtainedWhileWaitingToShutDown) {
+  std::unique_ptr<NearbyProcessManager::NearbyProcessReference> reference =
+      CreateReference();
+  VerifyBound(reference.get());
+
+  // Delete the reference; the timer should be running so that the process is
+  // shut down after the cleanup timeout.
+  reference.reset();
+  EXPECT_TRUE(IsTimerRunning());
+
+  // Obtain a new reference; the timer should have stopped.
+  reference = CreateReference();
+  VerifyBound(reference.get());
+  EXPECT_FALSE(IsTimerRunning());
+
+  // Delete the reference and let the timer fire to shut down the process.
+  reference.reset();
+  FireTimer();
+  base::RunLoop().RunUntilIdle();
+  VerifyNotBound();
+  EXPECT_EQ(0u, num_process_stopped_calls());
 }
 
 }  // namespace nearby

chrome/browser/nearby_sharing/nearby_process_manager_unittest.cc

@@ -62,7 +62,8 @@ class FakeNearbyProcessManagerFactory
   // chromeos::nearby::NearbyProcessManagerImpl::Factory:
   std::unique_ptr<chromeos::nearby::NearbyProcessManager> BuildInstance(
       chromeos::nearby::NearbyConnectionsDependenciesProvider*
-          nearby_connections_dependencies_provider) override {
+          nearby_connections_dependencies_provider,
+      std::unique_ptr<base::OneShotTimer> timer) override {
     auto instance =
         std::make_unique<chromeos::nearby::FakeNearbyProcessManager>();
     instance_ = instance.get();