Skip to content

Commit

Permalink
[Nearby] Distribute weak pointers for PayloadStatusListener
Browse files Browse the repository at this point in the history 
Background:
ShareTargetInfo owns the PayloadStatusListener (PayloadTracker), which
handles payload status updates from Nearby Connections. In order to
trigger these updates, the Nearby Connections managers needs access to
the listener. Previously, ShareTargetInfo was distributing raw listener
pointers to the Nearby Connection manager. This is an issue when
multiple payloads are included in the same transfer. The payloads all
share the same listener. When a transfer is cancelled or fails, we
receive a cancellation/failure signal for *each* payload; however, the
listener is destroyed after the *first* cancellation/failure signal. The
Nearby Connections manager will then try to use a non-existent listener
when it receives subsequent cancellation/failure updates for the other
payloads.

Fix:
In this CL, we distribute weak pointers instead of raw pointers. When
ShareTargetInfo destroys the PayloadStatusListener, the weak pointers
are invalidated and the Nearby Connections manager knows not to use the
listener anymore.

Tested:
Manually verified that the browser process crashes before this fix but
not after the fix. Also, verified that the added unit test crashes
before this fix but not after.

(cherry picked from commit e4e1955)

Fixed: 1196435, b/181274197
Change-Id: I6cd34883aaed847ac05d963cdd4719749cb5a3b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2818518
Reviewed-by: James Vecore <vecore@google.com>
Commit-Queue: Josh Nohle <nohle@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#871988}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2826790
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Josh Nohle <nohle@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4472@{#62}
Cr-Branched-From: 3d60439-refs/heads/master@{#870763}
  • Loading branch information
@nohle
nohle authored and Chromium LUCI CQ committed Apr 14, 2021
1 parent 8bf5a8e commit 1edff80
Show file tree
Hide file tree
Showing 9 changed files with 273 additions and 68 deletions.
13 changes: 7 additions & 6 deletions chrome/browser/nearby_sharing/fake_nearby_connections_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,18 @@ void FakeNearbyConnectionsManager::Disconnect(const std::string& endpoint_id) {
connection_endpoint_infos_.erase(endpoint_id);
}

void FakeNearbyConnectionsManager::Send(const std::string& endpoint_id,
PayloadPtr payload,
PayloadStatusListener* listener) {
void FakeNearbyConnectionsManager::Send(
const std::string& endpoint_id,
PayloadPtr payload,
base::WeakPtr<PayloadStatusListener> listener) {
DCHECK(!is_shutdown());
if (send_payload_callback_)
send_payload_callback_.Run(std::move(payload), listener);
}

void FakeNearbyConnectionsManager::RegisterPayloadStatusListener(
int64_t payload_id,
PayloadStatusListener* listener) {
base::WeakPtr<PayloadStatusListener> listener) {
DCHECK(!is_shutdown());

payload_status_listeners_[payload_id] = listener;
Expand Down Expand Up @@ -124,7 +125,7 @@ FakeNearbyConnectionsManager::GetIncomingPayload(int64_t payload_id) {

void FakeNearbyConnectionsManager::Cancel(int64_t payload_id) {
DCHECK(!is_shutdown());
PayloadStatusListener* listener =
base::WeakPtr<PayloadStatusListener> listener =
GetRegisteredPayloadStatusListener(payload_id);
if (listener) {
listener->OnStatusUpdate(
Expand Down Expand Up @@ -207,7 +208,7 @@ void FakeNearbyConnectionsManager::SetPayloadPathStatus(
payload_path_status_[payload_id] = status;
}

FakeNearbyConnectionsManager::PayloadStatusListener*
base::WeakPtr<FakeNearbyConnectionsManager::PayloadStatusListener>
FakeNearbyConnectionsManager::GetRegisteredPayloadStatusListener(
int64_t payload_id) {
auto it = payload_status_listeners_.find(payload_id);
Expand Down
21 changes: 13 additions & 8 deletions chrome/browser/nearby_sharing/fake_nearby_connections_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <vector>

#include "base/callback.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "chrome/browser/nearby_sharing/nearby_connections_manager.h"
#include "chromeos/services/nearby/public/mojom/nearby_connections.mojom.h"
Expand Down Expand Up @@ -46,9 +47,10 @@ class FakeNearbyConnectionsManager
void Disconnect(const std::string& endpoint_id) override;
void Send(const std::string& endpoint_id,
PayloadPtr payload,
PayloadStatusListener* listener) override;
void RegisterPayloadStatusListener(int64_t payload_id,
PayloadStatusListener* listener) override;
base::WeakPtr<PayloadStatusListener> listener) override;
void RegisterPayloadStatusListener(
int64_t payload_id,
base::WeakPtr<PayloadStatusListener> listener) override;
void RegisterPayloadPath(int64_t payload_id,
const base::FilePath& file_path,
ConnectionsCallback callback) override;
Expand All @@ -74,7 +76,8 @@ class FakeNearbyConnectionsManager
bool IsDiscovering() const;
bool DidUpgradeBandwidth(const std::string& endpoint_id) const;
void SetPayloadPathStatus(int64_t payload_id, ConnectionsStatus status);
PayloadStatusListener* GetRegisteredPayloadStatusListener(int64_t payload_id);
base::WeakPtr<PayloadStatusListener> GetRegisteredPayloadStatusListener(
int64_t payload_id);
void SetIncomingPayload(int64_t payload_id, PayloadPtr payload);
base::Optional<base::FilePath> GetRegisteredPayloadPath(int64_t payload_id);
bool WasPayloadCanceled(const int64_t& payload_id) const;
Expand All @@ -89,8 +92,8 @@ class FakeNearbyConnectionsManager
}
DataUsage connected_data_usage() const { return connected_data_usage_; }
void set_send_payload_callback(
base::RepeatingCallback<void(PayloadPtr, PayloadStatusListener*)>
callback) {
base::RepeatingCallback<
void(PayloadPtr, base::WeakPtr<PayloadStatusListener>)> callback) {
send_payload_callback_ = std::move(callback);
}
const base::Optional<std::vector<uint8_t>>& advertising_endpoint_info() {
Expand Down Expand Up @@ -118,7 +121,8 @@ class FakeNearbyConnectionsManager
std::map<std::string, std::vector<uint8_t>> endpoint_auth_tokens_;
NearbyConnection* connection_ = nullptr;
DataUsage connected_data_usage_ = DataUsage::kUnknown;
base::RepeatingCallback<void(PayloadPtr, PayloadStatusListener*)>
base::RepeatingCallback<void(PayloadPtr,
base::WeakPtr<PayloadStatusListener>)>
send_payload_callback_;
base::Optional<std::vector<uint8_t>> advertising_endpoint_info_;
std::set<std::string> disconnected_endpoints_;
Expand All @@ -128,7 +132,8 @@ class FakeNearbyConnectionsManager
std::map<std::string, std::vector<uint8_t>> connection_endpoint_infos_;

std::map<int64_t, ConnectionsStatus> payload_path_status_;
std::map<int64_t, PayloadStatusListener*> payload_status_listeners_;
std::map<int64_t, base::WeakPtr<PayloadStatusListener>>
payload_status_listeners_;
std::map<int64_t, PayloadPtr> incoming_payloads_;
std::map<int64_t, base::FilePath> registered_payload_paths_;
};
Expand Down
11 changes: 11 additions & 0 deletions chrome/browser/nearby_sharing/nearby_connections_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,14 @@ std::string NearbyConnectionsManager::ConnectionsStatusToString(
return "kPayloadUnknown";
}
}

NearbyConnectionsManager::PayloadStatusListener::PayloadStatusListener() =
default;

NearbyConnectionsManager::PayloadStatusListener::~PayloadStatusListener() =
default;

base::WeakPtr<NearbyConnectionsManager::PayloadStatusListener>
NearbyConnectionsManager::PayloadStatusListener::GetWeakPtr() const {
return weak_ptr_factory_.GetWeakPtr();
}
20 changes: 11 additions & 9 deletions chrome/browser/nearby_sharing/nearby_connections_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include "base/callback.h"
#include "base/files/file_path.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "chrome/browser/nearby_sharing/common/nearby_share_enums.h"
#include "chrome/browser/nearby_sharing/nearby_connection.h"
Expand Down Expand Up @@ -60,13 +61,18 @@ class NearbyConnectionsManager {
using PayloadTransferUpdatePtr =
location::nearby::connections::mojom::PayloadTransferUpdatePtr;

virtual ~PayloadStatusListener() = default;
PayloadStatusListener();
virtual ~PayloadStatusListener();

base::WeakPtr<PayloadStatusListener> GetWeakPtr() const;

// Note: |upgraded_medium| is passed in for use in metrics, and it is
// base::nullopt if the bandwidth has not upgraded yet or if the upgrade
// status is not known.
virtual void OnStatusUpdate(PayloadTransferUpdatePtr update,
base::Optional<Medium> upgraded_medium) = 0;

base::WeakPtrFactory<PayloadStatusListener> weak_ptr_factory_{this};
};

// Converts the status to a logging-friendly string.
Expand Down Expand Up @@ -110,19 +116,15 @@ class NearbyConnectionsManager {
// Disconnects from remote |endpoint_id| through Nearby Connections.
virtual void Disconnect(const std::string& endpoint_id) = 0;

// Sends |payload| through Nearby Connections. Caller is expected to ensure
// |listener| remains valid until kSuccess/kFailure/kCancelled is invoked with
// OnStatusUpdate.
// Sends |payload| through Nearby Connections.
virtual void Send(const std::string& endpoint_id,
PayloadPtr payload,
PayloadStatusListener* listener) = 0;
base::WeakPtr<PayloadStatusListener> listener) = 0;

// Register a |listener| with |payload_id|. Caller is expected to ensure
// |listener| remains valid until kSuccess/kFailure/kCancelled is invoked with
// OnStatusUpdate.
// Register a |listener| with |payload_id|.
virtual void RegisterPayloadStatusListener(
int64_t payload_id,
PayloadStatusListener* listener) = 0;
base::WeakPtr<PayloadStatusListener> listener) = 0;

// Register a |file_path| for receiving incoming payload with |payload_id|.
virtual void RegisterPayloadPath(int64_t payload_id,
Expand Down
45 changes: 28 additions & 17 deletions chrome/browser/nearby_sharing/nearby_connections_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "chrome/browser/nearby_sharing/common/nearby_share_features.h"
#include "chrome/browser/nearby_sharing/constants.h"
#include "chrome/browser/nearby_sharing/logging/logging.h"
#include "chrome/browser/nearby_sharing/nearby_connections_manager.h"
#include "chromeos/services/nearby/public/mojom/nearby_connections_types.mojom.h"
#include "crypto/random.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
Expand Down Expand Up @@ -308,9 +309,10 @@ void NearbyConnectionsManagerImpl::Disconnect(const std::string& endpoint_id) {
NS_LOG(INFO) << "Disconnected from " << endpoint_id;
}
void NearbyConnectionsManagerImpl::Send(const std::string& endpoint_id,
PayloadPtr payload,
PayloadStatusListener* listener) {
void NearbyConnectionsManagerImpl::Send(
const std::string& endpoint_id,
PayloadPtr payload,
base::WeakPtr<PayloadStatusListener> listener) {
// TODO(https://crbug.com/1177088): Determine if we should attempt to bind to
// process.
if (!process_reference_)
Expand All @@ -333,7 +335,7 @@ void NearbyConnectionsManagerImpl::Send(const std::string& endpoint_id,
void NearbyConnectionsManagerImpl::RegisterPayloadStatusListener(
int64_t payload_id,
PayloadStatusListener* listener) {
base::WeakPtr<PayloadStatusListener> listener) {
payload_status_listeners_.insert_or_assign(payload_id, listener);
}
Expand Down Expand Up @@ -383,17 +385,20 @@ void NearbyConnectionsManagerImpl::Cancel(int64_t payload_id) {
auto it = payload_status_listeners_.find(payload_id);
if (it != payload_status_listeners_.end()) {
it->second->OnStatusUpdate(
PayloadTransferUpdate::New(payload_id, PayloadStatus::kCanceled,
/*total_bytes=*/0,
/*bytes_transferred=*/0),
/*upgraded_medium=*/base::nullopt);
// 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.
base::WeakPtr<PayloadStatusListener> listener = it->second;
payload_status_listeners_.erase(payload_id);
// Note: The listener might be invalidated, for example, if it is shared
// with another payload in the same transfer.
if (listener) {
listener->OnStatusUpdate(
PayloadTransferUpdate::New(payload_id, PayloadStatus::kCanceled,
/*total_bytes=*/0,
/*bytes_transferred=*/0),
/*upgraded_medium=*/base::nullopt);
}
}
process_reference_->GetNearbyConnections()->CancelPayload(
kServiceId, payload_id,
base::BindOnce(
Expand Down Expand Up @@ -634,20 +639,26 @@ void NearbyConnectionsManagerImpl::OnPayloadTransferUpdate(
return;
// If this is a payload we've registered for, then forward its status to the
// PayloadStatusListener. We don't need to do anything more with the payload.
// PayloadStatusListener if it still exists. We don't need to do anything more
// with the payload.
auto listener_it = payload_status_listeners_.find(update->payload_id);
if (listener_it != payload_status_listeners_.end()) {
PayloadStatusListener* listener = listener_it->second;
base::WeakPtr<PayloadStatusListener> listener = listener_it->second;
switch (update->status) {
case PayloadStatus::kInProgress:
break;
case PayloadStatus::kSuccess:
case PayloadStatus::kCanceled:
case PayloadStatus::kFailure:
payload_status_listeners_.erase(listener_it);
payload_status_listeners_.erase(update->payload_id);
break;
}
listener->OnStatusUpdate(std::move(update), GetUpgradedMedium(endpoint_id));
// Note: The listener might be invalidated, for example, if it is shared
// with another payload in the same transfer.
if (listener) {
listener->OnStatusUpdate(std::move(update),
GetUpgradedMedium(endpoint_id));
}
return;
}
Expand Down
12 changes: 7 additions & 5 deletions chrome/browser/nearby_sharing/nearby_connections_manager_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@ class NearbyConnectionsManagerImpl
void Disconnect(const std::string& endpoint_id) override;
void Send(const std::string& endpoint_id,
PayloadPtr payload,
PayloadStatusListener* listener) override;
void RegisterPayloadStatusListener(int64_t payload_id,
PayloadStatusListener* listener) override;
base::WeakPtr<PayloadStatusListener> listener) override;
void RegisterPayloadStatusListener(
int64_t payload_id,
base::WeakPtr<PayloadStatusListener> listener) override;
void RegisterPayloadPath(int64_t payload_id,
const base::FilePath& file_path,
ConnectionsCallback callback) override;
Expand Down Expand Up @@ -153,8 +154,9 @@ class NearbyConnectionsManagerImpl
// A map of endpoint_id to timers that timeout a connection request.
base::flat_map<std::string, std::unique_ptr<base::OneShotTimer>>
connect_timeout_timers_;
// A map of payload_id to PayloadStatusListener*.
base::flat_map<int64_t, PayloadStatusListener*> payload_status_listeners_;
// A map of payload_id to PayloadStatusListener weak pointer.
base::flat_map<int64_t, base::WeakPtr<PayloadStatusListener>>
payload_status_listeners_;
// A map of payload_id to PayloadPtr.
base::flat_map<int64_t, PayloadPtr> incoming_payloads_;

Expand Down

0 comments on commit 1edff80

Please sign in to comment.