WebKit Bugzilla
Attachment 357357 Details for
Bug 192717
: CallFrame::convertToStackOverflowFrame() needs to keep the top CodeBlock alive.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
proposed patch.
bug-192717.patch (text/plain), 11.83 KB, created by
Mark Lam
on 2018-12-14 16:50:27 PST
(
hide
)
Description:
proposed patch.
Filename:
MIME Type:
Creator:
Mark Lam
Created:
2018-12-14 16:50:27 PST
Size:
11.83 KB
patch
obsolete
>Index: JSTests/ChangeLog >=================================================================== >--- JSTests/ChangeLog (revision 239241) >+++ JSTests/ChangeLog (working copy) >@@ -1,3 +1,13 @@ >+2018-12-14 Mark Lam <mark.lam@apple.com> >+ >+ CallFrame::convertToStackOverflowFrame() needs to keep the top CodeBlock alive. >+ https://bugs.webkit.org/show_bug.cgi?id=192717 >+ <rdar://problem/46660677> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ * stress/regress-192717.js: Added. >+ > 2018-12-14 Commit Queue <commit-queue@webkit.org> > > Unreviewed, rolling out r239153, r239154, and r239155. >Index: JSTests/stress/regress-192717.js >=================================================================== >--- JSTests/stress/regress-192717.js (nonexistent) >+++ JSTests/stress/regress-192717.js (working copy) >@@ -0,0 +1,16 @@ >+//@ runDefault("--useLLInt=false", "--forceCodeBlockToJettisonDueToOldAge=true", "--maxPerThreadStackUsage=200000", "--exceptionStackTraceLimit=1", "--defaultErrorStackTraceLimit=1") >+//@ skip if $memoryLimited or $buildType == "debug" >+ >+let foo = 'let a'; >+for (let i = 0; i < 400000; i++) >+ foo += ',a' + i; >+ >+var exception; >+try { >+ new Function(foo)(); >+} catch (e) { >+ exception = e; >+} >+ >+if (exception != "RangeError: Maximum call stack size exceeded.") >+ throw "FAILED"; >Index: Source/JavaScriptCore/ChangeLog >=================================================================== >--- Source/JavaScriptCore/ChangeLog (revision 239235) >+++ Source/JavaScriptCore/ChangeLog (working copy) >@@ -1,3 +1,48 @@ >+2018-12-14 Mark Lam <mark.lam@apple.com> >+ >+ CallFrame::convertToStackOverflowFrame() needs to keep the top CodeBlock alive. >+ https://bugs.webkit.org/show_bug.cgi?id=192717 >+ <rdar://problem/46660677> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ 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 correspond 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. >+ >+ We removed the call to convertToStackOverflowFrame() in >+ lookupExceptionHandlerFromCallerFrame() because it is redundant. >+ lookupExceptionHandlerFromCallerFrame() will only every be called after >+ a StackOverFlowError has been thrown. Hence, the top frame is already >+ guaranteed to be a StackOverflowFrame, and there should always be a >+ StackOverFlowError exception pending. We added assertions for these >+ instead. >+ >+ * interpreter/CallFrame.cpp: >+ (JSC::CallFrame::convertToStackOverflowFrame): >+ * interpreter/CallFrame.h: >+ * jit/JITOperations.cpp: >+ * llint/LLIntSlowPaths.cpp: >+ (JSC::LLInt::LLINT_SLOW_PATH_DECL): >+ * runtime/CommonSlowPaths.cpp: >+ (JSC::SLOW_PATH_DECL): >+ * runtime/CommonSlowPaths.h: >+ (JSC::CommonSlowPaths::codeBlockFromCallFrameCallee): >+ (JSC::CommonSlowPaths::arityCheckFor): >+ * runtime/VM.h: >+ (JSC::VM::exceptionForInspection const): >+ > 2018-12-14 Commit Queue <commit-queue@webkit.org> > > Unreviewed, rolling out r239153, r239154, and r239155. >Index: Source/JavaScriptCore/interpreter/CallFrame.cpp >=================================================================== >--- Source/JavaScriptCore/interpreter/CallFrame.cpp (revision 239235) >+++ Source/JavaScriptCore/interpreter/CallFrame.cpp (working copy) >@@ -337,9 +337,10 @@ const char* CallFrame::describeFrame() > return buffer; > } > >-void CallFrame::convertToStackOverflowFrame(VM& vm) >+void CallFrame::convertToStackOverflowFrame(VM& vm, CodeBlock* codeBlockToKeepAliveUntilFrameIsUnwound) > { > ASSERT(!isGlobalExec()); >+ ASSERT(codeBlockToKeepAliveUntilFrameIsUnwound->inherits<CodeBlock>(vm)); > > EntryFrame* entryFrame = vm.topEntryFrame; > CallFrame* throwOriginFrame = this; >@@ -350,7 +351,7 @@ void CallFrame::convertToStackOverflowFr > JSObject* originCallee = throwOriginFrame ? throwOriginFrame->jsCallee() : vmEntryRecord(vm.topEntryFrame)->callee(); > JSObject* stackOverflowCallee = originCallee->globalObject()->stackOverflowFrameCallee(); > >- setCodeBlock(nullptr); >+ setCodeBlock(codeBlockToKeepAliveUntilFrameIsUnwound); > setCallee(stackOverflowCallee); > setArgumentCountIncludingThis(0); > } >Index: Source/JavaScriptCore/interpreter/CallFrame.h >=================================================================== >--- Source/JavaScriptCore/interpreter/CallFrame.h (revision 239235) >+++ Source/JavaScriptCore/interpreter/CallFrame.h (working copy) >@@ -257,7 +257,7 @@ namespace JSC { > return callerFrameAndPC().callerFrame == noCaller() && callerFrameAndPC().returnPC == nullptr; > } > >- void convertToStackOverflowFrame(VM&); >+ void convertToStackOverflowFrame(VM&, CodeBlock* codeBlockToKeepAliveUntilFrameIsUnwound); > inline bool isStackOverflowFrame() const; > inline bool isWasmFrame() const; > >Index: Source/JavaScriptCore/jit/JITOperations.cpp >=================================================================== >--- Source/JavaScriptCore/jit/JITOperations.cpp (revision 239235) >+++ Source/JavaScriptCore/jit/JITOperations.cpp (working copy) >@@ -102,7 +102,7 @@ void JIT_OPERATION operationThrowStackOv > // We pass in our own code block, because the callframe hasn't been populated. > VM* vm = codeBlock->vm(); > auto scope = DECLARE_THROW_SCOPE(*vm); >- exec->convertToStackOverflowFrame(*vm); >+ exec->convertToStackOverflowFrame(*vm, codeBlock); > NativeCallFrameTracer tracer(vm, exec); > throwStackOverflowError(exec, scope); > } >@@ -113,8 +113,9 @@ int32_t JIT_OPERATION operationCallArity > auto scope = DECLARE_THROW_SCOPE(*vm); > > int32_t missingArgCount = CommonSlowPaths::arityCheckFor(exec, *vm, CodeForCall); >- if (missingArgCount < 0) { >- exec->convertToStackOverflowFrame(*vm); >+ if (UNLIKELY(missingArgCount < 0)) { >+ CodeBlock* codeBlock = CommonSlowPaths::codeBlockFromCallFrameCallee(exec, CodeForCall); >+ exec->convertToStackOverflowFrame(*vm, codeBlock); > NativeCallFrameTracer tracer(vm, exec); > throwStackOverflowError(vm->topCallFrame, scope); > } >@@ -128,8 +129,9 @@ int32_t JIT_OPERATION operationConstruct > auto scope = DECLARE_THROW_SCOPE(*vm); > > int32_t missingArgCount = CommonSlowPaths::arityCheckFor(exec, *vm, CodeForConstruct); >- if (missingArgCount < 0) { >- exec->convertToStackOverflowFrame(*vm); >+ if (UNLIKELY(missingArgCount < 0)) { >+ CodeBlock* codeBlock = CommonSlowPaths::codeBlockFromCallFrameCallee(exec, CodeForCall); >+ exec->convertToStackOverflowFrame(*vm, codeBlock); > NativeCallFrameTracer tracer(vm, exec); > throwStackOverflowError(vm->topCallFrame, scope); > } >@@ -2432,7 +2434,8 @@ void JIT_OPERATION lookupExceptionHandle > > void JIT_OPERATION lookupExceptionHandlerFromCallerFrame(VM* vm, ExecState* exec) > { >- exec->convertToStackOverflowFrame(*vm); >+ ASSERT(exec->isStackOverflowFrame()); >+ ASSERT(jsCast<ErrorInstance*>(vm->exceptionForInspection()->value().asCell())->isStackOverflowError()); > lookupExceptionHandler(vm, exec); > } > >Index: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp >=================================================================== >--- Source/JavaScriptCore/llint/LLIntSlowPaths.cpp (revision 239235) >+++ Source/JavaScriptCore/llint/LLIntSlowPaths.cpp (working copy) >@@ -559,7 +559,7 @@ LLINT_SLOW_PATH_DECL(stack_check) > } > #endif > >- exec->convertToStackOverflowFrame(vm); >+ exec->convertToStackOverflowFrame(vm, codeBlock); > ErrorHandlingScope errorScope(vm); > throwStackOverflowError(exec, throwScope); > pc = returnToThrow(exec); >Index: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp >=================================================================== >--- Source/JavaScriptCore/runtime/CommonSlowPaths.cpp (revision 239235) >+++ Source/JavaScriptCore/runtime/CommonSlowPaths.cpp (working copy) >@@ -177,8 +177,9 @@ SLOW_PATH_DECL(slow_path_call_arityCheck > { > BEGIN(); > int slotsToAdd = CommonSlowPaths::arityCheckFor(exec, vm, CodeForCall); >- if (slotsToAdd < 0) { >- exec->convertToStackOverflowFrame(vm); >+ if (UNLIKELY(slotsToAdd < 0)) { >+ CodeBlock* codeBlock = CommonSlowPaths::codeBlockFromCallFrameCallee(exec, CodeForCall); >+ exec->convertToStackOverflowFrame(vm, codeBlock); > NativeCallFrameTracer tracer(&vm, exec); > ErrorHandlingScope errorScope(vm); > throwScope.release(); >@@ -192,8 +193,9 @@ SLOW_PATH_DECL(slow_path_construct_arity > { > BEGIN(); > int slotsToAdd = CommonSlowPaths::arityCheckFor(exec, vm, CodeForConstruct); >- if (slotsToAdd < 0) { >- exec->convertToStackOverflowFrame(vm); >+ if (UNLIKELY(slotsToAdd < 0)) { >+ CodeBlock* codeBlock = CommonSlowPaths::codeBlockFromCallFrameCallee(exec, CodeForCall); >+ exec->convertToStackOverflowFrame(vm, codeBlock); > NativeCallFrameTracer tracer(&vm, exec); > ErrorHandlingScope errorScope(vm); > throwArityCheckStackOverflowError(exec, throwScope); >Index: Source/JavaScriptCore/runtime/CommonSlowPaths.h >=================================================================== >--- Source/JavaScriptCore/runtime/CommonSlowPaths.h (revision 239235) >+++ Source/JavaScriptCore/runtime/CommonSlowPaths.h (working copy) >@@ -72,11 +72,16 @@ ALWAYS_INLINE int numberOfStackPaddingSl > return numberOfStackPaddingSlots(codeBlock, argumentCountIncludingThis) + numberOfExtraSlots(argumentCountIncludingThis); > } > >-ALWAYS_INLINE int arityCheckFor(ExecState* exec, VM& vm, CodeSpecializationKind kind) >+ALWAYS_INLINE CodeBlock* codeBlockFromCallFrameCallee(ExecState* exec, CodeSpecializationKind kind) > { > JSFunction* callee = jsCast<JSFunction*>(exec->jsCallee()); > ASSERT(!callee->isHostFunction()); >- CodeBlock* newCodeBlock = callee->jsExecutable()->codeBlockFor(kind); >+ return callee->jsExecutable()->codeBlockFor(kind); >+} >+ >+ALWAYS_INLINE int arityCheckFor(ExecState* exec, VM& vm, CodeSpecializationKind kind) >+{ >+ CodeBlock* newCodeBlock = codeBlockFromCallFrameCallee(exec, kind); > ASSERT(exec->argumentCountIncludingThis() < static_cast<unsigned>(newCodeBlock->numParameters())); > int padding = numberOfStackPaddingSlotsWithExtraSlots(newCodeBlock, exec->argumentCountIncludingThis()); > >Index: Source/JavaScriptCore/runtime/VM.h >=================================================================== >--- Source/JavaScriptCore/runtime/VM.h (revision 239235) >+++ Source/JavaScriptCore/runtime/VM.h (working copy) >@@ -693,6 +693,10 @@ public: > Exception* lastException() const { return m_lastException; } > JSCell** addressOfLastException() { return reinterpret_cast<JSCell**>(&m_lastException); } > >+ // This should only be used for test or assertion code that wants to inspect >+ // the pending exception without interfering with Throw/CatchScopes. >+ Exception* exceptionForInspection() const { return m_exception; } >+ > void setFailNextNewCodeBlock() { m_failNextNewCodeBlock = true; } > bool getAndClearFailNextNewCodeBlock() > {
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 192717
:
357344
| 357357