[Nearby] Resolve duplicate share targets in contact-based sharing

Currently, the discovery page might show multiple share targets
corresponding to the same physical device. This can happen if the
receiving device goes in and out of high-visibility mode, locks/unlocks
the screen, etc. The endpoint ID and share target ID change after
these events, and the stale share target records are not reported as
"lost" until a timeout occurs after ~30s.

For contact-based sharing, a share target has an immutable device ID
corresponding to a physical device. In this CL, we remove stale,
previously discovered share target records that have the same device ID
as the most recently discovered share target.

Note: If the receiving device is not using contact-based sharing or is
not a contact of the sending device, there is no device ID available
in the share target to dedup on. A different solution, if one exists,
will need to be considered for that scenario.

Fixed: 1146075
Change-Id: Ife49eb677fa0e973be845f2b715ce0db3f580cfe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2589040Reviewed-by: James Vecore <vecore@google.com>
Commit-Queue: Josh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836894}

chrome/browser/nearby_sharing/nearby_per_session_discovery_manager.cc

@@ -9,6 +9,7 @@
 #include "base/callback_helpers.h"
 #include "base/containers/contains.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/strings/string_number_conversions.h"
 #include "chrome/browser/nearby_sharing/attachment.h"
 #include "chrome/browser/nearby_sharing/logging/logging.h"
 #include "chrome/browser/nearby_sharing/nearby_confirmation_manager.h"
@@ -41,7 +42,8 @@ base::Optional<nearby_share::mojom::TransferStatus> GetTransferStatus(
     case TransferMetadata::Status::kFailed:
       return nearby_share::mojom::TransferStatus::kFailed;
     case TransferMetadata::Status::kAwaitingRemoteAcceptanceFailed:
-      return nearby_share::mojom::TransferStatus::kAwaitingRemoteAcceptanceFailed;
+      return nearby_share::mojom::TransferStatus::
+          kAwaitingRemoteAcceptanceFailed;
     case TransferMetadata::Status::kUnknown:
       return nearby_share::mojom::TransferStatus::kUnknown;
     case TransferMetadata::Status::kConnecting:
@@ -99,6 +101,13 @@ nearby_share::mojom::ShareType GetFileShareType(
   }
 }
 
