[inspector] Consistently treat promise rejections as side-effecting.

Previously we'd treat %_AsyncFunctionReject (and %AsyncFunctionReject)
as side-effect free (in async functions), but that's not correct, since
promise rejections have side-effects (at the very least triggering the
unhandled promise rejection machinery in the browser).

This required a minor refactoring as previously we'd classify functions
as side-effecting or not depending on whether they contain any calls to
side-effecting intrinsics, no matter whether this call is actually
executed or not. That would break REPL mode however if we'd generally
treat all async functions with %_AsyncFunctionReject intrinsic calls as
side-effecting, so instead of performing the intrinsic checks ahead of
time, we now perform the test at execution time.

Before: https://imgur.com/5BvJP9d.png
After: https://imgur.com/10FanNr.png
Fixed: chromium:1249275
Change-Id: Ib06f945ba21f1e06ee9b13a1363fad342464fd9a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3197712
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#77183}

src/debug/debug-evaluate.cc

@@ -288,9 +288,8 @@ void DebugEvaluate::ContextBuilder::UpdateValues() {
   }
 }
 
-namespace {
-
-bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
+// static
+bool DebugEvaluate::IsSideEffectFreeIntrinsic(Runtime::FunctionId id) {
 // Use macro to include only the non-inlined version of an intrinsic.
 #define INTRINSIC_ALLOWLIST(V)                \
   /* Conversions */                           \
@@ -385,7 +384,6 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
   V(StringMaxLength)                          \
   V(StringToArray)                            \
   V(AsyncFunctionEnter)                       \
-  V(AsyncFunctionReject)                      \
   V(AsyncFunctionResolve)                     \
   /* Test */                                  \
   V(GetOptimizationStatus)                    \
@@ -395,7 +393,6 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
 // Intrinsics with inline versions have to be allowlisted here a second time.
 #define INLINE_INTRINSIC_ALLOWLIST(V) \
   V(AsyncFunctionEnter)               \
-  V(AsyncFunctionReject)              \
   V(AsyncFunctionResolve)
 
 #define CASE(Name) case Runtime::k##Name:
@@ -418,6 +415,8 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
 #undef INLINE_INTRINSIC_ALLOWLIST
 }
 
+namespace {
+
 bool BytecodeHasNoSideEffect(interpreter::Bytecode bytecode) {
   using interpreter::Bytecode;
   using interpreter::Bytecodes;
@@ -976,7 +975,7 @@ bool BytecodeRequiresRuntimeCheck(interpreter::Bytecode bytecode) {
     case Bytecode::kStaCurrentContextSlot:
       return true;
     default:
-      return false;
+      return interpreter::Bytecodes::IsCallRuntime(bytecode);
   }
 }
 
@@ -1003,16 +1002,6 @@ DebugInfo::SideEffectState DebugEvaluate::FunctionGetSideEffectState(
     for (interpreter::BytecodeArrayIterator it(bytecode_array); !it.done();
          it.Advance()) {
       interpreter::Bytecode bytecode = it.current_bytecode();
-
-      if (interpreter::Bytecodes::IsCallRuntime(bytecode)) {
-        Runtime::FunctionId id =
-            (bytecode == interpreter::Bytecode::kInvokeIntrinsic)
-                ? it.GetIntrinsicIdOperand(0)
-                : it.GetRuntimeIdOperand(0);
-        if (IntrinsicHasNoSideEffect(id)) continue;
-        return DebugInfo::kHasSideEffects;
-      }
-
       if (BytecodeHasNoSideEffect(bytecode)) continue;
       if (BytecodeRequiresRuntimeCheck(bytecode)) {
         requires_runtime_checks = true;

src/debug/debug-evaluate.h

@@ -53,6 +53,7 @@ class DebugEvaluate : public AllStatic {
   static DebugInfo::SideEffectState FunctionGetSideEffectState(
       Isolate* isolate, Handle<SharedFunctionInfo> info);
   static void ApplySideEffectChecks(Handle<BytecodeArray> bytecode_array);
+  static bool IsSideEffectFreeIntrinsic(Runtime::FunctionId id);
 
 #ifdef DEBUG
   static void VerifyTransitiveBuiltins(Isolate* isolate);

src/debug/debug.cc

@@ -2686,6 +2686,18 @@ bool Debug::PerformSideEffectCheckAtBytecode(InterpretedFrame* frame) {
       handle(bytecode_array, isolate_), offset);
 
   Bytecode bytecode = bytecode_iterator.current_bytecode();
+  if (interpreter::Bytecodes::IsCallRuntime(bytecode)) {
+    auto id = (bytecode == Bytecode::kInvokeIntrinsic)
+                  ? bytecode_iterator.GetIntrinsicIdOperand(0)
+                  : bytecode_iterator.GetRuntimeIdOperand(0);
+    if (DebugEvaluate::IsSideEffectFreeIntrinsic(id)) {
+      return true;
+    }
+    side_effect_check_failed_ = true;
+    // Throw an uncatchable termination exception.
+    isolate_->TerminateExecution();
+    return false;
+  }
   interpreter::Register reg;
   switch (bytecode) {
     case Bytecode::kStaCurrentContextSlot:

test/inspector/debugger/side-effect-free-debug-evaluate-expected.txt

@@ -7,6 +7,10 @@ g() returns 2
 f() returns 1
 g() throws EvalError
 
+Running test: testAsyncFunctions
+testAsyncFunction("resolve") : ok
+testAsyncFunction("reject") : throws
+
 Running test: testDate
 someGlobalDate.setDate(10) : throws
 new Date().setDate(10) : ok
@@ -20,3 +24,6 @@ someGlobalDate.getFullYear() : ok
 new Date().getFullYear() : ok
 someGlobalDate.getHours() : ok
 new Date().getHours() : ok
+
+Running test: testPromiseReject
+Promise.reject() : throws

test/inspector/debugger/side-effect-free-debug-evaluate.js

@@ -14,8 +14,19 @@ function testFunction()
   f,g;
   debugger;
 }
+async function testAsyncFunction(action) {
+  switch (action) {
+    case "resolve": return 1;
+    case "reject": throw new Error();
+  }
+}
 //# sourceURL=foo.js`);
 
+const check = async (expression) => {
+  const {result:{exceptionDetails}} = await Protocol.Runtime.evaluate({expression, throwOnSideEffect: true});
+  InspectorTest.log(expression + ' : ' + (exceptionDetails ? 'throws' : 'ok'));
+};
+
 InspectorTest.runAsyncTestSuite([
   async function basicTest() {
     Protocol.Debugger.enable();
@@ -32,11 +43,12 @@ InspectorTest.runAsyncTestSuite([
     InspectorTest.log('g() throws ' + className);
   },
 
+  async function testAsyncFunctions() {
+    await check('testAsyncFunction("resolve")');
+    await check('testAsyncFunction("reject")');
+  },
+
   async function testDate() {
-    const check = async (expression) => {
-      const {result:{exceptionDetails}} = await Protocol.Runtime.evaluate({expression, throwOnSideEffect: true});
-      InspectorTest.log(expression + ' : ' + (exceptionDetails ? 'throws' : 'ok'));
-    };
     // setters are only ok on temporary objects
     await check('someGlobalDate.setDate(10)');
     await check('new Date().setDate(10)');
@@ -51,5 +63,9 @@ InspectorTest.runAsyncTestSuite([
     await check('new Date().getFullYear()');
     await check('someGlobalDate.getHours()');
     await check('new Date().getHours()');
+  },
+
+  async function testPromiseReject() {
+    await check('Promise.reject()');
   }
 ]);