Bug 66994 - Implement Error.stack
: Implement Error.stack
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Oliver Hunt
:
Depends on: 67010 68927 79029
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-25 15:51 PDT by Juan C. Montemayor
Modified: 2012-03-07 18:12 PST (History)
12 users (show)

See Also:


Attachments
proposed patch (21.56 KB, patch)
2011-08-25 22:38 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
sunspider test results showing no measurable speed regression (3.41 KB, text/plain)
2011-08-25 22:39 PDT, Juan C. Montemayor
no flags Details
patch (22.90 KB, patch)
2011-08-26 00:40 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
patch w win and chromium fixes (23.99 KB, patch)
2011-08-26 09:39 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
patch with changes (26.29 KB, patch)
2011-08-26 15:55 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
Patch (26.74 KB, patch)
2011-09-12 19:14 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
proposed patch that will apply (26.81 KB, patch)
2011-09-13 06:37 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
patch (26.92 KB, patch)
2011-09-13 07:18 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
fixed patch for win-bot (26.87 KB, patch)
2011-09-13 13:00 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
updated patch (25.40 KB, patch)
2011-09-14 20:31 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
updated patch (25.03 KB, patch)
2011-09-27 07:42 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
fixing win bot (24.17 KB, patch)
2011-09-27 08:59 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
patch (25.06 KB, patch)
2011-09-27 09:58 PDT, Juan C. Montemayor
no flags Details | Formatted Diff | Diff
Patch (40.96 KB, patch)
2012-01-31 15:02 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (44.72 KB, patch)
2012-02-16 13:57 PST, Oliver Hunt
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Juan C. Montemayor 2011-08-25 15:51:28 PDT
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.
Comment 1 Juan C. Montemayor 2011-08-25 22:38:12 PDT
Created attachment 105313 [details]
proposed patch
Comment 2 Juan C. Montemayor 2011-08-25 22:39:27 PDT
Created attachment 105314 [details]
sunspider test results showing no measurable speed regression
Comment 3 WebKit Review Bot 2011-08-25 23:11:16 PDT
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
Comment 4 Juan C. Montemayor 2011-08-26 00:40:02 PDT
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...
Comment 5 WebKit Review Bot 2011-08-26 01:02:30 PDT
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
Comment 6 Oliver Hunt 2011-08-26 07:57:11 PDT
(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.
Comment 7 Juan C. Montemayor 2011-08-26 09:39:40 PDT
Created attachment 105364 [details]
patch w win and chromium fixes
Comment 8 Oliver Hunt 2011-08-26 10:41:08 PDT
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.
Comment 9 Oliver Hunt 2011-08-26 11:13:51 PDT
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);
Comment 10 Juan C. Montemayor 2011-08-26 15:50:30 PDT
> > 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.
Comment 11 Juan C. Montemayor 2011-08-26 15:55:02 PDT
Created attachment 105417 [details]
patch with changes
Comment 12 Oliver Hunt 2011-08-26 16:44:39 PDT
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
Comment 13 Juan C. Montemayor 2011-09-12 19:14:43 PDT
Created attachment 107128 [details]
Patch
Comment 14 Sam Weinig 2011-09-12 21:28:51 PDT
It seems like the current implementation will potentially leak function names cross origin.  This should probably be avoided.
Comment 15 Juan C. Montemayor 2011-09-13 06:37:41 PDT
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
Comment 16 Early Warning System Bot 2011-09-13 06:53:11 PDT
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
Comment 17 Juan C. Montemayor 2011-09-13 07:18:44 PDT
Created attachment 107174 [details]
patch

This will hopefully fix the build errors in qt et al.
Comment 18 Juan C. Montemayor 2011-09-13 13:00:01 PDT
Created attachment 107215 [details]
fixed patch for win-bot
Comment 19 Oliver Hunt 2011-09-14 10:51:40 PDT
(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.
Comment 20 Sam Weinig 2011-09-14 14:18:51 PDT
(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.
Comment 21 Oliver Hunt 2011-09-14 15:10:58 PDT
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
Comment 22 Juan C. Montemayor 2011-09-14 20:31:26 PDT
Created attachment 107446 [details]
updated patch
Comment 23 Juan C. Montemayor 2011-09-14 20:34:13 PDT
(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.
Comment 24 Oliver Hunt 2011-09-23 18:14:13 PDT
Comment on attachment 107446 [details]
updated patch

r=me, sorry i missed the updated patch going up
Comment 25 WebKit Review Bot 2011-09-23 18:17:50 PDT
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
Comment 26 Juan C. Montemayor 2011-09-24 06:55:51 PDT
(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.
Comment 27 Juan C. Montemayor 2011-09-27 07:42:10 PDT
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+
Comment 28 Juan C. Montemayor 2011-09-27 08:59:47 PDT
Created attachment 108854 [details]
fixing win bot

please disregard this patch, i want to see what win-bot tells me it needs.
Comment 29 Oliver Hunt 2011-09-27 09:11:27 PDT
Comment on attachment 108854 [details]
fixing win bot

Looks like your patch has lost the changes to JavaScriptCore.def
Comment 30 Juan C. Montemayor 2011-09-27 09:13:32 PDT
(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
Comment 31 Juan C. Montemayor 2011-09-27 09:17:17 PDT
(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?
Comment 32 Oliver Hunt 2011-09-27 09:37:30 PDT
(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
Comment 33 Juan C. Montemayor 2011-09-27 09:58:25 PDT
Created attachment 108861 [details]
patch

this should be it
Comment 34 WebKit Review Bot 2011-09-27 10:58:02 PDT
Comment on attachment 108861 [details]
patch

Clearing flags on attachment: 108861

Committed r96131: <http://trac.webkit.org/changeset/96131>
Comment 35 WebKit Review Bot 2011-09-27 10:58:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Oliver Hunt 2011-09-27 11:02:30 PDT
Yay!
Comment 37 Csaba Osztrogonác 2011-09-27 13:12:17 PDT
(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
...
Comment 38 Csaba Osztrogonác 2011-09-27 13:17:25 PDT
Rolled out by http://trac.webkit.org/changeset/96146
Comment 39 Csaba Osztrogonác 2011-09-27 13:20:17 PDT
(In reply to comment #38)
> Rolled out by http://trac.webkit.org/changeset/96146

It seems the problem occured only on 32 bit platoforms.
Comment 40 Juan C. Montemayor 2011-09-27 13:21:27 PDT
(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).
Comment 41 Oliver Hunt 2012-01-31 15:02:17 PST
Created attachment 124830 [details]
Patch
Comment 42 Oliver Hunt 2012-01-31 15:25:48 PST
Committed r106407
Comment 43 Csaba Osztrogonác 2012-02-01 00:41:01 PST
(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?
Comment 44 Csaba Osztrogonác 2012-02-01 02:06:59 PST
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.)
Comment 45 Oliver Hunt 2012-02-16 13:57:01 PST
Created attachment 127437 [details]
Patch
Comment 46 Gavin Barraclough 2012-02-16 14:36:09 PST
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.
Comment 47 Oliver Hunt 2012-02-16 14:39:01 PST
Committed r107980: <http://trac.webkit.org/changeset/107980>
Comment 48 Eric Seidel 2012-02-16 14:44:40 PST
Seems like a feature possibly worth blogging about. :)
Comment 49 Erik Arvidsson 2012-02-16 15:49:30 PST
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.
Comment 50 Oliver Hunt 2012-02-16 15:54:37 PST
(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.
Comment 51 Erik Arvidsson 2012-02-16 16:08:53 PST
(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.
Comment 52 Oliver Hunt 2012-02-16 16:13:01 PST
(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)
Comment 53 Csaba Osztrogonác 2012-02-16 22:52:52 PST
(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.
Comment 54 Oliver Hunt 2012-02-17 11:47:36 PST
(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.
Comment 55 Oliver Hunt 2012-02-17 13:18:53 PST
Committed r108112: <http://trac.webkit.org/changeset/108112>
Comment 56 Gavin Barraclough 2012-03-07 18:12:36 PST
*** Bug 13646 has been marked as a duplicate of this bug. ***