Bug 160438

Summary: REGRESSION (r203990): JSC Debug test stress/arity-check-ftl-throw.js failing
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, buildbot, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, mhahnenb, msaboff, oliver, rniwa, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 160441    
Attachments:
Description Flags
Crashlog
none
the patch
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
better patch
mark.lam: review+
patch for landing
none
for real this time none

Description Ryan Haddad 2016-08-01 18:05:41 PDT
Running stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool
stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool: ASSERTION FAILED: exec == vm.topCallFrame || exec == exec->lexicalGlobalObject()->globalExec() || exec == exec->vmEntryGlobalObject()->globalExec()
stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool: /Volumes/Data/slave/yosemite-debug/build/Source/JavaScriptCore/runtime/Error.cpp(144) : bool JSC::addErrorInfoAndGetBytecodeOffset(JSC::ExecState *, JSC::VM &, JSC::JSObject *, bool, CallFrame *&, unsigned int *)
stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool: test_script_1195: line 2: 70136 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --jitMemoryReservationSize\=50000 --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 arity-check-ftl-throw-more-args.js )
stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool
Running stress/arity-check-ftl-throw.js.default
Running stress/arity-check-ftl-throw.js.always-trigger-copy-phase
stress/arity-check-ftl-throw.js.default: test_script_1196: line 2: 70150 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useFTLJIT\=true arity-check-ftl-throw.js )
stress/arity-check-ftl-throw.js.default: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw.js.default
Running stress/arity-check-ftl-throw.js.no-llint
Running stress/arity-check-ftl-throw.js.no-cjit-validate-phases
Running stress/arity-check-ftl-throw.js.dfg-eager
Running stress/arity-check-ftl-throw.js.dfg-eager-no-cjit-validate
Running stress/arity-check-ftl-throw.js.dfg-maximal-flush-validate-no-cjit
Running stress/arity-check-ftl-throw.js.no-ftl
Running stress/arity-check-ftl-throw.js.ftl-no-cjit-validate-sampling-profiler
Running stress/arity-check-ftl-throw.js.ftl-no-cjit-no-put-stack-validate
stress/arity-check-ftl-throw.js.ftl-no-cjit-validate-sampling-profiler: test_script_1204: line 2: 70226 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --validateGraph\=true --useSamplingProfiler\=true --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 arity-check-ftl-throw.js )
stress/arity-check-ftl-throw.js.ftl-no-cjit-validate-sampling-profiler: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw.js.ftl-no-cjit-validate-sampling-profiler
Running stress/arity-check-ftl-throw.js.ftl-no-cjit-no-inline-validate
Running stress/arity-check-ftl-throw.js.ftl-eager
stress/arity-check-ftl-throw.js.ftl-no-cjit-no-put-stack-validate: test_script_1205: line 2: 70234 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --validateGraph\=true --usePutStackSinking\=false --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 arity-check-ftl-throw.js )
stress/arity-check-ftl-throw.js.ftl-no-cjit-no-put-stack-validate: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw.js.ftl-no-cjit-no-put-stack-validate
Running stress/arity-check-ftl-throw.js.ftl-eager-no-cjit
stress/arity-check-ftl-throw.js.ftl-no-cjit-no-inline-validate: test_script_1206: line 2: 70248 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --validateGraph\=true --maximumInliningDepth\=1 --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 arity-check-ftl-throw.js )
stress/arity-check-ftl-throw.js.ftl-no-cjit-no-inline-validate: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw.js.ftl-no-cjit-no-inline-validate
Running stress/arity-check-ftl-throw.js.ftl-no-cjit-small-pool
stress/arity-check-ftl-throw.js.ftl-eager: test_script_1207: line 2: 70257 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useFTLJIT\=true --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 arity-check-ftl-throw.js )
stress/arity-check-ftl-throw.js.ftl-eager: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw.js.ftl-eager
Running stress/array-concat-spread-object.js.default
stress/arity-check-ftl-throw.js.ftl-eager-no-cjit: test_script_1208: line 2: 70270 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --validateGraph\=true --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 arity-check-ftl-throw.js )
stress/arity-check-ftl-throw.js.ftl-eager-no-cjit: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw.js.ftl-eager-no-cjit
Running stress/array-concat-spread-object.js.always-trigger-copy-phase
stress/arity-check-ftl-throw.js.ftl-no-cjit-small-pool: test_script_1209: line 2: 70283 Segmentation fault: 11  ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --jitMemoryReservationSize\=50000 --useFTLJIT\=true --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 arity-check-ftl-throw.js )
stress/arity-check-ftl-throw.js.ftl-no-cjit-small-pool: ERROR: Unexpected exit code: 139
FAIL: stress/arity-check-ftl-throw.js.ftl-no-cjit-small-pool

** The following JSC stress test failures have been introduced:
	stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool
	stress/arity-check-ftl-throw.js.default
	stress/arity-check-ftl-throw.js.ftl-eager
	stress/arity-check-ftl-throw.js.ftl-eager-no-cjit
	stress/arity-check-ftl-throw.js.ftl-no-cjit-no-inline-validate
	stress/arity-check-ftl-throw.js.ftl-no-cjit-no-put-stack-validate
	stress/arity-check-ftl-throw.js.ftl-no-cjit-small-pool
	stress/arity-check-ftl-throw.js.ftl-no-cjit-validate-sampling-profiler
