| Summary: | Add support for Error.stackTraceLimit. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, fpizlo, ggaren, jfbastien, joepeck, keith_miller, kling, mkwst, msaboff, rniwa, saam | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | WebKit Local Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 304982 [details]
proposed patch.
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 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 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
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 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
Comment on attachment 304982 [details]
proposed patch.
Interesting. So many test failures. Will fix.
Created attachment 305010 [details]
proposed patch.
I've run the layout tests and JSC tests locally, and all seems well. Created attachment 305064 [details]
proposed patch: rebased.
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. 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. 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(), ...) 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. Created attachment 305143 [details]
patch for landing.
Thanks for the review. The bots are happy. Will land. Landed in r214289: <http://trac.webkit.org/r214289>. |
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.