Do not pause on breaks while installing additional command line API

A break may cause the session disconnect (and therefore agents destruction)
on a nested message loop. The runtime agent code is generally prepared to
handle this during evaluate, but the code outside of it may be not. Besides,
having a break before the console API installed is generally not what
user wants or expects, so just disable all breaks while installing the API.

Bug: chromium:1122487
Change-Id: I1d40f5007f2e1e4ec07a50ef57988513d0309b7e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2437383
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70209}

src/api/api.cc

@@ -10288,6 +10288,12 @@ debug::PostponeInterruptsScope::PostponeInterruptsScope(v8::Isolate* isolate)
 
 debug::PostponeInterruptsScope::~PostponeInterruptsScope() = default;
 
+debug::DisableBreakScope::DisableBreakScope(v8::Isolate* isolate)
+    : scope_(std::make_unique<i::DisableBreak>(
+          reinterpret_cast<i::Isolate*>(isolate)->debug())) {}
+
+debug::DisableBreakScope::~DisableBreakScope() = default;
+
 Local<String> CpuProfileNode::GetFunctionName() const {
   const i::ProfileNode* node = reinterpret_cast<const i::ProfileNode*>(this);
   i::Isolate* isolate = node->isolate();

src/debug/debug-interface.h

@@ -24,6 +24,7 @@ struct CoverageScript;
 struct TypeProfileEntry;
 struct TypeProfileScript;
 class Coverage;
+class DisableBreak;
 class PostponeInterruptsScope;
 class Script;
 class TypeProfile;
@@ -541,6 +542,15 @@ class PostponeInterruptsScope {
   std::unique_ptr<i::PostponeInterruptsScope> scope_;
 };
 
+class DisableBreakScope {
+ public:
+  explicit DisableBreakScope(v8::Isolate* isolate);
+  ~DisableBreakScope();
+
+ private:
+  std::unique_ptr<i::DisableBreak> scope_;
+};
+
 class WeakMap : public v8::Object {
  public:
   WeakMap() = delete;

src/inspector/injected-script.cc

@@ -34,6 +34,8 @@
 #include <unordered_set>
 
 #include "../../third_party/inspector_protocol/crdtp/json.h"
+#include "include/v8-inspector.h"
+#include "src/debug/debug-interface.h"
 #include "src/inspector/custom-preview.h"
 #include "src/inspector/inspected-context.h"
 #include "src/inspector/protocol/Protocol.h"
@@ -46,8 +48,6 @@
 #include "src/inspector/v8-value-utils.h"
 #include "src/inspector/value-mirror.h"
 
-#include "include/v8-inspector.h"
-
 namespace v8_inspector {
 
 namespace {
@@ -861,6 +861,7 @@ Response InjectedScript::wrapEvaluateResult(
 
 v8::Local<v8::Object> InjectedScript::commandLineAPI() {
   if (m_commandLineAPI.IsEmpty()) {
+    v8::debug::DisableBreakScope disable_break(m_context->isolate());
     m_commandLineAPI.Reset(
         m_context->isolate(),
         m_context->inspector()->console()->createCommandLineAPI(

test/inspector/debugger/destroy-in-break-program2-expected.txt

+Check we're not pausing on breaks while installing console API
+frobnicate("test", "self") = self test
+paused in: the-right-place.js

test/inspector/debugger/destroy-in-break-program2.js

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+const {session, contextGroup, Protocol} = InspectorTest.start(
+    'Check we\'re not pausing on breaks while installing console API');
+
+(async function test(){
+  // Set up additional console API to give evaluate() a chance to pause
+  // there (which it shouldn't) while installing the API upon first eval.
+  utils.setAdditionalConsoleApi(`function frobnicate() {
+    return [...arguments].reverse().join(' ');
+  }`);
+
+  // Perform self-test, i.e. assure setAdditionalConsoleApi above has effect.
+  Protocol.Runtime.enable();
+  const expression = 'frobnicate("test", "self")';
+  const {result} = await Protocol.Runtime.evaluate({
+      expression,
+      includeCommandLineAPI: true,
+      returnByValue: true
+  });
+  InspectorTest.log(`${expression} = ${result.result.value}`);
+
+  // Now for the actual test: get a clean context so that Runtime.evaluate
+  // would install the API again.
+  const contextGroup = new InspectorTest.ContextGroup();
+  const session2 = contextGroup.connect();
+  const Protocol2 = session2.Protocol;
+
+  Protocol2.Runtime.enable();
+  Protocol2.Debugger.enable();
+  await Protocol2.Debugger.pause();
+
+  // Add a sourceURL to double check we're paused in the right place.
+  const sourceURL = '//# sourceURL=the-right-place.js';
+  Protocol2.Runtime.evaluate({
+      expression: `frobnicate("real", "test");\n${sourceURL}`,
+      includeCommandLineAPI: true
+  });
+
+  const paused = (await Protocol2.Debugger.oncePaused()).params;
+  InspectorTest.log(`paused in: ${paused.callFrames[0].url}`);
+
+  // Now if we're paused in the wrong place, we will likely crash.
+  session2.disconnect();
+
+  InspectorTest.quitImmediately();
+})();

test/inspector/inspector-test.cc

@@ -334,6 +334,9 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
     utils->Set(isolate, "setLogConsoleApiMessageCalls",
                v8::FunctionTemplate::New(
                    isolate, &UtilsExtension::SetLogConsoleApiMessageCalls));
+    utils->Set(isolate, "setAdditionalConsoleApi",
+               v8::FunctionTemplate::New(
+                   isolate, &UtilsExtension::SetAdditionalConsoleApi));
     utils->Set(
         isolate, "setLogMaxAsyncCallStackDepthChanged",
         v8::FunctionTemplate::New(
@@ -545,6 +548,20 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
         args[0].As<v8::Boolean>()->Value());
   }
 
+  static void SetAdditionalConsoleApi(
+      const v8::FunctionCallbackInfo<v8::Value>& args) {
+    if (args.Length() != 1 || !args[0]->IsString()) {
+      fprintf(stderr, "Internal error: SetAdditionalConsoleApi(string).");
+      Exit();
+    }
+    std::vector<uint16_t> script =
+        ToVector(args.GetIsolate(), args[0].As<v8::String>());
+    RunSyncTask(backend_runner_, [&script](IsolateData* data) {
+      data->SetAdditionalConsoleApi(
+          v8_inspector::StringView(script.data(), script.size()));
+    });
+  }
+
   static void CreateContextGroup(
       const v8::FunctionCallbackInfo<v8::Value>& args) {
     if (args.Length() != 0) {

test/inspector/isolate-data.cc

@@ -413,6 +413,11 @@ void IsolateData::SetLogMaxAsyncCallStackDepthChanged(bool log) {
   log_max_async_call_stack_depth_changed_ = log;
 }
 
+void IsolateData::SetAdditionalConsoleApi(v8_inspector::StringView api_script) {
+  v8::HandleScope handle_scope(isolate());
+  additional_console_api_.Reset(isolate(), ToString(isolate(), api_script));
+}
+
 v8::MaybeLocal<v8::Value> IsolateData::memoryInfo(v8::Isolate* isolate,
                                                   v8::Local<v8::Context>) {
   if (memory_info_.IsEmpty()) return v8::MaybeLocal<v8::Value>();
@@ -429,6 +434,21 @@ void IsolateData::quitMessageLoopOnPause() {
   task_runner_->QuitMessageLoop();
 }
 
+void IsolateData::installAdditionalCommandLineAPI(
+    v8::Local<v8::Context> context, v8::Local<v8::Object> object) {
+  if (additional_console_api_.IsEmpty()) return;
+  CHECK(context->GetIsolate() == isolate());
+  v8::HandleScope handle_scope(isolate());
+  v8::Context::Scope context_scope(context);
+  v8::ScriptOrigin origin(
+      v8::String::NewFromUtf8Literal(isolate(), "internal-console-api"));
+  v8::ScriptCompiler::Source scriptSource(
+      additional_console_api_.Get(isolate()), origin);
+  v8::MaybeLocal<v8::Script> script =
+      v8::ScriptCompiler::Compile(context, &scriptSource);
+  CHECK(!script.ToLocalChecked()->Run(context).IsEmpty());
+}
+
 void IsolateData::consoleAPIMessage(int contextGroupId,
                                     v8::Isolate::MessageErrorLevel level,
                                     const v8_inspector::StringView& message,

test/inspector/isolate-data.h

@@ -73,6 +73,7 @@ class IsolateData : public v8_inspector::V8InspectorClient {
   void SetMemoryInfo(v8::Local<v8::Value> memory_info);
   void SetLogConsoleApiMessageCalls(bool log);
   void SetLogMaxAsyncCallStackDepthChanged(bool log);
+  void SetAdditionalConsoleApi(v8_inspector::StringView api_script);
   void SetMaxAsyncTaskStacksForTest(int limit);
   void DumpAsyncTaskStacksStateForTest();
   void FireContextCreated(v8::Local<v8::Context> context, int context_group_id);
@@ -109,6 +110,8 @@ class IsolateData : public v8_inspector::V8InspectorClient {
                                        v8::Local<v8::Context>) override;
   void runMessageLoopOnPause(int context_group_id) override;
   void quitMessageLoopOnPause() override;
+  void installAdditionalCommandLineAPI(v8::Local<v8::Context>,
+                                       v8::Local<v8::Object>) override;
   void consoleAPIMessage(int contextGroupId,
                          v8::Isolate::MessageErrorLevel level,
                          const v8_inspector::StringView& message,
@@ -148,6 +151,7 @@ class IsolateData : public v8_inspector::V8InspectorClient {
   bool log_max_async_call_stack_depth_changed_ = false;
   v8::Global<v8::Private> not_inspectable_private_;
   v8::Global<v8::String> resource_name_prefix_;
+  v8::Global<v8::String> additional_console_api_;
 
   DISALLOW_COPY_AND_ASSIGN(IsolateData);
 };