+std::string GetDeviceIdForLogs(const ShareTarget& share_target) {
+  return (share_target.device_id
+              ? base::HexEncode(share_target.device_id.value().data(),
+                                share_target.device_id.value().size())
+              : "[null]");
+}
+
 }  // namespace
 
 NearbyPerSessionDiscoveryManager::NearbyPerSessionDiscoveryManager(
@@ -150,6 +159,9 @@ void NearbyPerSessionDiscoveryManager::OnTransferUpdate(
 
 void NearbyPerSessionDiscoveryManager::OnShareTargetDiscovered(
     ShareTarget share_target) {
+  NS_LOG(VERBOSE) << "NearbyPerSessionDiscoveryManager::" << __func__
+                  << ": id=" << share_target.id
+                  << ", device_id=" << GetDeviceIdForLogs(share_target);
   // Update metrics.
   UpdateFurthestDiscoveryProgressIfNecessary(
       DiscoveryProgress::kDiscoveredShareTargetNothingSent);
@@ -165,16 +177,44 @@ void NearbyPerSessionDiscoveryManager::OnShareTargetDiscovered(
         base::TimeTicks::Now() - *discovery_start_time_);
   }
 
-  base::InsertOrAssign(discovered_share_targets_, share_target.id,
-                       share_target);
+  // Dedup by the more stable device ID if possible.
+  if (share_target.device_id) {
+    auto it = std::find_if(discovered_share_targets_.begin(),
+                           discovered_share_targets_.end(),
+                           [&share_target](const auto& id_share_target_pair) {
+                             return share_target.device_id ==
+                                    id_share_target_pair.second.device_id;
+                           });
+
+    if (it != discovered_share_targets_.end()) {
+      NS_LOG(VERBOSE) << "NearbyPerSessionDiscoveryManager::" << __func__
+                      << ": Removing previously discovered share target with "
+                      << "identical device_id="
+                      << GetDeviceIdForLogs(share_target);
+      OnShareTargetLost(it->second);
+    }
+  }
+
+  discovered_share_targets_.insert_or_assign(share_target.id, share_target);
   share_target_listener_->OnShareTargetDiscovered(share_target);
 }
 
 void NearbyPerSessionDiscoveryManager::OnShareTargetLost(
     ShareTarget share_target) {
-  if (base::Contains(discovered_share_targets_, share_target.id)) {
-    ++num_lost_;
+  NS_LOG(VERBOSE) << "NearbyPerSessionDiscoveryManager::" << __func__
+                  << ": id=" << share_target.id
+                  << ", device_id=" << GetDeviceIdForLogs(share_target);
+
+  // It is possible that we already removed a ShareTarget from the map when
+  // deduping by ShareTarget device_id.
+  if (!base::Contains(discovered_share_targets_, share_target.id)) {
+    NS_LOG(VERBOSE) << "NearbyPerSessionDiscoveryManager::" << __func__
+                    << ": Share target id=" << share_target.id
+                    << " already removed. Taking no action.";
+    return;
   }
+
+  ++num_lost_;
   discovered_share_targets_.erase(share_target.id);
   share_target_listener_->OnShareTargetLost(share_target);
 }

chrome/browser/nearby_sharing/nearby_per_session_discovery_manager_unittest.cc

@@ -197,6 +197,44 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnShareTargetDiscovered) {
       .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
 }
 
+TEST_F(NearbyPerSessionDiscoveryManagerTest,
+       OnShareTargetDiscovered_DuplicateDeviceId) {
+  MockShareTargetListener listener;
+  EXPECT_CALL(sharing_service(),
+              RegisterSendSurface(testing::_, testing::_, testing::_))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
+
+  manager().StartDiscovery(listener.Bind(), base::DoNothing());
+
+  ShareTarget share_target1;
+  share_target1.device_id = "device_id";
+  ShareTarget share_target2;
+  share_target2.device_id = "device_id";
+
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(listener, OnShareTargetDiscovered(MatchesTarget(share_target1)))
+        .WillOnce(testing::InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    manager().OnShareTargetDiscovered(share_target1);
+    run_loop.Run();
+  }
+
+  // If a newly discovered share target has the same device ID as a previously
+  // discovered share target, remove the old share target then add the new share
+  // target.
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(listener, OnShareTargetLost(MatchesTarget(share_target1)));
+    EXPECT_CALL(listener, OnShareTargetDiscovered(MatchesTarget(share_target2)))
+        .WillOnce(testing::InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    manager().OnShareTargetDiscovered(share_target2);
+    run_loop.Run();
+  }
+
+  EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
+}
+
 TEST_F(NearbyPerSessionDiscoveryManagerTest, OnShareTargetLost) {
   MockShareTargetListener listener;
   EXPECT_CALL(sharing_service(),
@@ -207,13 +245,26 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnShareTargetLost) {
 
   ShareTarget share_target;
 
-  base::RunLoop run_loop;
+  // A share-target-lost notification should only be sent if the lost share
+  // target has already been discovered and has not already been removed from
+  // the list of discovered devices.
   EXPECT_CALL(listener, OnShareTargetLost(MatchesTarget(share_target)))
-      .WillOnce(testing::InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
-
+      .Times(0);
   manager().OnShareTargetLost(share_target);
-
-  run_loop.Run();
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(listener, OnShareTargetDiscovered(MatchesTarget(share_target)))
+        .WillOnce(testing::InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    manager().OnShareTargetDiscovered(share_target);
+    run_loop.Run();
+  }
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(listener, OnShareTargetLost(MatchesTarget(share_target)))
+        .WillOnce(testing::InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+    manager().OnShareTargetLost(share_target);
+    run_loop.Run();
+  }
 
   EXPECT_CALL(sharing_service(), UnregisterSendSurface(&manager(), &manager()))
       .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));