Improve internal stringifcation for custom Error objects.

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

Committed: https://code.google.com/p/v8/source/detail?r=16075

[Mike West, 2013-08-02 08:14:31]

Hey Michael, you might or might not be the right person to review this patch (I
don't really know who works on what yet...). If it looks like something you're
comfy with, I'd appreciate you reviewing. If not, I'd appreciate you helping me
find the right reviewer. :)

Thanks!

-mike

[Michael Starzinger, 2013-08-02 11:47:04]

Left comments and also adding Yang as a reviewer as he is more familiar with
this code.

https://codereview.chromium.org/21761002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/21761002/diff/1/src/messages.js#newcode232
src/messages.js:232: (obj instanceof $Error && obj.toString ===
$Error.prototype.toString)) {
As discussed offline: I am a little bit worried that the "obj.toString" access
here is observable and might throw an exception, or even worse return different
values every other time. Better use "GetPropertyWithoutInvokingMonkeyGetters" to
get to the value of the "toString" property.

Also "$Error.prototype.toString" is configurable and might not point to the
primordial implementation, it would be better to compare against "ErrorToString"
instead.

[Mike West, 2013-08-02 12:04:50]

Thanks, Michael. I look forward to what Yang has to say next week. :)

https://codereview.chromium.org/21761002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/21761002/diff/1/src/messages.js#newcode232
src/messages.js:232: (obj instanceof $Error && obj.toString ===
$Error.prototype.toString)) {
On 2013/08/02 11:47:04, Michael Starzinger wrote:
> As discussed offline: I am a little bit worried that the "obj.toString" access
> here is observable ...
> 
> Also "$Error.prototype.toString" is configurable ...

Both these suggestions make sense, and are in the latest patchset.

Thanks for taking a look!

[Michael Starzinger, 2013-08-02 12:16:01]

LGTM with one final comment to address. I would still wait for Yang's approval
though.

https://codereview.chromium.org/21761002/diff/8001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/21761002/diff/8001/src/messages.js#newcode231
src/messages.js:231: var objToString =
GetPropertyWithoutInvokingMonkeyGetters(obj, "toString");
Yep, better, final nit: Can we move this into the "IsNativeErrorObject" helper
to not duplicate it at all use-sites of that helper?

[Mike West, 2013-08-02 12:20:23]

On 2013/08/02 12:16:01, Michael Starzinger wrote:
> LGTM with one final comment to address. I would still wait for Yang's approval
> though.
> 
> https://codereview.chromium.org/21761002/diff/8001/src/messages.js
> File src/messages.js (right):
> 
> https://codereview.chromium.org/21761002/diff/8001/src/messages.js#newcode231
> src/messages.js:231: var objToString =
> GetPropertyWithoutInvokingMonkeyGetters(obj, "toString");
> Yep, better, final nit: Can we move this into the "IsNativeErrorObject" helper
> to not duplicate it at all use-sites of that helper?

Hrm. You're right, that function isn't used anywhere else.

I'd intentionally avoided changing its meaning, but since these are the only
callsites it probably makes sense to just rename it (to
"CanBeSafelyTreatedAsAnErrorObject"?) and update the comment.

[Michael Starzinger, 2013-08-02 12:21:57]

On 2013/08/02 12:20:23, Mike West wrote:
> On 2013/08/02 12:16:01, Michael Starzinger wrote:
> > LGTM with one final comment to address. I would still wait for Yang's
approval
> > though.
> > 
> > https://codereview.chromium.org/21761002/diff/8001/src/messages.js
> > File src/messages.js (right):
> > 
> >
https://codereview.chromium.org/21761002/diff/8001/src/messages.js#newcode231
> > src/messages.js:231: var objToString =
> > GetPropertyWithoutInvokingMonkeyGetters(obj, "toString");
> > Yep, better, final nit: Can we move this into the "IsNativeErrorObject"
helper
> > to not duplicate it at all use-sites of that helper?
> 
> Hrm. You're right, that function isn't used anywhere else.
> 
> I'd intentionally avoided changing its meaning, but since these are the only
> callsites it probably makes sense to just rename it (to
> "CanBeSafelyTreatedAsAnErrorObject"?) and update the comment.

I don't have an opinion about the name. Either way is fine for me.

[Mike West, 2013-08-02 12:32:58]

Thank you again. Latest patchset merges the check into a renamed method. Let's
see what Yang has to say.

[Yang, 2013-08-05 13:42:59]

On 2013/08/02 12:32:58, Mike West wrote:
> Thank you again. Latest patchset merges the check into a renamed method. Let's
> see what Yang has to say.

LGTM.

[Mike West, 2013-08-05 13:50:50]

On 2013/08/05 13:42:59, Yang wrote:
> On 2013/08/02 12:32:58, Mike West wrote:
> > Thank you again. Latest patchset merges the check into a renamed method.
Let's
> > see what Yang has to say.
> 
> LGTM.

Great. :)

Assuming it still applies to ToT, would one of you mind landing this for me when
you have some free time?

[Yang, 2013-08-06 11:15:08]

On 2013/08/05 13:50:50, Mike West wrote:
> On 2013/08/05 13:42:59, Yang wrote:
> > On 2013/08/02 12:32:58, Mike West wrote:
> > > Thank you again. Latest patchset merges the check into a renamed method.
> Let's
> > > see what Yang has to say.
> > 
> > LGTM.
> 
> Great. :)
> 
> Assuming it still applies to ToT, would one of you mind landing this for me
when
> you have some free time?

test/mjsunit/regress/regress-2398.js fails for me. Changing
GetPropertyWithoutInvokingMonkeyGetters to %GetDataProperty fixes the issue.

[Mike West, 2013-08-06 11:46:29]

On 2013/08/06 11:15:08, Yang wrote:
> On 2013/08/05 13:50:50, Mike West wrote:
> > On 2013/08/05 13:42:59, Yang wrote:
> > > On 2013/08/02 12:32:58, Mike West wrote:
> > > > Thank you again. Latest patchset merges the check into a renamed method.
> > Let's
> > > > see what Yang has to say.
> > > 
> > > LGTM.
> > 
> > Great. :)
> > 
> > Assuming it still applies to ToT, would one of you mind landing this for me
> when
> > you have some free time?
> 
> test/mjsunit/regress/regress-2398.js fails for me. Changing
> GetPropertyWithoutInvokingMonkeyGetters to %GetDataProperty fixes the issue.

I've updated the patch, thank you.

[Yang, 2013-08-06 13:58:26]

Committed patchset #4 manually as r16075 (presubmit successful).