From 8fb95efddabcf513395dc49f33dd0c2855204829 Mon Sep 17 00:00:00 2001 From: "yangguo@chromium.org" Date: Tue, 6 Aug 2013 13:58:21 +0000 Subject: [PATCH] Improve internal stringifcation for custom Error objects. If an developer attempts to "subclass" Error by running `MyError.prototype = new Error();`, then the internal v8::Message object that's produced and handed off to `window.onerror` handlers is poorly stringified as "[object Object]". This patch adjusts the stringification process for these objects to include not only native Error objects, but also objects that have Error in their prototype chain, and haven't overwritten Error.toString with some custom variant. BUG=2822 R=mstarzinger@chromium.org, yangguo@chromium.org Review URL: https://codereview.chromium.org/21761002 Patch from Mike West . git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@16075 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/messages.js | 22 +++++++++------- test/cctest/test-api.cc | 56 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/src/messages.js b/src/messages.js index b586d24882..2debbf8654 100644 --- a/src/messages.js +++ b/src/messages.js @@ -228,16 +228,18 @@ function NoSideEffectToString(obj) { } } } - if (IsNativeErrorObject(obj)) return %_CallFunction(obj, ErrorToString); + if (CanBeSafelyTreatedAsAnErrorObject(obj)) { + return %_CallFunction(obj, ErrorToString); + } return %_CallFunction(obj, ObjectToString); } - -// To check if something is a native error we need to check the -// concrete native error types. It is not sufficient to use instanceof -// since it possible to create an object that has Error.prototype on -// its prototype chain. This is the case for DOMException for example. -function IsNativeErrorObject(obj) { +// To determine whether we can safely stringify an object using ErrorToString +// without the risk of side-effects, we need to check whether the object is +// either an instance of a native error type (via '%_ClassOf'), or has $Error +// in its prototype chain and hasn't overwritten 'toString' with something +// strange and unusual. +function CanBeSafelyTreatedAsAnErrorObject(obj) { switch (%_ClassOf(obj)) { case 'Error': case 'EvalError': @@ -248,7 +250,9 @@ function IsNativeErrorObject(obj) { case 'URIError': return true; } - return false; + + var objToString = %GetDataProperty(obj, "toString"); + return obj instanceof $Error && objToString === ErrorToString; } @@ -257,7 +261,7 @@ function IsNativeErrorObject(obj) { // the error to string method. This is to avoid leaking error // objects between script tags in a browser setting. function ToStringCheckErrorObject(obj) { - if (IsNativeErrorObject(obj)) { + if (CanBeSafelyTreatedAsAnErrorObject(obj)) { return %_CallFunction(obj, ErrorToString); } else { return ToString(obj); diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index 511a9440b2..664f905105 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -4388,7 +4388,7 @@ TEST(APIThrowMessageOverwrittenToString) { } -static void check_custom_error_message( +static void check_custom_error_tostring( v8::Handle message, v8::Handle data) { const char* uncaught_error = "Uncaught MyError toString"; @@ -4399,7 +4399,7 @@ static void check_custom_error_message( TEST(CustomErrorToString) { LocalContext context; v8::HandleScope scope(context->GetIsolate()); - v8::V8::AddMessageListener(check_custom_error_message); + v8::V8::AddMessageListener(check_custom_error_tostring); CompileRun( "function MyError(name, message) { " " this.name = name; " @@ -4410,6 +4410,58 @@ TEST(CustomErrorToString) { " return 'MyError toString'; " "}; " "throw new MyError('my name', 'my message'); "); + v8::V8::RemoveMessageListeners(check_custom_error_tostring); +} + + +static void check_custom_error_message( + v8::Handle message, + v8::Handle data) { + const char* uncaught_error = "Uncaught MyError: my message"; + printf("%s\n", *v8::String::Utf8Value(message->Get())); + CHECK(message->Get()->Equals(v8_str(uncaught_error))); +} + + +TEST(CustomErrorMessage) { + LocalContext context; + v8::HandleScope scope(context->GetIsolate()); + v8::V8::AddMessageListener(check_custom_error_message); + + // Handlebars. + CompileRun( + "function MyError(msg) { " + " this.name = 'MyError'; " + " this.message = msg; " + "} " + "MyError.prototype = new Error(); " + "throw new MyError('my message'); "); + + // Closure. + CompileRun( + "function MyError(msg) { " + " this.name = 'MyError'; " + " this.message = msg; " + "} " + "inherits = function(childCtor, parentCtor) { " + " function tempCtor() {}; " + " tempCtor.prototype = parentCtor.prototype; " + " childCtor.superClass_ = parentCtor.prototype; " + " childCtor.prototype = new tempCtor(); " + " childCtor.prototype.constructor = childCtor; " + "}; " + "inherits(MyError, Error); " + "throw new MyError('my message'); "); + + // Object.create. + CompileRun( + "function MyError(msg) { " + " this.name = 'MyError'; " + " this.message = msg; " + "} " + "MyError.prototype = Object.create(Error.prototype); " + "throw new MyError('my message'); "); + v8::V8::RemoveMessageListeners(check_custom_error_message); }