OOP JIT server side error handling #1689
Conversation
| || bytesWritten != sizeCat) | ||
| { | ||
| Js::Throw::CheckAndThrowJITOperationFailed(); | ||
| Output::Print(_u("FATAL ERROR: WriteProcessMemory failed, GLE: %d\n"), GetLastError()); |
There was a problem hiding this comment.
| LPVOID addr = ::VirtualAllocEx(hProcess, tail->GetCommitEndAddress(), BlockSize, MEM_COMMIT, PAGE_READWRITE); | ||
| if (addr == nullptr) | ||
| { | ||
| if (hProcess != GetCurrentProcess()) |
There was a problem hiding this comment.
maybe we still should throw oom here?
There was a problem hiding this comment.
i'm thinking -- the WriteProcessMemory failing is more of a fatal error like AV (and we should at least disable JIT after that). This, it could be that proc legitimately is OOM.
In reply to: 81823255 [](ancestors = 81823255)
There was a problem hiding this comment.
in my next commit, I'm changing such failure to be fatal in client side. if the client process is terminated, it returns E_ACCESSDENIED, if the client process is in middle of terminating, the page might be de-commited, we won't AV in server side, but get back some error code other than E_ACCESSDENIED
In reply to: 81823623 [](ancestors = 81823623,81823255)
lib/Backend/NativeCodeGenerator.cpp
Outdated
| throw Js::StackOverflowException(); | ||
| case E_ACCESSDENIED: | ||
| // OOP JIT TODO: if server side can't handle request any more, use better error code and turn off JIT | ||
| throw Js::JITOperationFailedException(workItem->codeGenResult); |
There was a problem hiding this comment.
wonder if we want to assert against this or even throw fatal error? i think it should only happen in case this process is already dead
There was a problem hiding this comment.
I'm removing this, for client side just use fatal error for now. we can change to turning off JIT in some cases(like JIT server is dead) in the future.
In reply to: 81824509 [](ancestors = 81824509)
| AUTO_NESTED_HANDLED_EXCEPTION_TYPE(static_cast<ExceptionType>(ExceptionType_OutOfMemory | ExceptionType_StackOverflow)); | ||
|
|
||
| ServerThreadContext * contextInfo = HeapNew(ServerThreadContext, threadContextData); | ||
| ServerThreadContext * contextInfo = HeapNewNoThrow(ServerThreadContext, threadContextData); |
There was a problem hiding this comment.
i'm pretty sure rpc handles the OOM and returns E_OUTOFMEMORY. but if we don't want to use that, maybe we should remove the AUTO_NESTED_HANDLED_EXCEPTION_TYPE?
There was a problem hiding this comment.
I don't think rpc will handle our type of OOM, it might handle it's own OOM. but yes, I think we still should use throw allocator because the following RegisterThreadContext would throw as well
In reply to: 81825163 [](ancestors = 81825163)
There was a problem hiding this comment.
btw, looks we don't need AUTO_NESTED_HANDLED_EXCEPTION_TYPE anywhere in this file, that looks like mostly for run time in script to prevent unknown exception get out of scope. the Assertion inside is always true here in this file (see !JsUtil::ExternalApi::IsScriptActiveOnCurrentThreadContext() )
In reply to: 81828161 [](ancestors = 81828161,81825163)
There was a problem hiding this comment.
are you sure? I thought i have seen it fire before. but i guess my point was we should probably have try/catch or use no throw allocator
In reply to: 81830381 [](ancestors = 81830381,81828161,81825163)
|
|
|
@dotnet-bot test Copyright Check please |
adding a new type of exception to indicate system call failed(mostly because content process is terminated/crash and VM is deleted, the JIT server is still trying to do the VM operations, which fails) wrap all server call in exception handling make relationship between scriptcontext and threadcontext in server side
Assertion on server side for client calling with wrong args or at wrong state to help diagnose the issue
should check exception while freeing code address fix prefast warning and linux build
leirocks
force-pushed
the
oopjiterrorhandle
branch
from
61e0bad to
da70259
Compare
Merge pull request #1689 from leirocks:oopjiterrorhandle adding a new type of exception to indicate system call failed(mostly because content process is terminated/crash and VM is deleted, the JIT server is still trying to do the VM operations, which fails) wrap all server call in exception handling make relationship between scriptcontext and threadcontext in server side
5 participants
adding a new type of exception to indicate system call failed(mostly because content process is terminated/crash and VM is deleted, the JIT server is still trying to do the VM operations, which fails)
wrap all server call in exception handling
make relationship between scriptcontext and threadcontext in server side