[Nearby] Cancel payloads via Nearby Connections before removing payloads

Previously, the cancel implementation invoked the transfer-update
callback before cancelling any ongoing payloads via Nearby Connections.
The transfer-update callback subsequently cleaned up the payloads and
disconnected before the payloads could be cancelled; so, the payloads
would never be cancelled via Nearby Connections and the payload tracker
would never receive a cancellation notification. This is particularly
harmful to metrics.

In this CL, we
  * cancel payloads via Nearby Connections before doing anytyhing else;
  * ensure that we provide enough time for the remote device to process
    cancellation before disconnecting;
  * make some memory-management improvements, such as copying the
    ShareTarget in DoCancel()--the notification manager owns the
    reference passed into Cancel(), and that could easily be invalidated
    during the cancellation process;
  * ensure that the payload tracker does not process trivial data sent
    during a cancellation, notably a 0 value for the number of bytes
    transferred.

Manually verified all types of cancellation. Also, verified that
existing metrics and the metrics of https:/crrev.com/c/2571443 are
as expected. We still see crbug/1155412 and b/175064390, but those
are known, unrelated issues.

Fixed: 1156232
Change-Id: I0d480b25d3565a21138d1ae32377cc4f4fcd87c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2577767
Auto-Submit: Josh Nohle <nohle@chromium.org>
Commit-Queue: James Vecore <vecore@google.com>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#834597}

chrome/browser/nearby_sharing/nearby_connections_manager_impl.cc

@@ -326,7 +326,11 @@ void NearbyConnectionsManagerImpl::Cancel(int64_t payload_id) {
                                    /*total_bytes=*/0,
                                    /*bytes_transferred=*/0),
         /*upgraded_medium=*/base::nullopt);
