handle race condition in OOP JIT #1728
Conversation
CloseScriptContext call can happen before or in the middle of CodeGen call. In JIT server call, between detecting contexts alive and increasing the refcount, there can be close and initialize new context call, the new context can get same address from HeapAlloctor and causes codegen succeeded with wrong scriptContext. Also, there are two calls for cleaning up script context and both can cause server scriptContext delete thus new scriptContext get same address as the deleted one. Move the context closing stack recording to seperate location, so the context itself can be deleted normally
| void Release(); | ||
| void Close(); | ||
| void Close(); | ||
| DWORD GetRuntimePid() { return m_pid; } |
There was a problem hiding this comment.
#ifdef STACK_BACK_TRACE? #Resolved
lib/JITServer/JITServer.h
Outdated
| auto record = ClosedThreadContextList.Pop(); | ||
| if (record) | ||
| { | ||
| delete record; |
lib/JITServer/JITServer.h
Outdated
| auto record = ClosedScriptContextList.Pop(); | ||
| if (record) | ||
| { | ||
| delete record; |
|
|
| #if ENABLE_NATIVE_CODEGEN | ||
| if (m_remoteScriptContextAddr) | ||
| { | ||
| JITManager::GetJITManager()->CloseScriptContext(m_remoteScriptContextAddr); |
There was a problem hiding this comment.
Having thought more about this, I think we should have Close call still. With cleanup only, I think it may prevent us from short circuiting JIT on scripts that are closing (I think the JIT work item will keep the ScriptContext alive?)
However, Close should never actually free the ServerScriptContext (to prevent the races we see now). #Resolved
There was a problem hiding this comment.
what do you want the server to do if the scriptContext is closed but not cleaned up, rejecting the call? if yes we can just move current cleanup call to client scriptContext::Close(). if you allow the server call after scriptContext is closed in client side, current implementation is already reflecting that. also, because of the racing, neither rejecting nor allow can be strictly enforced, unless the client side can change to not sending other calls after scriptContext is closed or pending close.
In reply to: 83082897 [](ancestors = 83082897)
There was a problem hiding this comment.
If it is closed but not cleaned up, we don't need to reject the call. It should really only be for telling codegen that it can give up on any current JIT calls (we have periodic checks for IsClosed during codegen).
In reply to: 83088845 [](ancestors = 83088845,83082897)
we still need two phase closing/cleanup for scriptContext, in case when CodeGen call is marshalling in RPC, client has already finished closes scriptcontext and initialize new scriptContext call and get same address. fixed another issue: if a call is made with scriptContext, there's gap between increasing refcount of scriptContext and threadContext, some other closing/cleanup call can happen in the gap, can also result in wrong context is used. adjust some cleanup orders make ChakraMemSet not throw.
|
|
Merge pull request #1728 from leirocks:oopjitrace1 CloseScriptContext call can happen before or in the middle of CodeGen call. In JIT server call, between detecting contexts alive and increasing the refcount, there can be close and initialize new context call, the new context can get same address from HeapAlloctor and causes codegen succeeded with wrong scriptContext.
5 participants
CloseScriptContext call can happen before or in the middle of CodeGen call.
In JIT server call, between detecting contexts alive and increasing the refcount, there can be close and initialize new context call, the new context can get same address from HeapAlloctor and causes codegen succeeded with wrong scriptContext.
Also, there are two calls for cleaning up script context and both can cause server scriptContext delete thus new scriptContext get same address as the deleted one.
Move the context closing stack recording to seperate location, so the context itself can be deleted normally