Attachment 369199 Details for Bug 197634 – Patch

[patch] Patch

bug-197634-20190506163957.patch (text/plain), 9.69 KB, created by Yusuke Suzuki on 2019-05-06 16:39:58 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-05-06 16:39:58 PDT

Size: 9.69 KB

patch

obsolete

>Subversion Revision: 244982
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 4026445f7e96217c4b4f0ab8ba29cc0e9ad294a0..2c8241dca3299e7753e43d4f3ac4c07d13a46525 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,28 @@
>+2019-05-06  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] We should check OOM for description string of Symbol
>+        https://bugs.webkit.org/show_bug.cgi?id=197634
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When resoling JSString for description of Symbol, we should check OOM error.
>+        We also change JSValueMakeSymbol(..., nullptr) to returning a symbol value
>+        without description, (1) to simplify the code and (2) give a way for JSC API
>+        to create a symbol value without description.
>+
>+        * API/JSValueRef.cpp:
>+        (JSValueMakeSymbol):
>+        * API/tests/testapi.cpp:
>+        (TestAPI::symbolsTypeof):
>+        (TestAPI::symbolsDescription):
>+        (testCAPIViaCpp):
>+        * dfg/DFGOperations.cpp:
>+        * runtime/Symbol.cpp:
>+        (JSC::Symbol::createWithDescription):
>+        * runtime/Symbol.h:
>+        * runtime/SymbolConstructor.cpp:
>+        (JSC::callSymbol):
>+
> 2019-05-06  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         TemplateObject passed to template literal tags are not always identical for the same source location.
>diff --git a/Source/JavaScriptCore/API/JSValueRef.cpp b/Source/JavaScriptCore/API/JSValueRef.cpp
>index dd64d26a886c89c2e149444511cebafbebd928e1..2b39fbead3297e728044696f48f8ccb62c5c4541 100644
>--- a/Source/JavaScriptCore/API/JSValueRef.cpp
>+++ b/Source/JavaScriptCore/API/JSValueRef.cpp
>@@ -331,13 +331,12 @@ JSValueRef JSValueMakeSymbol(JSContextRef ctx, JSStringRef description)
>         return nullptr;
>     }
>     ExecState* exec = toJS(ctx);
>+    VM& vm = exec->vm();
>     JSLockHolder locker(exec);
>-    auto scope = DECLARE_CATCH_SCOPE(exec->vm());
>-
>-    JSString* jsDescription = jsString(exec, description ? description->string() : String());
>-    RETURN_IF_EXCEPTION(scope, nullptr);
> 
>-    return toRef(exec, Symbol::create(exec, jsDescription));
>+    if (!description)
>+        return toRef(exec, Symbol::create(vm));
>+    return toRef(exec, Symbol::createWithDescription(vm, description->string()));
> }
> 
> JSValueRef JSValueMakeString(JSContextRef ctx, JSStringRef string)
>diff --git a/Source/JavaScriptCore/API/tests/testapi.cpp b/Source/JavaScriptCore/API/tests/testapi.cpp
>index bca06071c39e56a3734652bea78f2a5330e85945..1cfd4c03a08bad422e94141885bc97c0e8d0eb66 100644
>--- a/Source/JavaScriptCore/API/tests/testapi.cpp
>+++ b/Source/JavaScriptCore/API/tests/testapi.cpp
>@@ -130,6 +130,7 @@ class TestAPI {
> 
>     void basicSymbol();
>     void symbolsTypeof();
>+    void symbolsDescription();
>     void symbolsGetPropertyForKey();
>     void symbolsSetPropertyForKey();
>     void symbolsHasPropertyForKey();
>@@ -268,6 +269,7 @@ APIVector<JSValueRef> TestAPI::interestingKeys()
> }
> 
> static const char* isSymbolFunction = "(function isSymbol(symbol) { return typeof(symbol) === 'symbol'; })";
>+static const char* getSymbolDescription = "(function getSymbolDescription(symbol) { return symbol.description; })";
> static const char* getFunction = "(function get(object, key) { return object[key]; })";
> static const char* setFunction = "(function set(object, key, value) { object[key] = value; })";
> 
>@@ -281,9 +283,30 @@ void TestAPI::basicSymbol()
> 
> void TestAPI::symbolsTypeof()
> {
>-    APIString description("dope");
>-    JSValueRef symbol = JSValueMakeSymbol(context, description);
>-    check(functionReturnsTrue(isSymbolFunction, symbol), "JSValueMakeSymbol makes a symbol value");
>+    {
>+        JSValueRef symbol = JSValueMakeSymbol(context, nullptr);
>+        check(functionReturnsTrue(isSymbolFunction, symbol), "JSValueMakeSymbol makes a symbol value");
>+    }
>+    {
>+        APIString description("dope");
>+        JSValueRef symbol = JSValueMakeSymbol(context, description);
>+        check(functionReturnsTrue(isSymbolFunction, symbol), "JSValueMakeSymbol makes a symbol value");
>+    }
>+}
>+
>+void TestAPI::symbolsDescription()
>+{
>+    {
>+        JSValueRef symbol = JSValueMakeSymbol(context, nullptr);
>+        auto result = callFunction(getSymbolDescription, symbol);
>+        check(JSValueIsStrictEqual(context, result.value(), JSValueMakeUndefined(context)), "JSValueMakeSymbol with nullptr description produces a symbol value without description");
>+    }
>+    {
>+        APIString description("dope");
>+        JSValueRef symbol = JSValueMakeSymbol(context, description);
>+        auto result = callFunction(getSymbolDescription, symbol);
>+        check(JSValueIsStrictEqual(context, result.value(), JSValueMakeString(context, description)), "JSValueMakeSymbol with description string produces a symbol value with description");
>+    }
> }
> 
> void TestAPI::symbolsGetPropertyForKey()
>@@ -491,6 +514,7 @@ int testCAPIViaCpp(const char* filter)
> 
>     RUN(basicSymbol());
>     RUN(symbolsTypeof());
>+    RUN(symbolsDescription());
>     RUN(symbolsGetPropertyForKey());
>     RUN(symbolsSetPropertyForKey());
>     RUN(symbolsHasPropertyForKey());
>diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>index fea350da028fe031092d780ee6989c81ebfa0312..fad644131e64455a80380f645d4f06cf170a51fb 100644
>--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>@@ -2291,8 +2291,12 @@ Symbol* JIT_OPERATION operationNewSymbolWithDescription(ExecState* exec, JSStrin
> {
>     VM& vm = exec->vm();
>     NativeCallFrameTracer tracer(&vm, exec);
>+    auto scope = DECLARE_THROW_SCOPE(vm);
>+
>+    String string = description->value(exec);
>+    RETURN_IF_EXCEPTION(scope, nullptr);
> 
>-    return Symbol::create(exec, description);
>+    return Symbol::createWithDescription(vm, string);
> }
> 
> JSCell* JIT_OPERATION operationNewStringObject(ExecState* exec, JSString* string, Structure* structure)
>diff --git a/Source/JavaScriptCore/runtime/Symbol.cpp b/Source/JavaScriptCore/runtime/Symbol.cpp
>index caf45e5f7500645d39a242a1f6006322cce73450..c8be03af825015d7485209ee7cee1ab9a2063005 100644
>--- a/Source/JavaScriptCore/runtime/Symbol.cpp
>+++ b/Source/JavaScriptCore/runtime/Symbol.cpp
>@@ -116,11 +116,9 @@ Symbol* Symbol::create(VM& vm)
>     return symbol;
> }
> 
>-Symbol* Symbol::create(ExecState* exec, JSString* description)
>+Symbol* Symbol::createWithDescription(VM& vm, const String& description)
> {
>-    VM& vm = exec->vm();
>-    String desc = description->value(exec);
>-    Symbol* symbol = new (NotNull, allocateCell<Symbol>(vm.heap)) Symbol(vm, desc);
>+    Symbol* symbol = new (NotNull, allocateCell<Symbol>(vm.heap)) Symbol(vm, description);
>     symbol->finishCreation(vm);
>     return symbol;
> }
>diff --git a/Source/JavaScriptCore/runtime/Symbol.h b/Source/JavaScriptCore/runtime/Symbol.h
>index 1f1786ec6cc342413e0a98402c0a105e3f7060ae..ca9942e55ae7a19a2c3ebc9229d8550b39315418 100644
>--- a/Source/JavaScriptCore/runtime/Symbol.h
>+++ b/Source/JavaScriptCore/runtime/Symbol.h
>@@ -46,7 +46,7 @@ class Symbol final : public JSCell {
>     }
> 
>     static Symbol* create(VM&);
>-    static Symbol* create(ExecState*, JSString* description);
>+    static Symbol* createWithDescription(VM&, const String&);
>     JS_EXPORT_PRIVATE static Symbol* create(VM&, SymbolImpl& uid);
> 
>     PrivateName privateName() const { return m_privateName; }
>diff --git a/Source/JavaScriptCore/runtime/SymbolConstructor.cpp b/Source/JavaScriptCore/runtime/SymbolConstructor.cpp
>index 483f0850c1e16cc6b871cd054bb3be775d5923ec..0bec27ca5af29bc041a8002462b95bb3ffe04341 100644
>--- a/Source/JavaScriptCore/runtime/SymbolConstructor.cpp
>+++ b/Source/JavaScriptCore/runtime/SymbolConstructor.cpp
>@@ -79,10 +79,16 @@ void SymbolConstructor::finishCreation(VM& vm, SymbolPrototype* prototype)
> 
> static EncodedJSValue JSC_HOST_CALL callSymbol(ExecState* exec)
> {
>+    VM& vm = exec->vm();
>+    auto scope = DECLARE_THROW_SCOPE(vm);
>+
>     JSValue description = exec->argument(0);
>     if (description.isUndefined())
>-        return JSValue::encode(Symbol::create(exec->vm()));
>-    return JSValue::encode(Symbol::create(exec, description.toString(exec)));
>+        return JSValue::encode(Symbol::create(vm));
>+
>+    String string = description.toWTFString(exec);
>+    RETURN_IF_EXCEPTION(scope, { });
>+    return JSValue::encode(Symbol::createWithDescription(vm, string));
> }
> 
> EncodedJSValue JSC_HOST_CALL symbolConstructorFor(ExecState* exec)
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index bcfaa84fc537552e3fbd9e7e8f8e9c5d921f00db..ed25f207ded78625be10eacbb39d8361b2f964f1 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-05-06  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] We should check OOM for description string of Symbol
>+        https://bugs.webkit.org/show_bug.cgi?id=197634
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/check-symbol-description-oom.js: Added.
>+        (shouldThrow):
>+
> 2019-05-06  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         Unreviewed, land one more test
>diff --git a/JSTests/stress/check-symbol-description-oom.js b/JSTests/stress/check-symbol-description-oom.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..57ba5e462a72d52428e20a365b458f3c7c590e4f
>--- /dev/null
>+++ b/JSTests/stress/check-symbol-description-oom.js
>@@ -0,0 +1,18 @@
>+function shouldThrow(func, errorMessage) {
>+    var errorThrown = false;
>+    var error = null;
>+    try {
>+        func();
>+    } catch (e) {
>+        errorThrown = true;
>+        error = e;
>+    }
>+    if (!errorThrown)
>+        throw new Error('not thrown');
>+    if (String(error) !== errorMessage)
>+        throw new Error(`bad error: ${String(error)}`);
>+}
>+
>+const s0 = (-1).toLocaleString();
>+const s1 = s0.padEnd(2147483647, '  ');
>+shouldThrow(() => Symbol(s1), `Error: Out of memory`);

Flags:

keith_miller: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197634: 369199