When throwing a StackOverflowError, we convert the topCallFrame into a StackOverflowFrame. Previously, we would nullify the codeBlock field in the frame because a StackOverflowFrame is only a sentinel and doesn't really correlate to any CodeBlocks. However, this is a problem because the topCallFrame may be the only remaining place that references the CodeBlock that the stack overflow is triggered in. The way we handle exceptions in JIT code is to return (from the runtime operation function throwing the exception) to the JIT code to check for the exception and if needed, do some clean up before jumping to the exception handling thunk. As a result, we need to keep that JIT code alive, which means we need to keep its CodeBlock alive. We only need to keep this CodeBlock alive until we've unwound (in terms of exception handling) out of it. We fix this issue by storing the CodeBlock to keep alive in the StackOverflowFrame for the GC to scan while the frame is still on the stack. <rdar://problem/46660677>
Created attachment 357344 [details] proposed patch.
Comment on attachment 357344 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=357344&action=review Are you sure that everywhere you're calling exec->codeBlock() we've already materialized a CodeBlock onto the frame? > Source/JavaScriptCore/interpreter/CallFrame.cpp:343 > + ASSERT(jsDynamicCast<CodeBlock*>(vm, codeBlockToKeepAliveUntilFrameIsUnwound)); inherits is the more proper way to do this
(In reply to Saam Barati from comment #2) > Comment on attachment 357344 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357344&action=review > > Are you sure that everywhere you're calling exec->codeBlock() we've already > materialized a CodeBlock onto the frame? Pretty sure but I'll do a comprehensive audit. > > Source/JavaScriptCore/interpreter/CallFrame.cpp:343 > > + ASSERT(jsDynamicCast<CodeBlock*>(vm, codeBlockToKeepAliveUntilFrameIsUnwound)); > > inherits is the more proper way to do this Will fix. I'm also seeing some strange JSC test failures. Will investigate before posting a new patch.
(In reply to Mark Lam from comment #3) > (In reply to Saam Barati from comment #2) > > Comment on attachment 357344 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=357344&action=review > > > > Are you sure that everywhere you're calling exec->codeBlock() we've already > > materialized a CodeBlock onto the frame? > > Pretty sure but I'll do a comprehensive audit. You are right. The FTL code doesn't pre-polutate the codeBlock field in the CallFrame. I will fix it. > I'm also seeing some strange JSC test failures. Will investigate before > posting a new patch. Turns out the failures were not due to my patch. New patch coming soon.
Created attachment 357357 [details] proposed patch. Let's try this on the EWS bots first.
Comment on attachment 357357 [details] proposed patch. The tests are looking good as expected. Let's get a review.
Comment on attachment 357357 [details] proposed patch. I think we should use DeferGCForAWhile instead. I believe that's typically how we solve these issues when we're in a place we can't go into a GC safepoint
Comment on attachment 357357 [details] proposed patch. r=me. I'm ok with this solution too. I'll let you decide what you think will lead to fewer bugs in the future.
Thanks for the review. I'm going to land this. I feel that DeferGC is a blunt tool, and this solution is not hard to understand.
Comment on attachment 357357 [details] proposed patch. Clearing flags on attachment: 357357 Committed r239244: <https://trac.webkit.org/changeset/239244>
All reviewed patches have been landed. Closing bug.