Bug 169904 - Add support for Error.stackTraceLimit.
Summary: Add support for Error.stackTraceLimit.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-20 19:13 PDT by Mark Lam
Modified: 2017-03-22 18:15 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 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.
Comment 1 Mark Lam 2017-03-20 19:21:59 PDT
Created attachment 304982 [details]
proposed patch.
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Mark Lam 2017-03-20 20:53:03 PDT
Comment on attachment 304982 [details]
proposed patch.

Interesting.  So many test failures.  Will fix.
Comment 8 Mark Lam 2017-03-21 12:29:56 PDT
Created attachment 305010 [details]
proposed patch.
Comment 9 Mark Lam 2017-03-21 17:03:59 PDT
I've run the layout tests and JSC tests locally, and all seems well.
Comment 10 Mark Lam 2017-03-21 19:10:42 PDT
Created attachment 305064 [details]
proposed patch: rebased.
Comment 11 Saam Barati 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.
Comment 12 Mark Lam 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.
Comment 13 Saam Barati 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(), ...)
Comment 14 Mark Lam 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.
Comment 15 Mark Lam 2017-03-22 17:28:18 PDT
Created attachment 305143 [details]
patch for landing.
Comment 16 Mark Lam 2017-03-22 18:13:34 PDT
Thanks for the review.  The bots are happy.  Will land.
Comment 17 Mark Lam 2017-03-22 18:15:52 PDT
Landed in r214289: <http://trac.webkit.org/r214289>.