Bug 192717 – CallFrame::convertToStackOverflowFrame() needs to keep the top CodeBlock alive.

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

[Mark Lam]

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>

[Mark Lam]

Created attachment 357344 [details]
proposed patch.

[Saam Barati]

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

[Mark Lam]

(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.

[Mark Lam]

(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.

[Mark Lam]

Created attachment 357357 [details]
proposed patch.

Let's try this on the EWS bots first.

[Mark Lam]

Comment on attachment 357357 [details]
proposed patch.

The tests are looking good as expected.  Let's get a review.

[Saam Barati]

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

[Saam Barati]

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.

[Mark Lam]

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.

[WebKit Commit Bot]

Comment on attachment 357357 [details]
proposed patch.

Clearing flags on attachment: 357357

Committed r239244: <https://trac.webkit.org/changeset/239244>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.