Revert "[inspector] added timeout for Debugger.evaluateOnCallFrame method"

This reverts commit 436faae0.

Reason for revert: Introduces flakes:
https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/24482
https://build.chromium.org/p/client.v8/builders/V8%20Win32/builds/13557
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN/builds/25210

Original change's description:
> [inspector] added timeout for Debugger.evaluateOnCallFrame method
> 
> R=​dgozman@chromium.org,yangguo@chromium.org
> 
> Bug: none
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I569899f245190ca2fa720bdb837db1263e8058d5
> Reviewed-on: https://chromium-review.googlesource.com/1023035
> Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
> Reviewed-by: Yang Guo <yangguo@chromium.org>
> Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#52798}

TBR=dgozman@chromium.org,yangguo@chromium.org,kozyatinskiy@chromium.org

Change-Id: I63ee0d19642856a7c0c2128bfa4c4620974d1919
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: none
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1029910Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52800}

src/debug/debug-stack-trace-iterator.cc

@@ -172,8 +172,6 @@ v8::MaybeLocal<v8::Value> DebugStackTraceIterator::Evaluate(
     v8::Local<v8::String> source, bool throw_on_side_effect) {
   DCHECK(!Done());
   Handle<Object> value;
-  i::SafeForInterruptsScope safe_for_interrupt_scope(
-      isolate_, i::StackGuard::TERMINATE_EXECUTION);
   if (!DebugEvaluate::Local(isolate_, iterator_.frame()->id(),
                             inlined_frame_index_, Utils::OpenHandle(*source),
                             throw_on_side_effect)

src/inspector/DEPS

@@ -5,7 +5,6 @@ include_rules = [
   "+src/base/macros.h",
   "+src/base/logging.h",
   "+src/base/platform/platform.h",
-  "+src/base/platform/mutex.h",
   "+src/conversions.h",
   "+src/flags.h",
   "+src/utils.h",

src/inspector/js_protocol.json

@@ -391,13 +391,6 @@
                             "description": "Whether to throw an exception if side effect cannot be ruled out during evaluation.",
                             "optional": true,
                             "type": "boolean"
-                        },
-                        {
-                            "name": "timeout",
-                            "description": "Terminate execution after timing out (number of milliseconds).",
-                            "experimental": true,
-                            "optional": true,
-                            "$ref": "Runtime.TimeDelta"
                         }
                     ],
                     "returns": [

src/inspector/js_protocol.pdl

@@ -191,8 +191,6 @@ domain Debugger
       experimental optional boolean generatePreview
       # Whether to throw an exception if side effect cannot be ruled out during evaluation.
       optional boolean throwOnSideEffect
-      # Terminate execution after timing out (number of milliseconds).
-      experimental optional Runtime.TimeDelta timeout
     returns
       # Object wrapper for the evaluation result.
       Runtime.RemoteObject result

src/inspector/v8-debugger-agent-impl.cc

@@ -1111,8 +1111,7 @@ Response V8DebuggerAgentImpl::evaluateOnCallFrame(
     const String16& callFrameId, const String16& expression,
     Maybe<String16> objectGroup, Maybe<bool> includeCommandLineAPI,
     Maybe<bool> silent, Maybe<bool> returnByValue, Maybe<bool> generatePreview,
-    Maybe<bool> throwOnSideEffect, Maybe<double> timeout,
-    std::unique_ptr<RemoteObject>* result,
+    Maybe<bool> throwOnSideEffect, std::unique_ptr<RemoteObject>* result,
     Maybe<protocol::Runtime::ExceptionDetails>* exceptionDetails) {
   if (!isPaused()) return Response::Error(kDebuggerNotPaused);
   InjectedScript::CallFrameScope scope(m_session, callFrameId);
@@ -1126,17 +1125,8 @@ Response V8DebuggerAgentImpl::evaluateOnCallFrame(
   if (it->Done()) {
     return Response::Error("Could not find call frame with given id");
   }
-
-  v8::MaybeLocal<v8::Value> maybeResultValue;
-  {
-    V8InspectorImpl::EvaluateScope evaluateScope(m_isolate);
-    if (timeout.isJust()) {
-      response = evaluateScope.setTimeout(timeout.fromJust() / 1000.0);
-      if (!response.isSuccess()) return response;
-    }
-    maybeResultValue = it->Evaluate(toV8String(m_isolate, expression),
-                                    throwOnSideEffect.fromMaybe(false));
-  }
+  v8::MaybeLocal<v8::Value> maybeResultValue = it->Evaluate(
+      toV8String(m_isolate, expression), throwOnSideEffect.fromMaybe(false));
   // Re-initialize after running client's code, as it could have destroyed
   // context or session.
   response = scope.initialize();

src/inspector/v8-debugger-agent-impl.h

@@ -105,7 +105,6 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
       Maybe<String16> objectGroup, Maybe<bool> includeCommandLineAPI,
       Maybe<bool> silent, Maybe<bool> returnByValue,
       Maybe<bool> generatePreview, Maybe<bool> throwOnSideEffect,
-      Maybe<double> timeout,
       std::unique_ptr<protocol::Runtime::RemoteObject>* result,
       Maybe<protocol::Runtime::ExceptionDetails>*) override;
   Response setVariableValue(

src/inspector/v8-inspector-impl.cc

@@ -32,7 +32,6 @@
 
 #include <vector>
 
-#include "src/base/platform/mutex.h"
 #include "src/inspector/inspected-context.h"
 #include "src/inspector/string-util.h"
 #include "src/inspector/v8-console-agent-impl.h"
@@ -45,8 +44,6 @@
 #include "src/inspector/v8-runtime-agent-impl.h"
 #include "src/inspector/v8-stack-trace-impl.h"
 
-#include "include/v8-platform.h"
-
 namespace v8_inspector {
 
 std::unique_ptr<V8Inspector> V8Inspector::create(v8::Isolate* isolate,
@@ -379,53 +376,18 @@ void V8InspectorImpl::forEachSession(
   }
 }
 
-V8InspectorImpl::EvaluateScope::EvaluateScope(v8::Isolate* isolate)
-    : m_isolate(isolate), m_safeForTerminationScope(isolate) {}
-
-struct V8InspectorImpl::EvaluateScope::CancelToken {
-  v8::base::Mutex m_mutex;
-  bool m_canceled = false;
-};
-
-V8InspectorImpl::EvaluateScope::~EvaluateScope() {
-  if (m_cancelToken) {
-    v8::base::LockGuard<v8::base::Mutex> lock(&m_cancelToken->m_mutex);
-    m_cancelToken->m_canceled = true;
-    m_isolate->CancelTerminateExecution();
-  }
+intptr_t V8InspectorImpl::evaluateStarted() {
+  intptr_t id = ++m_lastEvaluateId;
+  m_runningEvaluates.insert(id);
+  return id;
 }
 
-class V8InspectorImpl::EvaluateScope::TerminateTask : public v8::Task {
- public:
-  TerminateTask(v8::Isolate* isolate, std::shared_ptr<CancelToken> token)
-      : m_isolate(isolate), m_token(token) {}
-
-  void Run() {
-    // CancelToken contains m_canceled bool which may be changed from main
-    // thread, so lock mutex first.
-    v8::base::LockGuard<v8::base::Mutex> lock(&m_token->m_mutex);
-    if (m_token->m_canceled) return;
-    m_isolate->TerminateExecution();
-  }
-
- private:
-  v8::Isolate* m_isolate;
-  std::shared_ptr<CancelToken> m_token;
-};
+void V8InspectorImpl::evaluateFinished(intptr_t id) {
+  m_runningEvaluates.erase(id);
+}
 
-protocol::Response V8InspectorImpl::EvaluateScope::setTimeout(double timeout) {
-  if (m_isolate->IsExecutionTerminating()) {
-    return protocol::Response::Error("Execution was terminated");
-  }
-  std::shared_ptr<v8::TaskRunner> taskRunner =
-      v8::debug::GetCurrentPlatform()->GetWorkerThreadsTaskRunner(m_isolate);
-  if (!taskRunner) {
-    return protocol::Response::Error("Timeout is not supported by embedder");
-  }
-  m_cancelToken.reset(new CancelToken());
-  taskRunner->PostDelayedTask(
-      v8::base::make_unique<TerminateTask>(m_isolate, m_cancelToken), timeout);
-  return protocol::Response::OK();
+bool V8InspectorImpl::evaluateStillRunning(intptr_t id) {
+  return m_runningEvaluates.find(id) != m_runningEvaluates.end();
 }
 
 }  // namespace v8_inspector

src/inspector/v8-inspector-impl.h

@@ -33,9 +33,9 @@
 
 #include <functional>
 #include <map>
+#include <set>
 
 #include "src/base/macros.h"
-#include "src/base/platform/mutex.h"
 #include "src/inspector/protocol/Protocol.h"
 
 #include "include/v8-inspector.h"
@@ -121,20 +121,9 @@ class V8InspectorImpl : public V8Inspector {
   void forEachSession(int contextGroupId,
                       std::function<void(V8InspectorSessionImpl*)> callback);
 
-  class EvaluateScope {
-   public:
-    explicit EvaluateScope(v8::Isolate* isolate);
-    ~EvaluateScope();
-
-    protocol::Response setTimeout(double timeout);
-
-   private:
-    v8::Isolate* m_isolate;
-    class TerminateTask;
-    struct CancelToken;
-    std::shared_ptr<CancelToken> m_cancelToken;
-    v8::Isolate::SafeForTerminationScope m_safeForTerminationScope;
-  };
+  intptr_t evaluateStarted();
+  void evaluateFinished(intptr_t);
+  bool evaluateStillRunning(intptr_t);
 
  private:
   v8::Isolate* m_isolate;
@@ -167,6 +156,9 @@ class V8InspectorImpl : public V8Inspector {
 
   std::unique_ptr<V8Console> m_console;
 
+  intptr_t m_lastEvaluateId = 0;
+  std::set<intptr_t> m_runningEvaluates;
+
   DISALLOW_COPY_AND_ASSIGN(V8InspectorImpl);
 };
 

src/inspector/v8-runtime-agent-impl.cc

@@ -214,6 +214,31 @@ Response ensureContext(V8InspectorImpl* inspector, int contextGroupId,
   return Response::OK();
 }
 
+class TerminateTask : public v8::Task {
+ public:
+  TerminateTask(V8InspectorImpl* inspector, intptr_t evaluateId)
+      : m_inspector(inspector), m_evaluateId(evaluateId) {}
+
+  void Run() {
+    if (!m_inspector->evaluateStillRunning(m_evaluateId)) return;
+    // We should call terminateExecution on the main thread.
+    m_inspector->isolate()->RequestInterrupt(
+        InterruptCallback, reinterpret_cast<void*>(m_evaluateId));
+  }
+
+  static void InterruptCallback(v8::Isolate* isolate, void* rawEvaluateId) {
+    intptr_t evaluateId = reinterpret_cast<intptr_t>(rawEvaluateId);
+    V8InspectorImpl* inspector =
+        static_cast<V8InspectorImpl*>(v8::debug::GetInspector(isolate));
+    if (!inspector->evaluateStillRunning(evaluateId)) return;
+    inspector->debugger()->terminateExecution(nullptr);
+  }
+
+ private:
+  V8InspectorImpl* m_inspector;
+  intptr_t m_evaluateId;
+};
+
 }  // namespace
 
 V8RuntimeAgentImpl::V8RuntimeAgentImpl(
@@ -258,22 +283,31 @@ void V8RuntimeAgentImpl::evaluate(
 
   // Temporarily enable allow evals for inspector.
   scope.allowCodeGenerationFromStrings();
-  v8::MaybeLocal<v8::Value> maybeResultValue;
-  {
-    V8InspectorImpl::EvaluateScope evaluateScope(m_inspector->isolate());
+
+  V8InspectorImpl* inspector = m_inspector;
+  intptr_t evaluateId = inspector->evaluateStarted();
   if (timeout.isJust()) {
-      response = evaluateScope.setTimeout(timeout.fromJust() / 1000.0);
-      if (!response.isSuccess()) {
-        callback->sendFailure(response);
+    std::shared_ptr<v8::TaskRunner> taskRunner =
+        v8::debug::GetCurrentPlatform()->GetWorkerThreadsTaskRunner(
+            m_inspector->isolate());
+    if (!taskRunner) {
+      callback->sendFailure(
+          Response::Error("Timeout is not supported by embedder"));
       return;
     }
+    taskRunner->PostDelayedTask(
+        v8::base::make_unique<TerminateTask>(m_inspector, evaluateId),
+        timeout.fromJust() / 1000.0);
   }
+  v8::MaybeLocal<v8::Value> maybeResultValue;
+  {
     v8::MicrotasksScope microtasksScope(m_inspector->isolate(),
                                         v8::MicrotasksScope::kRunMicrotasks);
     maybeResultValue = v8::debug::EvaluateGlobal(
         m_inspector->isolate(), toV8String(m_inspector->isolate(), expression),
         throwOnSideEffect.fromMaybe(false));
   }  // Run microtasks before returning result.
+  inspector->evaluateFinished(evaluateId);
 
   // Re-initialize after running client's code, as it could have destroyed
   // context or session.

test/inspector/debugger/evaluate-on-call-frame-timeout-expected.txt

-Tests that Debugger.evaluateOnCallFrame's timeout argument
-Run trivial expression:
-Evaluate finished!
-Run expression without interrupts:
-Evaluate finished!
-Run infinite loop:
-Evaluate finished!

test/inspector/debugger/evaluate-on-call-frame-timeout.js

-// Copyright 2018 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 {Protocol} = InspectorTest.start(
-    `Tests that Debugger.evaluateOnCallFrame's timeout argument`);
-
-(async function test(){
-  Protocol.Debugger.enable();
-  Protocol.Runtime.evaluate({expression: "debugger;"});
-  const {params:{callFrames:[{callFrameId}]}} = await Protocol.Debugger.oncePaused();
-  {
-    InspectorTest.log('Run trivial expression:');
-    const result = await Protocol.Debugger.evaluateOnCallFrame({
-      callFrameId,
-      expression: 'function foo() {} foo()',
-      timeout: 0
-    });
-    InspectorTest.log('Evaluate finished!');
-  }
-  {
-    InspectorTest.log('Run expression without interrupts:');
-    const result = await Protocol.Debugger.evaluateOnCallFrame({
-      callFrameId,
-      expression: '',
-      timeout: 0
-    });
-    InspectorTest.log('Evaluate finished!');
-  }
-  {
-    InspectorTest.log('Run infinite loop:');
-    const result = await Protocol.Debugger.evaluateOnCallFrame({
-      callFrameId,
-      expression: 'for(;;){}',
-      timeout: 0
-    });
-    InspectorTest.log('Evaluate finished!');
-  }
-  InspectorTest.completeTest();
-})();