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. On Thu, Nov 8, 2012 at 8:46 AM, Yang Guo <[email protected]> wrote: > 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 > > > On Wed, Nov 7, 2012 at 7:21 PM, Dave Smith <[email protected]> wrote: >> >> 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. >> >> On Wed, Nov 7, 2012 at 4:05 AM, Yang Guo <[email protected]> wrote: >> > 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 >> > >> > >> > On Mon, Nov 5, 2012 at 1:27 PM, Dave Smith <[email protected]> wrote: >> >> >> >> 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. >> >> >> >> On Mon, Nov 5, 2012 at 4:37 AM, Yang Guo <[email protected]> wrote: >> >> > (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 >> >> > >> >> > >> >> > On Mon, Nov 5, 2012 at 12:36 PM, Yang Guo <[email protected]> >> >> > wrote: >> >> >> >> >> >> 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 >> >> >> >> >> >> >> >> >> On Sat, Nov 3, 2012 at 4:02 AM, Dave Smith <[email protected]> >> >> >> wrote: >> >> >>> >> >> >>> 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. >> >> >>> >> >> >>> >> >> >>> >> >> >>> >> >> >>> -- >> >> >>> v8-dev mailing list >> >> >>> [email protected] >> >> >>> http://groups.google.com/group/v8-dev >> >> >> >> >> >> >> >> > >> > >> > > > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
