Rewrite Error.prototype.toString in C++.

This avoids many back-and-forth calls to the runtime.

This also slightly changes the way we avoid getters. Previously, we circumvent getting the name property of ReferenceError, SyntaxError and TypeError due to crbug/69187 (in order to avoid leaking information from those errors through a 'name' getter installed on their prototypes). Now we do that for all errors created by V8.

R=jkummerow@chromium.org, rossberg@chromium.org
BUG=crbug:513472, crbug:69187
LOG=N

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

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

src/bootstrapper.cc

@@ -2742,6 +2742,15 @@ bool Genesis::InstallSpecialObjects(Handle<Context> native_context) {
                                 factory->stack_trace_symbol(), NONE),
                             false);
 
+  // Expose the internal error symbol to native JS
+  RETURN_ON_EXCEPTION_VALUE(isolate,
+                            JSObject::SetOwnPropertyIgnoreAttributes(
+                                handle(native_context->builtins(), isolate),
+                                factory->InternalizeOneByteString(
+                                    STATIC_CHAR_VECTOR("$internalErrorSymbol")),
+                                factory->internal_error_symbol(), NONE),
+                            false);
+
   // Expose the debug global object in global if a name for it is specified.
   if (FLAG_expose_debug_as != NULL && strlen(FLAG_expose_debug_as) != 0) {
     // If loading fails we just bail out without installing the

src/heap/heap.h

@@ -324,7 +324,8 @@ namespace internal {
   V(class_end_position_symbol)      \
   V(error_start_pos_symbol)         \
   V(error_end_pos_symbol)           \
-  V(error_script_symbol)
+  V(error_script_symbol)            \
+  V(internal_error_symbol)
 
 #define PUBLIC_SYMBOL_LIST(V)                                    \
   V(has_instance_symbol, symbolHasInstance, Symbol.hasInstance)  \

src/isolate.h

@@ -23,6 +23,7 @@
 #include "src/handles.h"
 #include "src/hashmap.h"
 #include "src/heap/heap.h"
+#include "src/messages.h"
 #include "src/optimizing-compile-dispatcher.h"
 #include "src/regexp-stack.h"
 #include "src/runtime/runtime.h"
@@ -994,6 +995,10 @@ class Isolate {
     date_cache_ = date_cache;
   }
 
+  ErrorToStringHelper* error_tostring_helper() {
+    return &error_tostring_helper_;
+  }
+
   Map* get_initial_js_array_map(ElementsKind kind,
                                 Strength strength = Strength::WEAK);
 
@@ -1296,6 +1301,7 @@ class Isolate {
       regexp_macro_assembler_canonicalize_;
   RegExpStack* regexp_stack_;
   DateCache* date_cache_;
+  ErrorToStringHelper error_tostring_helper_;
   unibrow::Mapping<unibrow::Ecma262Canonicalize> interp_canonicalize_mapping_;
   CallInterfaceDescriptorData* call_descriptor_data_;
   base::RandomNumberGenerator* random_number_generator_;

src/messages.cc

@@ -367,5 +367,97 @@ MaybeHandle<String> MessageTemplate::FormatMessage(int template_index,
 
   return builder.Finish();
 }
+
+
+MaybeHandle<String> ErrorToStringHelper::Stringify(Isolate* isolate,
+                                                   Handle<JSObject> error) {
+  VisitedScope scope(this, error);
+  if (scope.has_visited()) return isolate->factory()->empty_string();
+
+  Handle<String> name;
+  Handle<String> message;
+  Handle<Name> internal_key = isolate->factory()->internal_error_symbol();
+  Handle<String> message_string =
+      isolate->factory()->NewStringFromStaticChars("message");
+  Handle<String> name_string = isolate->factory()->name_string();
+  LookupIterator internal_error_lookup(
+      error, internal_key, LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR);
+  LookupIterator message_lookup(
+      error, message_string, LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR);
+  LookupIterator name_lookup(error, name_string,
+                             LookupIterator::PROTOTYPE_CHAIN_SKIP_INTERCEPTOR);
+
+  // Find out whether an internally created error object is on the prototype
+  // chain. If the name property is found on a holder prior to the internally
+  // created error object, use that name property. Otherwise just use the
+  // constructor name to avoid triggering possible side effects.
+  // Similar for the message property. If the message property shadows the
+  // internally created error object, use that message property. Otherwise
+  // use empty string as message.
+  if (internal_error_lookup.IsFound()) {
+    if (!ShadowsInternalError(isolate, &name_lookup, &internal_error_lookup)) {
+      Handle<JSObject> holder = internal_error_lookup.GetHolder<JSObject>();
+      name = Handle<String>(holder->constructor_name());
+    }
+    if (!ShadowsInternalError(isolate, &message_lookup,
+                              &internal_error_lookup)) {
+      message = isolate->factory()->empty_string();
+    }
+  }
+  if (name.is_null()) {
+    ASSIGN_RETURN_ON_EXCEPTION(
+        isolate, name,
+        GetStringifiedProperty(isolate, &name_lookup,
+                               isolate->factory()->Error_string()),
+        String);
+  }
+  if (message.is_null()) {
+    ASSIGN_RETURN_ON_EXCEPTION(
+        isolate, message,
+        GetStringifiedProperty(isolate, &message_lookup,
+                               isolate->factory()->empty_string()),
+        String);
+  }
+
+  if (name->length() == 0) return message;
+  if (message->length() == 0) return name;
+  IncrementalStringBuilder builder(isolate);
+  builder.AppendString(name);
+  builder.AppendCString(": ");
+  builder.AppendString(message);
+  return builder.Finish();
+}
+
+
+bool ErrorToStringHelper::ShadowsInternalError(
+    Isolate* isolate, LookupIterator* property_lookup,
+    LookupIterator* internal_error_lookup) {
+  Handle<JSObject> holder = property_lookup->GetHolder<JSObject>();
+  // It's fine if the property is defined on the error itself.
+  if (holder.is_identical_to(property_lookup->GetReceiver())) return true;
+  PrototypeIterator it(isolate, holder, PrototypeIterator::START_AT_RECEIVER);
+  while (true) {
+    if (it.IsAtEnd()) return false;
+    if (it.IsAtEnd(internal_error_lookup->GetHolder<JSObject>())) return true;
+    it.AdvanceIgnoringProxies();
+  }
+}
+
+
+MaybeHandle<String> ErrorToStringHelper::GetStringifiedProperty(
+    Isolate* isolate, LookupIterator* property_lookup,
+    Handle<String> default_value) {
+  if (!property_lookup->IsFound()) return default_value;
+  Handle<Object> obj;
+  ASSIGN_RETURN_ON_EXCEPTION(isolate, obj, Object::GetProperty(property_lookup),
+                             String);
+  if (obj->IsUndefined()) return default_value;
+  if (!obj->IsString()) {
+    ASSIGN_RETURN_ON_EXCEPTION(isolate, obj, Execution::ToString(isolate, obj),
+                               String);
+  }
+  return Handle<String>::cast(obj);
+}
+
 }  // namespace internal
 }  // namespace v8

src/messages.h

@@ -457,6 +457,46 @@ class MessageHandler {
   static base::SmartArrayPointer<char> GetLocalizedMessage(Isolate* isolate,
                                                            Handle<Object> data);
 };
+
+
+class ErrorToStringHelper {
+ public:
+  ErrorToStringHelper() : visited_(0) {}
+
+  MUST_USE_RESULT MaybeHandle<String> Stringify(Isolate* isolate,
+                                                Handle<JSObject> error);
+
+ private:
+  class VisitedScope {
+   public:
+    VisitedScope(ErrorToStringHelper* helper, Handle<JSObject> error)
+        : helper_(helper), has_visited_(false) {
+      for (const Handle<JSObject>& visited : helper->visited_) {
+        if (visited.is_identical_to(error)) {
+          has_visited_ = true;
+          break;
+        }
+      }
+      helper->visited_.Add(error);
+    }
+    ~VisitedScope() { helper_->visited_.RemoveLast(); }
+    bool has_visited() { return has_visited_; }
+
+   private:
+    ErrorToStringHelper* helper_;
+    bool has_visited_;
+  };
+
+  static bool ShadowsInternalError(Isolate* isolate,
+                                   LookupIterator* property_lookup,
+                                   LookupIterator* internal_error_lookup);
+
+  static MUST_USE_RESULT MaybeHandle<String> GetStringifiedProperty(
+      Isolate* isolate, LookupIterator* property_lookup,
+      Handle<String> default_value);
+
+  List<Handle<JSObject> > visited_;
+};
 } }  // namespace v8::internal
 
 #endif  // V8_MESSAGES_H_