-    payload_status_listeners_.erase(it);
+
+    // Erase using the payload ID key instead of the iterator. The
+    // OnStatusUpdate() call might result in iterator invalidation, for example,
+    // if the listener map entry is removed during a resulting payload clean-up.
+    payload_status_listeners_.erase(payload_id);
   }
   nearby_connections_->CancelPayload(
       kServiceId, payload_id,
@@ -345,11 +349,7 @@ void NearbyConnectionsManagerImpl::ClearIncomingPayloads() {
   std::vector<PayloadPtr> payloads;
   for (auto& it : incoming_payloads_) {
     payloads.push_back(std::move(it.second));
-    // Make sure to clean up the raw pointer to the payload listener.
-    auto listener_it = payload_status_listeners_.find(it.first);
-    if (listener_it != payload_status_listeners_.end()) {
-      payload_status_listeners_.erase(listener_it);
-    }
+    payload_status_listeners_.erase(it.first);
   }
 
   file_handler_.ReleaseFilePayloads(std::move(payloads));

chrome/browser/nearby_sharing/nearby_sharing_service_impl.cc

@@ -762,11 +762,10 @@ void NearbySharingServiceImpl::Cancel(
 }
 
 void NearbySharingServiceImpl::DoCancel(
-    const ShareTarget& share_target,
+    ShareTarget share_target,
     StatusCodesCallback status_codes_callback,
     bool write_cancel_frame) {
   ShareTargetInfo* info = GetShareTargetInfo(share_target);
-
   if (!info || !info->endpoint_id()) {
     NS_LOG(ERROR) << __func__
                   << ": Cancel invoked for unknown share target, returning "
@@ -777,10 +776,27 @@ void NearbySharingServiceImpl::DoCancel(
     return;
   }
 
-  // We immediately inform the user that the transfer has been cancelled because
-  // subsequent disconnections might be interpreted as failure. The
+  // Cancel all ongoing payload transfers before invoking the transfer update
+  // callback. Invoking the transfer update callback first could result in
+  // payload cleanup before we have a chance to cancel the payload via Nearby
+  // Connections, and the payload tracker might not receive the expected
+  // cancellation signals. Also, note that there might not be any ongoing
+  // payload transfer, for example, if a connection has not been established
+  // yet.
+  for (int64_t attachment_id : share_target.GetAttachmentIds()) {
+    base::Optional<int64_t> payload_id = GetAttachmentPayloadId(attachment_id);
+    if (payload_id) {
+      nearby_connections_manager_->Cancel(*payload_id);
+    }
+  }
+
+  // Inform the user that the transfer has been cancelled before disconnecting
+  // because subsequent disconnections might be interpreted as failure. The
   // TransferUpdateDecorator will ignore subsequent statuses in favor of this
-  // cancelled status.
+  // cancelled status. Note that the transfer update callback might have already
+  // been invoked as a result of the payload cancellations above, but again,
+  // superfluous status updates are handled gracefully by the
+  // TransferUpdateDecorator.
   if (info->transfer_update_callback()) {
     info->transfer_update_callback()->OnTransferUpdate(
         share_target, TransferMetadataBuilder()
@@ -788,23 +804,15 @@ void NearbySharingServiceImpl::DoCancel(
                           .build());
   }
 
-  // Before disconnecting, cancel all ongoing payloads.
-  for (int64_t attachment_id : share_target.GetAttachmentIds()) {
-    base::Optional<int64_t> payload_id = GetAttachmentPayloadId(attachment_id);
-    if (payload_id) {
-      nearby_connections_manager_->Cancel(*payload_id);
-    }
-  }
-
-  // If a connection exists, close the connection. Otherwise disconnect from
+  // If a connection exists, close the connection after a short delay that
+  // allows for final processing by the other device. Otherwise, disconnect from
   // endpoint id directly. Note: A share attempt can be cancelled by the user
-  // before a connection is fully established, so info->connection() might be
-  // null.
+  // before a connection is fully established, in which case, info->connection()
+  // will be null.
   if (info->connection()) {
     info->connection()->SetDisconnectionListener(
         base::BindOnce(&NearbySharingServiceImpl::UnregisterShareTarget,
                        weak_ptr_factory_.GetWeakPtr(), share_target));
-
     base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&NearbySharingServiceImpl::CloseConnection,
@@ -816,6 +824,7 @@ void NearbySharingServiceImpl::DoCancel(
     }
   } else {
     nearby_connections_manager_->Disconnect(*info->endpoint_id());
+    UnregisterShareTarget(share_target);
   }
 
   std::move(status_codes_callback).Run(StatusCodes::kOk);
@@ -3224,7 +3233,11 @@ void NearbySharingServiceImpl::OnPayloadTransferUpdate(
   if (info && info->transfer_update_callback())
     info->transfer_update_callback()->OnTransferUpdate(share_target, metadata);
 
-  if (TransferMetadata::IsFinalStatus(metadata.status())) {
+  // Cancellation has its own disconnection strategy, possibly adding a delay
+  // before disconnection to provide the other party time to process the
+  // cancellation.
+  if (TransferMetadata::IsFinalStatus(metadata.status()) &&
+      metadata.status() != TransferMetadata::Status::kCancelled) {
     Disconnect(share_target, metadata);
   }
 }
@@ -3400,8 +3413,9 @@ ShareTargetInfo& NearbySharingServiceImpl::GetOrCreateShareTargetInfo(
     info.set_endpoint_id(endpoint_id);
     return info;
   } else {
-    // We need to explicitly remove any previous share target for |endpoint_id|
-    // if one exists, notifying observers that a share target is lost.
+    // We need to explicitly remove any previous share target for
+    // |endpoint_id| if one exists, notifying observers that a share target is
+    // lost.
     const auto it = outgoing_share_target_map_.find(endpoint_id);
     if (it != outgoing_share_target_map_.end() &&
         it->second.id != share_target.id) {

chrome/browser/nearby_sharing/nearby_sharing_service_impl.h

@@ -325,7 +325,10 @@ class NearbySharingServiceImpl
       NearbyConnectionsManager::ConnectionsStatus status);
   void SetInHighVisibility(bool in_high_visibility);
 
-  void DoCancel(const ShareTarget& share_target,
+  // Note: |share_target| is intentionally passed by value. A share target
+  // reference could likely be invalidated by the owner during the multi-step
+  // cancellation process.
+  void DoCancel(ShareTarget share_target,
                 StatusCodesCallback status_codes_callback,
                 bool write_cancel_frame);
 

chrome/browser/nearby_sharing/payload_tracker.cc

@@ -67,13 +67,20 @@ void PayloadTracker::OnStatusUpdate(PayloadTransferUpdatePtr update,
     last_upgraded_medium_ = upgraded_medium;
   }
 
-  it->second.amount_transferred = update->bytes_transferred;
   if (it->second.status != update->status) {
     it->second.status = update->status;
 
     NS_LOG(VERBOSE) << __func__ << ": Payload id " << update->payload_id
                     << " had status change: " << update->status;
   }
+
+  // The number of bytes transferred should never go down. That said, some
+  // status updates like cancellation might send a value of 0. In that case, we
+  // retain the last known value for use in metrics.
+  if (update->bytes_transferred > it->second.amount_transferred) {
+    it->second.amount_transferred = update->bytes_transferred;
+  }
+
   OnTransferUpdate();
 }