Revert of [debugger] expose side-effect free evaluate to inspector. (patchset...

Revert of [debugger] expose side-effect free evaluate to inspector. (patchset #3 id:40001 of https://codereview.chromium.org/2685483002/ )

Reason for revert:
Speculative revert. Seems to block the roll:
https://codereview.chromium.org/2685933005/

Original issue's description:
> [debugger] expose side-effect free evaluate to inspector.
>
> R=jgruber@chromium.org, kozyatinskiy@chromium.org
> BUG=v8:5821
>
> Review-Url: https://codereview.chromium.org/2685483002
> Cr-Commit-Position: refs/heads/master@{#43049}
> Committed: https://chromium.googlesource.com/v8/v8/+/1a989bdeefdc679745215ae547007773edb3d29e

TBR=jgruber@chromium.org,kozyatinskiy@chromium.org,pfeldman@chromium.org,yangguo@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5821

Review-Url: https://codereview.chromium.org/2687013003
Cr-Commit-Position: refs/heads/master@{#43060}

src/debug/debug-evaluate.cc

@@ -41,14 +41,13 @@ MaybeHandle<Object> DebugEvaluate::Global(Isolate* isolate,
   Handle<Context> context = isolate->native_context();
   Handle<JSObject> receiver(context->global_proxy());
   Handle<SharedFunctionInfo> outer_info(context->closure()->shared(), isolate);
-  return Evaluate(isolate, outer_info, context, receiver, source, false);
+  return Evaluate(isolate, outer_info, context, receiver, source);
 }
 
 MaybeHandle<Object> DebugEvaluate::Local(Isolate* isolate,
                                          StackFrame::Id frame_id,
                                          int inlined_jsframe_index,
-                                         Handle<String> source,
-                                         bool throw_on_side_effect) {
+                                         Handle<String> source) {
   // Handle the processing of break.
   DisableBreak disable_break_scope(isolate->debug());
 
@@ -75,9 +74,8 @@ MaybeHandle<Object> DebugEvaluate::Local(Isolate* isolate,
 
   Handle<Context> context = context_builder.evaluation_context();
   Handle<JSObject> receiver(context->global_proxy());
-  MaybeHandle<Object> maybe_result =
-      Evaluate(isolate, context_builder.outer_info(), context, receiver, source,
-               throw_on_side_effect);
+  MaybeHandle<Object> maybe_result = Evaluate(
+      isolate, context_builder.outer_info(), context, receiver, source);
   if (!maybe_result.is_null()) context_builder.UpdateValues();
   return maybe_result;
 }
@@ -86,8 +84,7 @@ MaybeHandle<Object> DebugEvaluate::Local(Isolate* isolate,
 // Compile and evaluate source for the given context.
 MaybeHandle<Object> DebugEvaluate::Evaluate(
     Isolate* isolate, Handle<SharedFunctionInfo> outer_info,
-    Handle<Context> context, Handle<Object> receiver, Handle<String> source,
-    bool throw_on_side_effect) {
+    Handle<Context> context, Handle<Object> receiver, Handle<String> source) {
   Handle<JSFunction> eval_fun;
   ASSIGN_RETURN_ON_EXCEPTION(
       isolate, eval_fun,
@@ -98,7 +95,8 @@ MaybeHandle<Object> DebugEvaluate::Evaluate(
 
   Handle<Object> result;
   {
-    NoSideEffectScope no_side_effect(isolate, throw_on_side_effect);
+    NoSideEffectScope no_side_effect(isolate,
+                                     FLAG_side_effect_free_debug_evaluate);
     ASSIGN_RETURN_ON_EXCEPTION(
         isolate, result, Execution::Call(isolate, eval_fun, receiver, 0, NULL),
         Object);

src/debug/debug-evaluate.h

@@ -22,8 +22,7 @@ class DebugEvaluate : public AllStatic {
   // - The arguments object needs to materialized.
   static MaybeHandle<Object> Local(Isolate* isolate, StackFrame::Id frame_id,
                                    int inlined_jsframe_index,
-                                   Handle<String> source,
-                                   bool throw_on_side_effect);
+                                   Handle<String> source);
 
   static bool FunctionHasNoSideEffect(Handle<SharedFunctionInfo> info);
   static bool CallbackHasNoSideEffect(Address function_addr);
@@ -87,8 +86,7 @@ class DebugEvaluate : public AllStatic {
                                       Handle<SharedFunctionInfo> outer_info,
                                       Handle<Context> context,
                                       Handle<Object> receiver,
-                                      Handle<String> source,
-                                      bool throw_on_side_effect);
+                                      Handle<String> source);
 };
 
 

src/debug/mirrors.js

@@ -1919,12 +1919,11 @@ FrameMirror.prototype.allScopes = function(opt_ignore_nested_scopes) {
 };
 
 
-FrameMirror.prototype.evaluate = function(source, throw_on_side_effect = false) {
+FrameMirror.prototype.evaluate = function(source) {
   return MakeMirror(%DebugEvaluate(this.break_id_,
                                    this.details_.frameId(),
                                    this.details_.inlinedFrameIndex(),
-                                   source,
-                                   throw_on_side_effect));
+                                   source));
 };
 
 

src/flag-definitions.h

@@ -718,6 +718,8 @@ DEFINE_IMPLICATION(trace_array_abuse, trace_external_array_abuse)
 // debugger
 DEFINE_BOOL(trace_debug_json, false, "trace debugging JSON request/response")
 DEFINE_BOOL(enable_liveedit, true, "enable liveedit experimental feature")
+DEFINE_BOOL(side_effect_free_debug_evaluate, false,
+            "use side-effect-free debug-evaluate for testing")
 DEFINE_BOOL(
     trace_side_effect_free_debug_evaluate, false,
     "print debug messages for side-effect-free debug-evaluate for testing")

src/inspector/debugger-script.js

@@ -420,12 +420,11 @@ DebuggerScript._frameMirrorToJSCallFrame = function(frameMirror)
 
     /**
      * @param {string} expression
-     * @param {boolean} throwOnSideEffect
      * @return {*}
      */
-    function evaluate(expression, throwOnSideEffect)
+    function evaluate(expression)
     {
-        return frameMirror.evaluate(expression, throwOnSideEffect).value();
+        return frameMirror.evaluate(expression).value();
     }
 
     /** @return {undefined} */

src/inspector/debugger_script_externs.js

@@ -31,7 +31,7 @@ var JavaScriptCallFrameDetails;
 /** @typedef {{
         contextId: function():number,
         thisObject: !Object,
-        evaluate: function(string, boolean):*,
+        evaluate: function(string):*,
         restart: function():undefined,
         setVariableValue: function(number, string, *):undefined,
         isAtReturn: boolean,
@@ -351,9 +351,8 @@ FrameMirror.prototype.script = function() {}
 
 /**
  * @param {string} source
- * @param {boolean} throwOnSideEffect
  */
-FrameMirror.prototype.evaluate = function(source, throwOnSideEffect) {}
+FrameMirror.prototype.evaluate = function(source) {}
 
 FrameMirror.prototype.restart = function() {}
 

src/inspector/java-script-call-frame.cc

@@ -98,7 +98,7 @@ v8::MaybeLocal<v8::Object> JavaScriptCallFrame::details() const {
 }
 
 v8::MaybeLocal<v8::Value> JavaScriptCallFrame::evaluate(
-    v8::Local<v8::Value> expression, bool throwOnSideEffect) {
+    v8::Local<v8::Value> expression) {
   v8::MicrotasksScope microtasks(m_isolate,
                                  v8::MicrotasksScope::kRunMicrotasks);
   v8::Local<v8::Context> context =
@@ -108,9 +108,7 @@ v8::MaybeLocal<v8::Value> JavaScriptCallFrame::evaluate(
   v8::Local<v8::Function> evalFunction = v8::Local<v8::Function>::Cast(
       callFrame->Get(context, toV8StringInternalized(m_isolate, "evaluate"))
           .ToLocalChecked());
-  v8::Local<v8::Value> argv[] = {
-      expression, v8::Boolean::New(m_isolate, throwOnSideEffect)};
-  return evalFunction->Call(context, callFrame, arraysize(argv), argv);
+  return evalFunction->Call(context, callFrame, 1, &expression);
 }
 
 v8::MaybeLocal<v8::Value> JavaScriptCallFrame::restart() {

src/inspector/java-script-call-frame.h

@@ -54,8 +54,7 @@ class JavaScriptCallFrame {
   bool isAtReturn() const;
   v8::MaybeLocal<v8::Object> details() const;
 
-  v8::MaybeLocal<v8::Value> evaluate(v8::Local<v8::Value> expression,
-                                     bool throwOnSideEffect);
+  v8::MaybeLocal<v8::Value> evaluate(v8::Local<v8::Value> expression);
   v8::MaybeLocal<v8::Value> restart();
   v8::MaybeLocal<v8::Value> setVariableValue(int scopeNumber,
                                              v8::Local<v8::Value> variableName,

src/inspector/js_protocol.json

@@ -635,8 +635,7 @@
                     { "name": "includeCommandLineAPI", "type": "boolean", "optional": true, "description": "Specifies whether command line API should be available to the evaluated expression, defaults to false." },
                     { "name": "silent", "type": "boolean", "optional": true, "description": "In silent mode exceptions thrown during evaluation are not reported and do not pause execution. Overrides <code>setPauseOnException</code> state." },
                     { "name": "returnByValue", "type": "boolean", "optional": true, "description": "Whether the result is expected to be a JSON object that should be sent by value." },
-                    { "name": "generatePreview", "type": "boolean", "optional": true, "experimental": true, "description": "Whether preview should be generated for the result." },
-                    { "name": "throwOnSideEffect", "type": "boolean", "optional": true, "experimental": true, "description": "Whether to throw an exception if side effect cannot be ruled out during evaluation." }
+                    { "name": "generatePreview", "type": "boolean", "optional": true, "experimental": true, "description": "Whether preview should be generated for the result." }
                 ],
                 "returns": [
                     { "name": "result", "$ref": "Runtime.RemoteObject", "description": "Object wrapper for the evaluation result." },

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

@@ -703,7 +703,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, std::unique_ptr<RemoteObject>* result,
+    std::unique_ptr<RemoteObject>* result,
     Maybe<protocol::Runtime::ExceptionDetails>* exceptionDetails) {
   if (!isPaused()) return Response::Error(kDebuggerNotPaused);
   InjectedScript::CallFrameScope scope(m_inspector, m_session->contextGroupId(),
@@ -718,8 +718,7 @@ Response V8DebuggerAgentImpl::evaluateOnCallFrame(
 
   v8::MaybeLocal<v8::Value> maybeResultValue =
       m_pausedCallFrames[scope.frameOrdinal()]->evaluate(
-          toV8String(m_isolate, expression),
-          throwOnSideEffect.fromMaybe(false));
+          toV8String(m_isolate, expression));
 
   // Re-initialize after running client's code, as it could have destroyed
   // context or session.

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

@@ -93,7 +93,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
       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<bool> generatePreview,
       std::unique_ptr<protocol::Runtime::RemoteObject>* result,
       Maybe<protocol::Runtime::ExceptionDetails>*) override;
   Response setVariableValue(

src/runtime/runtime-debug.cc

@@ -1209,20 +1209,19 @@ RUNTIME_FUNCTION(Runtime_DebugEvaluate) {
 
   // Check the execution state and decode arguments frame and source to be
   // evaluated.
-  DCHECK_EQ(5, args.length());
+  DCHECK_EQ(4, args.length());
   CONVERT_NUMBER_CHECKED(int, break_id, Int32, args[0]);
   CHECK(isolate->debug()->CheckExecutionState(break_id));
 
   CONVERT_SMI_ARG_CHECKED(wrapped_id, 1);
   CONVERT_NUMBER_CHECKED(int, inlined_jsframe_index, Int32, args[2]);
   CONVERT_ARG_HANDLE_CHECKED(String, source, 3);
-  CONVERT_BOOLEAN_ARG_CHECKED(throw_on_side_effect, 4);
 
   StackFrame::Id id = DebugFrameHelper::UnwrapFrameId(wrapped_id);
 
   RETURN_RESULT_OR_FAILURE(
-      isolate, DebugEvaluate::Local(isolate, id, inlined_jsframe_index, source,
-                                    throw_on_side_effect));
+      isolate,
+      DebugEvaluate::Local(isolate, id, inlined_jsframe_index, source));
 }
 
 

src/runtime/runtime.h

@@ -172,7 +172,7 @@ namespace internal {
   F(IsBreakOnException, 1, 1)                   \
   F(PrepareStep, 2, 1)                          \
   F(ClearStepping, 0, 1)                        \
-  F(DebugEvaluate, 5, 1)                        \
+  F(DebugEvaluate, 4, 1)                        \
   F(DebugEvaluateGlobal, 2, 1)                  \
   F(DebugGetLoadedScripts, 0, 1)                \
   F(DebugReferencedBy, 3, 1)                    \

test/debugger/debug/debug-evaluate-no-side-effect-builtins.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --ignition
+// Flags: --ignition --side-effect-free-debug-evaluate
 
 Debug = debug.Debug
 
@@ -12,12 +12,10 @@ function listener(event, exec_state, event_data, data) {
   if (event != Debug.DebugEvent.Break) return;
   try {
     function success(expectation, source) {
-      assertEquals(expectation,
-                   exec_state.frame(0).evaluate(source, true).value());
+      assertEquals(expectation, exec_state.frame(0).evaluate(source).value());
     }
     function fail(source) {
-      assertThrows(() => exec_state.frame(0).evaluate(source, true),
-                   EvalError);
+      assertThrows(() => exec_state.frame(0).evaluate(source), EvalError);
     }
 
     // Test Math functions.

test/debugger/debug/debug-evaluate-no-side-effect.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --ignition
+// Flags: --ignition --side-effect-free-debug-evaluate
 
 Debug = debug.Debug
 
@@ -24,12 +24,10 @@ function listener(event, exec_state, event_data, data) {
   if (event != Debug.DebugEvent.Break) return;
   try {
     function success(expectation, source) {
-      assertEquals(expectation,
-                   exec_state.frame(0).evaluate(source, true).value());
+      assertEquals(expectation, exec_state.frame(0).evaluate(source).value());
     }
     function fail(source) {
-      assertThrows(() => exec_state.frame(0).evaluate(source, true),
-                   EvalError);
+      assertThrows(() => exec_state.frame(0).evaluate(source), EvalError);
     }
     // Simple test.
     success(3, "1 + 2");

test/debugger/test-api.js

@@ -677,13 +677,12 @@ class DebugWrapper {
            };
   }
 
-  evaluateOnCallFrame(frame, expr, throw_on_side_effect = false) {
+  evaluateOnCallFrame(frame, expr) {
     const frameid = frame.callFrameId;
     const {msgid, msg} = this.createMessage(
         "Debugger.evaluateOnCallFrame",
         { callFrameId : frameid,
-          expression : expr,
-          throwOnSideEffect : throw_on_side_effect,
+          expression : expr
         });
     this.sendMessage(msg);
     const reply = this.takeReplyChecked(msgid);
@@ -728,8 +727,7 @@ class DebugWrapper {
              sourceLine : () => line + 1,
              sourceLineText : () => loc.sourceText,
              sourcePosition : () => loc.position,
-             evaluate : (expr, throw_on_side_effect) =>
-                 this.evaluateOnCallFrame(frame, expr, throw_on_side_effect),
+             evaluate : (expr) => this.evaluateOnCallFrame(frame, expr),
              functionName : () => frame.functionName,
              func : () => func,
              index : () => index,