[JSC] Reload CodeBlock or suppress GC while setting up calls
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Mar 2020 23:20:45 +0000 (23:20 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Mar 2020 23:20:45 +0000 (23:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=209033
<rdar://problem/58946936>

Reviewed by Saam Barati.

The sequence of Interpreter::execute is the following.

    1. Getting CodeBlock from Executable
    2. Doing a lot of setups
    3. Setting (1)'s CodeBlock to ProtoFrame
    4. Calling code through Executable

During (2), it would be possible that GC happens and it replaces CodeBlock in Executable.
Then, when executing JITCode with CodeBlock in (4), we use new JITCode with old CodeBlock.

In this patch,

For ProgramExecutable, FunctionExecutable, ModuleProgramExecutable, we ensure that no GC happens
after getting CodeBlock by placing DisallowGC. For EvalExecutable, we reload CodeBlock after setting
up environment. It is possible that FunctionExecutable* stored in CodeBlock can be different when
executing a new CodeBlock, but this is OK since this different does not appear and we do not rely on
this: we are touching `name` of FunctionExecutable* which is retrieved from CodeBlock. But this name
will not be changed since this is derived from UnlinkedFunctionExecutable which is shared by multiple
CodeBlocks. And FunctionExecutable* generation ordering must be the same for every CodeBlock generation
from the same UnlinkedCodeBlock.

* bytecode/CodeBlock.h:
(JSC::ScriptExecutable::prepareForExecution):
* interpreter/Interpreter.cpp:
(JSC::Interpreter::executeProgram):
(JSC::Interpreter::executeCall):
(JSC::Interpreter::executeConstruct):
(JSC::Interpreter::execute):
(JSC::Interpreter::executeModuleProgram):
* interpreter/InterpreterInlines.h:
(JSC::Interpreter::execute):
* runtime/DisallowScope.h:
(JSC::DisallowScope::disable):
* runtime/StringPrototype.cpp:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@258443 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/interpreter/Interpreter.cpp
Source/JavaScriptCore/interpreter/InterpreterInlines.h
Source/JavaScriptCore/runtime/DisallowScope.h
Source/JavaScriptCore/runtime/StringPrototype.cpp

index bca8a34..462eeaf 100644 (file)
@@ -1,3 +1,46 @@
+2020-03-13  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Reload CodeBlock or suppress GC while setting up calls
+        https://bugs.webkit.org/show_bug.cgi?id=209033
+        <rdar://problem/58946936>
+
+        Reviewed by Saam Barati.
+
+        The sequence of Interpreter::execute is the following.
+
+            1. Getting CodeBlock from Executable
+            2. Doing a lot of setups
+            3. Setting (1)'s CodeBlock to ProtoFrame
+            4. Calling code through Executable
+
+        During (2), it would be possible that GC happens and it replaces CodeBlock in Executable.
+        Then, when executing JITCode with CodeBlock in (4), we use new JITCode with old CodeBlock.
+
+        In this patch,
+
+        For ProgramExecutable, FunctionExecutable, ModuleProgramExecutable, we ensure that no GC happens
+        after getting CodeBlock by placing DisallowGC. For EvalExecutable, we reload CodeBlock after setting
+        up environment. It is possible that FunctionExecutable* stored in CodeBlock can be different when
+        executing a new CodeBlock, but this is OK since this different does not appear and we do not rely on
+        this: we are touching `name` of FunctionExecutable* which is retrieved from CodeBlock. But this name
+        will not be changed since this is derived from UnlinkedFunctionExecutable which is shared by multiple
+        CodeBlocks. And FunctionExecutable* generation ordering must be the same for every CodeBlock generation
+        from the same UnlinkedCodeBlock.
+
+        * bytecode/CodeBlock.h:
+        (JSC::ScriptExecutable::prepareForExecution):
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::executeProgram):
+        (JSC::Interpreter::executeCall):
+        (JSC::Interpreter::executeConstruct):
+        (JSC::Interpreter::execute):
+        (JSC::Interpreter::executeModuleProgram):
+        * interpreter/InterpreterInlines.h:
+        (JSC::Interpreter::execute):
+        * runtime/DisallowScope.h:
+        (JSC::DisallowScope::disable):
+        * runtime/StringPrototype.cpp:
+
 2020-03-12  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] Delete IC creation should check mayNeedToCheckCell/canCacheDeleteIC regardless of Structure::outOfLineCapacity
