Bug 192717

Summary: CallFrame::convertToStackOverflowFrame() needs to keep the top CodeBlock alive.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, guijemont, guijemont+jsc-armv7-ews, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
none
proposed patch. none

Description Mark Lam 2018-12-14 14:19:57 PST
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>
Comment 1 Mark Lam 2018-12-14 14:32:27 PST
Created attachment 357344 [details]
proposed patch.
Comment 2 Saam Barati 2018-12-14 14:53:21 PST
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
Comment 3 Mark Lam 2018-12-14 14:55:36 PST
(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.
Comment 4 Mark Lam 2018-12-14 16:26:00 PST
(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.
Comment 5 Mark Lam 2018-12-14 16:50:27 PST
Created attachment 357357 [details]
proposed patch.

Let's try this on the EWS bots first.
Comment 6 Mark Lam 2018-12-14 17:34:13 PST
Comment on attachment 357357 [details]
proposed patch.

The tests are looking good as expected.  Let's get a review.
Comment 7 Saam Barati 2018-12-14 17:42:50 PST
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 8 Saam Barati 2018-12-14 17:57:39 PST
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.
Comment 9 Mark Lam 2018-12-14 18:01:40 PST
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 10 WebKit Commit Bot 2018-12-14 18:28:25 PST
Comment on attachment 357357 [details]
proposed patch.

Clearing flags on attachment: 357357

Committed r239244: <https://trac.webkit.org/changeset/239244>
Comment 11 WebKit Commit Bot 2018-12-14 18:28:27 PST
All reviewed patches have been landed.  Closing bug.