Rewrite Error.prototype.toString in C++.

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

Committed: https://crrev.com/2e2765a6ebfdec83188d423c24a786c96c5fa6ed
Cr-Commit-Position: refs/heads/master@{#30105}

[Yang, 2015-08-07 06:47:43]

yangguo@chromium.org changed reviewers:
+ erights@gmail.com

[Yang, 2015-08-07 06:48:12]

On 2015/08/07 06:47:43, Yang wrote:
> mailto:yangguo@chromium.org changed reviewers:
> + mailto:erights@gmail.com

Mark, could you take a look at whether the change to the fix for crbug/69187
makes sense?

[Jakob Kummerow, 2015-08-08 21:56:09]

Looks good. I have a couple of nits, and one high-level comment:

IIUC, this doesn't distinguish between getters and data properties. So if you
had this:
  var error = /* some internally thrown error */
  delete error.message;
  error.__proto__ = {message: "My message"};
  error.toString();
then you'd get the empty default message instead of "My message". Is that
intentional? I guess it's in line with what the previous code did, so it's
probably OK (but a bit surprising).

https://codereview.chromium.org/1281833002/diff/2/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1281833002/diff/2/src/messages.cc#newcode384
src/messages.cc:384: isolate->factory()->NewStringFromStaticChars("name");
we already have isolate->factory()->name_string() ;-)

https://codereview.chromium.org/1281833002/diff/2/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode474
src/messages.h:474: for (const auto& visited : helper->visited_) {
nit: I'd prefer the explicitness of s/auto/Handle<JSObject>/

https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode483
src/messages.h:483: bool HasVisited() { return has_visited_; }
nit: unix_hacker_style() for simple getters

https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode490
src/messages.h:490: 
nit: one empty line is enough here

https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode491
src/messages.h:491: bool ShadowsInternalError(Isolate* isolate, LookupIterator*
property_lookup,
this could be declared static, right?

https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode494
src/messages.h:494: MUST_USE_RESULT MaybeHandle<String> GetStringifiedProperty(
this could be declared static too, right?

[Yang, 2015-08-10 09:04:05]

On 2015/08/08 21:56:09, Jakob wrote:
> Looks good. I have a couple of nits, and one high-level comment:
> 
> IIUC, this doesn't distinguish between getters and data properties. So if you
> had this:
>   var error = /* some internally thrown error */
>   delete error.message;
>   error.__proto__ = {message: "My message"};
>   error.toString();
> then you'd get the empty default message instead of "My message". Is that
> intentional? I guess it's in line with what the previous code did, so it's
> probably OK (but a bit surprising).

This is intentional. I agree that this may be surprising. Then again this little
detail never bothered anyone and I'd rather not change this behavior.

> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.cc
> File src/messages.cc (right):
> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.cc#newcode384
> src/messages.cc:384: isolate->factory()->NewStringFromStaticChars("name");
> we already have isolate->factory()->name_string() ;-)
> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.h
> File src/messages.h (right):
> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode474
> src/messages.h:474: for (const auto& visited : helper->visited_) {
> nit: I'd prefer the explicitness of s/auto/Handle<JSObject>/
> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode483
> src/messages.h:483: bool HasVisited() { return has_visited_; }
> nit: unix_hacker_style() for simple getters
> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode490
> src/messages.h:490: 
> nit: one empty line is enough here
> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode491
> src/messages.h:491: bool ShadowsInternalError(Isolate* isolate,
LookupIterator*
> property_lookup,
> this could be declared static, right?
> 
> https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode494
> src/messages.h:494: MUST_USE_RESULT MaybeHandle<String>
GetStringifiedProperty(
> this could be declared static too, right?

[Yang, 2015-08-10 09:04:24]

On 2015/08/10 09:04:05, Yang wrote:
> On 2015/08/08 21:56:09, Jakob wrote:
> > Looks good. I have a couple of nits, and one high-level comment:
> > 
> > IIUC, this doesn't distinguish between getters and data properties. So if
you
> > had this:
> >   var error = /* some internally thrown error */
> >   delete error.message;
> >   error.__proto__ = {message: "My message"};
> >   error.toString();
> > then you'd get the empty default message instead of "My message". Is that
> > intentional? I guess it's in line with what the previous code did, so it's
> > probably OK (but a bit surprising).
> 
> This is intentional. I agree that this may be surprising. Then again this
little
> detail never bothered anyone and I'd rather not change this behavior.
> 
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.cc
> > File src/messages.cc (right):
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.cc#newcode384
> > src/messages.cc:384: isolate->factory()->NewStringFromStaticChars("name");
> > we already have isolate->factory()->name_string() ;-)
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.h
> > File src/messages.h (right):
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode474
> > src/messages.h:474: for (const auto& visited : helper->visited_) {
> > nit: I'd prefer the explicitness of s/auto/Handle<JSObject>/
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode483
> > src/messages.h:483: bool HasVisited() { return has_visited_; }
> > nit: unix_hacker_style() for simple getters
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode490
> > src/messages.h:490: 
> > nit: one empty line is enough here
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode491
> > src/messages.h:491: bool ShadowsInternalError(Isolate* isolate,
> LookupIterator*
> > property_lookup,
> > this could be declared static, right?
> > 
> > https://codereview.chromium.org/1281833002/diff/2/src/messages.h#newcode494
> > src/messages.h:494: MUST_USE_RESULT MaybeHandle<String>
> GetStringifiedProperty(
> > this could be declared static too, right?

I addressed the rest of the comments btw.

[Jakob Kummerow, 2015-08-10 11:07:28]

LGTM with a nit.

https://codereview.chromium.org/1281833002/diff/20001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1281833002/diff/20001/src/messages.cc#newcode396
src/messages.cc:396: // internally created error object, use that messingage
property. Otherwise
nit: typo

[Yang, 2015-08-11 08:22:04]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-08-11 08:22:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1281833002/#ps40001
(title: "fix comment typo")

[commit-bot: I haz the power, 2015-08-11 08:22:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281833002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281833002/40001

[commit-bot: I haz the power, 2015-08-11 08:32:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-11 08:32:35]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8552)

[Yang, 2015-08-11 08:52:36]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-08-11 08:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1281833002/#ps60001
(title: "fix test case")

[commit-bot: I haz the power, 2015-08-11 08:52:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281833002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281833002/60001

[commit-bot: I haz the power, 2015-08-11 09:15:34]

Committed patchset #5 (id:60001)

[commit-bot: I haz the power, 2015-08-11 09:15:48]

Patchset 5 (id:??) landed as
https://crrev.com/2e2765a6ebfdec83188d423c24a786c96c5fa6ed
Cr-Commit-Position: refs/heads/master@{#30105}