index f020be4..671fb3e 100644 (file)
@@ -1058,16 +1058,23 @@ template <typename ExecutableType>
 Exception* ScriptExecutable::prepareForExecution(VM& vm, JSFunction* function, JSScope* scope, CodeSpecializationKind kind, CodeBlock*& resultCodeBlock)
 {
     if (hasJITCodeFor(kind)) {
-        if (std::is_same<ExecutableType, EvalExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<EvalExecutable*>(this)->codeBlock());
-        else if (std::is_same<ExecutableType, ProgramExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ProgramExecutable*>(this)->codeBlock());
-        else if (std::is_same<ExecutableType, ModuleProgramExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ModuleProgramExecutable*>(this)->codeBlock());
-        else if (std::is_same<ExecutableType, FunctionExecutable>::value)
-            resultCodeBlock = jsCast<CodeBlock*>(jsCast<FunctionExecutable*>(this)->codeBlockFor(kind));
-        else
-            RELEASE_ASSERT_NOT_REACHED();
+        if constexpr (std::is_same<ExecutableType, EvalExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlock());
+            return nullptr;
+        }
+        if constexpr (std::is_same<ExecutableType, ProgramExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlock());
+            return nullptr;
+        }
+        if constexpr (std::is_same<ExecutableType, ModuleProgramExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlock());
+            return nullptr;
+        }
+        if constexpr (std::is_same<ExecutableType, FunctionExecutable>::value) {
+            resultCodeBlock = jsCast<CodeBlock*>(jsCast<ExecutableType*>(this)->codeBlockFor(kind));
+            return nullptr;
+        }
+        RELEASE_ASSERT_NOT_REACHED();
         return nullptr;
     }
     return prepareForExecutionImpl(vm, function, scope, kind, resultCodeBlock);
index bf2984a..1ec7070 100644 (file)
@@ -822,6 +822,15 @@ failedJSONP:
     if (UNLIKELY(error))
         return checkedReturn(throwException(globalObject, throwScope, error));
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    if (scope->structure(vm)->isUncacheableDictionary())
+        scope->flattenDictionaryObject(vm);
+
     ProgramCodeBlock* codeBlock;
     {
         CodeBlock* tempCodeBlock;
@@ -830,25 +839,21 @@ failedJSONP:
         if (UNLIKELY(error))
             return checkedReturn(error);
         codeBlock = jsCast<ProgramCodeBlock*>(tempCodeBlock);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
     }
 
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
-    }
-
-    if (scope->structure(vm)->isUncacheableDictionary())
-        scope->flattenDictionaryObject(vm);
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
 
-    ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
+    RefPtr<JITCode> jitCode = program->generatedJITCode();
 
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(codeBlock, globalObject, globalCallee, thisObj, 1);
 
     // Execute the code:
+    disallowGC.disable();
     throwScope.release();
-    JSValue result = program->generatedJITCode()->execute(&vm, &protoCallFrame);
+    ASSERT(jitCode == program->generatedJITCode().ptr());
+    JSValue result = jitCode->execute(&vm, &protoCallFrame);
     return checkedReturn(result);
 }
 
