Bug 169904 – Add support for Error.stackTraceLimit.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

Created attachment 304982 [details]
proposed patch.

[Build Bot]

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

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

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 on attachment 304982 [details]
proposed patch.

Interesting.  So many test failures.  Will fix.

[Mark Lam]

Created attachment 305010 [details]
proposed patch.

[Mark Lam]

I've run the layout tests and JSC tests locally, and all seems well.

[Mark Lam]

Created attachment 305064 [details]
proposed patch: rebased.

[Saam Barati]

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

Created attachment 305143 [details]
patch for landing.

[Mark Lam]

Thanks for the review.  The bots are happy.  Will land.

[Mark Lam]

Landed in r214289: <http://trac.webkit.org/r214289>.