Bug 66994

Created attachment 105313 [details] proposed patch

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

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

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 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

(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.

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

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 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);

> > 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.

Created attachment 105417 [details] patch with changes

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

Created attachment 107128 [details] Patch

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

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 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

Created attachment 107174 [details] patch This will hopefully fix the build errors in qt et al.

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

(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.

(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 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

Created attachment 107446 [details] updated patch

(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 on attachment 107446 [details] updated patch r=me, sorry i missed the updated patch going up

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

(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.

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+

Created attachment 108854 [details] fixing win bot please disregard this patch, i want to see what win-bot tells me it needs.

Comment on attachment 108854 [details] fixing win bot Looks like your patch has lost the changes to JavaScriptCore.def

(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

(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?

(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

Created attachment 108861 [details] patch this should be it

Comment on attachment 108861 [details] patch Clearing flags on attachment: 108861 Committed r96131: <http://trac.webkit.org/changeset/96131>

All reviewed patches have been landed. Closing bug.

Yay!

(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 ...

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

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

(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).

Created attachment 124830 [details] Patch

Committed r106407

(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?

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.)

Created attachment 127437 [details] Patch

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.

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

Seems like a feature possibly worth blogging about. :)

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.

(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.

(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.

(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)

(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.

(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.

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

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