Bug 66994 – Implement Error.stack

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Juan C. Montemayor]

Continuation of Bug 66571 - Keep track of topCallFrame for Stack traces
https://bugs.webkit.org/show_bug.cgi?id=66571

This is the second patch of two to implement stack traces in jsc. This patch utilizes topCallFrame to create a stack trace when an error is thrown. Users will also be able to use the stack() command in jsc to get stack traces.

[Juan C. Montemayor]

Created attachment 105313 [details]
proposed patch

[Juan C. Montemayor]

Created attachment 105314 [details]
sunspider test results showing no measurable speed regression

[WebKit Review Bot]

Comment on attachment 105313 [details]
proposed patch

Attachment 105313 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9508831

New failing tests:
fast/js/stack-trace.html

[Juan C. Montemayor]

Created attachment 105322 [details]
patch

why don't I get the results of the chromium bot? Oh well.. This patch will attempt to pass all tests...

[WebKit Review Bot]

Comment on attachment 105322 [details]
patch

Attachment 105322 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9512846

New failing tests:
fast/js/stack-trace.html

[Oliver Hunt]

(In reply to comment #4)
> Created an attachment (id=105322) [details]
> patch
> 
> why don't I get the results of the chromium bot? Oh well.. This patch will attempt to pass all tests...

I would just add the test to the chrome skip list, it's probably the easiest way.

[Juan C. Montemayor]

Created attachment 105364 [details]
patch w win and chromium fixes

[Oliver Hunt]

Comment on attachment 105364 [details]
patch w win and chromium fixes

Please return an array rather than a string.  There are plenty of places where having programatic knowledge of the stack trace would be useful (say the inspector), by creating a single string you're essentially requiring users to manually parse the string you just built up back into an array.

[Oliver Hunt]

Comment on attachment 105364 [details]
patch w win and chromium fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=105364&action=review

Have included a number of suggestions and questions

> Source/JavaScriptCore/interpreter/Interpreter.cpp:756
> +const UString Interpreter::getStackTrace(JSGlobalData* globalData, int line)

void Interpreter::getStackTrace(JSGlobalData& globalData, int line, Vector<TraceInfo>& result)

> Source/JavaScriptCore/interpreter/Interpreter.cpp:768
> +        while (callFrame && callFrame != CallFrame::noCaller() && !callFrame->codeBlock())
> +            callFrame = callFrame->callerFrame()->removeHostCallFrameFlag();

This has the effect of removing native calls from the stack -- we may want to show these, but currently it's not necessary

> Source/JavaScriptCore/interpreter/Interpreter.cpp:775
> +        trace += getTraceLine(callFrame, callFrame->codeBlock()->codeType(), sourceURL, line, i);

result.append(TraceInfo(globalData, callFrame->codeBlock()->ownerExecutable(), line));

> Source/JavaScriptCore/interpreter/Interpreter.cpp:777
> +        getCallerLine(globalData, callFrame, line);

If you're called by a host function, this will not update line, so your next iteration may produce a bogus line number, what happens if I do:
> var o = {}; o.toString = function() {  throw {}; }
function () {  throw {}; }
> print(o)

> Source/JavaScriptCore/interpreter/Interpreter.cpp:802
> +            UString stackTrace = getStackTrace(globalData, codeBlock->lineNumberForBytecodeOffset(bytecodeOffset));

Vector<TraceInfo> trace;
getStackTrace(*globalData, codeBlock->lineNumberForBytecodeOffset(bytecodeOffset), trace);
addErrorInfo(callFrame, exception, codeBlock->lineNumberForBytecodeOffset(bytecodeOffset), codeBlock->ownerExecutable()->source(), stackTrace);

> Source/JavaScriptCore/interpreter/Interpreter.h:131
> +        static const UString getStackTrace(JSGlobalData*, int);

I don't like this string return, i think you're better doing:

struct TraceInfo {
    Strong<Executable> code;
    int lineNumber;
}

static void getStackTrace(JSGlobalData&, Vector<TraceInfo>& result);

> Source/JavaScriptCore/runtime/Error.cpp:119
> +JSObject* addErrorInfo(JSGlobalData* globalData, JSObject* error, int line, const SourceCode& source, const UString& stackTrace)

JSObject* addErrorInfo(JSGlobalData* globalData, JSObject* error, int line, const SourceCode& source, const Vector<TraceInfo>& stackTrace)

> Source/JavaScriptCore/runtime/Error.cpp:131
> +        error->putWithAttributes(globalData, globalData->propertyNames->stack, jsString(globalData, stackTrace), ReadOnly | DontDelete);

JSArray* stackTrace = JSArray::createEmpty(*globalData, globalObject);
for (unsigned i = 0; i < stackTrace.size(); i++) { JSString* string = jsString(globalData, formatTraceLine(stackTrace[i])); stackTrace->push(*globalData, string); }
error->putWithAttributes(globalData, globalData->propertyNames->stack, stackTrace, ReadOnly | DontDelete);

[Juan C. Montemayor]

> > Source/JavaScriptCore/interpreter/Interpreter.cpp:777
> > +        getCallerLine(globalData, callFrame, line);
> 
> If you're called by a host function, this will not update line, so your next iteration may produce a bogus line number

If a line number is not available for the callFrame, then a line number will not be printed out.

[Juan C. Montemayor]

Created attachment 105417 [details]
patch with changes

[Oliver Hunt]

Comment on attachment 105417 [details]
patch with changes

View in context: https://bugs.webkit.org/attachment.cgi?id=105417&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:742
> +static void updateSourceURL(CallFrame* callFrame, UString& sourceURL) 

Just return the sourceURL -- const UString& is your friend.

Need to check callframe for host flag

> Source/JavaScriptCore/interpreter/Interpreter.cpp:767
> +        while (callFrame && callFrame != CallFrame::noCaller() && !callFrame->codeBlock())
> +            callFrame = callFrame->callerFrame()->removeHostCallFrameFlag();

Remove these lines

> Source/JavaScriptCore/interpreter/Interpreter.cpp:774
> +        traceLevel = getTraceLine(callFrame, callFrame->codeBlock()->codeType(), sourceURL, line).impl();

remove this as it won't be necessary

> Source/JavaScriptCore/interpreter/Interpreter.cpp:776
> +        StackTraceLevel s = {Strong<ScriptExecutable>(*globalData, callFrame->codeBlock()->ownerExecutable()), line, callFrame->codeBlock()->codeType(), sourceURL, traceLevel};

Strong<ScriptExecutable> -> Strong<Executable>

Make a helper function to return the correct value for your "code type" enum from the callFrame, eg.
if (callFrame->hasHostCa...) return StackFrameTypeNative;
switch (callFrame->codeBlock()->codeType()) {
   case Blah: return StackFrameTypeBlah;
   ...
}

> Source/JavaScriptCore/interpreter/Interpreter.h:66
> +    struct StackTraceLevel {

rename this to StackFrame

> Source/JavaScriptCore/interpreter/Interpreter.h:67
> +        Strong<ScriptExecutable> executable;

Make this an ordinary Strong<Executable>
and add
Strong<JSObject> callee

> Source/JavaScriptCore/interpreter/Interpreter.h:69
> +        int codeType;

Make your own enum here

> Source/JavaScriptCore/interpreter/Interpreter.h:71
> +        UString stackLevelString;

I don't think this is necessary -- the Vector<> implies the level you're at

> Source/JavaScriptCore/interpreter/Interpreter.h:140
> +        static const UString getStackTrace(JSGlobalData*, int);

die!!!!!1111!!!!1111one!

Might be worth moving the prettyprint function to StackFrame

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:64
> +    macro(stack) \

change this to jscStack

> Source/JavaScriptCore/runtime/Error.cpp:134
> +            JSString* string = jsString(globalData, stackTrace[i].stackLevelString); 

stackTrace[i].toString() or whatever

[Juan C. Montemayor]

Created attachment 107128 [details]
Patch

[Sam Weinig]

It seems like the current implementation will potentially leak function names cross origin.  This should probably be avoided.

[Juan C. Montemayor]

Created attachment 107169 [details]
proposed patch that will apply

Sam, I'm going to try and find you on #webkit so we can talk about this

[Early Warning System Bot]

Comment on attachment 107169 [details]
proposed patch that will apply

Attachment 107169 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9661100

[Juan C. Montemayor]

Created attachment 107174 [details]
patch

This will hopefully fix the build errors in qt et al.

[Juan C. Montemayor]

Created attachment 107215 [details]
fixed patch for win-bot

[Oliver Hunt]

(In reply to comment #14)
> It seems like the current implementation will potentially leak function names cross origin.  This should probably be avoided.

How does this compare to the leak from walking the call stack with function.caller?  Afaict if you were able to call a function (directly or indirectly) you must have some origin permissions.  Should we maybe restrict .caller if the global objects don't match?  It seems sufficiently edge case to probably be safe?

That said I did just realize something more significant that the origin bypass -> isolated worlds aren't sufficiently isolated: they share a global data, so a change that triggers synchronous JS by code in an isolated world (changing text or something with a world containing a change handler in an isolated world).  If such case should be created it may be possible to create a stack trace that leaked the presence of a particular extension, etc.

[Sam Weinig]

(In reply to comment #19)
> (In reply to comment #14)
> > It seems like the current implementation will potentially leak function names cross origin.  This should probably be avoided.
> 
> How does this compare to the leak from walking the call stack with function.caller?  Afaict if you were able to call a function (directly or indirectly) you must have some origin permissions.  Should we maybe restrict .caller if the global objects don't match?  It seems sufficiently edge case to probably be safe?

I think you are right, I retract my comment/objection.

[Oliver Hunt]

Comment on attachment 107215 [details]
fixed patch for win-bot

View in context: https://bugs.webkit.org/attachment.cgi?id=107215&action=review

r- due to the problem with printStack, but that's it.

> LayoutTests/fast/js/stack-trace-expected.txt:9
> +    0   normalInner at file:///Users/jmont/webkit/OpenSource/LayoutTests/fast/js/script-tests/stack-trace.js:21
> +    1   normalOuter at file:///Users/jmont/webkit/OpenSource/LayoutTests/fast/js/script-tests/stack-trace.js:20
> +    2   at file:///Users/jmont/webkit/OpenSource/LayoutTests/fast/js/script-tests/stack-trace.js:26

These will lead to sadness

> LayoutTests/fast/js/script-tests/stack-trace.js:9
> +		debug("    " + i + "   " + stackTrace[level]);

I think you want to do stackTrace[level].replace(location, "") here to make these tests happier

[Juan C. Montemayor]

Created attachment 107446 [details]
updated patch

[Juan C. Montemayor]

(In reply to comment #21)
> These will lead to sadness

good catch. I fixed these with the new patch.

Also, although I am fairly certain this patch will work with the win-bot, I never confirmed this since the win-bot has been backed up lately.

[Oliver Hunt]

Comment on attachment 107446 [details]
updated patch

r=me, sorry i missed the updated patch going up

[WebKit Review Bot]

Comment on attachment 107446 [details]
updated patch

Rejecting attachment 107446 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
file LayoutTests/fast/js/stack-trace.html
patching file LayoutTests/fast/js/script-tests/exception-properties.js
patching file LayoutTests/fast/js/script-tests/stack-trace.js
patching file LayoutTests/platform/chromium/test_expectations.txt
Hunk #1 FAILED at 477.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9839128

[Juan C. Montemayor]

(In reply to comment #25)
> (From update of attachment 107446 [details])
> Rejecting attachment 107446 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> file LayoutTests/fast/js/stack-trace.html
> patching file LayoutTests/fast/js/script-tests/exception-properties.js
> patching file LayoutTests/fast/js/script-tests/stack-trace.js
> patching file LayoutTests/platform/chromium/test_expectations.txt
> Hunk #1 FAILED at 477.
> 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1
> 
> Full output: http://queues.webkit.org/results/9839128

Seems like a lot of time passed since the patch upload till now, hehe. I'll upload an updated patch later tonight.

[Juan C. Montemayor]

Created attachment 108847 [details]
updated patch

updated so that patch will succeed. Oliver, I'll send you an email this time to remind you to r+

[Juan C. Montemayor]

Created attachment 108854 [details]
fixing win bot

please disregard this patch, i want to see what win-bot tells me it needs.

[Oliver Hunt]

Comment on attachment 108854 [details]
fixing win bot

Looks like your patch has lost the changes to JavaScriptCore.def

[Juan C. Montemayor]

(In reply to comment #29)
> (From update of attachment 108854 [details])
> Looks like your patch has lost the changes to JavaScriptCore.def

Yes, I don't have a windows machine to test on, so I want the bot to tell me what it wants in JavaScriptCore.def

[Juan C. Montemayor]

(In reply to comment #30)
> (In reply to comment #29)
> > (From update of attachment 108854 [details] [details])
> > Looks like your patch has lost the changes to JavaScriptCore.def
> 
> Yes, I don't have a windows machine to test on, so I want the bot to tell me what it wants in JavaScriptCore.def

Is there a better way to to this?

[Oliver Hunt]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > (From update of attachment 108854 [details] [details] [details])
> > > Looks like your patch has lost the changes to JavaScriptCore.def
> > 
> > Yes, I don't have a windows machine to test on, so I want the bot to tell me what it wants in JavaScriptCore.def
> 
> Is there a better way to to this?

Historically my "better way" has been to wish for a faster EWS bot :D

[Juan C. Montemayor]

Created attachment 108861 [details]
patch

this should be it

[WebKit Review Bot]

Comment on attachment 108861 [details]
patch

Clearing flags on attachment: 108861

Committed r96131: <http://trac.webkit.org/changeset/96131>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

Yay!

[Csaba Osztrogonác]

(In reply to comment #34)
> (From update of attachment 108861 [details])
> Clearing flags on attachment: 108861
> 
> Committed r96131: <http://trac.webkit.org/changeset/96131>

It made many tests crash on all platform:
GTK: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/17784
Qt: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/37937
Leopard: 
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/33790
...

[Csaba Osztrogonác]

Rolled out by http://trac.webkit.org/changeset/96146

[Csaba Osztrogonác]

(In reply to comment #38)
> Rolled out by http://trac.webkit.org/changeset/96146

It seems the problem occured only on 32 bit platoforms.

[Juan C. Montemayor]

(In reply to comment #39)
> (In reply to comment #38)
> > Rolled out by http://trac.webkit.org/changeset/96146
> 
> It seems the problem occured only on 32 bit platoforms.

Will investigate and roll out a patch (hopefully later today).

[Oliver Hunt]

Created attachment 124830 [details]
Patch

[Oliver Hunt]

Committed r106407

[Csaba Osztrogonác]

(In reply to comment #42)
> Committed r106407

Reopen, because it made many test crash on 32 bit platforms again:
- Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r106434%20%2842748%29/results.html
- GTK: http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r106432%20%2821296%29/results.html

Could you fix it?

[Csaba Osztrogonác]

I tried to ping you on IRC and here, but it's night now.

I rolled it out again, because crash is always serious bug: http://trac.webkit.org/changeset/106454 (I had to solve conflicts with http://trac.webkit.org/changeset/106414 and http://trac.webkit.org/changeset/106433 manually.)

[Oliver Hunt]

Created attachment 127437 [details]
Patch

[Gavin Barraclough]

Comment on attachment 127437 [details]
Patch

Stack tracing should not really be in interpreter.cpp, but of course nor should the rest of the exception handling code.  Really should move this out some time.

[Oliver Hunt]

Committed r107980: <http://trac.webkit.org/changeset/107980>

[Eric Seidel (no email)]

Seems like a feature possibly worth blogging about. :)

[Erik Arvidsson]

Sorry for being late to the party.

What other engine does this match?

I agree that having a structure is better than a formatted string but having an array of strings does not seem good to me. Either go all the way where the array contains objects with lineNumber, url etc or stick with what other browsers do, which is a single formatted string.

[Oliver Hunt]

(In reply to comment #49)
> Sorry for being late to the party.
> 
> What other engine does this match?
> 
> I agree that having a structure is better than a formatted string but having an array of strings does not seem good to me. Either go all the way where the array contains objects with lineNumber, url etc or stick with what other browsers do, which is a single formatted string.

The other engines don't agree on formatting of the string, so matching other browsers isn't something that is possible.  I'm unaware of any compatibility concerns involving stack, and implicit toString on an array will produce something perfectly sensible, so i'm not sure what there is to gain from changing this.

[Erik Arvidsson]

(In reply to comment #50)
> (In reply to comment #49)
> > Sorry for being late to the party.
> > 
> > What other engine does this match?
> > 
> > I agree that having a structure is better than a formatted string but having an array of strings does not seem good to me. Either go all the way where the array contains objects with lineNumber, url etc or stick with what other browsers do, which is a single formatted string.
> 
> The other engines don't agree on formatting of the string, so matching other browsers isn't something that is possible.  I'm unaware of any compatibility concerns involving stack, and implicit toString on an array will produce something perfectly sensible, so i'm not sure what there is to gain from changing this.

What is the benefit of the Array if the individual items have to be parsed anyway?

Matching V8 would at least have simplified the tests and 4 different formats are better than 5.

[Oliver Hunt]

(In reply to comment #51)
> (In reply to comment #50)
> > (In reply to comment #49)
> > > Sorry for being late to the party.
> > > 
> > > What other engine does this match?
> > > 
> > > I agree that having a structure is better than a formatted string but having an array of strings does not seem good to me. Either go all the way where the array contains objects with lineNumber, url etc or stick with what other browsers do, which is a single formatted string.
> > 
> > The other engines don't agree on formatting of the string, so matching other browsers isn't something that is possible.  I'm unaware of any compatibility concerns involving stack, and implicit toString on an array will produce something perfectly sensible, so i'm not sure what there is to gain from changing this.
> 
> What is the benefit of the Array if the individual items have to be parsed anyway?
> 
> Matching V8 would at least have simplified the tests and 4 different formats are better than 5.

Step one in every use of single string stack traces that i've seen is a split() to get an array of strings, given this is purely a debug feature doing this ahead of time seems sensible.  I would move to an array of custom descriptor objects first rather than a single string if it weren't for the need to then implement a bunch of toString methods on those objects.  I'm kind of leaning in that way anyway (as it would make built-in vs. js functions vs. program code vs. eval code easier to distinguish)

[Csaba Osztrogonác]

(In reply to comment #47)
> Committed r107980: <http://trac.webkit.org/changeset/107980>

It broke 32 bit platforms again and again. :(( And you left the tree broken of course ... It isn't a fairplay thing. Please don't break other platforms again and again and leave them broken deliberately.

I tried to ping you on IRC again when I noticed ... but you didn't answer, so I had to rollout the patch again - http://trac.webkit.org/changeset/108036
(Otherwise you should have watch the bots and do the rollout yourself.)

I can understand if you can't test on a 32 bit machine. But why don't you ask for a help to test your patch before landing? I willingly help you in testing if you ask me. Maybe Zoltán or Gábor can help you to debug this bug on 32 bit Qt platform.

[Oliver Hunt]

(In reply to comment #53)
> (In reply to comment #47)
> > Committed r107980: <http://trac.webkit.org/changeset/107980>
> 
> It broke 32 bit platforms again and again. :(( And you left the tree broken of course ... It isn't a fairplay thing. Please don't break other platforms again and again and leave them broken deliberately.
> 
> I tried to ping you on IRC again when I noticed ... but you didn't answer, so I had to rollout the patch again - http://trac.webkit.org/changeset/108036
> (Otherwise you should have watch the bots and do the rollout yourself.)
> 
> I can understand if you can't test on a 32 bit machine. But why don't you ask for a help to test your patch before landing? I willingly help you in testing if you ask me. Maybe Zoltán or Gábor can help you to debug this bug on 32 bit Qt platform.

I tested on 32 bit, and I waited for the qt bots to go green.

If the qt bots going green != the qt bots working then there's a problem.

[Oliver Hunt]

Committed r108112: <http://trac.webkit.org/changeset/108112>

[Gavin Barraclough]

*** Bug 13646 has been marked as a duplicate of this bug. ***