FLEDGE: refactor to a single DebugCommandQueue owned by V8Helper.
Aborting debugger pauses correctly when a worklet is unloaded seems to
be only reasonably implemented by synchronizing via the
DebugCommandQueue lock, which in turn means we want closer integration
between AuctionV8Helper and DebugCommandQueue.
... So this makes AuctionV8Helper own a single DebugCommandQueue rather
than have per-worklet ones, so it should be able to talk to it directly
and invoke atomic ops needed to get cleanup stuff race-free.
Change-Id: I5ed9aba266bd14e18b25347f9835dde240966910
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3272238
Commit-Queue: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#940545}
diff --git a/content/services/auction_worklet/auction_v8_devtools_agent.cc b/content/services/auction_worklet/auction_v8_devtools_agent.cc
index 2dc0987..ee355b8 100644
--- a/content/services/auction_worklet/auction_v8_devtools_agent.cc
+++ b/content/services/auction_worklet/auction_v8_devtools_agent.cc
@@ -23,9 +23,11 @@
AuctionV8DevToolsAgent::AuctionV8DevToolsAgent(
AuctionV8Helper* v8_helper,
+ DebugCommandQueue* debug_command_queue,
scoped_refptr<base::SequencedTaskRunner> io_session_receiver_sequence)
: v8_helper_(v8_helper),
- io_session_receiver_sequence_(std::move(io_session_receiver_sequence)) {}
+ io_session_receiver_sequence_(std::move(io_session_receiver_sequence)),
+ debug_command_queue_(debug_command_queue) {}
AuctionV8DevToolsAgent::~AuctionV8DevToolsAgent() {
DCHECK_CALLED_ON_VALID_SEQUENCE(v8_sequence_checker_);
@@ -78,8 +80,8 @@
// `sessions_` guarantees `session_impl` won't outlast `this`.
base::Unretained(this));
auto session_impl = std::make_unique<AuctionV8DevToolsSession>(
- v8_helper_, context_group_info.command_queue, context_group_id,
- session_id, client_expects_binary_responses, std::move(host),
+ v8_helper_, debug_command_queue_, context_group_id, session_id,
+ client_expects_binary_responses, std::move(host),
io_session_receiver_sequence_, std::move(io_session_receiver),
std::move(session_destroyed));
context_group_info.sessions.insert(session_impl.get());
@@ -101,22 +103,23 @@
void AuctionV8DevToolsAgent::runMessageLoopOnPause(int context_group_id) {
DCHECK_CALLED_ON_VALID_SEQUENCE(v8_sequence_checker_);
- DCHECK(!paused_context_group_queue_);
+ DCHECK(!paused_);
auto it = context_groups_.find(context_group_id);
DCHECK(it != context_groups_.end());
- paused_context_group_queue_ = it->second.command_queue;
v8_helper_->PauseTimeoutTimer();
- paused_context_group_queue_->PauseForDebuggerAndRunCommands();
+ paused_ = true;
+ debug_command_queue_->PauseForDebuggerAndRunCommands();
+ DCHECK(paused_);
v8_helper_->ResumeTimeoutTimer();
- paused_context_group_queue_ = nullptr;
+ paused_ = false;
}
void AuctionV8DevToolsAgent::quitMessageLoopOnPause() {
DCHECK_CALLED_ON_VALID_SEQUENCE(v8_sequence_checker_);
- DCHECK(paused_context_group_queue_);
- paused_context_group_queue_->QuitPauseForDebugger();
+ DCHECK(paused_);
+ debug_command_queue_->QuitPauseForDebugger();
}
void AuctionV8DevToolsAgent::runIfWaitingForDebugger(int context_group_id) {
@@ -130,13 +133,8 @@
auto it = context_groups_.find(session->context_group_id());
DCHECK(it != context_groups_.end());
it->second.sessions.erase(session);
- if (it->second.sessions.empty()) {
- // TODO(morlovich): This currently can't happen since we can't see
- // SessionDestroyed when paused, but the scenario of navigating away when
- // paused may be a trouble spot.
- DCHECK_NE(it->second.command_queue, paused_context_group_queue_);
+ if (it->second.sessions.empty())
context_groups_.erase(it);
- }
}
} // namespace auction_worklet
diff --git a/content/services/auction_worklet/auction_v8_devtools_agent.h b/content/services/auction_worklet/auction_v8_devtools_agent.h
index 4784ccc..1db7bb7f 100644
--- a/content/services/auction_worklet/auction_v8_devtools_agent.h
+++ b/content/services/auction_worklet/auction_v8_devtools_agent.h
@@ -12,7 +12,6 @@
#include "base/memory/scoped_refptr.h"
#include "base/sequence_checker.h"
#include "base/task/sequenced_task_runner.h"
-#include "content/services/auction_worklet/debug_command_queue.h"
#include "mojo/public/cpp/bindings/pending_associated_receiver.h"
#include "mojo/public/cpp/bindings/pending_associated_remote.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -25,6 +24,7 @@
class AuctionV8Helper;
class AuctionV8DevToolsSession;
+class DebugCommandQueue;
// Implementation of blink.mojom.DevToolsAgent for things run via
// AuctionV8Helper.
@@ -35,11 +35,10 @@
// as they are required to be on a different thread for use when the V8
// thread is busy.
//
-// Receiver for per-context group blink::mojom::DevToolsAgent pipes. Manages
-// per-context group DebugCommandQueues and creates/manages the lifetimes of the
-// blink::mojom::DevToolsSessions. Also serves as the
-// v8_inspector::V8InspectorClient to handle pause/resume calls received back
-// from V8.
+// Receiver for per-context group blink::mojom::DevToolsAgent pipes.
+// Creates/manages the lifetimes of the blink::mojom::DevToolsSessions. Also
+// serves as the v8_inspector::V8InspectorClient to handle pause/resume calls
+// received back from V8.
//
// To summarize, the thread split is as follows:
//
@@ -56,11 +55,13 @@
public v8_inspector::V8InspectorClient {
public:
// `v8_helper` is expected to own `this`.
+ // `debug_command_queue` is expected to be owned by `v8_helper`.
// `io_session_receiver_sequence` must be distinct from
// `v8_helper->v8_runner()`, and be able to grab mutexes (for short duration)
// and handle a Mojo connection.
AuctionV8DevToolsAgent(
AuctionV8Helper* v8_helper,
+ DebugCommandQueue* debug_command_queue,
scoped_refptr<base::SequencedTaskRunner> io_session_receiver_sequence);
AuctionV8DevToolsAgent(const AuctionV8DevToolsAgent&) = delete;
AuctionV8DevToolsAgent& operator=(const AuctionV8DevToolsAgent&) = delete;
@@ -87,9 +88,6 @@
ContextGroupInfo();
~ContextGroupInfo();
- scoped_refptr<DebugCommandQueue> command_queue =
- base::MakeRefCounted<DebugCommandQueue>();
-
// Owned by `sessions_` in the AuctionV8DevToolsAgent object; stale entries
// removed by its SessionDestroyed().
std::set<AuctionV8DevToolsSession*> sessions;
@@ -140,10 +138,9 @@
// ~AuctionV8DevToolsSession (via a callback).
std::map<int, ContextGroupInfo> context_groups_;
- // Command queue for context group that has currently paused V8 execution.
- // While non-empty, the V8 thread's message loop should be blocked
- // waiting on more commands to be queued, and executing queued commands.
- scoped_refptr<DebugCommandQueue> paused_context_group_queue_;
+ // Owned by `v8_helper` which owns `this`.
+ DebugCommandQueue* const debug_command_queue_;
+ bool paused_ = false;
SEQUENCE_CHECKER(v8_sequence_checker_);
};
diff --git a/content/services/auction_worklet/auction_v8_devtools_session.cc b/content/services/auction_worklet/auction_v8_devtools_session.cc
index 27f0963..416910da0 100644
--- a/content/services/auction_worklet/auction_v8_devtools_session.cc
+++ b/content/services/auction_worklet/auction_v8_devtools_session.cc
@@ -103,10 +103,10 @@
static void Create(
mojo::PendingReceiver<blink::mojom::DevToolsSession> io_session_receiver,
scoped_refptr<base::SequencedTaskRunner> io_session_receiver_sequence,
- scoped_refptr<DebugCommandQueue> debug_command_queue,
+ DebugCommandQueue* debug_command_queue,
RunDispatch v8_thread_dispatch) {
- auto instance = base::WrapUnique(new IOSession(
- std::move(debug_command_queue), std::move(v8_thread_dispatch)));
+ auto instance = base::WrapUnique(
+ new IOSession(debug_command_queue, std::move(v8_thread_dispatch)));
io_session_receiver_sequence->PostTask(
FROM_HERE,
base::BindOnce(&IOSession::ConnectReceiver, std::move(instance),
@@ -127,7 +127,7 @@
}
private:
- IOSession(scoped_refptr<DebugCommandQueue> debug_command_queue,
+ IOSession(DebugCommandQueue* debug_command_queue,
RunDispatch v8_thread_dispatch)
: debug_command_queue_(debug_command_queue),
v8_thread_dispatch_(v8_thread_dispatch) {
@@ -144,7 +144,7 @@
std::move(io_session_receiver));
}
- scoped_refptr<DebugCommandQueue> debug_command_queue_;
+ DebugCommandQueue* const debug_command_queue_;
RunDispatch v8_thread_dispatch_;
SEQUENCE_CHECKER(io_session_receiver_sequence_checker_);
@@ -152,7 +152,7 @@
AuctionV8DevToolsSession::AuctionV8DevToolsSession(
AuctionV8Helper* v8_helper,
- scoped_refptr<DebugCommandQueue> debug_command_queue,
+ DebugCommandQueue* debug_command_queue,
int context_group_id,
const std::string& session_id,
bool client_expects_binary_responses,
@@ -161,7 +161,7 @@
mojo::PendingReceiver<blink::mojom::DevToolsSession> io_session_receiver,
SessionDestroyedCallback on_delete_callback)
: v8_helper_(v8_helper),
- debug_command_queue_(std::move(debug_command_queue)),
+ debug_command_queue_(debug_command_queue),
context_group_id_(context_group_id),
session_id_(session_id),
client_expects_binary_responses_(client_expects_binary_responses),
diff --git a/content/services/auction_worklet/auction_v8_devtools_session.h b/content/services/auction_worklet/auction_v8_devtools_session.h
index e44fabc..b948c9f 100644
--- a/content/services/auction_worklet/auction_v8_devtools_session.h
+++ b/content/services/auction_worklet/auction_v8_devtools_session.h
@@ -51,7 +51,7 @@
//
AuctionV8DevToolsSession(
AuctionV8Helper* v8_helper,
- scoped_refptr<DebugCommandQueue> debug_command_queue,
+ DebugCommandQueue* debug_command_queue,
int context_group_id,
const std::string& session_id,
bool client_expects_binary_responses,
@@ -110,7 +110,7 @@
std::vector<uint8_t> message) const;
AuctionV8Helper* const v8_helper_; // owns agent owns this.
- scoped_refptr<DebugCommandQueue> debug_command_queue_;
+ DebugCommandQueue* const debug_command_queue_; // owned by `v8_helper`.
const int context_group_id_;
const std::string session_id_;
const bool client_expects_binary_responses_;
diff --git a/content/services/auction_worklet/auction_v8_helper.cc b/content/services/auction_worklet/auction_v8_helper.cc
index e52e8c3..7a77c23 100644
--- a/content/services/auction_worklet/auction_v8_helper.cc
+++ b/content/services/auction_worklet/auction_v8_helper.cc
@@ -25,6 +25,7 @@
#include "base/timer/timer.h"
#include "build/build_config.h"
#include "content/services/auction_worklet/auction_v8_devtools_agent.h"
+#include "content/services/auction_worklet/debug_command_queue.h"
#include "gin/array_buffer.h"
#include "gin/converter.h"
#include "gin/gin_features.h"
@@ -580,7 +581,7 @@
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!devtools_agent_) {
devtools_agent_ = std::make_unique<AuctionV8DevToolsAgent>(
- this, std::move(mojo_sequence));
+ this, debug_command_queue_.get(), std::move(mojo_sequence));
v8_inspector_ =
v8_inspector::V8Inspector::create(isolate(), devtools_agent_.get());
}
@@ -639,7 +640,7 @@
AuctionV8Helper::AuctionV8Helper(
scoped_refptr<base::SingleThreadTaskRunner> v8_runner)
: base::RefCountedDeleteOnSequence<AuctionV8Helper>(v8_runner),
- v8_runner_(std::move(v8_runner)),
+ v8_runner_(v8_runner),
timer_task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})) {
DETACH_FROM_SEQUENCE(sequence_checker_);
@@ -670,6 +671,11 @@
gin::IsolateHolder::IsolateType::kUtility);
FullIsolateScope v8_scope(this);
scratch_context_.Reset(isolate(), CreateContext());
+
+ // This is mostly unneeded unless the debugger agent is in use, but having it
+ // always wil lbe helpful for preventing races if debugger is being created
+ // right as a worklet is being unloaded.
+ debug_command_queue_ = std::make_unique<DebugCommandQueue>();
}
// static
diff --git a/content/services/auction_worklet/auction_v8_helper.h b/content/services/auction_worklet/auction_v8_helper.h
index 7881405..c7d4b469 100644
--- a/content/services/auction_worklet/auction_v8_helper.h
+++ b/content/services/auction_worklet/auction_v8_helper.h
@@ -39,6 +39,7 @@
namespace auction_worklet {
class AuctionV8DevToolsAgent;
+class DebugCommandQueue;
// Helper for Javascript operations. Owns a V8 isolate, and manages operations
// on it. Must be deleted after all V8 objects created using its isolate. It
@@ -323,6 +324,9 @@
std::map<int, base::OnceClosure> resume_callbacks_
GUARDED_BY_CONTEXT(sequence_checker_);
+ std::unique_ptr<DebugCommandQueue> debug_command_queue_
+ GUARDED_BY_CONTEXT(sequence_checker_);
+
// Destruction order between `devtools_agent_` and `v8_inspector_` is
// relevant; see also comment in ~AuctionV8Helper().
std::unique_ptr<AuctionV8DevToolsAgent> devtools_agent_
diff --git a/content/services/auction_worklet/debug_command_queue.cc b/content/services/auction_worklet/debug_command_queue.cc
index 6fdd4fb..af22331 100644
--- a/content/services/auction_worklet/debug_command_queue.cc
+++ b/content/services/auction_worklet/debug_command_queue.cc
@@ -9,13 +9,20 @@
namespace auction_worklet {
DebugCommandQueue::DebugCommandQueue()
- : v8_runner_(base::SequencedTaskRunnerHandle::Get()), wake_up_(&lock_) {}
+ : v8_runner_(base::SequencedTaskRunnerHandle::Get()), wake_up_(&lock_) {
+ // Create helper for posting RunQueue here to bind it to the weak pointer on
+ // the right thread.
+ run_queue_closure_ = base::BindRepeating(&DebugCommandQueue::RunQueue,
+ weak_ptr_factory_.GetWeakPtr());
+}
+
+DebugCommandQueue::~DebugCommandQueue() = default;
void DebugCommandQueue::PauseForDebuggerAndRunCommands() {
DCHECK(v8_runner_->RunsTasksInCurrentSequence());
base::AutoLock auto_lock(lock_);
- DCHECK(!v8_thread_paused_);
+ CHECK(!v8_thread_paused_);
v8_thread_paused_ = true;
while (true) {
RunQueueWithLockHeld();
@@ -45,12 +52,9 @@
}
}
-DebugCommandQueue::~DebugCommandQueue() = default;
-
void DebugCommandQueue::PostRunQueue() EXCLUSIVE_LOCKS_REQUIRED(lock_) {
if (!queue_.empty()) {
- v8_runner_->PostTask(FROM_HERE,
- base::BindOnce(&DebugCommandQueue::RunQueue, this));
+ v8_runner_->PostTask(FROM_HERE, run_queue_closure_);
}
}
diff --git a/content/services/auction_worklet/debug_command_queue.h b/content/services/auction_worklet/debug_command_queue.h
index 03ab67c..25db9a8f 100644
--- a/content/services/auction_worklet/debug_command_queue.h
+++ b/content/services/auction_worklet/debug_command_queue.h
@@ -7,8 +7,7 @@
#include "base/callback.h"
#include "base/containers/queue.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
#include "base/synchronization/condition_variable.h"
#include "base/synchronization/lock.h"
#include "base/task/sequenced_task_runner.h"
@@ -18,14 +17,15 @@
// DebugCommandQueue helps coordinate command transfer between Session (lives on
// V8 thread) and IOSession (lives on mojo thread), as well as blocking
-// execution of V8 thread when paused in debugger. It's jointly owned by Session
-// and IOSession.
-class DebugCommandQueue : public base::RefCountedThreadSafe<DebugCommandQueue> {
+// execution of V8 thread when paused in debugger. It's owned by the
+// AuctionV8Helper
+class DebugCommandQueue {
public:
- // This must be created on the v8 thread.
+ // Must be created and destroyed on the v8 thread.
DebugCommandQueue();
DebugCommandQueue(const DebugCommandQueue&) = delete;
DebugCommandQueue& operator=(const DebugCommandQueue&) = delete;
+ ~DebugCommandQueue();
// Blocks the current thread until QuitPauseForDebugger() is called, executing
// only things added via Post().
@@ -49,9 +49,6 @@
void QueueTaskForV8Thread(base::OnceClosure task);
private:
- friend class base::RefCountedThreadSafe<DebugCommandQueue>;
- ~DebugCommandQueue();
-
void PostRunQueue() EXCLUSIVE_LOCKS_REQUIRED(lock_);
void RunQueue();
void RunQueueWithLockHeld() EXCLUSIVE_LOCKS_REQUIRED(lock_);
@@ -62,6 +59,9 @@
base::ConditionVariable wake_up_ GUARDED_BY(lock_);
base::queue<base::OnceClosure> queue_ GUARDED_BY(lock_);
bool v8_thread_paused_ GUARDED_BY(lock_) = false;
+
+ base::RepeatingClosure run_queue_closure_;
+ base::WeakPtrFactory<DebugCommandQueue> weak_ptr_factory_{this};
};
} // namespace auction_worklet
diff --git a/content/services/auction_worklet/debug_command_queue_unittest.cc b/content/services/auction_worklet/debug_command_queue_unittest.cc
index 51750a9a..61c0289 100644
--- a/content/services/auction_worklet/debug_command_queue_unittest.cc
+++ b/content/services/auction_worklet/debug_command_queue_unittest.cc
@@ -8,8 +8,10 @@
#include <vector>
#include "base/synchronization/lock.h"
+#include "base/task/sequenced_task_runner.h"
#include "base/test/task_environment.h"
#include "base/thread_annotations.h"
+#include "base/threading/sequenced_task_runner_handle.h"
#include "content/services/auction_worklet/auction_v8_helper.h"
#include "testing/gmock/include/gmock/gmock-matchers.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -20,14 +22,23 @@
class DebugCommandQueueTest : public testing::Test {
public:
- DebugCommandQueueTest() : v8_runner_(AuctionV8Helper::CreateTaskRunner()) {
+ DebugCommandQueueTest()
+ : v8_runner_(AuctionV8Helper::CreateTaskRunner()),
+ command_queue_(nullptr, base::OnTaskRunnerDeleter(nullptr)) {
base::RunLoop run_loop;
// Create `DebugCommandQueue on `v8_runner_`.
v8_runner_->PostTask(
FROM_HERE,
base::BindOnce(
- [](scoped_refptr<DebugCommandQueue>* out, base::OnceClosure done) {
- *out = base::MakeRefCounted<DebugCommandQueue>();
+ [](std::unique_ptr<DebugCommandQueue, base::OnTaskRunnerDeleter>*
+ out,
+ base::OnceClosure done) {
+ *out =
+ std::unique_ptr<DebugCommandQueue, base::OnTaskRunnerDeleter>(
+ new DebugCommandQueue,
+ base::OnTaskRunnerDeleter(
+ base::SequencedTaskRunnerHandle::Get()));
+
std::move(done).Run();
},
&command_queue_, run_loop.QuitClosure()));
@@ -39,12 +50,12 @@
v8_runner_->PostTask(
FROM_HERE,
base::BindOnce(
- [](scoped_refptr<DebugCommandQueue> queue,
- base::OnceClosure to_post, base::OnceClosure done) {
+ [](DebugCommandQueue* queue, base::OnceClosure to_post,
+ base::OnceClosure done) {
queue->QueueTaskForV8Thread(std::move(to_post));
std::move(done).Run();
},
- command_queue_, std::move(to_post), run_loop.QuitClosure()));
+ command_queue_.get(), std::move(to_post), run_loop.QuitClosure()));
run_loop.Run();
}
@@ -90,7 +101,7 @@
base::test::TaskEnvironment task_environment_;
scoped_refptr<base::SingleThreadTaskRunner> v8_runner_;
- scoped_refptr<DebugCommandQueue> command_queue_;
+ std::unique_ptr<DebugCommandQueue, base::OnTaskRunnerDeleter> command_queue_;
std::vector<std::string> log_ GUARDED_BY(lock_);
base::Lock lock_;
};
@@ -142,7 +153,7 @@
// A task that itself queues more tasks.
base::RunLoop run_loop;
command_queue_->QueueTaskForV8Thread(base::BindOnce(
- [](scoped_refptr<DebugCommandQueue> command_queue, base::OnceClosure log1,
+ [](DebugCommandQueue* command_queue, base::OnceClosure log1,
base::OnceClosure log2, base::OnceClosure log3,
base::OnceClosure quit_closure) {
std::move(log1).Run();
@@ -150,7 +161,7 @@
command_queue->QueueTaskForV8Thread(std::move(log3));
command_queue->QueueTaskForV8Thread(std::move(quit_closure));
},
- command_queue_, LogString("1"), LogString("2"), LogString("3"),
+ command_queue_.get(), LogString("1"), LogString("2"), LogString("3"),
run_loop.QuitClosure()));
run_loop.Run();
EXPECT_THAT(TakeLog(), ElementsAre("1", "2", "3"));
@@ -162,7 +173,7 @@
base::RunLoop run_loop;
command_queue_->QueueTaskForV8Thread(PauseForDebuggerAndRunCommands());
command_queue_->QueueTaskForV8Thread(base::BindOnce(
- [](scoped_refptr<DebugCommandQueue> command_queue, base::OnceClosure log1,
+ [](DebugCommandQueue* command_queue, base::OnceClosure log1,
base::OnceClosure log2, base::OnceClosure log3,
base::OnceClosure quit_closure) {
std::move(log1).Run();
@@ -170,7 +181,7 @@
command_queue->QueueTaskForV8Thread(std::move(log3));
command_queue->QueueTaskForV8Thread(std::move(quit_closure));
},
- command_queue_, LogString("1"), LogString("2"), LogString("3"),
+ command_queue_.get(), LogString("1"), LogString("2"), LogString("3"),
run_loop.QuitClosure()));
run_loop.Run();
