Better reporting of call stack overflows

[Dave Smith]

Hi,

I've been using node.js (and hence, v8) for a little bit and one thing that has driven me nuts is the lack of reporting when the call stack overflows. So, in keeping with open source tradition, I've knocked together a (simplistic) patch to fix this specific problem. My patch is here:

https://github.com/dizzyd/node/commit/40434019540ffc17e984ff0653500a3c5db87deb.patch

(note that it's against the fork of the node codebase, which embeds v8 directly).

My lack of familiarity with v8 is probably grossly evident, but perhaps someone could provide some direction on improvements if this is an itch bothering other people. :)

Thanks!

D.

[Yang Guo]

Hi Dave,

I wasn't aware of this problem before. We currently already have redundant ways to capture stack trace (at throw site and at creation site of the Error object). I wanted to consolidate those 

D.

--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev

[Yang Guo]

(previously sent without actually finishing the mail...)

Hi Dave,

I wasn't aware of this problem before. We currently already have redundant ways to capture stack trace (at throw site and at creation site of the Error object). I've been wanting to consolidate this, which would also enable me to solve this issue.

Your current approach has some subtle problems. If you have some patience, as I will be working on this issue, it will be solved eventually. I'll file a bug for this.

Yang

[Dave Smith]

Hi Yang,

Thanks for the feedback. If you have the time/inclination to describe
(roughly) the sort of patch you'd like to see and the subtle problems,
I'd be happy to revise my patch and try to help move things forward.

Thanks,

d.

[Yang Guo]

Hi Dave,

I completed a patch fixing this problem. It's currently under review: http://codereview.chromium.org/11275186/ 

You are welcome to point out any short-comings in this patch.

The problem with your approach is that it reimplements the stack trace formatting in C++ while we already have it in javascript. It is also not compatible with our stack trace API.

Regards,

Yang

[Dave Smith]

Hi Yang,

Thanks for the update. I've applied the patch against node 0.8.14's
embedded version of V8 (which is, I think, 3.11.10.25) -- the patch
applied mostly cleanly except one reference to "global_object()" in
isolate.cc, which seems to be just a new name for "global()".

For my simplest test case, the patch definitely helps. However for the
production test case, I'm still not seeing any stack trace. A bit of
digging reveals that it's exiting here:

+ Handle<Object> error = GetProperty(global(), "Error");
+ if (!error->IsJSObject()) return Failure::Exception();

So it seems like there are still situations where no stack trace will
be provided -- can you elaborate on those a bit, please?

Thanks,

D.

[Yang Guo]

Hi Dave,

I think the problem is that you are overwriting the Error object somewhere in your production code. The code you found tries to find the stack trace size limit from Error.stackTraceLimit. If it fails at this, no stack trace is captured, mimicking the behavior captureStackTrace in messages.js.

I uploaded a new revision to fix this. Please check whether it solves your problem.

Yang

[Dave Smith]

Hi Yang,

Yup -- version 3 of the patch works just fine on the production test
case. Thanks so much for taking the time to get this right. Now I just
gotta convince the node maintainers to merge the patch. :)

Thanks again!

D.

[Yang Guo]

Hi Dave,

this patch has not even been landed yet, and is currently blocked by another issue that is also under review. I assume that this won't take long. However, the policy is that we don't back port things that are not bug fixes to older branches. Given that this is a new feature, I'm positive that we won't back port this patch, much less to 3.11 for which we don't receive any real-world crash data anymore. I think your options are:

- wait for things to take its usual course, this patch will eventually end up in node

- compile your own node with a newer V8 once this patch has landed.

I hope one of the two is viable for you.

Regards,

Yang

On Thu, Nov 8, 2012 at 7:51 PM, Dave Smith <da...@singly.com> wrote:

Hi Yang,

The node maintainers asked me to see about getting it backported to
3.11.10 -- apparently that's what they want to use. If I backport your
patch to that tag (?) do you think the team might include it?

Thanks again for all your help. This really does make things a lot
better for people using V8 server-side. I've only been using node a
few weeks and was stupefied when I saw that people were ok not having
stack traces. :)

D.

On Thu, Nov 8, 2012 at 11:43 AM, Yang Guo <yan...@chromium.org> wrote:
> Hi Dave,
>

> I assume that this patch will end up in V8 3.15 eventually, so it would just
> be a matter of time until node gets it as well (depending on when they
> update their V8 version).
>
> Yang