[api] Make running scripts in AddMessageListener callback work in debug mode

addaleax · Commit Bot · commit 09b53eef4ca6 · 2017-11-18T23:39:29.000Z
The existance of an `AllowJavascriptExecutionDebugOnly` scope in `Isolate::ReportPendingMessages()` indicates that the API supports running arbitrary JS code in a `AddMessageListener` callback. Currently, this can fail in debug mode: The `!isolate->external_caught_exception()` condition is checked when entering API methods inside such a handler. However, if there is a verbose `TryCatch` active when the exception occurs, this check fails, and when calling `ToString()` on the exception object leaves a pending exception itself, the flag is re-set to `true`. Fix this problem by clearing the flag and the pending exception if there was one during `ToString()`. This matches the code a few lines up in `messages.cc`, so the exception state is now consistent during the callback. This currently makes a Node.js test fail in debug mode (`parallel/test-error-reporting`). Bug: node:7144 Bug: node:17016 Change-Id: I060d00fea3e9a497f4df34c6ff8d6e29ebe96321 Reviewed-on: https://chromium-review.googlesource.com/718096 Commit-Queue: Yang Guo <yangguo@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#49466}
diff --git a/AUTHORS b/AUTHORS
@@ -44,7 +44,7 @@ Alexis Campailla <alexis@janeasystems.com>
 Andreas Anyuru <andreas.anyuru@gmail.com>
 Andrew Paprocki <andrew@ishiboo.com>
 Andrei Kashcha <anvaka@gmail.com>
-Anna Henningsen <addaleax@gmail.com>
+Anna Henningsen <anna@addaleax.net>
 Bangfu Tao <bangfu.tao@samsung.com>
 Ben Noordhuis <info@bnoordhuis.nl>
 Benjamin Tan <demoneaux@gmail.com>
diff --git a/src/messages.cc b/src/messages.cc
@@ -113,6 +113,9 @@ void MessageHandler::ReportMessage(Isolate* isolate, const MessageLocation* loc,
       }
 
       if (!maybe_stringified.ToHandle(&stringified)) {
+        DCHECK(isolate->has_pending_exception());
+        isolate->clear_pending_exception();
+        isolate->set_external_caught_exception(false);
         stringified =
             isolate->factory()->NewStringFromAsciiChecked("exception");
       }
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
@@ -5696,23 +5696,55 @@ TEST(CustomErrorMessage) {
 
 static void check_custom_rethrowing_message(v8::Local<v8::Message> message,
                                             v8::Local<v8::Value> data) {
+  CHECK(data->IsExternal());
+  int* callcount = static_cast<int*>(data.As<v8::External>()->Value());
+  ++*callcount;
+
   const char* uncaught_error = "Uncaught exception";
   CHECK(message->Get()
             ->Equals(CcTest::isolate()->GetCurrentContext(),
                      v8_str(uncaught_error))
             .FromJust());
+  // Test that compiling code inside a message handler works.
+  CHECK(CompileRunChecked(CcTest::isolate(), "(function(a) { return a; })(42)")
+            ->Equals(CcTest::isolate()->GetCurrentContext(),
+                     v8::Integer::NewFromUnsigned(CcTest::isolate(), 42))
+            .FromJust());
 }
 
 
 TEST(CustomErrorRethrowsOnToString) {
+  int callcount = 0;
   LocalContext context;
-  v8::HandleScope scope(context->GetIsolate());
-  context->GetIsolate()->AddMessageListener(check_custom_rethrowing_message);
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::HandleScope scope(isolate);
+  context->GetIsolate()->AddMessageListener(
+      check_custom_rethrowing_message, v8::External::New(isolate, &callcount));
+
+  CompileRun(
+      "var e = { toString: function() { throw e; } };"
+      "try { throw e; } finally {}");
+
+  CHECK_EQ(callcount, 1);
+  context->GetIsolate()->RemoveMessageListeners(
+      check_custom_rethrowing_message);
+}
+
+TEST(CustomErrorRethrowsOnToStringInsideVerboseTryCatch) {
+  int callcount = 0;
+  LocalContext context;
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::TryCatch try_catch(isolate);
+  try_catch.SetVerbose(true);
+  context->GetIsolate()->AddMessageListener(
+      check_custom_rethrowing_message, v8::External::New(isolate, &callcount));
 
   CompileRun(
       "var e = { toString: function() { throw e; } };"
       "try { throw e; } finally {}");
 
+  CHECK_EQ(callcount, 1);
   context->GetIsolate()->RemoveMessageListeners(
       check_custom_rethrowing_message);
 }

Original file line number	Diff line number	Diff line change
`@@ -113,6 +113,9 @@ void MessageHandler::ReportMessage(Isolate* isolate, const MessageLocation* loc,`
`113`	`113`	`}`
`114`	`114`
`115`	`115`	`if (!maybe_stringified.ToHandle(&stringified)) {`
	`116`	`+ DCHECK(isolate->has_pending_exception());`
	`117`	`+ isolate->clear_pending_exception();`
	`118`	`+ isolate->set_external_caught_exception(false);`
`116`	`119`	`stringified =`
`117`	`120`	`isolate->factory()->NewStringFromAsciiChecked("exception");`
`118`	`121`	`}`