[Nearby] Fixed dropped transfer updates while sending

Fixes an issue where the foreground surface was getting unregistered
when the user leaves the discovery page due to a mojo disconnect
handler getting invoked. We remove the disconnect handler here so the
surface stays registered as long as the webui is up so it can receive
transfer status updates while the confirm page is being show. When the
webui is close, the surface will unregistered itself from the destructor
anyways.

Change-Id: Ib9890ae996a21fffab42c940eb688db4709f1ccf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2412937Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Commit-Queue: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#808047}

chrome/browser/nearby_sharing/nearby_per_session_discovery_manager.cc

@@ -85,12 +85,13 @@ void NearbyPerSessionDiscoveryManager::OnShareTargetLost(
 void NearbyPerSessionDiscoveryManager::StartDiscovery(
     mojo::PendingRemote<nearby_share::mojom::ShareTargetListener> listener,
     StartDiscoveryCallback callback) {
+  // Starting discovery again closes any previous discovery session.
+  share_target_listener_.reset();
   share_target_listener_.Bind(std::move(listener));
-  // |share_target_listener_| owns the callbacks and is guaranteed to be
-  // destroyed before |this|, therefore making base::Unretained() safe to use.
-  share_target_listener_.set_disconnect_handler(
-      base::BindOnce(&NearbyPerSessionDiscoveryManager::UnregisterSendSurface,
-                     base::Unretained(this)));
+  // NOTE: Previously we set a disconnect handler here that called
+  // UnregisterSendSurface, but this causes transfer updates to stop flowing to
+  // to the UI. Instead, we rely on the destructor's call to
+  // UnregisterSendSurface which will trigger when the share sheet goes away.
 
   if (nearby_sharing_service_->RegisterSendSurface(
           this, this, NearbySharingService::SendSurfaceState::kForeground) !=
@@ -100,7 +101,10 @@ void NearbyPerSessionDiscoveryManager::StartDiscovery(
     std::move(callback).Run(/*success=*/false);
     return;
   }
-
+  // Once this object is registered as send surface, we stay registered until
+  // UnregisterSendSurface is called so that the transfer update listeners can
+  // get updates even if Discovery is stopped.
+  registered_as_send_surface_ = true;
   std::move(callback).Run(/*success=*/true);
 }
 
@@ -193,13 +197,13 @@ void NearbyPerSessionDiscoveryManager::GetSendPreview(
 }
 
 void NearbyPerSessionDiscoveryManager::UnregisterSendSurface() {
-  if (!share_target_listener_.is_bound())
-    return;
+  if (registered_as_send_surface_) {
+    if (nearby_sharing_service_->UnregisterSendSurface(this, this) !=
+        NearbySharingService::StatusCodes::kOk) {
+      NS_LOG(WARNING) << "Failed to unregister send surface";
+    }
+    registered_as_send_surface_ = false;
+  }
 
   share_target_listener_.reset();
-
-  if (nearby_sharing_service_->UnregisterSendSurface(this, this) !=
-      NearbySharingService::StatusCodes::kOk) {
-    NS_LOG(WARNING) << "Failed to unregister send surface";
-  }
 }

chrome/browser/nearby_sharing/nearby_per_session_discovery_manager.h

@@ -52,6 +52,7 @@ class NearbyPerSessionDiscoveryManager
   // Unregisters this class from the NearbySharingService.
   void UnregisterSendSurface();
 
+  bool registered_as_send_surface_ = false;
   NearbySharingService* nearby_sharing_service_;
   std::vector<std::unique_ptr<Attachment>> attachments_;
   mojo::Remote<nearby_share::mojom::ShareTargetListener> share_target_listener_;

chrome/browser/nearby_sharing/nearby_per_session_discovery_manager_unittest.cc

@@ -6,7 +6,9 @@
 
 #include <string>
 
+#include "base/callback_forward.h"
 #include "base/optional.h"
+#include "base/run_loop.h"
 #include "base/test/mock_callback.h"
 #include "chrome/browser/nearby_sharing/mock_nearby_sharing_service.h"
 #include "chrome/browser/nearby_sharing/share_target.h"
@@ -57,6 +59,8 @@ class MockShareTargetListener
     return receiver_.BindNewPipeAndPassRemote();
   }
 
+  void reset() { receiver_.reset(); }
+
   // nearby_share::mojom::ShareTargetListener:
   MOCK_METHOD(void, OnShareTargetDiscovered, (const ShareTarget&), (override));
   MOCK_METHOD(void, OnShareTargetLost, (const ShareTarget&), (override));
@@ -131,11 +135,28 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, StartDiscovery_Success) {
       RegisterSendSurface(&manager(), &manager(),
                           NearbySharingService::SendSurfaceState::kForeground))
       .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
