Do not eagerly convert exception to string when creating a message object

R=mstarzinger@chromium.org
BUG=chromium:490680
LOG=Y

Review URL: https://codereview.chromium.org/1157563005

Cr-Commit-Position: refs/heads/master@{#28670}

src/debug.cc

@@ -629,7 +629,7 @@ bool Debug::CompileDebuggerScript(Isolate* isolate, int index) {
     DCHECK(!isolate->has_pending_exception());
     MessageLocation computed_location;
     isolate->ComputeLocation(&computed_location);
-    Handle<Object> message = MessageHandler::MakeMessageObject(
+    Handle<JSMessageObject> message = MessageHandler::MakeMessageObject(
         isolate, MessageTemplate::kDebuggerLoading, &computed_location,
         isolate->factory()->undefined_value(), Handle<JSArray>());
     DCHECK(!isolate->has_pending_exception());

src/isolate.cc

@@ -1377,17 +1377,6 @@ Handle<JSMessageObject> Isolate::CreateMessage(Handle<Object> exception,
     location = &potential_computed_location;
   }
 
-  // If the exception argument is a custom object, turn it into a string
-  // before throwing as uncaught exception.  Note that the pending
-  // exception object to be set later must not be turned into a string.
-  if (exception->IsJSObject() && !IsErrorObject(exception)) {
-    MaybeHandle<Object> maybe_exception =
-        Execution::ToDetailString(this, exception);
-    if (!maybe_exception.ToHandle(&exception)) {
-      exception =
-          factory()->InternalizeOneByteString(STATIC_CHAR_VECTOR("exception"));
-    }
-  }
   return MessageHandler::MakeMessageObject(
       this, MessageTemplate::kUncaughtException, location, exception,
       stack_trace_object);

src/messages.cc

@@ -59,9 +59,8 @@ Handle<JSMessageObject> MessageHandler::MakeMessageObject(
 }
 
 
-void MessageHandler::ReportMessage(Isolate* isolate,
-                                   MessageLocation* loc,
-                                   Handle<Object> message) {
+void MessageHandler::ReportMessage(Isolate* isolate, MessageLocation* loc,
+                                   Handle<JSMessageObject> message) {
   // We are calling into embedder's code which can throw exceptions.
   // Thus we need to save current exception state, reset it to the clean one
   // and ignore scheduled exceptions callbacks can throw.
@@ -71,14 +70,29 @@ void MessageHandler::ReportMessage(Isolate* isolate,
   if (isolate->has_pending_exception()) {
     exception_object = isolate->pending_exception();
   }
-  Handle<Object> exception_handle(exception_object, isolate);
+  Handle<Object> exception(exception_object, isolate);
 
   Isolate::ExceptionScope exception_scope(isolate);
   isolate->clear_pending_exception();
   isolate->set_external_caught_exception(false);
 
+  // Turn the exception on the message into a string if it is an object.
+  if (message->argument()->IsJSObject()) {
+    HandleScope scope(isolate);
+    Handle<Object> argument(message->argument(), isolate);
+    Handle<Object> args[] = {argument};
+    MaybeHandle<Object> maybe_stringified = Execution::TryCall(
+        isolate->to_detail_string_fun(), isolate->js_builtins_object(),
+        arraysize(args), args);
+    Handle<Object> stringified;
+    if (!maybe_stringified.ToHandle(&stringified)) {
+      stringified = isolate->factory()->NewStringFromAsciiChecked("exception");
+    }
+    message->set_argument(*stringified);
+  }
+
   v8::Local<v8::Message> api_message_obj = v8::Utils::MessageToLocal(message);
-  v8::Local<v8::Value> api_exception_obj = v8::Utils::ToLocal(exception_handle);
+  v8::Local<v8::Value> api_exception_obj = v8::Utils::ToLocal(exception);
 
   v8::NeanderArray global_listeners(isolate->factory()->message_listeners());
   int global_length = global_listeners.length();
@@ -280,22 +294,27 @@ Handle<String> MessageTemplate::FormatMessage(Isolate* isolate,
                                               int template_index,
                                               Handle<Object> arg) {
   Factory* factory = isolate->factory();
-  Handle<String> fmt_str = factory->InternalizeOneByteString(
-      STATIC_CHAR_VECTOR("$noSideEffectToString"));
-  Handle<JSFunction> fun = Handle<JSFunction>::cast(
-      Object::GetProperty(isolate->js_builtins_object(), fmt_str)
-          .ToHandleChecked());
-
-  MaybeHandle<Object> maybe_result =
-      Execution::TryCall(fun, isolate->js_builtins_object(), 1, &arg);
-  Handle<Object> result;
-  if (!maybe_result.ToHandle(&result) || !result->IsString()) {
-    return factory->InternalizeOneByteString(STATIC_CHAR_VECTOR("<error>"));
+  Handle<String> result_string;
+  if (arg->IsString()) {
+    result_string = Handle<String>::cast(arg);
+  } else {
+    Handle<String> fmt_str = factory->InternalizeOneByteString(
+        STATIC_CHAR_VECTOR("$noSideEffectToString"));
+    Handle<JSFunction> fun = Handle<JSFunction>::cast(
+        Object::GetProperty(isolate->js_builtins_object(), fmt_str)
+            .ToHandleChecked());
+
+    MaybeHandle<Object> maybe_result =
+        Execution::TryCall(fun, isolate->js_builtins_object(), 1, &arg);
+    Handle<Object> result;
+    if (!maybe_result.ToHandle(&result) || !result->IsString()) {
+      return factory->InternalizeOneByteString(STATIC_CHAR_VECTOR("<error>"));
+    }
+    result_string = Handle<String>::cast(result);
   }
   MaybeHandle<String> maybe_result_string = MessageTemplate::FormatMessage(
-      template_index, Handle<String>::cast(result), factory->empty_string(),
+      template_index, result_string, factory->empty_string(),
       factory->empty_string());
-  Handle<String> result_string;
   if (!maybe_result_string.ToHandle(&result_string)) {
     return factory->InternalizeOneByteString(STATIC_CHAR_VECTOR("<error>"));
   }

src/messages.h

@@ -438,7 +438,7 @@ class MessageHandler {
 
   // Report a formatted message (needs JS allocation).
   static void ReportMessage(Isolate* isolate, MessageLocation* loc,
-                            Handle<Object> message);
+                            Handle<JSMessageObject> message);
 
   static void DefaultMessageReport(Isolate* isolate, const MessageLocation* loc,
                                    Handle<Object> message_obj);

test/cctest/test-api.cc

@@ -4950,6 +4950,26 @@ TEST(CustomErrorMessage) {
 }
 
 
+static void check_custom_rethrowing_message(v8::Handle<v8::Message> message,
+                                            v8::Handle<v8::Value> data) {
+  const char* uncaught_error = "Uncaught exception";
+  CHECK(message->Get()->Equals(v8_str(uncaught_error)));
+}
+
+
+TEST(CustomErrorRethrowsOnToString) {
+  LocalContext context;
+  v8::HandleScope scope(context->GetIsolate());
+  v8::V8::AddMessageListener(check_custom_rethrowing_message);
+
+  CompileRun(
+      "var e = { toString: function() { throw e; } };"
+      "try { throw e; } finally {}");
+
+  v8::V8::RemoveMessageListeners(check_custom_rethrowing_message);
+}
+
+
 static void receive_message(v8::Handle<v8::Message> message,
                             v8::Handle<v8::Value> data) {
   message->Get();

test/mjsunit/regress/regress-crbug-490680.js

+// Copyright 2015 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.
+
+var sentinel = null;
+
+try {
+  throw { toString: function() { sentinel = "observed"; } };
+} catch (e) {
+}
+
+L: try {
+  throw { toString: function() { sentinel = "observed"; } };
+} finally {
+  break L;
+}
+
+assertNull(sentinel);