[V8] Use Function.name in Error.stack

[V8] Use Function.name in Error.stack

Error.stack contains function.name if its type is string.
Otherwise if function have inferred name then .stack contains it.
For functions from eval .stack property contains "eval".

LOG=N
BUG=chromium:17356
R=yurys@chromium.org

Committed: https://crrev.com/ec73e0886059d1fd3e84ea0171b2de9dcf9bfabe
Cr-Commit-Position: refs/heads/master@{#27186}

[kozy, 2015-02-11 14:52:38]

Yury, please take a look!

[aandrey, 2015-02-11 15:47:38]

aandrey@chromium.org changed reviewers:
+ aandrey@chromium.org

[aandrey, 2015-02-11 15:47:39]

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

https://codereview.chromium.org/919653002/diff/1/src/messages.js#newcode827
src/messages.js:827: var name = GET_PRIVATE(this,
CallSiteFunctionKey).displayName;
unlike Function.name, this can throw.
you can use %GetDataProperty(..., "displayName")

[aandrey, 2015-02-11 15:50:40]

https://codereview.chromium.org/919653002/diff/1/test/mjsunit/stack-traces.js
File test/mjsunit/stack-traces.js (right):

https://codereview.chromium.org/919653002/diff/1/test/mjsunit/stack-traces.js...
test/mjsunit/stack-traces.js:101: FAIL;
nit: use { } in v8 code

[kozy, 2015-02-11 16:11:56]

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

https://codereview.chromium.org/919653002/diff/1/src/messages.js#newcode827
src/messages.js:827: var name = GET_PRIVATE(this,
CallSiteFunctionKey).displayName;
On 2015/02/11 15:47:39, aandrey wrote:
> unlike Function.name, this can throw.
> you can use %GetDataProperty(..., "displayName")

Done.

https://codereview.chromium.org/919653002/diff/1/test/mjsunit/stack-traces.js
File test/mjsunit/stack-traces.js (right):

https://codereview.chromium.org/919653002/diff/1/test/mjsunit/stack-traces.js...
test/mjsunit/stack-traces.js:101: FAIL;
On 2015/02/11 15:50:40, aandrey wrote:
> nit: use { } in v8 code

Done.

[kozy, 2015-02-12 10:17:32]

Andrey, please take a look.

[aandrey, 2015-02-12 10:20:10]

lgtm

[kozy, 2015-02-12 10:28:07]

kozyatinskiy@chromium.org changed reviewers:
+ yangguo@chromium.org

[kozy, 2015-02-12 10:28:21]

Yang, please take a look.

[yurys, 2015-02-12 12:45:37]

lgtm

[Yang, 2015-02-12 14:15:29]

I'm not sure we really should support this non-standard feature. This will
likely regress jsbench performance too.

https://codereview.chromium.org/919653002/diff/20001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/919653002/diff/20001/src/messages.js#newcode829
src/messages.js:829: if (name && IS_STRING(name)) {
I think a single IS_STRING(name) would suffice, as undefined is not a String.

Unless you want to filter out the empty string, since it evaluates to false. In
which case it would be better to explicitly do a string length comparison.

[kozy, 2015-02-12 14:57:03]

New patchsets have been uploaded after l-g-t-m from
aandrey@chromium.org,yurys@chromium.org

[kozy, 2015-02-12 14:59:57]

Our stack getter is lazy, isn't it? I suppose if user want to receive stack
trace and set for some functions displayName then we can spend one more moment
for return more useful stack trace.

https://codereview.chromium.org/919653002/diff/20001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/919653002/diff/20001/src/messages.js#newcode829
src/messages.js:829: if (name && IS_STRING(name)) {
On 2015/02/12 14:15:29, Yang wrote:
> I think a single IS_STRING(name) would suffice, as undefined is not a String.
> 
> Unless you want to filter out the empty string, since it evaluates to false.
In
> which case it would be better to explicitly do a string length comparison.

Done.

[kozy, 2015-03-06 10:05:03]

Yury, please take a look. I've uploaded new version of this patch. It uses
Function.name instead displayName.

[yurys, 2015-03-06 11:11:26]

https://codereview.chromium.org/919653002/diff/60001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/919653002/diff/60001/src/messages.js#newcode837
src/messages.js:837: if (IS_STRING(name)) {
Is it supposed to return in case of empty string?

https://codereview.chromium.org/919653002/diff/60001/test/mjsunit/stack-trace...
File test/mjsunit/stack-traces.js (right):

https://codereview.chromium.org/919653002/diff/60001/test/mjsunit/stack-trace...
test/mjsunit/stack-traces.js:108: Object.defineProperty(f, 'name', {writable:
true});
 Can you also test the case when value is passed as a configuration option?

[kozy, 2015-03-06 12:49:59]

https://codereview.chromium.org/919653002/diff/60001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/919653002/diff/60001/src/messages.js#newcode837
src/messages.js:837: if (IS_STRING(name)) {
On 2015/03/06 11:11:26, yurys wrote:
> Is it supposed to return in case of empty string?

If user set empty function name then we show this like anonymous function. I
suppose it's correct behavior.

https://codereview.chromium.org/919653002/diff/60001/test/mjsunit/stack-trace...
File test/mjsunit/stack-traces.js (right):

https://codereview.chromium.org/919653002/diff/60001/test/mjsunit/stack-trace...
test/mjsunit/stack-traces.js:108: Object.defineProperty(f, 'name', {writable:
true});
On 2015/03/06 11:11:26, yurys wrote:
>  Can you also test the case when value is passed as a configuration option?

Done.

[kozy, 2015-03-10 08:05:20]

Yury, ptal.

[yurys, 2015-03-10 09:31:24]

lgtm

[kozy, 2015-03-10 09:31:59]

Yang, ptal.

[Yang, 2015-03-10 10:48:55]

https://codereview.chromium.org/919653002/diff/80001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/919653002/diff/80001/src/messages.js#newcode836
src/messages.js:836: var name = %GetDataProperty(fun, 'name');
How about we make this part of %FunctionGetDebugName as well? So that we have
one single place where we decide on what function name to display, not two
places that act as fallback to each other.

https://codereview.chromium.org/919653002/diff/80001/test/mjsunit/stack-trace...
File test/mjsunit/stack-traces.js (right):

https://codereview.chromium.org/919653002/diff/80001/test/mjsunit/stack-trace...
test/mjsunit/stack-traces.js:289: [" at foo_0 ", " at foo_1", " at foo ", " at
boo_3 ", " at foo "]);
Do we test the inferred name code path? I thought the inferred name in this case
would be "f". You would of course have to assign an anonymous function to it in
order to not use shared->name().

[kozy, 2015-03-10 15:26:09]

Yang, ptal.

https://codereview.chromium.org/919653002/diff/80001/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/919653002/diff/80001/src/messages.js#newcode836
src/messages.js:836: var name = %GetDataProperty(fun, 'name');
On 2015/03/10 10:48:55, Yang wrote:
> How about we make this part of %FunctionGetDebugName as well? So that we have
> one single place where we decide on what function name to display, not two
> places that act as fallback to each other.

Moved to JSFunction::DebugName.

https://codereview.chromium.org/919653002/diff/80001/test/mjsunit/stack-trace...
File test/mjsunit/stack-traces.js (right):

https://codereview.chromium.org/919653002/diff/80001/test/mjsunit/stack-trace...
test/mjsunit/stack-traces.js:289: [" at foo_0 ", " at foo_1", " at foo ", " at
boo_3 ", " at foo "]);
On 2015/03/10 10:48:55, Yang wrote:
> Do we test the inferred name code path? I thought the inferred name in this
case
> would be "f". You would of course have to assign an anonymous function to it
in
> order to not use shared->name().

Test added.

https://codereview.chromium.org/919653002/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/919653002/diff/120001/src/objects.cc#newcode1...
src/objects.cc:10258: HandleScope scope(GetIsolate());
Without this handle scope some tests fails with
# Fatal error in v8::HandleScope::CreateHandle()
# Cannot create a handle without a HandleScope
It seems strange for me, because we can't call this function without outer scope
in messages.js or CaptureStackTraceHelper.
Yang, please take a look on this line.

[kozy, 2015-03-13 13:09:54]

New version uploaded.
Yang, please take a look.

[Yang, 2015-03-13 13:19:13]

Almost there.

https://codereview.chromium.org/919653002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/919653002/diff/140001/src/objects.cc#newcode1...
src/objects.cc:10330: String* JSFunction::DebugName() {
This is not GC-safe. GetDataProperty may cause GC, at which point the JSFunction
itself (this) may be moved and becomes invalid. Calling shared() would then
crash.

Please turn all of this into handlified code with this signature:

(static) Handle<String> JSFunction::DebugName(Handle<JSFunction>).

[kozy, 2015-03-13 14:05:14]

https://codereview.chromium.org/919653002/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/919653002/diff/140001/src/objects.cc#newcode1...
src/objects.cc:10330: String* JSFunction::DebugName() {
On 2015/03/13 13:19:13, Yang wrote:
> This is not GC-safe. GetDataProperty may cause GC, at which point the
JSFunction
> itself (this) may be moved and becomes invalid. Calling shared() would then
> crash.
> 
> Please turn all of this into handlified code with this signature:
> 
> (static) Handle<String> JSFunction::DebugName(Handle<JSFunction>).

Done.

[yurys, 2015-03-13 14:18:21]

https://codereview.chromium.org/919653002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/919653002/diff/160001/src/objects.cc#newcode1...
src/objects.cc:10334: String* debug_name = NULL;
You should avoid raw pointers in methods that may cause GC. In this case it
should be Handle<String> or event better you can avoid this local variable
altogether:

if (name->IsString())
  return Handle<String>::cast(name);

[kozy, 2015-03-13 14:54:47]

Yang, please take a look.

https://codereview.chromium.org/919653002/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/919653002/diff/160001/src/objects.cc#newcode1...
src/objects.cc:10334: String* debug_name = NULL;
On 2015/03/13 14:18:21, yurys wrote:
> You should avoid raw pointers in methods that may cause GC. In this case it
> should be Handle<String> or event better you can avoid this local variable
> altogether:
> 
> if (name->IsString())
>   return Handle<String>::cast(name);

Done.

[Yang, 2015-03-13 15:09:00]

On 2015/03/13 14:54:47, kozyatinskiy wrote:
> Yang, please take a look.
> 
> https://codereview.chromium.org/919653002/diff/160001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/919653002/diff/160001/src/objects.cc#newcode1...
> src/objects.cc:10334: String* debug_name = NULL;
> On 2015/03/13 14:18:21, yurys wrote:
> > You should avoid raw pointers in methods that may cause GC. In this case it
> > should be Handle<String> or event better you can avoid this local variable
> > altogether:
> > 
> > if (name->IsString())
> >   return Handle<String>::cast(name);
> 
> Done.

lgtm.

[kozy, 2015-03-13 15:10:15]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2015-03-13 15:10:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
aandrey@chromium.org, yurys@chromium.org
Link to the patchset: https://codereview.chromium.org/919653002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-03-13 15:10:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/919653002/180001

[commit-bot: I haz the power, 2015-03-13 15:11:52]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-13 15:12:05]

Patchset 10 (id:??) landed as
https://crrev.com/ec73e0886059d1fd3e84ea0171b2de9dcf9bfabe
Cr-Commit-Position: refs/heads/master@{#27186}