@@ -864,7 +869,6 @@ JSValue Interpreter::executeCall(JSGlobalObject* lexicalGlobalObject, JSObject*
 
     bool isJSCall = (callType == CallType::JS);
     JSScope* scope = nullptr;
-    CodeBlock* newCodeBlock;
     size_t argsCount = 1 + args.size(); // implicit "this" parameter
 
     JSGlobalObject* globalObject;
@@ -881,6 +885,13 @@ JSValue Interpreter::executeCall(JSGlobalObject* lexicalGlobalObject, JSObject*
     if (UNLIKELY(!vm.isSafeToRecurseSoft() || args.size() > maxArguments))
         return checkedReturn(throwStackOverflowError(globalObject, throwScope));
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    CodeBlock* newCodeBlock = nullptr;
     if (isJSCall) {
         // Compile the callee:
         Exception* compileError = callData.js.functionExecutable->prepareForExecution<FunctionExecutable>(vm, jsCast<JSFunction*>(function), scope, CodeForCall, newCodeBlock);
@@ -890,24 +901,23 @@ JSValue Interpreter::executeCall(JSGlobalObject* lexicalGlobalObject, JSObject*
 
         ASSERT(!!newCodeBlock);
         newCodeBlock->m_shouldAlwaysBeInlined = false;
-    } else
-        newCodeBlock = 0;
-
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
     }
 
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+
+    RefPtr<JITCode> jitCode = callData.js.functionExecutable->generatedJITCodeForCall();
+
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(newCodeBlock, globalObject, function, thisValue, argsCount, args.data());
 
     JSValue result;
     {
         // Execute the code:
+        disallowGC.disable();
         if (isJSCall) {
             throwScope.release();
-            result = callData.js.functionExecutable->generatedJITCodeForCall()->execute(&vm, &protoCallFrame);
+            ASSERT(jitCode == callData.js.functionExecutable->generatedJITCodeForCall().ptr());
+            result = jitCode->execute(&vm, &protoCallFrame);
         } else {
             result = JSValue::decode(vmEntryToNative(callData.native.function.rawPointer(), &vm, &protoCallFrame));
             RETURN_IF_EXCEPTION(throwScope, JSValue());
@@ -933,7 +943,6 @@ JSObject* Interpreter::executeConstruct(JSGlobalObject* lexicalGlobalObject, JSO
 
     bool isJSConstruct = (constructType == ConstructType::JS);
     JSScope* scope = nullptr;
-    CodeBlock* newCodeBlock;
     size_t argsCount = 1 + args.size(); // implicit "this" parameter
 
     JSGlobalObject* globalObject;
@@ -952,6 +961,13 @@ JSObject* Interpreter::executeConstruct(JSGlobalObject* lexicalGlobalObject, JSO
         return nullptr;
     }
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, nullptr);
+    }
+
+    CodeBlock* newCodeBlock = nullptr;
     if (isJSConstruct) {
         // Compile the callee:
         Exception* compileError = constructData.js.functionExecutable->prepareForExecution<FunctionExecutable>(vm, jsCast<JSFunction*>(constructor), scope, CodeForConstruct, newCodeBlock);
@@ -961,24 +977,23 @@ JSObject* Interpreter::executeConstruct(JSGlobalObject* lexicalGlobalObject, JSO
 
         ASSERT(!!newCodeBlock);
         newCodeBlock->m_shouldAlwaysBeInlined = false;
-    } else
-        newCodeBlock = 0;
-
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, nullptr);
     }
 
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+
+    RefPtr<JITCode> jitCode = constructData.js.functionExecutable->generatedJITCodeForConstruct();
+
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(newCodeBlock, globalObject, constructor, newTarget, argsCount, args.data());
 
     JSValue result;
     {
         // Execute the code.
-        if (isJSConstruct)
-            result = constructData.js.functionExecutable->generatedJITCodeForConstruct()->execute(&vm, &protoCallFrame);
-        else {
+        disallowGC.disable();
+        if (isJSConstruct) {
+            ASSERT(jitCode == constructData.js.functionExecutable->generatedJITCodeForConstruct().ptr());
+            result = jitCode->execute(&vm, &protoCallFrame);
+        } else {
             result = JSValue::decode(vmEntryToNative(constructData.native.function.rawPointer(), &vm, &protoCallFrame));
 
             if (LIKELY(!throwScope.exception()))
@@ -1058,14 +1073,29 @@ JSValue Interpreter::execute(EvalExecutable* eval, JSGlobalObject* lexicalGlobal
         }
     }
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    auto loadCodeBlock = [&](Exception*& compileError) -> EvalCodeBlock* {
+        CodeBlock* tempCodeBlock;
+        compileError = eval->prepareForExecution<EvalExecutable>(vm, nullptr, scope, CodeForCall, tempCodeBlock);
+        EXCEPTION_ASSERT(throwScope.exception() == compileError);
+        if (UNLIKELY(!!compileError))
+            return nullptr;
+        return jsCast<EvalCodeBlock*>(tempCodeBlock);
+    };
+
     EvalCodeBlock* codeBlock;
     {
-        CodeBlock* tempCodeBlock;
-        Exception* compileError = eval->prepareForExecution<EvalExecutable>(vm, nullptr, scope, CodeForCall, tempCodeBlock);
+        Exception* compileError = nullptr;
+        codeBlock = loadCodeBlock(compileError);
         EXCEPTION_ASSERT(throwScope.exception() == compileError);
         if (UNLIKELY(!!compileError))
             return checkedReturn(compileError);
-        codeBlock = jsCast<EvalCodeBlock*>(tempCodeBlock);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
     }
     UnlinkedEvalCodeBlock* unlinkedCodeBlock = codeBlock->unlinkedEvalCodeBlock();
 
@@ -1148,25 +1178,34 @@ JSValue Interpreter::execute(EvalExecutable* eval, JSGlobalObject* lexicalGlobal
         }
     }
 
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
-    }
-
-    ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
-
     JSCallee* callee = nullptr;
     if (scope == globalObject->globalScope())
         callee = globalObject->globalCallee();
     else
         callee = JSCallee::create(vm, globalObject, scope);
+
+    // Reload CodeBlock. It is possible that we replaced CodeBlock while setting up the environment.
+    {
+        Exception* compileError = nullptr;
+        codeBlock = loadCodeBlock(compileError);
+        EXCEPTION_ASSERT(throwScope.exception() == compileError);
+        if (UNLIKELY(!!compileError))
+            return checkedReturn(compileError);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
+    }
+
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+
+    RefPtr<JITCode> jitCode = eval->generatedJITCode();
+
     ProtoCallFrame protoCallFrame;
     protoCallFrame.init(codeBlock, globalObject, callee, thisValue, 1);
 
     // Execute the code:
+    disallowGC.disable();
     throwScope.release();
-    JSValue result = eval->generatedJITCode()->execute(&vm, &protoCallFrame);
+    ASSERT(jitCode == eval->generatedJITCode().ptr());
+    JSValue result = jitCode->execute(&vm, &protoCallFrame);
 
     return checkedReturn(result);
 }
@@ -1188,6 +1227,16 @@ JSValue Interpreter::executeModuleProgram(ModuleProgramExecutable* executable, J
     if (UNLIKELY(!vm.isSafeToRecurseSoft()))
         return checkedReturn(throwStackOverflowError(globalObject, throwScope));
 
+    constexpr auto trapsMask = VMTraps::interruptingTraps();
+    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
+        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
+    }
+
+    if (scope->structure(vm)->isUncacheableDictionary())
+        scope->flattenDictionaryObject(vm);
+
+    JSCallee* callee = JSCallee::create(vm, globalObject, scope);
     ModuleProgramCodeBlock* codeBlock;
     {
         CodeBlock* tempCodeBlock;
@@ -1196,28 +1245,24 @@ JSValue Interpreter::executeModuleProgram(ModuleProgramExecutable* executable, J
         if (UNLIKELY(!!compileError))
             return checkedReturn(compileError);
         codeBlock = jsCast<ModuleProgramCodeBlock*>(tempCodeBlock);
+        ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
     }
 
-    constexpr auto trapsMask = VMTraps::interruptingTraps();
-    if (UNLIKELY(vm.needTrapHandling(trapsMask))) {
-        vm.handleTraps(globalObject, vm.topCallFrame, trapsMask);
-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
-    }
-
-    if (scope->structure(vm)->isUncacheableDictionary())
-        scope->flattenDictionaryObject(vm);
+    DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
 
-    ASSERT(codeBlock->numParameters() == 1); // 1 parameter for 'this'.
+    RefPtr<JITCode> jitCode = executable->generatedJITCode();
 
     // The |this| of the module is always `undefined`.
     // http://www.ecma-international.org/ecma-262/6.0/#sec-module-environment-records-hasthisbinding
     // http://www.ecma-international.org/ecma-262/6.0/#sec-module-environment-records-getthisbinding
     ProtoCallFrame protoCallFrame;
-    protoCallFrame.init(codeBlock, globalObject, JSCallee::create(vm, globalObject, scope), jsUndefined(), 1);
+    protoCallFrame.init(codeBlock, globalObject, callee, jsUndefined(), 1);
 
     // Execute the code:
+    disallowGC.disable();
     throwScope.release();
-    JSValue result = executable->generatedJITCode()->execute(&vm, &protoCallFrame);
+    ASSERT(jitCode == executable->generatedJITCode().ptr());
+    JSValue result = jitCode->execute(&vm, &protoCallFrame);
 
     return checkedReturn(result);
 }
index f44d965..45074fc 100644 (file)
 
 #include "CallFrameClosure.h"
 #include "Exception.h"
+#include "FunctionCodeBlock.h"
+#include "FunctionExecutable.h"
 #include "Instruction.h"
 #include "Interpreter.h"
 #include "JSCPtrTag.h"
 #include "LLIntData.h"
+#include "ProtoCallFrameInlines.h"
 #include "UnlinkedCodeBlock.h"
 #include <wtf/UnalignedAccess.h>
 
@@ -80,6 +83,17 @@ ALWAYS_INLINE JSValue Interpreter::execute(CallFrameClosure& closure)
         RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
     }
 
+    // Reload CodeBlock since GC can replace CodeBlock owned by Executable.
+    CodeBlock* codeBlock;
+    Exception* error = closure.functionExecutable->prepareForExecution<FunctionExecutable>(vm, closure.function, closure.scope, CodeForCall, codeBlock);
+    EXCEPTION_ASSERT(throwScope.exception() == error);
+    if (UNLIKELY(error))
+        return checkedReturn(error);
+    codeBlock->m_shouldAlwaysBeInlined = false;
+    {
+        DisallowGC disallowGC; // Ensure no GC happens. GC can replace CodeBlock in Executable.
+        closure.protoCallFrame->setCodeBlock(codeBlock);
+    }
     // Execute the code:
     throwScope.release();
     JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);
index 2e3ad5a..5311486 100644 (file)
@@ -41,6 +41,7 @@ public:
     ALWAYS_INLINE ~DisallowScope() { }
     ALWAYS_INLINE static bool isInEffectOnCurrentThread() { return false; }
     ALWAYS_INLINE void enable() { }
+    ALWAYS_INLINE void disable() { }
 
 #else // not NDEBUG
 
index 53b8778..fd38d6f 100644 (file)
@@ -27,6 +27,7 @@
 #include "ButterflyInlines.h"
 #include "CachedCall.h"
 #include "Error.h"
+#include "ExecutableBaseInlines.h"
 #include "FrameTracers.h"
 #include "InterpreterInlines.h"
 #include "IntlCollator.h"