RESOLVED FIXED 169904
Add support for Error.stackTraceLimit.
https://bugs.webkit.org/show_bug.cgi?id=169904
Summary Add support for Error.stackTraceLimit.
Mark Lam
Reported 2017-03-20 19:13:32 PDT
Since there's no standard for this yet, we'll implement Error.stackTraceLimit based on how Chrome does it. This includes some idiosyncrasies like: 1. If we set Error.stackTraceLimit = 0, then new Error().stack yields an empty stack trace (Chrome has a title with no stack frame entries). 2. If we set Error.stackTraceLimit = {] (i.e. to a non-number value), then new Error().stack is undefined. Chrome and IE defaults their Error.stackTraceLimit to 10. We'll default ours to 100 because 10 may be a bit too skimpy and it is not that costly to allow up to 100 frames instead of 10. Patch coming.
Attachments
proposed patch. (26.39 KB, patch)
2017-03-20 19:21 PDT, Mark Lam
mark.lam: review-
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.09 MB, application/zip)
2017-03-20 20:41 PDT, Build Bot
no flags
Archive of layout-test-results from ews114 for mac-elcapitan (1.87 MB, application/zip)
2017-03-20 20:52 PDT, Build Bot
no flags
proposed patch. (22.23 KB, patch)
2017-03-21 12:29 PDT, Mark Lam
no flags
proposed patch: rebased. (22.17 KB, patch)
2017-03-21 19:10 PDT, Mark Lam
saam: review+
patch for landing. (20.92 KB, patch)
2017-03-22 17:28 PDT, Mark Lam
no flags
Mark Lam
Comment 1 2017-03-20 19:21:59 PDT
Created attachment 304982 [details] proposed patch.
Build Bot
Comment 2 2017-03-20 20:04:01 PDT
Comment on attachment 304982 [details] proposed patch. Attachment 304982 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3373138 New failing tests: wasm.yaml/wasm/spec-tests/select.wast.js.default-wasm jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-cjit ChakraCore.yaml/ChakraCore/test/strict/bug212755.js.default wasm.yaml/wasm/spec-tests/call_indirect.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/strict/formal_samename2.js.default ChakraCore.yaml/ChakraCore/test/es6/supersyntax05.js.default ChakraCore.yaml/ChakraCore/test/es6/globalCatchNewTargetSyntaxError.js.default ChakraCore.yaml/ChakraCore/test/strict/stricteval2-deferred.js.default wasm.yaml/wasm/spec-tests/resizing.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/strict/strictargs2-deferred.js.default ChakraCore.yaml/ChakraCore/test/es6/supersyntax02.js.default wasm.yaml/wasm/function-tests/exceptions.js.default-wasm ChakraCore.yaml/ChakraCore/test/strict/delete.js.default ChakraCore.yaml/ChakraCore/test/strict/strictargs-deferred.js.default wasm.yaml/wasm/function-tests/function-import-return-value.js.default-wasm jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout wasm.yaml/wasm/function-tests/trap-store-2.js.default-wasm jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-no-cjit wasm.yaml/wasm/spec-tests/int_exprs.wast.js.default-wasm wasm.yaml/wasm/spec-tests/memory_trap.wast.js.default-wasm wasm.yaml/wasm/spec-tests/start.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/strict/stricteval-deferred.js.default ChakraCore.yaml/ChakraCore/test/strict/nonSimpleParameterList.js.default wasm.yaml/wasm/spec-tests/i64.wast.js.default-wasm jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-dfg-eager-no-cjit wasm.yaml/wasm/js-api/wrapper-function.js.default-wasm ChakraCore.yaml/ChakraCore/test/strict/strictargs3-deferred.js.default wasm.yaml/wasm/function-tests/trap-load-2.js.default-wasm wasm.yaml/wasm/spec-tests/func_ptrs.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/LetConst/defer1.js.default wasm.yaml/wasm/spec-tests/unreachable.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/es6/globalParamCatchNewTargetSyntaxError.js.default ChakraCore.yaml/ChakraCore/test/strict/formal_samename1.js.default wasm.yaml/wasm/spec-tests/imports.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/es6/unicode_6_identifier_Blue511452.js.default ChakraCore.yaml/ChakraCore/test/Function/deferredBadContinue.js.default wasm.yaml/wasm/spec-tests/conversions.wast.js.default-wasm wasm.yaml/wasm/function-tests/trap-load.js.default-wasm jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-llint wasm.yaml/wasm/spec-tests/address.wast.js.default-wasm wasm.yaml/wasm/spec-tests/traps.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/es6/letconst_global_shadow_builtins_nonconfigurable.js.default ChakraCore.yaml/ChakraCore/test/strict/multiunit.js.default wasm.yaml/wasm/spec-tests/unwind.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/es6/unicode_6_identifier_Blue524737.js.default ChakraCore.yaml/ChakraCore/test/es6/globalNewTargetSyntaxError.js.default wasm.yaml/wasm/function-tests/trap-store.js.default-wasm wasm.yaml/wasm/function-tests/grow-memory-3.js.default-wasm jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-ftl wasm.yaml/wasm/spec-tests/i32.wast.js.default-wasm ChakraCore.yaml/ChakraCore/test/es6/supersyntax06.js.default
Build Bot
Comment 3 2017-03-20 20:41:17 PDT
Comment on attachment 304982 [details] proposed patch. Attachment 304982 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3373202 New failing tests: js/Object-getOwnPropertyNames.html
Build Bot
Comment 4 2017-03-20 20:41:21 PDT
Created attachment 304984 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 5 2017-03-20 20:52:27 PDT
Comment on attachment 304982 [details] proposed patch. Attachment 304982 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3373225 New failing tests: js/Object-getOwnPropertyNames.html
Build Bot
Comment 6 2017-03-20 20:52:35 PDT
Created attachment 304986 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Mark Lam
Comment 7 2017-03-20 20:53:03 PDT
Comment on attachment 304982 [details] proposed patch. Interesting. So many test failures. Will fix.
Mark Lam
Comment 8 2017-03-21 12:29:56 PDT
Created attachment 305010 [details] proposed patch.
Mark Lam
Comment 9 2017-03-21 17:03:59 PDT
I've run the layout tests and JSC tests locally, and all seems well.
Mark Lam
Comment 10 2017-03-21 19:10:42 PDT
Created attachment 305064 [details] proposed patch: rebased.
Saam Barati
Comment 11 2017-03-22 11:44:15 PDT
Comment on attachment 305064 [details] proposed patch: rebased. View in context: https://bugs.webkit.org/attachment.cgi?id=305064&action=review > Source/JavaScriptCore/interpreter/Interpreter.cpp:685 > + ASSERT_UNUSED(scope, scope.exception() && (!Options::exceptionStackTraceLimit() || scope.exception()->stack().size())); This assertion looks wrong: Shouldn't you consult the global object's current value, not the option? > Source/JavaScriptCore/runtime/ErrorConstructor.cpp:50 > + unsigned defaultStackTraceLimit = Options::defaultErrorStackTraceLimit(); > + globalObject()->setErrorStackTraceLimit(defaultStackTraceLimit); > + putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(defaultStackTraceLimit), None); Why not just make this a getter/setter? What does V8 do? If we were to actually propose this as spec, I think a getter/setter is cleaner. > Source/JavaScriptCore/runtime/ErrorConstructor.cpp:112 > + if (propertyName == vm.propertyNames->stackTraceLimit) { > + JSGlobalObject* globalObject = thisObject->globalObject(); > + globalObject->clearErrorStackTraceLimit(); > + } This seems like weird behavior. Why not set it back to the default here? Again, I'd advocate for making this a setter/getter.
Mark Lam
Comment 12 2017-03-22 11:50:06 PDT
Comment on attachment 305064 [details] proposed patch: rebased. View in context: https://bugs.webkit.org/attachment.cgi?id=305064&action=review >> Source/JavaScriptCore/interpreter/Interpreter.cpp:685 >> + ASSERT_UNUSED(scope, scope.exception() && (!Options::exceptionStackTraceLimit() || scope.exception()->stack().size())); > > This assertion looks wrong: Shouldn't you consult the global object's current value, not the option? scope.exception() is always the Exception object, whose size is dictated by Options::exceptionStackTraceLimit(), not Options::defaultErrorStackTraceLimit() not the JSGlobalObject::m_errorStackTraceLimit. >> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:50 >> + putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(defaultStackTraceLimit), None); > > Why not just make this a getter/setter? What does V8 do? If we were to actually propose this as spec, I think a getter/setter is cleaner. Because then Object.getOwnPropertyDescriptor(Error, "stackTraceLimit") will return { value; undefined, ..., get: function ...., set: function ... }. Chrome's impl returns { value: 10, ... } with no get and set. >> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:112 >> + } > > This seems like weird behavior. Why not set it back to the default here? Again, I'd advocate for making this a setter/getter. Because this is how Chrome behaves and we're aiming for bug / feature compatibility in the absence of a spec. Incidentally, I tested for all this behavior in JSTests/stress/error-stack-trace-limit.js. I ran that test on Chrome (with 2 adjustments I noted in its comments) and it passes. With my current implementation, we behave identically to Chrome (except for our default limit being 100 instead of 10). With setter/getter, we'll behave differently in some ways.
Saam Barati
Comment 13 2017-03-22 12:19:30 PDT
Comment on attachment 305064 [details] proposed patch: rebased. View in context: https://bugs.webkit.org/attachment.cgi?id=305064&action=review Thanks for you response. r=me with a comment. >>> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:50 >>> + unsigned defaultStackTraceLimit = Options::defaultErrorStackTraceLimit(); >>> + globalObject()->setErrorStackTraceLimit(defaultStackTraceLimit); >>> + putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(defaultStackTraceLimit), None); >> >> Why not just make this a getter/setter? What does V8 do? If we were to actually propose this as spec, I think a getter/setter is cleaner. > > Because then Object.getOwnPropertyDescriptor(Error, "stackTraceLimit") will return { value; undefined, ..., get: function ...., set: function ... }. Chrome's impl returns { value: 10, ... } with no get and set. Style nit: It's weird that this code lives here. What I'd do is this: 1. Make global object set its own value via the option during JSGlobalObject() constructor. 2. Make this grab the value from the global object, and use that in putDirect, so something like: putDirect(..., globalObject->errorLimit() ? globalObject.errorLimit.get() : Options::defaultErrorStackTraceLimit(), ...)
Mark Lam
Comment 14 2017-03-22 12:28:06 PDT
Comment on attachment 305064 [details] proposed patch: rebased. View in context: https://bugs.webkit.org/attachment.cgi?id=305064&action=review >>>> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:50 >>>> + putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(defaultStackTraceLimit), None); >>> >>> Why not just make this a getter/setter? What does V8 do? If we were to actually propose this as spec, I think a getter/setter is cleaner. >> >> Because then Object.getOwnPropertyDescriptor(Error, "stackTraceLimit") will return { value; undefined, ..., get: function ...., set: function ... }. Chrome's impl returns { value: 10, ... } with no get and set. > > Style nit: > It's weird that this code lives here. What I'd do is this: > 1. Make global object set its own value via the option during JSGlobalObject() constructor. > 2. Make this grab the value from the global object, and use that in putDirect, so something like: putDirect(..., globalObject->errorLimit() ? globalObject.errorLimit.get() : Options::defaultErrorStackTraceLimit(), ...) I put m_errorStackTraceLimit in JSGlobalObject because I made the error of thinking that it's easier for clients to get the value from the globalObject instead of the ErrorConstructor. It technically belongs in the ErrorConstructor. In retrospect, I realize that clients can just get the ErrorConstructor from the globalObject instead. As discussed offline, I will apply this change and retest before landing. Thanks.
Mark Lam
Comment 15 2017-03-22 17:28:18 PDT
Created attachment 305143 [details] patch for landing.
Mark Lam
Comment 16 2017-03-22 18:13:34 PDT
Thanks for the review. The bots are happy. Will land.
Mark Lam
Comment 17 2017-03-22 18:15:52 PDT
Note You need to log in before you can comment on or make changes to this bug.