src/messages.js

@@ -6,6 +6,7 @@
 
 var $errorToString;
 var $getStackTraceLine;
+var $internalErrorSymbol;
 var $messageGetPositionInLine;
 var $messageGetLineNumber;
 var $messageGetSourceLine;
@@ -181,7 +182,9 @@ function ToDetailString(obj) {
 
 function MakeGenericError(constructor, type, arg0, arg1, arg2) {
   if (IS_UNDEFINED(arg0) && IS_STRING(type)) arg0 = [];
-  return new constructor(FormatMessage(type, arg0, arg1, arg2));
+  var error = new constructor(FormatMessage(type, arg0, arg1, arg2));
+  error[$internalErrorSymbol] = true;
+  return error;
 }
 
 
@@ -1000,66 +1003,12 @@ GlobalURIError = DefineError(global, function URIError() { });
 
 %AddNamedProperty(GlobalError.prototype, 'message', '', DONT_ENUM);
 
-// Global list of error objects visited during ErrorToString. This is
-// used to detect cycles in error toString formatting.
-var visited_errors = new InternalArray();
-var cyclic_error_marker = new GlobalObject();
-
-function GetPropertyWithoutInvokingMonkeyGetters(error, name) {
-  var current = error;
-  // Climb the prototype chain until we find the holder.
-  while (current && !%HasOwnProperty(current, name)) {
-    current = %_GetPrototype(current);
-  }
-  if (IS_NULL(current)) return UNDEFINED;
-  if (!IS_OBJECT(current)) return error[name];
-  // If the property is an accessor on one of the predefined errors that can be
-  // generated statically by the compiler, don't touch it. This is to address
-  // http://code.google.com/p/chromium/issues/detail?id=69187
-  var desc = %GetOwnProperty(current, name);
-  if (desc && desc[IS_ACCESSOR_INDEX]) {
-    var isName = name === "name";
-    if (current === GlobalReferenceError.prototype)
-      return isName ? "ReferenceError" : UNDEFINED;
-    if (current === GlobalSyntaxError.prototype)
-      return isName ? "SyntaxError" : UNDEFINED;
-    if (current === GlobalTypeError.prototype)
-      return isName ? "TypeError" : UNDEFINED;
-  }
-  // Otherwise, read normally.
-  return error[name];
-}
-
-function ErrorToStringDetectCycle(error) {
-  if (!%PushIfAbsent(visited_errors, error)) throw cyclic_error_marker;
-  try {
-    var name = GetPropertyWithoutInvokingMonkeyGetters(error, "name");
-    name = IS_UNDEFINED(name) ? "Error" : TO_STRING_INLINE(name);
-    var message = GetPropertyWithoutInvokingMonkeyGetters(error, "message");
-    message = IS_UNDEFINED(message) ? "" : TO_STRING_INLINE(message);
-    if (name === "") return message;
-    if (message === "") return name;
-    return name + ": " + message;
-  } finally {
-    visited_errors.length = visited_errors.length - 1;
-  }
-}
-
 function ErrorToString() {
   if (!IS_SPEC_OBJECT(this)) {
     throw MakeTypeError(kCalledOnNonObject, "Error.prototype.toString");
   }
 
-  try {
-    return ErrorToStringDetectCycle(this);
-  } catch(e) {
-    // If this error message was encountered already return the empty
-    // string for it instead of recursively formatting it.
-    if (e === cyclic_error_marker) {
-      return '';
-    }
-    throw e;
-  }
+  return %ErrorToStringRT(this);
 }
 
 utils.InstallFunctions(GlobalError.prototype, DONT_ENUM,

src/runtime/runtime-internal.cc

@@ -253,6 +253,18 @@ RUNTIME_FUNCTION(Runtime_MessageGetScript) {
 }
 
 
+RUNTIME_FUNCTION(Runtime_ErrorToStringRT) {
+  HandleScope scope(isolate);
+  DCHECK(args.length() == 1);
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, error, 0);
+  Handle<String> result;
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, result,
+      isolate->error_tostring_helper()->Stringify(isolate, error));
+  return *result;
+}
+
+
 RUNTIME_FUNCTION(Runtime_FormatMessageString) {
   HandleScope scope(isolate);
   DCHECK(args.length() == 4);

src/runtime/runtime.h

@@ -322,6 +322,7 @@ namespace internal {
   F(RenderCallSite, 0, 1)                     \
   F(MessageGetStartPosition, 1, 1)            \
   F(MessageGetScript, 1, 1)                   \
+  F(ErrorToStringRT, 1, 1)                    \
   F(FormatMessageString, 4, 1)                \
   F(CallSiteGetFileNameRT, 3, 1)              \
   F(CallSiteGetFunctionNameRT, 3, 1)          \

test/mjsunit/error-constructors.js

@@ -69,26 +69,53 @@ assertTrue(e.hasOwnProperty('stack'));
 // compiler errors. This is not specified, but allowing interception
 // through a getter can leak error objects from different
 // script tags in the same context in a browser setting.
-var errors = [SyntaxError, ReferenceError, TypeError];
+var errors = [SyntaxError, ReferenceError, TypeError, RangeError, URIError];
+var error_triggers = ["syntax error",
+                      "var error = reference",
+                      "undefined()",
+                      "String.fromCodePoint(0xFFFFFF)",
+                      "decodeURI('%F')"];
 for (var i in errors) {
-  var name = errors[i].prototype.toString();
+  var name = errors[i].name;
+
   // Monkey-patch prototype.
   var props = ["name", "message", "stack"];
   for (var j in props) {
     errors[i].prototype.__defineGetter__(props[j], fail);
   }
   // String conversion should not invoke monkey-patched getters on prototype.
-  var e = new errors[i];
-  assertEquals(name, e.toString());
+  var error;
+  try {
+    eval(error_triggers[i]);
+  } catch (e) {
+    error = e;
+  }
+  assertTrue(error.toString().startsWith(name));
+
+  // Deleting message on the error (exposing the getter) is fine.
+  delete error.message;
+  assertEquals(name, error.toString());
+
+  // Custom properties shadowing the name are fine.
+  var myerror = { name: "myerror", message: "mymessage"};
+  myerror.__proto__ = error;
+  assertEquals("myerror: mymessage", myerror.toString());
+
   // Custom getters in actual objects are welcome.
-  e.__defineGetter__("name", function() { return "mine"; });
-  assertEquals("mine", e.toString());
+  error.__defineGetter__("name", function() { return "mine"; });
+  assertEquals("mine", error.toString());
+
+  // Custom properties shadowing the name are fine.
+  var myerror2 = { message: "mymessage"};
+  myerror2.__proto__ = error;
+  assertEquals("mine: mymessage", myerror2.toString());
 }
 
-// Monkey-patching non-static errors should still be observable.
+// Monkey-patching non-internal errors should still be observable.
 function MyError() {}
 MyError.prototype = new Error;
-var errors = [Error, RangeError, EvalError, URIError, MyError];
+var errors = [Error, RangeError, EvalError, URIError,
+              SyntaxError, ReferenceError, TypeError, MyError];
 for (var i in errors) {
   errors[i].prototype.__defineGetter__("name", function() { return "my"; });
   errors[i].prototype.__defineGetter__("message", function() { return "moo"; });

test/mjsunit/regress/regress-crbug-513472.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.
+
+"use strict";
+this.__proto__ = Error();
+assertThrows(function() { NaN = 1; });