Summary: | REGRESSION (r203990): JSC Debug test stress/arity-check-ftl-throw.js failing | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||||||||||
Component: | New Bugs | Assignee: | 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
Ryan Haddad
2016-08-01 18:05:41 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
I'm looking at this now. 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. Created attachment 285065 [details]
the patch
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 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. (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 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"? (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 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 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 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 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
Created attachment 285070 [details]
better patch
I forgot a null check in that last one.
Comment on attachment 285070 [details]
better patch
r=me
Created attachment 285072 [details]
patch for landing
Created attachment 285073 [details]
for real this time
Fix an assertion after addressing the NativeCallFrameTracer assertion feedback from Saam
Landed in https://trac.webkit.org/changeset/204010 |