Commit a8d29f0a authored by James Vecore's avatar James Vecore Committed by Commit Bot

[Nearby] Fix receive status updates getting lost

Due to an order of operations issue in OnPayloadTransferUpdate()  when
receiving files Disconnect() was being called before the transfer
updates were communicated. This caused the share target to get cleaned
up before sending the completed status which left the transfer hanging
around in the UI. This was missed in unit-tests due to the fact that
the |FakeNearbyConnections| triggers the disconnect handler on Close()
instead of the destructor like the real one. By changing the way
Disconnect() behaves to call Close() on the connection  we can get the
tests to fail due to the missing status. Then the fix gets the tests
passing again.

Verified on device with a real transfer.

Fixed: 1126620
Change-Id: Ida0ab7dda01a81ce7fb470333059029e461972ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2413397
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: default avatarRyan Hansberry <hansberry@chromium.org>
Auto-Submit: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#808069}
parent 6108e39a
......@@ -2898,16 +2898,19 @@ void NearbySharingServiceImpl::OnPayloadTransferUpdate(
text.set_text_body(std::string());
}
// Make sure to call this before calling Disconnect or we risk loosing some
// transfer updates in the receive case due to the Disconnect call cleaning up
// share targets.
ShareTargetInfo* info = GetShareTargetInfo(share_target);
if (info && info->transfer_update_callback())
info->transfer_update_callback()->OnTransferUpdate(share_target, metadata);
if (TransferMetadata::IsFinalStatus(metadata.status())) {
if (metadata.status() != TransferMetadata::Status::kComplete)
OnPayloadsFailed(share_target);
Disconnect(share_target, metadata);
}
ShareTargetInfo* info = GetShareTargetInfo(share_target);
if (info && info->transfer_update_callback())
info->transfer_update_callback()->OnTransferUpdate(share_target, metadata);
}
bool NearbySharingServiceImpl::OnIncomingPayloadsComplete(
......@@ -3023,13 +3026,21 @@ void NearbySharingServiceImpl::Disconnect(const ShareTarget& share_target,
// Failed to send or receive. No point in continuing, so disconnect
// immediately.
if (metadata.status() != TransferMetadata::Status::kComplete) {
nearby_connections_manager_->Disconnect(*endpoint_id);
if (share_target_info->connection()) {
share_target_info->connection()->Close();
} else {
nearby_connections_manager_->Disconnect(*endpoint_id);
}
return;
}
// Files received successfully. Receivers can immediately cancel.
if (share_target.is_incoming) {
nearby_connections_manager_->Disconnect(*endpoint_id);
if (share_target_info->connection()) {
share_target_info->connection()->Close();
} else {
nearby_connections_manager_->Disconnect(*endpoint_id);
}
return;
}
......
......@@ -2485,7 +2485,7 @@ TEST_F(NearbySharingServiceImplTest, AcceptValidShareTarget_PayloadSuccessful) {
EXPECT_FALSE(
fake_nearby_connections_manager_->connection_endpoint_info(kEndpointId));
EXPECT_TRUE(fake_nearby_connections_manager_->has_incoming_payloads());
EXPECT_FALSE(fake_nearby_connections_manager_->has_incoming_payloads());
// TODO(crbug.com/1123022): This check is flaky, should be investigated.
// EXPECT_TRUE(FileExists(file_path));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment