Attachment 357357 Details for Bug 192717 – proposed patch.

[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:

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()
>     {

Actions: View | Formatted Diff | Diff

Attachments on bug 192717: 357344 | 357357