Devtools: Fix a number of issues with protocol message scheduling

- Change how protocol messages are dispatched to/in the renderer.
Previously the browser process would send 'interrupting' messages using
a different DevToolsSession, io_session_. This mojo channel was bound
using a task runner on the IO thread in the renderer process, meaning
messages would still be processed even when the main thread was blocked.
The IO thread would then post a task to the InspectorTaskRunner, causing
it to interrupt V8 execution and run the posted task.

To simplify this as much as possible, the browser now always uses the
io_session_, but the IO thread will post a (non)interrupting task as
appropriate for the message. This removes the need to keep the list of
interrupting tasks in sync across the browser/renderer.

Messages in-transit now always go via the IO thread and get posted to
the InspectorTaskRunner, meaning pending messages can be tracked clearly
using tracing, rather than sitting in a mojo channel somewhere waiting
to get processed.

- Add a new task queue for interrupting tasks only. Previously during
an interrupt callback, we would run all tasks in the task queue, which
could cause tasks that we not meant to be interrupting to be run during
an interrupt. Now we run all tasks only from the new queue.

- Remove IgnoreInterruptsScope. This was mistakenly applied to all
running tasks during a refactoring. There used to be two ways to run
tasks, one was refactored to call the other, but then they both used
this interrupt scope. We actually don't need it at all because we now
only run 'interrupting' tasks during interrupts rather than flushing
the entire queue, which might contain non-interrupting tasks.

- Remove the use of IgnoreInterruptScope in
LocalWindowProxy::Initialize. The comment there doesn't apply now that
we ensure we don't run non-interrupting tasks during interrupts. We
don't have any calls that can cause JS to run in the
ShouldInterruptForMethod list, so no JS will run there.

- Add trace events to show how protocol messages are dispatched in the
renderer process. These were really useful for debugging all of these
issues.

- Remove InspectorTaskRunner::IsRunningTask which was not used. Remove
all the associated state handling around running_task_.

- Remove InspectorTaskRunner::WaitForAndRunSingleTask which was not
used. Remove the condition variable and all the handling for it as it
was only used from this unused method.

Bug: 1010534, 992560
Change-Id: I58c9193ecd1fc784cda22b054a9c5b7bc2f09969
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1926588Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719957}

content/browser/devtools/devtools_session.cc

