Bug 226096

Summary: Finalize DFG/FTL code refs on the compiler threads
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 226180    
Bug Blocks:    
Attachments:
Description Flags
WIP
ews-feeder: commit-queue-
WIP
none
WIP
none
patch
ews-feeder: commit-queue-
patch
mark.lam: review+, ews-feeder: commit-queue-
[fast-cq] patch for landing none

Description Saam Barati 2021-05-21 11:41:15 PDT
...
Comment 1 Saam Barati 2021-05-21 17:06:56 PDT
Created attachment 429369 [details]
WIP
Comment 2 Saam Barati 2021-05-21 17:38:11 PDT
Created attachment 429376 [details]
WIP
Comment 3 Saam Barati 2021-05-21 18:03:18 PDT
Created attachment 429378 [details]
WIP
Comment 4 Saam Barati 2021-05-24 17:28:29 PDT
Created attachment 429599 [details]
patch
Comment 5 Saam Barati 2021-05-24 17:46:09 PDT
Created attachment 429602 [details]
patch
Comment 6 Mark Lam 2021-05-24 19:56:31 PDT
Comment on attachment 429602 [details]
patch

LGTM but the mac-wk1 EWS's failure seems real.  The failure is in the RELEASE_ASSERT in ~LinkBuffer on a worker thread at VM shutdown.  

Thread 77 Crashed:: WebCore: Worker
0   com.apple.JavaScriptCore      	0x0000000107d106d3 WTFCrashWithInfo(int, char const*, char const*, int) + 19
1   com.apple.JavaScriptCore      	0x00000001073eff0a JSC::LinkBuffer::~LinkBuffer() + 58
2   com.apple.JavaScriptCore      	0x0000000107839e5d JSC::DFG::JITFinalizer::~JITFinalizer() + 45
3   com.apple.JavaScriptCore      	0x0000000107906b18 JSC::DFG::Plan::~Plan() + 312
4   com.apple.JavaScriptCore      	0x000000010795e264 JSC::DFG::Worklist::removeAllReadyPlansForVM(JSC::VM&) + 116
5   com.apple.JavaScriptCore      	0x0000000107f961e7 JSC::VM::~VM() + 279
6   com.apple.JavaScriptCore      	0x0000000107e3a7d7 JSC::JSLockHolder::~JSLockHolder() + 119
7   com.apple.WebCore             	0x000000010e069754 WebCore::WorkerOrWorkletScriptController::~WorkerOrWorkletScriptController() + 212
8   com.apple.WebCore             	0x000000010e071195 WTF::Detail::CallableWrapper<WebCore::WorkerOrWorkletThread::stop(WTF::Function<void ()>&&)::$_34::operator()(WebCore::ScriptExecutionContext&) const::'lambda'(WebCore::ScriptExecutionContext&), void, WebCore::ScriptExecutionContext&>::call(WebCore::ScriptExecutionContext&) + 37
9   com.apple.WebCore             	0x000000010e071816 WebCore::WorkerRunLoop::runCleanupTasks(WebCore::WorkerOrWorkletGlobalScope*) + 262
10  com.apple.WebCore             	0x000000010e0713bf WebCore::WorkerRunLoop::run(WebCore::WorkerOrWorkletGlobalScope*) + 111
11  com.apple.WebCore             	0x000000010e06ca30 WebCore::WorkerOrWorkletThread::workerOrWorkletThread() + 768
12  com.apple.JavaScriptCore      	0x00000001070739ec WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 124
13  com.apple.JavaScriptCore      	0x0000000107076259 WTF::wtfThreadEntryPoint(void*) + 9
14  libsystem_pthread.dylib       	0x00007fff72768109 _pthread_start + 148
15  libsystem_pthread.dylib       	0x00007fff72763b8b thread_start + 15

Looks like possibly the plan wasn't cancelled, or not cancelled in time?
Comment 7 Saam Barati 2021-05-24 20:31:03 PDT
Comment on attachment 429602 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429602&action=review

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:49
> +        RELEASE_ASSERT(m_mainThreadFinalizationTasks.isEmpty());

I’ll remove this RELEASE_ASSERT. It’s taking a lot of plumbing to make it work. Which is usually a bad reason to keep an assert
Comment 8 Mark Lam 2021-05-24 20:52:07 PDT
Comment on attachment 429602 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429602&action=review

r=me

>> Source/JavaScriptCore/assembler/LinkBuffer.cpp:49
>> +        RELEASE_ASSERT(m_mainThreadFinalizationTasks.isEmpty());
> 
> I’ll remove this RELEASE_ASSERT. It’s taking a lot of plumbing to make it work. Which is usually a bad reason to keep an assert

I'm fine with removing it.  The failure was at VM destruction, and m_mainThreadFinalizationTasks will get cleaned out anyway.
Comment 9 Saam Barati 2021-05-25 11:15:32 PDT
Created attachment 429668 [details]
[fast-cq] patch for landing
Comment 10 EWS 2021-05-25 11:16:55 PDT
Committed r278030 (238127@main): <https://commits.webkit.org/238127@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429668 [details].
Comment 11 Radar WebKit Bug Importer 2021-05-25 11:17:20 PDT
<rdar://problem/78466944>