[inspector] Don't trigger window.onerror with side-effects disabled.

This was an oversight in https://crrev.com/c/3557234, which led to a
really weird developer experience: once a `window.onerror` handler was
installed, typing into the Console or other side-effect free debug
evaluations triggered this handler.

Fixed: chromium:1328008
Bug: chromium:1295750
Change-Id: I4029ff19ceb7cfe0a8eb6afff19c3ef9a4a82e25
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3660253
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80696}

src/inspector/injected-script.cc

@@ -73,7 +73,7 @@ class InjectedScript::ProtocolPromiseHandler {
   static bool add(V8InspectorSessionImpl* session,
                   v8::Local<v8::Context> context, v8::Local<v8::Value> value,
                   int executionContextId, const String16& objectGroup,
-                  WrapMode wrapMode, bool replMode,
+                  WrapMode wrapMode, bool replMode, bool throwOnSideEffect,
                   EvaluateCallback* callback) {
     v8::Local<v8::Promise::Resolver> resolver;
     if (!v8::Promise::Resolver::New(context).ToLocal(&resolver)) {
@@ -90,8 +90,8 @@ class InjectedScript::ProtocolPromiseHandler {
                            : v8::MaybeLocal<v8::Promise>();
     V8InspectorImpl* inspector = session->inspector();
     ProtocolPromiseHandler* handler = new ProtocolPromiseHandler(
-        session, executionContextId, objectGroup, wrapMode, replMode, callback,
-        originalPromise);
+        session, executionContextId, objectGroup, wrapMode, replMode,
+        throwOnSideEffect, callback, originalPromise);
     v8::Local<v8::Value> wrapper = handler->m_wrapper.Get(inspector->isolate());
     v8::Local<v8::Function> thenCallbackFunction =
         v8::Function::New(context, thenCallback, wrapper, 0,
@@ -143,7 +143,7 @@ class InjectedScript::ProtocolPromiseHandler {
   ProtocolPromiseHandler(V8InspectorSessionImpl* session,
                          int executionContextId, const String16& objectGroup,
                          WrapMode wrapMode, bool replMode,
-                         EvaluateCallback* callback,
+                         bool throwOnSideEffect, EvaluateCallback* callback,
                          v8::MaybeLocal<v8::Promise> maybeEvaluationResult)
       : m_inspector(session->inspector()),
         m_sessionId(session->sessionId()),
@@ -152,6 +152,7 @@ class InjectedScript::ProtocolPromiseHandler {
         m_objectGroup(objectGroup),
         m_wrapMode(wrapMode),
         m_replMode(replMode),
+        m_throwOnSideEffect(throwOnSideEffect),
         m_callback(std::move(callback)),
         m_wrapper(m_inspector->isolate(),
                   v8::External::New(m_inspector->isolate(), this)) {
@@ -249,8 +250,10 @@ class InjectedScript::ProtocolPromiseHandler {
     // we try to capture a fresh stack trace.
     if (maybeMessage.ToLocal(&message)) {
       v8::Local<v8::Value> exception = result;
+      if (!m_throwOnSideEffect) {
         session->inspector()->client()->dispatchError(scope.context(), message,
                                                       exception);
+      }
       protocol::PtrMaybe<protocol::Runtime::ExceptionDetails> exceptionDetails;
       response = scope.injectedScript()->createExceptionDetails(
           message, exception, m_objectGroup, &exceptionDetails);
@@ -333,6 +336,7 @@ class InjectedScript::ProtocolPromiseHandler {
   String16 m_objectGroup;
   WrapMode m_wrapMode;
   bool m_replMode;
+  bool m_throwOnSideEffect;
   EvaluateCallback* m_callback;
   v8::Global<v8::External> m_wrapper;
   v8::Global<v8::Promise> m_evaluationResult;
@@ -651,17 +655,17 @@ std::unique_ptr<protocol::Runtime::RemoteObject> InjectedScript::wrapTable(
 void InjectedScript::addPromiseCallback(
     V8InspectorSessionImpl* session, v8::MaybeLocal<v8::Value> value,
     const String16& objectGroup, WrapMode wrapMode, bool replMode,
-    std::unique_ptr<EvaluateCallback> callback) {
+    bool throwOnSideEffect, std::unique_ptr<EvaluateCallback> callback) {
   if (value.IsEmpty()) {
     callback->sendFailure(Response::InternalError());
     return;
   }
   v8::MicrotasksScope microtasksScope(m_context->isolate(),
                                       v8::MicrotasksScope::kRunMicrotasks);
-  if (ProtocolPromiseHandler::add(session, m_context->context(),
-                                  value.ToLocalChecked(),
-                                  m_context->contextId(), objectGroup, wrapMode,
-                                  replMode, callback.get())) {
+  if (ProtocolPromiseHandler::add(
+          session, m_context->context(), value.ToLocalChecked(),
+          m_context->contextId(), objectGroup, wrapMode, replMode,
+          throwOnSideEffect, callback.get())) {
     m_evaluateCallbacks.insert(callback.release());
   }
 }
@@ -840,7 +844,7 @@ Response InjectedScript::createExceptionDetails(
 
 Response InjectedScript::wrapEvaluateResult(
     v8::MaybeLocal<v8::Value> maybeResultValue, const v8::TryCatch& tryCatch,
-    const String16& objectGroup, WrapMode wrapMode,
+    const String16& objectGroup, WrapMode wrapMode, bool throwOnSideEffect,
     std::unique_ptr<protocol::Runtime::RemoteObject>* result,
     Maybe<protocol::Runtime::ExceptionDetails>* exceptionDetails) {
   v8::Local<v8::Value> resultValue;
@@ -858,8 +862,10 @@ Response InjectedScript::wrapEvaluateResult(
       return Response::ServerError("Execution was terminated");
     }
     v8::Local<v8::Value> exception = tryCatch.Exception();
+    if (!throwOnSideEffect) {
       m_context->inspector()->client()->dispatchError(
           m_context->context(), tryCatch.Message(), exception);
+    }
     Response response =
         wrapObject(exception, objectGroup,
                    exception->IsNativeError() ? WrapMode::kNoPreview

src/inspector/injected-script.h

@@ -113,7 +113,7 @@ class InjectedScript final {
   void addPromiseCallback(V8InspectorSessionImpl* session,
                           v8::MaybeLocal<v8::Value> value,
                           const String16& objectGroup, WrapMode wrapMode,
-                          bool replMode,
+                          bool replMode, bool throwOnSideEffect,
                           std::unique_ptr<EvaluateCallback> callback);
 
   Response findObject(const RemoteObjectId&, v8::Local<v8::Value>*) const;
@@ -133,7 +133,7 @@ class InjectedScript final {
 
   Response wrapEvaluateResult(
       v8::MaybeLocal<v8::Value> maybeResultValue, const v8::TryCatch&,
-      const String16& objectGroup, WrapMode wrapMode,
+      const String16& objectGroup, WrapMode wrapMode, bool throwOnSideEffect,
       std::unique_ptr<protocol::Runtime::RemoteObject>* result,
       Maybe<protocol::Runtime::ExceptionDetails>*);
   v8::Local<v8::Value> lastEvaluationResult() const;

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

@@ -1297,7 +1297,7 @@ Response V8DebuggerAgentImpl::evaluateOnCallFrame(
   if (returnByValue.fromMaybe(false)) mode = WrapMode::kForceValue;
   return scope.injectedScript()->wrapEvaluateResult(
       maybeResultValue, scope.tryCatch(), objectGroup.fromMaybe(""), mode,
-      result, exceptionDetails);
+      throwOnSideEffect.fromMaybe(false), result, exceptionDetails);
 }
 
 Response V8DebuggerAgentImpl::setVariableValue(

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

@@ -97,13 +97,14 @@ bool wrapEvaluateResultAsync(InjectedScript* injectedScript,
                              v8::MaybeLocal<v8::Value> maybeResultValue,
                              const v8::TryCatch& tryCatch,
                              const String16& objectGroup, WrapMode wrapMode,
+                             bool throwOnSideEffect,
                              ProtocolCallback* callback) {
   std::unique_ptr<RemoteObject> result;
   Maybe<protocol::Runtime::ExceptionDetails> exceptionDetails;
 
   Response response = injectedScript->wrapEvaluateResult(
-      maybeResultValue, tryCatch, objectGroup, wrapMode, &result,
-      &exceptionDetails);
+      maybeResultValue, tryCatch, objectGroup, wrapMode, throwOnSideEffect,
+      &result, &exceptionDetails);
   if (response.IsSuccess()) {
     callback->sendSuccess(std::move(result), std::move(exceptionDetails));
     return true;
@@ -117,7 +118,7 @@ void innerCallFunctionOn(
     v8::Local<v8::Value> recv, const String16& expression,
     Maybe<protocol::Array<protocol::Runtime::CallArgument>> optionalArguments,
     bool silent, WrapMode wrapMode, bool userGesture, bool awaitPromise,
-    const String16& objectGroup, bool throw_on_side_effect,
+    const String16& objectGroup, bool throwOnSideEffect,
     std::unique_ptr<V8RuntimeAgentImpl::CallFunctionOnCallback> callback) {
   V8InspectorImpl* inspector = session->inspector();
 
@@ -166,7 +167,7 @@ void innerCallFunctionOn(
   if (scope.tryCatch().HasCaught()) {
     wrapEvaluateResultAsync(scope.injectedScript(), maybeFunctionValue,
                             scope.tryCatch(), objectGroup, WrapMode::kNoPreview,
-                            callback.get());
+                            throwOnSideEffect, callback.get());
     return;
   }
 
@@ -184,7 +185,7 @@ void innerCallFunctionOn(
                                         v8::MicrotasksScope::kRunMicrotasks);
     maybeResultValue = v8::debug::CallFunctionOn(
         scope.context(), functionValue.As<v8::Function>(), recv, argc,
-        argv.get(), throw_on_side_effect);
+        argv.get(), throwOnSideEffect);
   }
   // Re-initialize after running client's code, as it could have destroyed
   // context or session.
@@ -197,12 +198,13 @@ void innerCallFunctionOn(
   if (!awaitPromise || scope.tryCatch().HasCaught()) {
     wrapEvaluateResultAsync(scope.injectedScript(), maybeResultValue,
                             scope.tryCatch(), objectGroup, wrapMode,
-                            callback.get());
+                            throwOnSideEffect, callback.get());
     return;
   }
 
   scope.injectedScript()->addPromiseCallback(
       session, maybeResultValue, objectGroup, wrapMode, false /* replMode */,
+      throwOnSideEffect,
       EvaluateCallbackWrapper<V8RuntimeAgentImpl::CallFunctionOnCallback>::wrap(
           std::move(callback)));
 }
@@ -331,12 +333,13 @@ void V8RuntimeAgentImpl::evaluate(
   if (!await || scope.tryCatch().HasCaught()) {
     wrapEvaluateResultAsync(scope.injectedScript(), maybeResultValue,
                             scope.tryCatch(), objectGroup.fromMaybe(""),
-                            wrap_mode, callback.get());
+                            wrap_mode, throwOnSideEffect.fromMaybe(false),
+                            callback.get());
     return;
   }
   scope.injectedScript()->addPromiseCallback(
       m_session, maybeResultValue, objectGroup.fromMaybe(""), wrap_mode,
-      replMode,
+      replMode, throwOnSideEffect.fromMaybe(false),
       EvaluateCallbackWrapper<EvaluateCallback>::wrap(std::move(callback)));
 }
 
@@ -360,7 +363,7 @@ void V8RuntimeAgentImpl::awaitPromise(
   if (returnByValue.fromMaybe(false)) mode = WrapMode::kForceValue;
   scope.injectedScript()->addPromiseCallback(
       m_session, scope.object(), scope.objectGroupName(), mode,
-      false /* replMode */,
+      false /* replMode */, false /* throwOnSideEffect */,
       EvaluateCallbackWrapper<AwaitPromiseCallback>::wrap(std::move(callback)));
 }
 
@@ -631,12 +634,12 @@ void V8RuntimeAgentImpl::runScript(
   if (!awaitPromise.fromMaybe(false) || scope.tryCatch().HasCaught()) {
     wrapEvaluateResultAsync(scope.injectedScript(), maybeResultValue,
                             scope.tryCatch(), objectGroup.fromMaybe(""), mode,
-                            callback.get());
+                            false /* throwOnSideEffect */, callback.get());
     return;
   }
   scope.injectedScript()->addPromiseCallback(
       m_session, maybeResultValue.ToLocalChecked(), objectGroup.fromMaybe(""),
-      mode, false /* replMode */,
+      mode, false /* replMode */, false /* throwOnSideEffect */,
       EvaluateCallbackWrapper<RunScriptCallback>::wrap(std::move(callback)));
 }