@@ -21,18 +21,6 @@
 
 namespace content {
 namespace {
-bool ShouldSendOnIO(const std::string& method) {
-  // Keep in sync with WebDevToolsAgent::ShouldInterruptForMethod.
-  // TODO(einbinder): find a way to share this.
-  return method == "Debugger.pause" || method == "Debugger.setBreakpoint" ||
-         method == "Debugger.setBreakpointByUrl" ||
-         method == "Debugger.removeBreakpoint" ||
-         method == "Debugger.setBreakpointsActive" ||
-         method == "Debugger.getStackTrace" ||
-         method == "Performance.getMetrics" || method == "Page.crash" ||
-         method == "Runtime.terminateExecution" ||
-         method == "Emulation.setScriptExecutionDisabled";
-}
 
 // Async control commands (such as CSS.enable) are idempotant and can
 // be safely replayed in the new render frame host. We will always forward
@@ -255,6 +243,10 @@ void DevToolsSession::HandleCommand(
   if (!dispatcher_->parseCommand(value.get(), &call_id, &method))
     return;
   if (browser_only_ || dispatcher_->canDispatch(method)) {
+    TRACE_EVENT_WITH_FLOW2("devtools",
+                           "DevToolsSession::HandleCommand in Browser", call_id,
+                           TRACE_EVENT_FLAG_FLOW_OUT, "method", method.c_str(),
+                           "call_id", call_id);
     dispatcher_->dispatch(call_id, method, std::move(value), message);
   } else {
     fallThrough(call_id, method, message);
@@ -284,14 +276,13 @@ void DevToolsSession::DispatchProtocolMessageToAgent(
   auto message_ptr = blink::mojom::DevToolsMessage::New();
   message_ptr->data = mojo_base::BigBuffer(base::make_span(
       reinterpret_cast<const uint8_t*>(message.data()), message.length()));
-  if (ShouldSendOnIO(method)) {
-    if (io_session_)
-      io_session_->DispatchProtocolCommand(call_id, method,
-                                           std::move(message_ptr));
-  } else {
-    if (session_)
-      session_->DispatchProtocolCommand(call_id, method,
-                                        std::move(message_ptr));
+  if (io_session_) {
+    TRACE_EVENT_WITH_FLOW2("devtools",
+                           "DevToolsSession::DispatchProtocolMessageToAgent",
+                           call_id, TRACE_EVENT_FLAG_FLOW_OUT, "method",
+                           method.c_str(), "call_id", call_id);
+    io_session_->DispatchProtocolCommand(call_id, method,
+                                         std::move(message_ptr));
   }
 }
 
@@ -366,6 +357,9 @@ void DevToolsSession::DispatchProtocolResponse(
     blink::mojom::DevToolsMessagePtr message,
     int call_id,
     blink::mojom::DevToolsSessionStatePtr updates) {
+  TRACE_EVENT_WITH_FLOW1("devtools",
+                         "DevToolsSession::DispatchProtocolResponse", call_id,
+                         TRACE_EVENT_FLAG_FLOW_IN, "call_id", call_id);
   ApplySessionStateUpdates(std::move(updates));
   auto it = waiting_for_response_.find(call_id);
   // TODO(johannes): Consider shutting down renderer instead of just

third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc

@@ -175,12 +175,6 @@ void LocalWindowProxy::Initialize() {
   CHECK(!GetFrame()->IsProvisional());
 
   ScriptForbiddenScope::AllowUserAgentScript allow_script;
-  // Inspector may request V8 interruption to process DevTools protocol
-  // commands, processing can force JavaScript execution. Since JavaScript
-  // evaluation is forbiden during creating of snapshot, we should ignore any
-  // inspector interruption to avoid JavaScript execution.
-  InspectorTaskRunner::IgnoreInterruptsScope inspector_ignore_interrupts(
-      GetFrame()->GetInspectorTaskRunner());
   v8::HandleScope handle_scope(GetIsolate());
 
   CreateContext();

third_party/blink/renderer/core/inspector/devtools_session.cc

@@ -32,8 +32,6 @@ namespace blink {
 namespace {
 const char kV8StateKey[] = "v8";
 bool ShouldInterruptForMethod(const String& method) {
-  // Keep in sync with DevToolsSession::ShouldSendOnIO.
-  // TODO(dgozman): find a way to share this.
   return method == "Debugger.pause" || method == "Debugger.setBreakpoint" ||
          method == "Debugger.setBreakpointByUrl" ||
          method == "Debugger.removeBreakpoint" ||
@@ -88,13 +86,24 @@ class DevToolsSession::IOSession : public mojom::blink::DevToolsSession {
       int call_id,
       const String& method,
       mojom::blink::DevToolsMessagePtr message) override {
-    DCHECK(ShouldInterruptForMethod(method));
+    TRACE_EVENT_WITH_FLOW1("devtools", "IOSession::DispatchProtocolCommand",
+                           call_id,
+                           TRACE_EVENT_FLAG_FLOW_OUT | TRACE_EVENT_FLAG_FLOW_IN,
+                           "call_id", call_id);
     // Crash renderer.
     if (method == "Page.crash")
       CHECK(false);
-    inspector_task_runner_->AppendTask(CrossThreadBindOnce(
-        &::blink::DevToolsSession::DispatchProtocolCommandImpl, session_,
-        call_id, method, UnwrapMessage(message)));
+    // Post a task to the worker or main renderer thread that will interrupt V8
+    // and be run immediately. Only methods that do not run JS code are safe.
+    if (ShouldInterruptForMethod(method)) {
+      inspector_task_runner_->AppendTask(CrossThreadBindOnce(
+          &::blink::DevToolsSession::DispatchProtocolCommandImpl, session_,
+          call_id, method, UnwrapMessage(message)));
+    } else {
+      inspector_task_runner_->AppendTaskDontInterrupt(CrossThreadBindOnce(
+          &::blink::DevToolsSession::DispatchProtocolCommandImpl, session_,
+          call_id, method, UnwrapMessage(message)));
+    }
   }
 
  private:
@@ -189,8 +198,9 @@ void DevToolsSession::DispatchProtocolCommand(
     int call_id,
     const String& method,
     blink::mojom::blink::DevToolsMessagePtr message_ptr) {
-  return DispatchProtocolCommandImpl(call_id, method,
-                                     UnwrapMessage(message_ptr));
+  // TODO(petermarshall): Remove the distinction between DevToolsSession and
+  // IOSession as we always use IOSession now.
+  NOTREACHED();
 }
 
 void DevToolsSession::DispatchProtocolCommandImpl(int call_id,
@@ -199,6 +209,10 @@ void DevToolsSession::DispatchProtocolCommandImpl(int call_id,
   DCHECK(crdtp::cbor::IsCBORMessage(
       crdtp::span<uint8_t>(data.data(), data.size())));
 
+  TRACE_EVENT_WITH_FLOW1(
+      "devtools", "DevToolsSession::DispatchProtocolCommandImpl", call_id,
+      TRACE_EVENT_FLAG_FLOW_OUT | TRACE_EVENT_FLAG_FLOW_IN, "call_id", call_id);
+
   // IOSession does not provide ordering guarantees relative to
   // Session, so a command may come to IOSession after Session is detached,
   // and get posted to main thread to this method.
@@ -267,6 +281,9 @@ void DevToolsSession::sendResponse(
 
 void DevToolsSession::SendProtocolResponse(int call_id,
                                            std::vector<uint8_t> message) {
+  TRACE_EVENT_WITH_FLOW1(
+      "devtools", "DevToolsSession::SendProtocolResponse", call_id,
+      TRACE_EVENT_FLAG_FLOW_OUT | TRACE_EVENT_FLAG_FLOW_IN, "call_id", call_id);
   if (IsDetached())
     return;
   flushProtocolNotifications();

third_party/blink/renderer/core/inspector/inspector_task_runner.cc

@@ -10,22 +10,9 @@
 
 namespace blink {
 
-InspectorTaskRunner::IgnoreInterruptsScope::IgnoreInterruptsScope(
-    scoped_refptr<InspectorTaskRunner> task_runner)
-    : was_ignoring_(task_runner->ignore_interrupts_),
-      task_runner_(task_runner) {
-  // There may be nested scopes e.g. when tasks are being executed on XHR
-  // breakpoint.
-  task_runner_->ignore_interrupts_ = true;
-}
-
-InspectorTaskRunner::IgnoreInterruptsScope::~IgnoreInterruptsScope() {
-  task_runner_->ignore_interrupts_ = was_ignoring_;
-}
-
 InspectorTaskRunner::InspectorTaskRunner(
     scoped_refptr<base::SingleThreadTaskRunner> isolate_task_runner)
-    : isolate_task_runner_(isolate_task_runner), condition_(mutex_) {}
+    : isolate_task_runner_(isolate_task_runner) {}
 
 InspectorTaskRunner::~InspectorTaskRunner() = default;
 
@@ -39,97 +26,53 @@ void InspectorTaskRunner::Dispose() {
   disposed_ = true;
   isolate_ = nullptr;
   isolate_task_runner_ = nullptr;
-  condition_.Broadcast();
 }
 
 void InspectorTaskRunner::AppendTask(Task task) {
   MutexLocker lock(mutex_);
   if (disposed_)
     return;
-  queue_.push_back(std::move(task));
-  condition_.Signal();
+  interrupting_task_queue_.push_back(std::move(task));
   PostCrossThreadTask(
       *isolate_task_runner_, FROM_HERE,
-      CrossThreadBindOnce(&InspectorTaskRunner::PerformSingleTaskDontWait,
-                          WrapRefCounted(this)));
+      CrossThreadBindOnce(
+          &InspectorTaskRunner::PerformSingleInterruptingTaskDontWait,
+          WrapRefCounted(this)));
   if (isolate_)
     isolate_->RequestInterrupt(&V8InterruptCallback, this);
 }
 
-bool InspectorTaskRunner::WaitForAndRunSingleTask() {
-  // |isolate_task_runner_| might be null in unit tests.
-  DCHECK(!isolate_task_runner_ ||
-         isolate_task_runner_->BelongsToCurrentThread());
-  {
-    MutexLocker lock(mutex_);
-    if (isolate_)
-      ThreadDebugger::IdleStarted(isolate_);
-  }
-  Task task = TakeNextTask(kWaitForTask);
-  {
-    MutexLocker lock(mutex_);
-    if (isolate_)
-      ThreadDebugger::IdleFinished(isolate_);
-  }
-  if (!task)
-    return false;
-  PerformSingleTask(std::move(task));
-  return true;
-}
-
-bool InspectorTaskRunner::IsRunningTask() {
+void InspectorTaskRunner::AppendTaskDontInterrupt(Task task) {
   MutexLocker lock(mutex_);
-  return running_task_;
+  if (disposed_)
+    return;
+  PostCrossThreadTask(*isolate_task_runner_, FROM_HERE, std::move(task));
 }
 
-InspectorTaskRunner::Task InspectorTaskRunner::TakeNextTask(
-    InspectorTaskRunner::WaitMode wait_mode) {
+InspectorTaskRunner::Task InspectorTaskRunner::TakeNextInterruptingTask() {
   MutexLocker lock(mutex_);
 
-  if (wait_mode == kWaitForTask) {
-    while (!disposed_ && queue_.IsEmpty())
-      condition_.Wait();
-  }
-
-  if (disposed_ || queue_.IsEmpty())
+  if (disposed_ || interrupting_task_queue_.IsEmpty())
     return Task();
 
-  return queue_.TakeFirst();
+  return interrupting_task_queue_.TakeFirst();
 }
 
-void InspectorTaskRunner::PerformSingleTask(Task task) {
-  DCHECK(isolate_task_runner_->BelongsToCurrentThread());
-  IgnoreInterruptsScope scope(this);
-  {
-    MutexLocker lock(mutex_);
-    DCHECK(!running_task_);
-    running_task_ = true;
-  }
-  std::move(task).Run();
-  {
-    MutexLocker lock(mutex_);
-    running_task_ = false;
-  }
-}
-
-void InspectorTaskRunner::PerformSingleTaskDontWait() {
-  Task task = TakeNextTask(kDontWaitForTask);
+void InspectorTaskRunner::PerformSingleInterruptingTaskDontWait() {
+  Task task = TakeNextInterruptingTask();
   if (task) {
     DCHECK(isolate_task_runner_->BelongsToCurrentThread());
-    PerformSingleTask(std::move(task));
+    std::move(task).Run();
   }
 }
 
 void InspectorTaskRunner::V8InterruptCallback(v8::Isolate*, void* data) {
   InspectorTaskRunner* runner = static_cast<InspectorTaskRunner*>(data);
-  if (runner->ignore_interrupts_)
+  Task task = runner->TakeNextInterruptingTask();
+  if (!task) {
     return;
-  while (true) {
-    Task task = runner->TakeNextTask(kDontWaitForTask);
-    if (!task)
-      return;
-    runner->PerformSingleTask(std::move(task));
   }
+  std::move(task).Run();
 }
 
 }  // namespace blink

third_party/blink/renderer/core/inspector/inspector_task_runner.h

@@ -23,7 +23,7 @@ namespace blink {
 // v8::Isolate to run the tasks.
 //
 // The tasks will be run on the isolate's thread after interruption or via
-// posting to it's task runner.
+// posting to its task runner.
 class CORE_EXPORT InspectorTaskRunner final
     : public ThreadSafeRefCounted<InspectorTaskRunner> {
   USING_FAST_MALLOC(InspectorTaskRunner);
@@ -46,26 +46,11 @@ class CORE_EXPORT InspectorTaskRunner final
   using Task = CrossThreadOnceClosure;
   void AppendTask(Task) LOCKS_EXCLUDED(mutex_);
 
-  // Can be called on any thread.
-  bool IsRunningTask() LOCKS_EXCLUDED(mutex_);
-
-  // Must be called on the isolate's thread.
-  // Returns |true| is the task was run, and |false| if the runner
-  // was disposed while waiting for the task.
-  bool WaitForAndRunSingleTask() LOCKS_EXCLUDED(mutex_);
-
-  // This can only be used on the isolate's thread.
-  class CORE_EXPORT IgnoreInterruptsScope final {
-    USING_FAST_MALLOC(IgnoreInterruptsScope);
-
-   public:
-    explicit IgnoreInterruptsScope(scoped_refptr<InspectorTaskRunner>);
-    ~IgnoreInterruptsScope();
-
-   private:
-    bool was_ignoring_;
-    scoped_refptr<InspectorTaskRunner> task_runner_;
-  };
+  // Can be called from any thread other than isolate's thread.
+  // This method appends a task and posts to the isolate's task runner to
+  // request that the next task be executed, but does not interrupt V8
+  // execution.
+  void AppendTaskDontInterrupt(Task) LOCKS_EXCLUDED(mutex_);
 
  private:
   friend ThreadSafeRefCounted<InspectorTaskRunner>;
@@ -74,20 +59,16 @@ class CORE_EXPORT InspectorTaskRunner final
   ~InspectorTaskRunner();
 
   // All these methods are run on the isolate's thread.
-  enum WaitMode { kWaitForTask, kDontWaitForTask };
-  Task TakeNextTask(WaitMode) LOCKS_EXCLUDED(mutex_);
-  void PerformSingleTask(Task) LOCKS_EXCLUDED(mutex_);
-  void PerformSingleTaskDontWait() LOCKS_EXCLUDED(mutex_);
+  Task TakeNextInterruptingTask() LOCKS_EXCLUDED(mutex_);
+  void PerformSingleInterruptingTaskDontWait() LOCKS_EXCLUDED(mutex_);
   static void V8InterruptCallback(v8::Isolate*, void* data);
 
   Mutex mutex_;
   scoped_refptr<base::SingleThreadTaskRunner> isolate_task_runner_;
   v8::Isolate* isolate_ GUARDED_BY(mutex_) = nullptr;
-  bool ignore_interrupts_ = false;
-  ThreadCondition condition_;
   Deque<Task> queue_;
+  Deque<Task> interrupting_task_queue_;
   bool disposed_ GUARDED_BY(mutex_) = false;
-  bool running_task_ GUARDED_BY(mutex_) = false;
   DISALLOW_COPY_AND_ASSIGN(InspectorTaskRunner);
 };