WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
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
Details
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
Details
proposed patch.
(22.23 KB, patch)
2017-03-21 12:29 PDT
,
Mark Lam
no flags
Details
Formatted Diff
Diff
proposed patch: rebased.
(22.17 KB, patch)
2017-03-21 19:10 PDT
,
Mark Lam
saam
: review+
Details
Formatted Diff
Diff
patch for landing.
(20.92 KB, patch)
2017-03-22 17:28 PDT
,
Mark Lam
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
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
Landed in
r214289
: <
http://trac.webkit.org/r214289
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug