[JSC] Retain PrivateName of Symbol before passing it to operations potentially incurr...
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 05:56:24 +0000 (05:56 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Mar 2019 05:56:24 +0000 (05:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195791
<rdar://problem/48806130>

Reviewed by Mark Lam.

JSTests:

* stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js: Added.
(foo):

Source/JavaScriptCore:

Consider the following example:

    void putByVal(JSObject*, PropertyName propertyName, ...);

    putByVal(object, symbol->privateName(), ...);

PropertyName does not retain the passed UniquedStringImpl*. It just holds the pointer to UniquedStringImpl*.
It means that since `Symbol::privateName()` returns `const PrivateName&` instead of `PrivateName`, putByVal
and its caller does not retain UniquedStringImpl* held in PropertyName. The problem happens when the putByVal
incurs GC, and when the `symbol` is missing in the conservative GC scan. The underlying UniquedStringImpl* of
PropertyName can be accidentally destroyed in the middle of the putByVal operation. We should retain PrivateName
before passing it to operations which takes it as PropertyName.

1. We use the code pattern like this.

    auto propertyName = symbol->privateName();
    someOperation(..., propertyName);

This pattern is well aligned to existing `JSValue::toPropertyKey(exec)` and `JSString::toIdentifier(exec)` code patterns.

    auto propertyName = value.toPropertyKey(exec);
    RETURN_IF_EXCEPTION(scope, { });
    someOperation(..., propertyName);

2. We change `Symbol::privateName()` to returning `PrivateName` instead of `const PrivateName&` to avoid
   potential dangerous use cases. This is OK because the code using `Symbol::privateName()` is not a critical path,
   and they typically need to retain PrivateName.

3. We audit similar functions `toPropertyKey(exec)` and `toIdentifier(exec)` for needed but missing exception checks.
   BTW, these functions are safe to the problem fixed in this patch since they return `Identifier` instead
   of `const Identifier&`.

Mark and Robin investigated and offered important data to understand what went wrong. And figured out the reason behind
the mysterious behavior shown in the data, and now, we confirm that this is the right fix for this bug.

* dfg/DFGOperations.cpp:
* jit/JITOperations.cpp:
(JSC::tryGetByValOptimize):
* runtime/JSFunction.cpp:
(JSC::JSFunction::setFunctionName):
* runtime/JSModuleLoader.cpp:
(JSC::printableModuleKey):
* runtime/JSONObject.cpp:
(JSC::Stringifier::Stringifier):
* runtime/Symbol.cpp:
(JSC::Symbol::descriptiveString const):
(JSC::Symbol::description const):
* runtime/Symbol.h:
* runtime/SymbolConstructor.cpp:
(JSC::symbolConstructorKeyFor):
* tools/JSDollarVM.cpp:
(JSC::functionGetGetterSetter):

Source/WebCore:

* bindings/js/ScriptController.cpp:
(WebCore::ScriptController::setupModuleScriptHandlers):

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

14 files changed:
JSTests/ChangeLog
JSTests/stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/runtime/JSFunction.cpp
Source/JavaScriptCore/runtime/JSModuleLoader.cpp
Source/JavaScriptCore/runtime/JSONObject.cpp
Source/JavaScriptCore/runtime/Symbol.cpp
Source/JavaScriptCore/runtime/Symbol.h
Source/JavaScriptCore/runtime/SymbolConstructor.cpp
Source/JavaScriptCore/tools/JSDollarVM.cpp
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/ScriptController.cpp

index b0c7a68..95c553a 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
+        https://bugs.webkit.org/show_bug.cgi?id=195791
+        <rdar://problem/48806130>
+
+        Reviewed by Mark Lam.
+
+        * stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js: Added.
+        (foo):
+
 2019-03-14  Saam barati  <sbarati@apple.com>
 
         We can't remove code after ForceOSRExit until after FixupPhase
diff --git a/JSTests/stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js b/JSTests/stress/symbol-is-destructed-before-refing-underlying-symbol-impl.js
new file mode 100644 (file)
index 0000000..4f414a7
--- /dev/null
@@ -0,0 +1,13 @@
+//@ skip if $buildType == "debug"
+//@ slow!
+//@ runDefault("--collectContinuously=1", "--slowPathAllocsBetweenGCs=100")
+
+function foo(a) {
+    a[Symbol()] = 1;
+}
+noInline(foo);
+
+for (let i = 0; i < 3e7; i++) {
+    let a = {};
+    foo(a);
+}
index 2c9ecad..2f17231 100644 (file)
@@ -1,5 +1,66 @@
 2019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
+        https://bugs.webkit.org/show_bug.cgi?id=195791
+        <rdar://problem/48806130>
+
+        Reviewed by Mark Lam.
+
+        Consider the following example:
+
+            void putByVal(JSObject*, PropertyName propertyName, ...);
+
+            putByVal(object, symbol->privateName(), ...);
+
+        PropertyName does not retain the passed UniquedStringImpl*. It just holds the pointer to UniquedStringImpl*.
+        It means that since `Symbol::privateName()` returns `const PrivateName&` instead of `PrivateName`, putByVal
+        and its caller does not retain UniquedStringImpl* held in PropertyName. The problem happens when the putByVal
+        incurs GC, and when the `symbol` is missing in the conservative GC scan. The underlying UniquedStringImpl* of
+        PropertyName can be accidentally destroyed in the middle of the putByVal operation. We should retain PrivateName
+        before passing it to operations which takes it as PropertyName.
+
+        1. We use the code pattern like this.
+
+            auto propertyName = symbol->privateName();
+            someOperation(..., propertyName);
+
+        This pattern is well aligned to existing `JSValue::toPropertyKey(exec)` and `JSString::toIdentifier(exec)` code patterns.
+
+            auto propertyName = value.toPropertyKey(exec);
+            RETURN_IF_EXCEPTION(scope, { });
+            someOperation(..., propertyName);
+
+        2. We change `Symbol::privateName()` to returning `PrivateName` instead of `const PrivateName&` to avoid
+           potential dangerous use cases. This is OK because the code using `Symbol::privateName()` is not a critical path,
+           and they typically need to retain PrivateName.
+
+        3. We audit similar functions `toPropertyKey(exec)` and `toIdentifier(exec)` for needed but missing exception checks.
+           BTW, these functions are safe to the problem fixed in this patch since they return `Identifier` instead
+           of `const Identifier&`.
+
+        Mark and Robin investigated and offered important data to understand what went wrong. And figured out the reason behind
+        the mysterious behavior shown in the data, and now, we confirm that this is the right fix for this bug.
+
+        * dfg/DFGOperations.cpp:
+        * jit/JITOperations.cpp:
+        (JSC::tryGetByValOptimize):
+        * runtime/JSFunction.cpp:
+        (JSC::JSFunction::setFunctionName):
+        * runtime/JSModuleLoader.cpp:
+        (JSC::printableModuleKey):
+        * runtime/JSONObject.cpp:
+        (JSC::Stringifier::Stringifier):
+        * runtime/Symbol.cpp:
+        (JSC::Symbol::descriptiveString const):
+        (JSC::Symbol::description const):
+        * runtime/Symbol.h:
+        * runtime/SymbolConstructor.cpp:
+        (JSC::symbolConstructorKeyFor):
+        * tools/JSDollarVM.cpp:
+        (JSC::functionGetGetterSetter):
+
+2019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
+
         REGRESSION(r242841): Fix conservative DFG OSR entry validation to accept values which will be stored in AnyInt / Double flush formats
         https://bugs.webkit.org/show_bug.cgi?id=195752
 
index a5d1848..7fb246e 100644 (file)
@@ -770,7 +770,8 @@ EncodedJSValue JIT_OPERATION operationGetByValObjectSymbol(ExecState* exec, JSCe
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
 
-    return JSValue::encode(getByValObject(exec, vm, asObject(base), asSymbol(symbol)->privateName()));
+    auto propertyName = asSymbol(symbol)->privateName();
+    return JSValue::encode(getByValObject(exec, vm, asObject(base), propertyName));
 }
 
 void JIT_OPERATION operationPutByValStrict(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedProperty, EncodedJSValue encodedValue)
@@ -826,7 +827,8 @@ void JIT_OPERATION operationPutByValCellSymbolStrict(ExecState* exec, JSCell* ce
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
 
-    putByValCellInternal<true, false>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
+    auto propertyName = asSymbol(symbol)->privateName();
+    putByValCellInternal<true, false>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
 }
 
 void JIT_OPERATION operationPutByValCellSymbolNonStrict(ExecState* exec, JSCell* cell, JSCell* symbol, EncodedJSValue encodedValue)
@@ -834,7 +836,8 @@ void JIT_OPERATION operationPutByValCellSymbolNonStrict(ExecState* exec, JSCell*
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
 
-    putByValCellInternal<false, false>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
+    auto propertyName = asSymbol(symbol)->privateName();
+    putByValCellInternal<false, false>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
 }
 
 void JIT_OPERATION operationPutByValBeyondArrayBoundsStrict(ExecState* exec, JSObject* object, int32_t index, EncodedJSValue encodedValue)
@@ -986,7 +989,8 @@ void JIT_OPERATION operationPutByValDirectCellSymbolStrict(ExecState* exec, JSCe
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
 
-    putByValCellInternal<true, true>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
+    auto propertyName = asSymbol(symbol)->privateName();
+    putByValCellInternal<true, true>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
 }
 
 void JIT_OPERATION operationPutByValDirectCellSymbolNonStrict(ExecState* exec, JSCell* cell, JSCell* symbol, EncodedJSValue encodedValue)
@@ -994,7 +998,8 @@ void JIT_OPERATION operationPutByValDirectCellSymbolNonStrict(ExecState* exec, J
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
 
-    putByValCellInternal<false, true>(exec, vm, cell, asSymbol(symbol)->privateName(), JSValue::decode(encodedValue));
+    auto propertyName = asSymbol(symbol)->privateName();
+    putByValCellInternal<false, true>(exec, vm, cell, propertyName, JSValue::decode(encodedValue));
 }
 
 void JIT_OPERATION operationPutByValDirectBeyondArrayBoundsStrict(ExecState* exec, JSObject* object, int32_t index, EncodedJSValue encodedValue)
@@ -2054,10 +2059,12 @@ char* JIT_OPERATION operationEnsureArrayStorage(ExecState* exec, JSCell* cell)
     return result;
 }
 
-EncodedJSValue JIT_OPERATION operationHasGenericProperty(ExecState* exec, EncodedJSValue encodedBaseValue, JSCell* propertyName)
+EncodedJSValue JIT_OPERATION operationHasGenericProperty(ExecState* exec, EncodedJSValue encodedBaseValue, JSCell* property)
 {
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     JSValue baseValue = JSValue::decode(encodedBaseValue);
     if (baseValue.isUndefinedOrNull())
         return JSValue::encode(jsBoolean(false));
@@ -2065,7 +2072,9 @@ EncodedJSValue JIT_OPERATION operationHasGenericProperty(ExecState* exec, Encode
     JSObject* base = baseValue.toObject(exec);
     if (!base)
         return JSValue::encode(JSValue());
-    return JSValue::encode(jsBoolean(base->hasPropertyGeneric(exec, asString(propertyName)->toIdentifier(exec), PropertySlot::InternalMethodType::GetOwnProperty)));
+    auto propertyName = asString(property)->toIdentifier(exec);
+    RETURN_IF_EXCEPTION(scope, { });
+    RELEASE_AND_RETURN(scope, JSValue::encode(jsBoolean(base->hasPropertyGeneric(exec, propertyName, PropertySlot::InternalMethodType::GetOwnProperty))));
 }
 
 size_t JIT_OPERATION operationHasIndexedPropertyByInt(ExecState* exec, JSCell* baseCell, int32_t subscript, int32_t internalMethodType)
index a496ce5..8261a69 100644 (file)
@@ -720,6 +720,7 @@ static OptimizationResult tryPutByValOptimize(ExecState* exec, JSValue baseValue
     OptimizationResult optimizationResult = OptimizationResult::NotOptimized;
 
     VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
 
     if (baseValue.isObject() && isCopyOnWrite(baseValue.getObject()->indexingMode()))
         return OptimizationResult::GiveUp;
@@ -750,6 +751,7 @@ static OptimizationResult tryPutByValOptimize(ExecState* exec, JSValue baseValue
 
     if (baseValue.isObject() && isStringOrSymbol(subscript)) {
         const Identifier propertyName = subscript.toPropertyKey(exec);
+        RETURN_IF_EXCEPTION(scope, OptimizationResult::GiveUp);
         if (subscript.isSymbol() || !parseIndex(propertyName)) {
             ASSERT(exec->bytecodeOffset());
             ASSERT(!byValInfo->stubRoutine);
@@ -790,16 +792,19 @@ void JIT_OPERATION operationPutByValOptimize(ExecState* exec, EncodedJSValue enc
 {
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
+    auto scope = DECLARE_THROW_SCOPE(vm);
 
     JSValue baseValue = JSValue::decode(encodedBaseValue);
     JSValue subscript = JSValue::decode(encodedSubscript);
     JSValue value = JSValue::decode(encodedValue);
-    if (tryPutByValOptimize(exec, baseValue, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS)) == OptimizationResult::GiveUp) {
+    OptimizationResult result = tryPutByValOptimize(exec, baseValue, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS));
+    RETURN_IF_EXCEPTION(scope, void());
+    if (result == OptimizationResult::GiveUp) {
         // Don't ever try to optimize.
         byValInfo->tookSlowPath = true;
         ctiPatchCallByReturnAddress(ReturnAddressPtr(OUR_RETURN_ADDRESS), operationPutByValGeneric);
     }
-    putByVal(exec, baseValue, subscript, value, byValInfo);
+    RELEASE_AND_RETURN(scope, putByVal(exec, baseValue, subscript, value, byValInfo));
 }
 
 static OptimizationResult tryDirectPutByValOptimize(ExecState* exec, JSObject* object, JSValue subscript, ByValInfo* byValInfo, ReturnAddressPtr returnAddress)
@@ -808,6 +813,7 @@ static OptimizationResult tryDirectPutByValOptimize(ExecState* exec, JSObject* o
     OptimizationResult optimizationResult = OptimizationResult::NotOptimized;
 
     VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
 
     if (subscript.isInt32()) {
         ASSERT(exec->bytecodeOffset());
@@ -832,6 +838,7 @@ static OptimizationResult tryDirectPutByValOptimize(ExecState* exec, JSObject* o
             optimizationResult = OptimizationResult::GiveUp;
     } else if (isStringOrSymbol(subscript)) {
         const Identifier propertyName = subscript.toPropertyKey(exec);
+        RETURN_IF_EXCEPTION(scope, OptimizationResult::GiveUp);
         if (subscript.isSymbol() || !parseIndex(propertyName)) {
             ASSERT(exec->bytecodeOffset());
             ASSERT(!byValInfo->stubRoutine);
@@ -872,19 +879,22 @@ void JIT_OPERATION operationDirectPutByValOptimize(ExecState* exec, EncodedJSVal
 {
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
+    auto scope = DECLARE_THROW_SCOPE(vm);
 
     JSValue baseValue = JSValue::decode(encodedBaseValue);
     JSValue subscript = JSValue::decode(encodedSubscript);
     JSValue value = JSValue::decode(encodedValue);
     RELEASE_ASSERT(baseValue.isObject());
     JSObject* object = asObject(baseValue);
-    if (tryDirectPutByValOptimize(exec, object, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS)) == OptimizationResult::GiveUp) {
+    OptimizationResult result = tryDirectPutByValOptimize(exec, object, subscript, byValInfo, ReturnAddressPtr(OUR_RETURN_ADDRESS));
+    RETURN_IF_EXCEPTION(scope, void());
+    if (result == OptimizationResult::GiveUp) {
         // Don't ever try to optimize.
         byValInfo->tookSlowPath = true;
         ctiPatchCallByReturnAddress(ReturnAddressPtr(OUR_RETURN_ADDRESS), operationDirectPutByValGeneric);
     }
 
-    directPutByVal(exec, object, subscript, value, byValInfo);
+    RELEASE_AND_RETURN(scope, directPutByVal(exec, object, subscript, value, byValInfo));
 }
 
 void JIT_OPERATION operationPutByValGeneric(ExecState* exec, EncodedJSValue encodedBaseValue, EncodedJSValue encodedSubscript, EncodedJSValue encodedValue, ByValInfo* byValInfo)
@@ -1873,6 +1883,7 @@ static OptimizationResult tryGetByValOptimize(ExecState* exec, JSValue baseValue
     OptimizationResult optimizationResult = OptimizationResult::NotOptimized;
 
     VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
 
     if (baseValue.isObject() && subscript.isInt32()) {
         JSObject* object = asObject(baseValue);
@@ -1903,6 +1914,7 @@ static OptimizationResult tryGetByValOptimize(ExecState* exec, JSValue baseValue
 
     if (baseValue.isObject() && isStringOrSymbol(subscript)) {
         const Identifier propertyName = subscript.toPropertyKey(exec);
+        RETURN_IF_EXCEPTION(scope, OptimizationResult::GiveUp);
         if (subscript.isSymbol() || !parseIndex(propertyName)) {
             ASSERT(exec->bytecodeOffset());
             ASSERT(!byValInfo->stubRoutine);
@@ -1956,17 +1968,20 @@ EncodedJSValue JIT_OPERATION operationGetByValOptimize(ExecState* exec, EncodedJ
 {
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
+    auto scope = DECLARE_THROW_SCOPE(vm);
 
     JSValue baseValue = JSValue::decode(encodedBase);
     JSValue subscript = JSValue::decode(encodedSubscript);
     ReturnAddressPtr returnAddress = ReturnAddressPtr(OUR_RETURN_ADDRESS);
-    if (tryGetByValOptimize(exec, baseValue, subscript, byValInfo, returnAddress) == OptimizationResult::GiveUp) {
+    OptimizationResult result = tryGetByValOptimize(exec, baseValue, subscript, byValInfo, returnAddress);
+    RETURN_IF_EXCEPTION(scope, { });
+    if (result == OptimizationResult::GiveUp) {
         // Don't ever try to optimize.
         byValInfo->tookSlowPath = true;
         ctiPatchCallByReturnAddress(returnAddress, operationGetByValGeneric);
     }
 
-    return JSValue::encode(getByVal(exec, baseValue, subscript, byValInfo, returnAddress));
+    RELEASE_AND_RETURN(scope, JSValue::encode(getByVal(exec, baseValue, subscript, byValInfo, returnAddress)));
 }
 
 EncodedJSValue JIT_OPERATION operationHasIndexedPropertyDefault(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedSubscript, ByValInfo* byValInfo)
index 83100c9..f592a1a 100644 (file)
@@ -669,7 +669,8 @@ void JSFunction::setFunctionName(ExecState* exec, JSValue value)
     ASSERT(jsExecutable()->ecmaName().isNull());
     String name;
     if (value.isSymbol()) {
-        SymbolImpl& uid = asSymbol(value)->privateName().uid();
+        PrivateName privateName = asSymbol(value)->privateName();
+        SymbolImpl& uid = privateName.uid();
         if (uid.isNullSymbol())
             name = emptyString();
         else
index 982baa7..9d1c221 100644 (file)
@@ -116,8 +116,12 @@ void JSModuleLoader::finishCreation(ExecState* exec, VM& vm, JSGlobalObject* glo
 static String printableModuleKey(ExecState* exec, JSValue key)
 {
     VM& vm = exec->vm();
-    if (key.isString() || key.isSymbol())
-        return key.toPropertyKey(exec).impl();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+    if (key.isString() || key.isSymbol()) {
+        auto propertyName = key.toPropertyKey(exec);
+        scope.assertNoException(); // This is OK since this function is just for debugging purpose.
+        return propertyName.impl();
+    }
     return vm.propertyNames->emptyIdentifier.impl();
 }
 
index 6545262..dd12bd3 100644 (file)
@@ -247,8 +247,11 @@ Stringifier::Stringifier(ExecState* exec, JSValue replacer, JSValue space)
                             continue;
                     } else if (!name.isNumber() && !name.isString())
                         continue;
-                    m_arrayReplacerPropertyNames.add(name.toString(exec)->toIdentifier(exec));
+                    JSString* propertyNameString = name.toString(exec);
                     RETURN_IF_EXCEPTION(scope, );
+                    auto propertyName = propertyNameString->toIdentifier(exec);
+                    RETURN_IF_EXCEPTION(scope, );
+                    m_arrayReplacerPropertyNames.add(WTFMove(propertyName));
                 }
             }
         }
index d83e31d..caf45e5 100644 (file)
@@ -100,12 +100,12 @@ void Symbol::destroy(JSCell* cell)
 
 String Symbol::descriptiveString() const
 {
-    return makeString("Symbol(", String(privateName().uid()), ')');
+    return makeString("Symbol(", String(m_privateName.uid()), ')');
 }
 
 String Symbol::description() const
 {
-    auto& uid = privateName().uid();
+    auto& uid = m_privateName.uid();
     return uid.isNullSymbol() ? String() : uid;
 }
 
index 52c9162..1f1786e 100644 (file)
@@ -49,7 +49,7 @@ public:
     static Symbol* create(ExecState*, JSString* description);
     JS_EXPORT_PRIVATE static Symbol* create(VM&, SymbolImpl& uid);
 
-    const PrivateName& privateName() const { return m_privateName; }
+    PrivateName privateName() const { return m_privateName; }
     String descriptiveString() const;
     String description() const;
 
index 0e0bcb8..483f085 100644 (file)
@@ -109,7 +109,8 @@ EncodedJSValue JSC_HOST_CALL symbolConstructorKeyFor(ExecState* exec)
     if (!symbolValue.isSymbol())
         return JSValue::encode(throwTypeError(exec, scope, SymbolKeyForTypeError));
 
-    SymbolImpl& uid = asSymbol(symbolValue)->privateName().uid();
+    PrivateName privateName = asSymbol(symbolValue)->privateName();
+    SymbolImpl& uid = privateName.uid();
     if (!uid.symbolRegistry())
         return JSValue::encode(jsUndefined());
 
index 5560e5a..826da01 100644 (file)
@@ -2103,6 +2103,9 @@ static EncodedJSValue JSC_HOST_CALL functionGlobalObjectForObject(ExecState* exe
 
 static EncodedJSValue JSC_HOST_CALL functionGetGetterSetter(ExecState* exec)
 {
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     JSValue value = exec->argument(0);
     if (!value.isObject())
         return JSValue::encode(jsUndefined());
@@ -2111,8 +2114,11 @@ static EncodedJSValue JSC_HOST_CALL functionGetGetterSetter(ExecState* exec)
     if (!property.isString())
         return JSValue::encode(jsUndefined());
 
+    auto propertyName = asString(property)->toIdentifier(exec);
+    RETURN_IF_EXCEPTION(scope, { });
+
     PropertySlot slot(value, PropertySlot::InternalMethodType::VMInquiry);
-    value.getPropertySlot(exec, asString(property)->toIdentifier(exec), slot);
+    value.getPropertySlot(exec, propertyName, slot);
 
     JSValue result;
     if (slot.isCacheableGetter())
index 124d12b..b9d982d 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-14  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Retain PrivateName of Symbol before passing it to operations potentially incurring GC
+        https://bugs.webkit.org/show_bug.cgi?id=195791
+        <rdar://problem/48806130>
+
+        Reviewed by Mark Lam.
+
+        * bindings/js/ScriptController.cpp:
+        (WebCore::ScriptController::setupModuleScriptHandlers):
+
 2019-03-14  Brent Fulgham  <bfulgham@apple.com>
 
         Move CoreCrypto SPI declarations to an appropriate PAL/spi header
index 784ae54..9d8b575 100644 (file)
@@ -278,8 +278,11 @@ void ScriptController::setupModuleScriptHandlers(LoadableModuleScript& moduleScr
 
     RefPtr<LoadableModuleScript> moduleScript(&moduleScriptRef);
 
-    auto& fulfillHandler = *JSNativeStdFunction::create(state.vm(), proxy.window(), 1, String(), [moduleScript](ExecState* exec) {
+    auto& fulfillHandler = *JSNativeStdFunction::create(state.vm(), proxy.window(), 1, String(), [moduleScript](ExecState* exec) -> JSC::EncodedJSValue {
+        VM& vm = exec->vm();
+        auto scope = DECLARE_THROW_SCOPE(vm);
         Identifier moduleKey = jsValueToModuleKey(exec, exec->argument(0));
+        RETURN_IF_EXCEPTION(scope, { });
         moduleScript->notifyLoadCompleted(*moduleKey.impl());
         return JSValue::encode(jsUndefined());
     });