Make task trace more useful for mojo::Shared{Associated,}Remote
Before this CL, crashing when invoking the reply callback would only
implicate internal Mojo machinery in the task trace. Needless to say,
this is not very useful the vast majority of the time, as Mojo itself is
not responsible for the contents of the reply callback.
After this CL, the task trace includes the creation location of the
shared remote. Ideally, this would be the location of the caller that
invoked the Mojo method, but that is not possible without major plumbing
changes.
Bug: 404190340
Change-Id: Ida791188a8732b4fd244a15955c6a9502e611dcb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6422672
Reviewed-by: Fred Shih <ffred@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1441786}
NOKEYCHECK=True
GitOrigin-RevId: f847e0452e09e8ad9b42696dca7622526a8ac068
diff --git a/public/cpp/bindings/interface_endpoint_client.h b/public/cpp/bindings/interface_endpoint_client.h
index 2d796ce..f683fea 100644
--- a/public/cpp/bindings/interface_endpoint_client.h
+++ b/public/cpp/bindings/interface_endpoint_client.h
@@ -102,7 +102,8 @@
AssociatedGroup* associated_group();
scoped_refptr<ThreadSafeProxy> CreateThreadSafeProxy(
- scoped_refptr<ThreadSafeProxy::Target> target);
+ scoped_refptr<ThreadSafeProxy::Target> target,
+ const base::Location& location);
// Sets a MessageFilter which can filter a message after validation but
// before dispatch.
diff --git a/public/cpp/bindings/lib/associated_interface_ptr_state.h b/public/cpp/bindings/lib/associated_interface_ptr_state.h
index dbb4a86..c76a345 100644
--- a/public/cpp/bindings/lib/associated_interface_ptr_state.h
+++ b/public/cpp/bindings/lib/associated_interface_ptr_state.h
@@ -15,6 +15,7 @@
#include "base/component_export.h"
#include "base/functional/bind.h"
#include "base/functional/callback_forward.h"
+#include "base/location.h"
#include "base/memory/ptr_util.h"
#include "base/memory/scoped_refptr.h"
#include "base/task/sequenced_task_runner.h"
@@ -76,8 +77,9 @@
}
scoped_refptr<ThreadSafeProxy> CreateThreadSafeProxy(
- scoped_refptr<ThreadSafeProxy::Target> target) {
- return endpoint_client_->CreateThreadSafeProxy(std::move(target));
+ scoped_refptr<ThreadSafeProxy::Target> target,
+ const base::Location& location) {
+ return endpoint_client_->CreateThreadSafeProxy(std::move(target), location);
}
protected:
diff --git a/public/cpp/bindings/lib/interface_endpoint_client.cc b/public/cpp/bindings/lib/interface_endpoint_client.cc
index 9615abc..0ecfee4 100644
--- a/public/cpp/bindings/lib/interface_endpoint_client.cc
+++ b/public/cpp/bindings/lib/interface_endpoint_client.cc
@@ -59,11 +59,13 @@
base::WeakPtr<InterfaceEndpointClient> endpoint,
scoped_refptr<ThreadSafeProxy::Target> target,
const AssociatedGroup& associated_group,
- scoped_refptr<base::SequencedTaskRunner> task_runner)
+ scoped_refptr<base::SequencedTaskRunner> task_runner,
+ const base::Location& location)
: endpoint_(std::move(endpoint)),
target_(std::move(target)),
associated_group_(associated_group),
- task_runner_(std::move(task_runner)) {}
+ task_runner_(std::move(task_runner)),
+ location_(location) {}
ThreadSafeInterfaceEndpointClientProxy(
const ThreadSafeInterfaceEndpointClientProxy&) = delete;
@@ -154,12 +156,14 @@
class ForwardToCallingThread : public MessageReceiver {
public:
- explicit ForwardToCallingThread(std::unique_ptr<MessageReceiver> responder)
+ explicit ForwardToCallingThread(std::unique_ptr<MessageReceiver> responder,
+ const base::Location& location)
: responder_(std::move(responder)),
- caller_task_runner_(base::SequencedTaskRunner::GetCurrentDefault()) {}
+ caller_task_runner_(base::SequencedTaskRunner::GetCurrentDefault()),
+ location_(location) {}
~ForwardToCallingThread() override {
- caller_task_runner_->DeleteSoon(FROM_HERE, std::move(responder_));
+ caller_task_runner_->DeleteSoon(location_, std::move(responder_));
}
private:
@@ -167,7 +171,7 @@
// `this` will be deleted immediately after this method returns. We must
// relinquish ownership of `responder_` so it doesn't get deleted.
caller_task_runner_->PostTask(
- FROM_HERE,
+ location_,
base::BindOnce(&ForwardToCallingThread::CallAcceptAndDeleteResponder,
std::move(responder_), std::move(*message)));
return true;
@@ -181,6 +185,7 @@
std::unique_ptr<MessageReceiver> responder_;
scoped_refptr<base::SequencedTaskRunner> caller_task_runner_;
+ const base::Location location_;
};
class ForwardSameThreadResponder : public MessageReceiver {
@@ -231,6 +236,7 @@
const scoped_refptr<base::SequencedTaskRunner> task_runner_;
const scoped_refptr<InProgressSyncCalls> sync_calls_{
base::MakeRefCounted<InProgressSyncCalls>()};
+ const base::Location location_;
};
void DetermineIfEndpointIsConnected(
@@ -381,8 +387,8 @@
// Async messages are always posted (even if `task_runner_` runs tasks on
// this sequence) to guarantee that two async calls can't be reordered.
if (!message.has_flag(Message::kFlagIsSync)) {
- auto reply_forwarder =
- std::make_unique<ForwardToCallingThread>(std::move(responder));
+ auto reply_forwarder = std::make_unique<ForwardToCallingThread>(
+ std::move(responder), location_);
task_runner_->PostTask(
FROM_HERE,
base::BindOnce(&ThreadSafeInterfaceEndpointClientProxy ::
@@ -510,10 +516,11 @@
}
scoped_refptr<ThreadSafeProxy> InterfaceEndpointClient::CreateThreadSafeProxy(
- scoped_refptr<ThreadSafeProxy::Target> target) {
+ scoped_refptr<ThreadSafeProxy::Target> target,
+ const base::Location& location) {
return base::MakeRefCounted<ThreadSafeInterfaceEndpointClientProxy>(
weak_ptr_factory_.GetWeakPtr(), std::move(target), *associated_group_,
- task_runner_);
+ task_runner_, location);
}
ScopedInterfaceEndpointHandle InterfaceEndpointClient::PassHandle() {
diff --git a/public/cpp/bindings/lib/interface_ptr_state.h b/public/cpp/bindings/lib/interface_ptr_state.h
index b6b88ee..860de77 100644
--- a/public/cpp/bindings/lib/interface_ptr_state.h
+++ b/public/cpp/bindings/lib/interface_ptr_state.h
@@ -17,6 +17,7 @@
#include "base/dcheck_is_on.h"
#include "base/functional/bind.h"
#include "base/functional/callback.h"
+#include "base/location.h"
#include "base/memory/scoped_refptr.h"
#include "base/task/sequenced_task_runner.h"
#include "base/time/time.h"
@@ -72,8 +73,9 @@
}
scoped_refptr<ThreadSafeProxy> CreateThreadSafeProxy(
- scoped_refptr<ThreadSafeProxy::Target> target) {
- return endpoint_client_->CreateThreadSafeProxy(std::move(target));
+ scoped_refptr<ThreadSafeProxy::Target> target,
+ const base::Location& location) {
+ return endpoint_client_->CreateThreadSafeProxy(std::move(target), location);
}
#if DCHECK_IS_ON()
diff --git a/public/cpp/bindings/shared_associated_remote.h b/public/cpp/bindings/shared_associated_remote.h
index 5a72c19..e9f5833 100644
--- a/public/cpp/bindings/shared_associated_remote.h
+++ b/public/cpp/bindings/shared_associated_remote.h
@@ -7,6 +7,7 @@
#include <tuple>
+#include "base/location.h"
#include "base/memory/scoped_refptr.h"
#include "base/task/sequenced_task_runner.h"
#include "mojo/public/cpp/bindings/associated_remote.h"
@@ -47,9 +48,10 @@
explicit SharedAssociatedRemote(
PendingAssociatedRemote<Interface> pending_remote,
scoped_refptr<base::SequencedTaskRunner> bind_task_runner =
- base::SequencedTaskRunner::GetCurrentDefault()) {
+ base::SequencedTaskRunner::GetCurrentDefault(),
+ const base::Location& location = base::Location::Current()) {
if (pending_remote.is_valid())
- Bind(std::move(pending_remote), std::move(bind_task_runner));
+ Bind(std::move(pending_remote), std::move(bind_task_runner), location);
}
bool is_bound() const { return remote_ != nullptr; }
@@ -83,20 +85,22 @@
// one of them, on `task_runner`. The other is returned as a receiver.
mojo::PendingAssociatedReceiver<Interface> BindNewEndpointAndPassReceiver(
scoped_refptr<base::SequencedTaskRunner> bind_task_runner =
- base::SequencedTaskRunner::GetCurrentDefault()) {
+ base::SequencedTaskRunner::GetCurrentDefault(),
+ const base::Location& location = base::Location::Current()) {
if (!internal::GetRuntimeFeature_ExpectEnabled<Interface>()) {
return PendingAssociatedReceiver<Interface>();
}
mojo::PendingAssociatedRemote<Interface> remote;
auto receiver = remote.InitWithNewEndpointAndPassReceiver();
- Bind(std::move(remote), std::move(bind_task_runner));
+ Bind(std::move(remote), std::move(bind_task_runner), location);
return receiver;
}
// Binds to `pending_remote` on `bind_task_runner`.
void Bind(PendingAssociatedRemote<Interface> pending_remote,
scoped_refptr<base::SequencedTaskRunner> bind_task_runner =
- base::SequencedTaskRunner::GetCurrentDefault()) {
+ base::SequencedTaskRunner::GetCurrentDefault(),
+ const base::Location& location = base::Location::Current()) {
DCHECK(!remote_);
DCHECK(pending_remote.is_valid());
if (!internal::GetRuntimeFeature_ExpectEnabled<Interface>()) {
@@ -104,7 +108,7 @@
return;
}
remote_ = SharedRemoteBase<AssociatedRemote<Interface>>::Create(
- std::move(pending_remote), std::move(bind_task_runner));
+ std::move(pending_remote), std::move(bind_task_runner), location);
}
private:
diff --git a/public/cpp/bindings/shared_remote.h b/public/cpp/bindings/shared_remote.h
index ded9752..a1fef2e 100644
--- a/public/cpp/bindings/shared_remote.h
+++ b/public/cpp/bindings/shared_remote.h
@@ -9,6 +9,7 @@
#include <tuple>
#include "base/functional/bind.h"
+#include "base/location.h"
#include "base/memory/ptr_util.h"
#include "base/memory/ref_counted.h"
#include "base/synchronization/waitable_event.h"
@@ -116,10 +117,11 @@
RemoteWrapper(const RemoteWrapper&) = delete;
RemoteWrapper& operator=(const RemoteWrapper&) = delete;
- std::unique_ptr<ThreadSafeForwarder<InterfaceType>> CreateForwarder() {
+ std::unique_ptr<ThreadSafeForwarder<InterfaceType>> CreateForwarder(
+ const base::Location& location) {
return std::make_unique<ThreadSafeForwarder<InterfaceType>>(
remote_.internal_state()->CreateThreadSafeProxy(
- base::MakeRefCounted<ProxyTarget>(this)));
+ base::MakeRefCounted<ProxyTarget>(this), location));
}
void set_disconnect_handler(
@@ -204,16 +206,21 @@
}
};
- explicit SharedRemoteBase(scoped_refptr<RemoteWrapper> wrapper)
- : wrapper_(std::move(wrapper)), forwarder_(wrapper_->CreateForwarder()) {}
+ explicit SharedRemoteBase(scoped_refptr<RemoteWrapper> wrapper,
+ const base::Location& location)
+ : wrapper_(std::move(wrapper)),
+ forwarder_(wrapper_->CreateForwarder(location)) {}
// Creates a SharedRemoteBase bound to `pending_remote`. All messages sent
// through the SharedRemote will first bounce through `task_runner`.
static scoped_refptr<SharedRemoteBase> Create(
PendingType pending_remote,
- scoped_refptr<base::SequencedTaskRunner> task_runner) {
- return new SharedRemoteBase(base::MakeRefCounted<RemoteWrapper>(
- std::move(pending_remote), std::move(task_runner)));
+ scoped_refptr<base::SequencedTaskRunner> task_runner,
+ const base::Location& location) {
+ return new SharedRemoteBase(
+ base::MakeRefCounted<RemoteWrapper>(std::move(pending_remote),
+ std::move(task_runner)),
+ location);
}
~SharedRemoteBase() = default;
@@ -248,15 +255,18 @@
// Constructs a SharedRemote bound to `pending_remote` on the calling
// sequence. See `Bind()` below for more details.
- explicit SharedRemote(PendingRemote<Interface> pending_remote) {
- Bind(std::move(pending_remote), nullptr);
+ explicit SharedRemote(
+ PendingRemote<Interface> pending_remote,
+ const base::Location& location = base::Location::Current()) {
+ Bind(std::move(pending_remote), nullptr, location);
}
// Constructs a SharedRemote bound to `pending_remote` on the sequence given
// by `bind_task_runner`. See `Bind()` below for more details.
SharedRemote(PendingRemote<Interface> pending_remote,
- scoped_refptr<base::SequencedTaskRunner> bind_task_runner) {
- Bind(std::move(pending_remote), std::move(bind_task_runner));
+ scoped_refptr<base::SequencedTaskRunner> bind_task_runner,
+ const base::Location& location = base::Location::Current()) {
+ Bind(std::move(pending_remote), std::move(bind_task_runner), location);
}
// SharedRemote supports both copy and move construction and assignment. These
@@ -311,31 +321,33 @@
// this call and re-bound to `pending_remote`. Any prior copies made are NOT
// affected and will retain their reference to the original Remote.
void Bind(PendingRemote<Interface> pending_remote,
- scoped_refptr<base::SequencedTaskRunner> bind_task_runner) {
+ scoped_refptr<base::SequencedTaskRunner> bind_task_runner,
+ const base::Location& location = base::Location::Current()) {
if (!internal::GetRuntimeFeature_ExpectEnabled<Interface>()) {
remote_.reset();
return;
}
if (bind_task_runner && pending_remote) {
remote_ = SharedRemoteBase<Remote<Interface>>::Create(
- std::move(pending_remote), std::move(bind_task_runner));
+ std::move(pending_remote), std::move(bind_task_runner), location);
} else if (pending_remote) {
remote_ = SharedRemoteBase<Remote<Interface>>::Create(
std::move(pending_remote),
- base::SequencedTaskRunner::GetCurrentDefault());
+ base::SequencedTaskRunner::GetCurrentDefault(), location);
}
}
// Creates a new pipe, binding this SharedRemote to one end on
// `bind_task_runner` and returning the other end as a PendingReceiver.
PendingReceiver<Interface> BindNewPipeAndPassReceiver(
- scoped_refptr<base::SequencedTaskRunner> bind_task_runner = nullptr) {
+ scoped_refptr<base::SequencedTaskRunner> bind_task_runner = nullptr,
+ const base::Location& location = base::Location::Current()) {
if (!internal::GetRuntimeFeature_ExpectEnabled<Interface>()) {
return PendingReceiver<Interface>();
}
PendingRemote<Interface> remote;
auto receiver = remote.InitWithNewPipeAndPassReceiver();
- Bind(std::move(remote), std::move(bind_task_runner));
+ Bind(std::move(remote), std::move(bind_task_runner), location);
return receiver;
}