Comment 2 Ryan Haddad 2016-08-01 18:11:33 PDT
Created attachment 285063 [details]
Crashlog

Crashlog from the assertion failure during stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-small-pool
Comment 3 Filip Pizlo 2016-08-01 18:20:02 PDT
I'm looking at this now.
Comment 4 Filip Pizlo 2016-08-01 18:43:40 PDT
I think I have a fix.  These were all latent bugs, that were masked by the fact that our varargs stack overflow handling basically never triggered.
Comment 5 Filip Pizlo 2016-08-01 19:38:58 PDT
Created attachment 285065 [details]
the patch
Comment 6 Filip Pizlo 2016-08-01 19:43:40 PDT
Comment on attachment 285065 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285065&action=review

> Source/JavaScriptCore/interpreter/StackVisitor.cpp:47
> +        while (static_cast<void*>(m_frame.m_VMEntryFrame) == static_cast<void*>(topFrame)) {

This should be an 'if'.
Comment 7 Mark Lam 2016-08-01 20:25:57 PDT
Comment on attachment 285065 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285065&action=review

> Source/JavaScriptCore/ChangeLog:29
> +        We mad to ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame

"mad to"?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:185
> +        NativeCallFrameTracer subTracer(&vm, exec);

This doesn't hurt, but I don't think this should have made a difference because CommonSlowPaths::interpreterThrowInCaller() will create a NativeCallFrameTracer() on the exec.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:199
> +        NativeCallFrameTracer subTracer(&vm, exec);

Ditto.
Comment 8 Filip Pizlo 2016-08-01 20:27:28 PDT
(In reply to comment #7)
> Comment on attachment 285065 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285065&action=review
> 
> > Source/JavaScriptCore/ChangeLog:29
> > +        We mad to ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame
> 
> "mad to"?
> 
> > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:185
> > +        NativeCallFrameTracer subTracer(&vm, exec);
> 
> This doesn't hurt, but I don't think this should have made a difference
> because CommonSlowPaths::interpreterThrowInCaller() will create a
> NativeCallFrameTracer() on the exec.

It makes a huge difference.  Before we get to internreterThrowInCaller(), we do createStackOverflowError() or whatever.  That's where we were dying.

> 
> > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:199
> > +        NativeCallFrameTracer subTracer(&vm, exec);
> 
> Ditto.

See above.
Comment 9 Mark Lam 2016-08-01 20:29:18 PDT
Comment on attachment 285065 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285065&action=review

>>> Source/JavaScriptCore/ChangeLog:29
>>> +        We mad to ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame
>> 
>> "mad to"?
> 
> It makes a huge difference.  Before we get to internreterThrowInCaller(), we do createStackOverflowError() or whatever.  That's where we were dying.

Ahh.  I see (re CommonSlowPaths.cpp).

FYI, you have a typo in the ChangeLog here.  "mad to"?
Comment 10 Filip Pizlo 2016-08-01 20:34:58 PDT
(In reply to comment #9)
> Comment on attachment 285065 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285065&action=review
> 
> >>> Source/JavaScriptCore/ChangeLog:29
> >>> +        We mad to ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame
> >> 
> >> "mad to"?
> > 
> > It makes a huge difference.  Before we get to internreterThrowInCaller(), we do createStackOverflowError() or whatever.  That's where we were dying.
> 
> Ahh.  I see (re CommonSlowPaths.cpp).
> 
> FYI, you have a typo in the ChangeLog here.  "mad to"?

I fixed it!  It was meant to be "may do".
Comment 11 Build Bot 2016-08-01 20:38:10 PDT
Comment on attachment 285065 [details]
the patch

Attachment 285065 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1796110

New failing tests:
inspector/debugger/breakpoint-syntax-error-top-level.html
Comment 12 Build Bot 2016-08-01 20:38:16 PDT
Created attachment 285068 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 13 Build Bot 2016-08-01 20:42:12 PDT
Comment on attachment 285065 [details]
the patch

Attachment 285065 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1796116

New failing tests:
inspector/debugger/breakpoint-syntax-error-top-level.html
Comment 14 Build Bot 2016-08-01 20:42:16 PDT
Created attachment 285069 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 15 Filip Pizlo 2016-08-01 20:42:46 PDT
Created attachment 285070 [details]
better patch

I forgot a null check in that last one.
Comment 16 Mark Lam 2016-08-01 20:59:36 PDT
Comment on attachment 285070 [details]
better patch

r=me
Comment 17 Filip Pizlo 2016-08-01 21:34:52 PDT
Created attachment 285072 [details]
patch for landing
Comment 18 Filip Pizlo 2016-08-01 21:45:06 PDT
Created attachment 285073 [details]
for real this time

Fix an assertion after addressing the NativeCallFrameTracer assertion feedback from Saam
Comment 19 Filip Pizlo 2016-08-01 22:40:41 PDT
Landed in https://trac.webkit.org/changeset/204010