Summary: | CallFrame::convertToStackOverflowFrame() needs to keep the top CodeBlock alive. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2018-12-14 14:19:57 PST
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. |