+
   EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
-      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
+      .Times(0);  // Should not be called!
 
   MockShareTargetListener listener;
   manager().StartDiscovery(listener.Bind(), callback.Get());
+
+  // Reset the listener here like the UI does when switching pages.
+  listener.reset();
+  // We have to run util idle to give the disconnect handler a chance to run.
+  // We no longer have a disconnect handler, but we want to very that once the
+  // mojo connection is torn down, that we don't unregister.
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();
+  // Verify that UnregisterSendSurface was NOT called due to the disconnect.
+  // This previously failed when disconnect handler was being registered.
+  EXPECT_TRUE(::testing::Mock::VerifyAndClearExpectations(&sharing_service()));
+
+  // However, we do expect UnregisterSendSurface to be called from the
+  // destructor.
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, StartDiscovery_Error) {
@@ -171,6 +192,9 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnShareTargetDiscovered) {
   manager().OnShareTargetDiscovered(share_target);
 
   run_loop.Run();
+
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, OnShareTargetLost) {
@@ -190,6 +214,9 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnShareTargetLost) {
   manager().OnShareTargetLost(share_target);
 
   run_loop.Run();
+
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, SelectShareTarget_Invalid) {
@@ -206,6 +233,9 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, SelectShareTarget_Invalid) {
           testing::IsFalse(), testing::IsFalse()));
 
   manager().SelectShareTarget({}, callback.Get());
+
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, SelectShareTarget_SendSuccess) {
@@ -234,6 +264,9 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, SelectShareTarget_SendSuccess) {
           }));
 
   manager().SelectShareTarget(share_target.id, callback.Get());
+
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, SelectShareTarget_SendError) {
@@ -264,6 +297,9 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, SelectShareTarget_SendError) {
           }));
 
   manager().SelectShareTarget(share_target.id, callback.Get());
+
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitRemote) {
@@ -275,7 +311,7 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitRemote) {
       .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 
   base::RunLoop run_loop;
-  EXPECT_CALL(transfer_listener, OnTransferUpdate)
+  EXPECT_CALL(transfer_listener, OnTransferUpdate(_, _))
       .WillOnce(testing::Invoke(
           [&run_loop](nearby_share::mojom::TransferStatus status,
                       const base::Optional<std::string>& token) {
@@ -288,6 +324,8 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitRemote) {
 
   manager().StartDiscovery(listener.Bind(), base::DoNothing());
   ShareTarget share_target;
+  EXPECT_CALL(listener, OnShareTargetDiscovered(MatchesTarget(share_target)))
+      .Times(1);
   manager().OnShareTargetDiscovered(share_target);
 
   MockSelectShareTargetCallback callback;
@@ -313,6 +351,8 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitRemote) {
   manager().OnTransferUpdate(share_target, metadata);
 
   run_loop.Run();
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitLocal) {
@@ -326,7 +366,7 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitLocal) {
   const std::string expected_token = "Test Token";
 
   base::RunLoop run_loop;
-  EXPECT_CALL(transfer_listener, OnTransferUpdate)
+  EXPECT_CALL(transfer_listener, OnTransferUpdate(_, _))
       .WillOnce(testing::Invoke([&run_loop, &expected_token](
                                     nearby_share::mojom::TransferStatus status,
                                     const base::Optional<std::string>& token) {
@@ -339,6 +379,8 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitLocal) {
 
   manager().StartDiscovery(listener.Bind(), base::DoNothing());
   ShareTarget share_target;
+  EXPECT_CALL(listener, OnShareTargetDiscovered(MatchesTarget(share_target)))
+      .Times(1);
   manager().OnShareTargetDiscovered(share_target);
 
   MockSelectShareTargetCallback callback;
@@ -365,4 +407,7 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnTransferUpdate_WaitLocal) {
   manager().OnTransferUpdate(share_target, metadata);
 
   run_loop.Run();
+
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }