Add SequenceChecker to NearbySharingService

- All NearbySharingService methods should run in same sequence, adding DCHECK_CALLED_ON_VALID_SEQUENCE to enforce that
- Also changed RegisterSendSurface/UnregisterSendSurface to synchronous

Bug: 1084644
Change-Id: I6eac1f5ad082144b2ff3d2c366b7855c93f42592
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2308410
Commit-Queue: Alex Chau <alexchau@chromium.org>
Reviewed-by: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790330}

chrome/browser/nearby_sharing/nearby_sharing_service.h

@@ -41,16 +41,14 @@ class NearbySharingService {
 
   // Registers a send surface for handling payload transfer status and device
   // discovery.
-  virtual void RegisterSendSurface(
+  virtual StatusCodes RegisterSendSurface(
       TransferUpdateCallback* transfer_callback,
-      ShareTargetDiscoveredCallback* discovery_callback,
-      StatusCodesCallback status_codes_callback) = 0;
+      ShareTargetDiscoveredCallback* discovery_callback) = 0;
 
   // Unregisters the current send surface.
-  virtual void UnregisterSendSurface(
+  virtual StatusCodes UnregisterSendSurface(
       TransferUpdateCallback* transfer_callback,
-      ShareTargetDiscoveredCallback* discovery_callback,
-      StatusCodesCallback status_codes_callback) = 0;
+      ShareTargetDiscoveredCallback* discovery_callback) = 0;
 
   // Registers a receiver surface for handling payload transfer status.
   virtual StatusCodes RegisterReceiveSurface(

chrome/browser/nearby_sharing/nearby_sharing_service_impl.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/nearby_sharing/fast_initiation_manager.h"
 #include "chrome/browser/nearby_sharing/logging/logging.h"
@@ -150,26 +151,28 @@ NearbySharingServiceImpl::~NearbySharingServiceImpl() {
     bluetooth_adapter_->RemoveObserver(this);
 }
 
-void NearbySharingServiceImpl::RegisterSendSurface(
+NearbySharingService::StatusCodes NearbySharingServiceImpl::RegisterSendSurface(
     TransferUpdateCallback* transfer_callback,
-    ShareTargetDiscoveredCallback* discovery_callback,
-    StatusCodesCallback status_codes_callback) {
-  register_send_surface_callback_ = std::move(status_codes_callback);
+    ShareTargetDiscoveredCallback* discovery_callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   StartFastInitiationAdvertising();
+  return StatusCodes::kOk;
 }
 
-void NearbySharingServiceImpl::UnregisterSendSurface(
+NearbySharingService::StatusCodes
+NearbySharingServiceImpl::UnregisterSendSurface(
     TransferUpdateCallback* transfer_callback,
-    ShareTargetDiscoveredCallback* discovery_callback,
-    StatusCodesCallback status_codes_callback) {
-  unregister_send_surface_callback_ = std::move(status_codes_callback);
+    ShareTargetDiscoveredCallback* discovery_callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   StopFastInitiationAdvertising();
+  return StatusCodes::kOk;
 }
 
 NearbySharingService::StatusCodes
 NearbySharingServiceImpl::RegisterReceiveSurface(
     TransferUpdateCallback* transfer_callback,
     ReceiveSurfaceState state) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(transfer_callback);
   DCHECK_NE(state, ReceiveSurfaceState::kUnknown);
   if (foreground_receive_callbacks_.HasObserver(transfer_callback) ||
@@ -202,6 +205,7 @@ NearbySharingServiceImpl::RegisterReceiveSurface(
 NearbySharingService::StatusCodes
 NearbySharingServiceImpl::UnregisterReceiveSurface(
     TransferUpdateCallback* transfer_callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(transfer_callback);
   bool is_foreground =
       foreground_receive_callbacks_.HasObserver(transfer_callback);
@@ -307,6 +311,7 @@ void NearbySharingServiceImpl::OnIncomingConnection(
     const std::string& endpoint_id,
     const std::vector<uint8_t>& endpoint_info,
     std::unique_ptr<NearbyConnection> connection) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // TODO(crbug/1085068): Handle incoming connection; use CertificateManager
 }
 
@@ -384,15 +389,13 @@ void NearbySharingServiceImpl::OnDataUsagePrefChanged() {
 
 void NearbySharingServiceImpl::StartFastInitiationAdvertising() {
   if (!IsBluetoothPresent() || !IsBluetoothPowered()) {
-    std::move(register_send_surface_callback_).Run(StatusCodes::kError);
+    NS_LOG(INFO) << "Failed to advertise FastInitiation. Bluetooth is not "
+                    "present or powered.";
     return;
   }
 
   if (fast_initiation_manager_) {
-    // TODO(hansenmichael): Do not invoke
-    // |register_send_surface_callback_| until Nearby Connections
-    // scanning is kicked off.
-    std::move(register_send_surface_callback_).Run(StatusCodes::kOk);
+    NS_LOG(INFO) << "Failed to advertise FastInitiation. Already advertising.";
     return;
   }
 
@@ -413,8 +416,7 @@ void NearbySharingServiceImpl::StartFastInitiationAdvertising() {
 
 void NearbySharingServiceImpl::StopFastInitiationAdvertising() {
   if (!fast_initiation_manager_) {
-    if (unregister_send_surface_callback_)
-      std::move(unregister_send_surface_callback_).Run(StatusCodes::kOk);
+    NS_LOG(INFO) << "Can't stop advertising FastInitiation. Not advertising.";
     return;
   }
 
@@ -431,7 +433,7 @@ void NearbySharingServiceImpl::GetBluetoothAdapter() {
   // Because this will be called from the constructor, GetAdapter() may call
   // OnGetBluetoothAdapter() immediately which can cause problems during tests
   // since the class is not fully constructed yet.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(
           &device::BluetoothAdapterFactory::GetAdapter,
@@ -450,22 +452,17 @@ void NearbySharingServiceImpl::OnStartFastInitiationAdvertising() {
   // TODO(hansenmichael): Do not invoke
   // |register_send_surface_callback_| until Nearby Connections
   // scanning is kicked off.
-  std::move(register_send_surface_callback_).Run(StatusCodes::kOk);
+  NS_LOG(VERBOSE) << "Started advertising FastInitiation.";
 }
 
 void NearbySharingServiceImpl::OnStartFastInitiationAdvertisingError() {
   fast_initiation_manager_.reset();
-  std::move(register_send_surface_callback_).Run(StatusCodes::kError);
+  NS_LOG(ERROR) << "Failed to start FastInitiation advertising.";
 }
 
 void NearbySharingServiceImpl::OnStopFastInitiationAdvertising() {
   fast_initiation_manager_.reset();
-
-  // TODO(hansenmichael): Do not invoke
-  // |unregister_send_surface_callback_| until Nearby Connections
-  // scanning is stopped.
-  if (unregister_send_surface_callback_)
-    std::move(unregister_send_surface_callback_).Run(StatusCodes::kOk);
+  NS_LOG(VERBOSE) << "Stopped advertising FastInitiation";
 }
 
 bool NearbySharingServiceImpl::IsBluetoothPresent() const {

chrome/browser/nearby_sharing/nearby_sharing_service_impl.h

@@ -13,6 +13,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
+#include "base/sequence_checker.h"
 #include "chrome/browser/nearby_sharing/nearby_connections_manager.h"
 #include "chrome/browser/nearby_sharing/nearby_constants.h"
 #include "chrome/browser/nearby_sharing/nearby_notification_manager.h"
@@ -26,6 +27,7 @@ class NearbyConnectionsManager;
 class PrefService;
 class Profile;
 
+// All methods should be called from the same sequence that created the service.
 class NearbySharingServiceImpl
     : public NearbySharingService,
       public KeyedService,
@@ -40,13 +42,12 @@ class NearbySharingServiceImpl
   ~NearbySharingServiceImpl() override;
 
   // NearbySharingService:
-  void RegisterSendSurface(TransferUpdateCallback* transfer_callback,
-                           ShareTargetDiscoveredCallback* discovery_callback,
-                           StatusCodesCallback status_codes_callback) override;
-  void UnregisterSendSurface(
+  StatusCodes RegisterSendSurface(
       TransferUpdateCallback* transfer_callback,
-      ShareTargetDiscoveredCallback* discovery_callback,
-      StatusCodesCallback status_codes_callback) override;
+      ShareTargetDiscoveredCallback* discovery_callback) override;
+  StatusCodes UnregisterSendSurface(
+      TransferUpdateCallback* transfer_callback,
+      ShareTargetDiscoveredCallback* discovery_callback) override;
   StatusCodes RegisterReceiveSurface(TransferUpdateCallback* transfer_callback,
                                      ReceiveSurfaceState state) override;
   StatusCodes UnregisterReceiveSurface(
@@ -110,8 +111,6 @@ class NearbySharingServiceImpl
       nearby_process_observer_{this};
   scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
   std::unique_ptr<FastInitiationManager> fast_initiation_manager_;
-  StatusCodesCallback register_send_surface_callback_;
-  StatusCodesCallback unregister_send_surface_callback_;
   NearbyNotificationManager nearby_notification_manager_;
 
   // A list of foreground receivers.
@@ -143,6 +142,7 @@ class NearbySharingServiceImpl
   // True if we're currently sending or receiving a file.
   bool is_transferring_files_ = false;
 
+  SEQUENCE_CHECKER(sequence_checker_);
   base::WeakPtrFactory<NearbySharingServiceImpl> weak_ptr_factory_{this};
 };
 

chrome/browser/nearby_sharing/nearby_sharing_service_impl_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
+#include "base/test/bind_test_util.h"
 #include "chrome/browser/nearby_sharing/fake_nearby_connections_manager.h"
 #include "chrome/browser/nearby_sharing/fast_initiation_manager.h"
 #include "chrome/browser/nearby_sharing/nearby_connections_manager.h"
@@ -181,40 +182,6 @@ class NearbySharingServiceImplTest : public testing::Test {
         fast_initiation_manager_factory_.get());
   }
 
-  NearbySharingService::StatusCodes RegisterSendSurfaceAndWait() {
-    base::RunLoop run_loop;
-    NearbySharingService::StatusCodes result;
-    service_->RegisterSendSurface(
-        /*transfer_callback=*/nullptr, /*discovery_callback=*/nullptr,
-        base::BindOnce(
-            [](base::OnceClosure quit_closure,
-               NearbySharingService::StatusCodes* result,
-               NearbySharingService::StatusCodes code) {
-              *result = code;
-              std::move(quit_closure).Run();
-            },
-            run_loop.QuitClosure(), &result));
-    run_loop.Run();
-    return result;
-  }
-
-  NearbySharingService::StatusCodes UnregisterSendSurfaceAndWait() {
-    base::RunLoop run_loop;
-    NearbySharingService::StatusCodes result;
-    service_->UnregisterSendSurface(
-        /*transfer_callback=*/nullptr, /*discovery_callback=*/nullptr,
-        base::BindOnce(
-            [](base::OnceClosure quit_closure,
-               NearbySharingService::StatusCodes* result,
-               NearbySharingService::StatusCodes code) {
-              *result = code;
-              std::move(quit_closure).Run();
-            },
-            run_loop.QuitClosure(), &result));
-    run_loop.Run();
-    return result;
-  }
-
   bool IsBluetoothPresent() { return is_bluetooth_present_; }
   bool IsBluetoothPowered() { return is_bluetooth_powered_; }
 
@@ -265,42 +232,49 @@ TEST_F(NearbySharingServiceImplTest, DisableNearbyShutdownConnections) {
 
 TEST_F(NearbySharingServiceImplTest, StartFastInitiationAdvertising) {
   EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
-            RegisterSendSurfaceAndWait());
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
   EXPECT_EQ(1u, fast_initiation_manager_factory_->StartAdvertisingCount());
 
   // Call RegisterSendSurface() a second time and make sure StartAdvertising is
   // not called again.
   EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
-            RegisterSendSurfaceAndWait());
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
   EXPECT_EQ(1u, fast_initiation_manager_factory_->StartAdvertisingCount());
 }
 
 TEST_F(NearbySharingServiceImplTest, StartFastInitiationAdvertisingError) {
   SetFakeFastInitiationManagerFactory(/*should_succeed_on_start=*/false);
-  EXPECT_EQ(NearbySharingService::StatusCodes::kError,
-            RegisterSendSurfaceAndWait());
+  EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
 }
 
 TEST_F(NearbySharingServiceImplTest,
        StartFastInitiationAdvertising_BluetoothNotPresent) {
   is_bluetooth_present_ = false;
-  EXPECT_EQ(NearbySharingService::StatusCodes::kError,
-            RegisterSendSurfaceAndWait());
+  EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
 }
 
 TEST_F(NearbySharingServiceImplTest,
        StartFastInitiationAdvertising_BluetoothNotPowered) {
   is_bluetooth_powered_ = false;
-  EXPECT_EQ(NearbySharingService::StatusCodes::kError,
-            RegisterSendSurfaceAndWait());
+  EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
 }
 
 TEST_F(NearbySharingServiceImplTest, StopFastInitiationAdvertising) {
   EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
-            RegisterSendSurfaceAndWait());
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
   EXPECT_EQ(1u, fast_initiation_manager_factory_->StartAdvertisingCount());
   EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
-            UnregisterSendSurfaceAndWait());
+            service_->UnregisterSendSurface(/*transfer_callback=*/nullptr,
+                                            /*discovery_callback=*/nullptr));
   EXPECT_TRUE(fast_initiation_manager_factory_
                   ->StopAdvertisingCalledAndManagerDestroyed());
 }
@@ -308,7 +282,8 @@ TEST_F(NearbySharingServiceImplTest, StopFastInitiationAdvertising) {
 TEST_F(NearbySharingServiceImplTest,
        StopFastInitiationAdvertising_BluetoothBecomesNotPresent) {
   EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
-            RegisterSendSurfaceAndWait());
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
   adapter_observer_->AdapterPresentChanged(mock_bluetooth_adapter_.get(),
                                            false);
   EXPECT_TRUE(fast_initiation_manager_factory_
@@ -318,7 +293,8 @@ TEST_F(NearbySharingServiceImplTest,
 TEST_F(NearbySharingServiceImplTest,
        StopFastInitiationAdvertising_BluetoothBecomesNotPowered) {
   EXPECT_EQ(NearbySharingService::StatusCodes::kOk,
-            RegisterSendSurfaceAndWait());
+            service_->RegisterSendSurface(/*transfer_callback=*/nullptr,
+                                          /*discovery_callback=*/nullptr));
   adapter_observer_->AdapterPoweredChanged(mock_bluetooth_adapter_.get(),
                                            false);
   EXPECT_TRUE(fast_initiation_manager_factory_