Attachment 360694 Details for Bug 194082 – Patch

[patch] Patch

bug-194082-20190130232227.patch (text/plain), 14.53 KB, created by Yusuke Suzuki on 2019-01-30 23:22:27 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-30 23:22:27 PST

Size: 14.53 KB

patch

obsolete

>Subversion Revision: 240759
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index aeeabc82b18046ae1c614422d45dfda790b7d8d2..39bf91795f23790cb082eae36b94b13625daaff9 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2019-01-30  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Symbol should be in destructibleCellSpace
>+        https://bugs.webkit.org/show_bug.cgi?id=194082
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Because Symbol's member was not poisoned, we changed the subspace for Symbol from destructibleCellSpace
>+        to cellJSValueOOBSpace. But the problem is cellJSValueOOBSpace is a space for cells which are not
>+        destructible. As a result, Symbol::destroy is never called, and SymbolImpl is leaked. This patch poisons
>+        the member of Symbol and make Symbol's space destructibleCellSpace again.
>+
>+        * dfg/DFGOperations.cpp:
>+        * dfg/DFGSpeculativeJIT64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::compileHasOwnProperty):
>+        * runtime/JSCJSValueInlines.h:
>+        (JSC::JSValue::toPropertyKey const):
>+        * runtime/JSCPoison.h:
>+        * runtime/JSFunction.cpp:
>+        (JSC::JSFunction::setFunctionName):
>+        * runtime/Symbol.cpp:
>+        (JSC::Symbol::Symbol):
>+        (JSC::Symbol::finishCreation):
>+        (JSC::Symbol::descriptiveString const):
>+        (JSC::Symbol::description const):
>+        * runtime/Symbol.h:
>+        * runtime/SymbolConstructor.cpp:
>+        (JSC::symbolConstructorKeyFor):
>+
> 2019-01-30  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         Unreviewed, fix GCC build after r240730
>diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>index 22750b7997af9c9a6b0eea3a5acfced1458c7f2e..afede9d4772d7c2d72ff39a04bf3e2df714aefae 100644
>--- a/Source/JavaScriptCore/dfg/DFGOperations.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGOperations.cpp
>@@ -782,7 +782,7 @@ 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()));
>+    return JSValue::encode(getByValObject(exec, vm, asObject(base), Identifier::fromUid(&vm, &asSymbol(symbol)->symbolImpl())));
> }
> 
> void JIT_OPERATION operationPutByValStrict(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedProperty, EncodedJSValue encodedValue)
>@@ -838,7 +838,7 @@ 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));
>+    putByValCellInternal<true, false>(exec, vm, cell, Identifier::fromUid(&vm, &asSymbol(symbol)->symbolImpl()), JSValue::decode(encodedValue));
> }
> 
> void JIT_OPERATION operationPutByValCellSymbolNonStrict(ExecState* exec, JSCell* cell, JSCell* symbol, EncodedJSValue encodedValue)
>@@ -846,7 +846,7 @@ 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));
>+    putByValCellInternal<false, false>(exec, vm, cell, Identifier::fromUid(&vm, &asSymbol(symbol)->symbolImpl()), JSValue::decode(encodedValue));
> }
> 
> void JIT_OPERATION operationPutByValBeyondArrayBoundsStrict(ExecState* exec, JSObject* object, int32_t index, EncodedJSValue encodedValue)
>@@ -998,7 +998,7 @@ 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));
>+    putByValCellInternal<true, true>(exec, vm, cell, Identifier::fromUid(&vm, &asSymbol(symbol)->symbolImpl()), JSValue::decode(encodedValue));
> }
> 
> void JIT_OPERATION operationPutByValDirectCellSymbolNonStrict(ExecState* exec, JSCell* cell, JSCell* symbol, EncodedJSValue encodedValue)
>@@ -1006,7 +1006,7 @@ 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));
>+    putByValCellInternal<false, true>(exec, vm, cell, Identifier::fromUid(&vm, &asSymbol(symbol)->symbolImpl()), JSValue::decode(encodedValue));
> }
> 
> void JIT_OPERATION operationPutByValDirectBeyondArrayBoundsStrict(ExecState* exec, JSObject* object, int32_t index, EncodedJSValue encodedValue)
>@@ -1558,7 +1558,7 @@ void JIT_OPERATION operationDefineDataPropertySymbol(ExecState* exec, JSObject*
> {
>     VM& vm = exec->vm();
>     NativeCallFrameTracer tracer(&vm, exec);
>-    defineDataProperty(exec, vm, base, Identifier::fromUid(property->privateName()), JSValue::decode(encodedValue), attributes);
>+    defineDataProperty(exec, vm, base, Identifier::fromUid(&vm, &property->symbolImpl()), JSValue::decode(encodedValue), attributes);
> }
> 
> ALWAYS_INLINE static void defineAccessorProperty(ExecState* exec, VM& vm, JSObject* base, const Identifier& propertyName, JSObject* getter, JSObject* setter, int32_t attributes)
>@@ -1604,7 +1604,7 @@ void JIT_OPERATION operationDefineAccessorPropertySymbol(ExecState* exec, JSObje
> {
>     VM& vm = exec->vm();
>     NativeCallFrameTracer tracer(&vm, exec);
>-    defineAccessorProperty(exec, vm, base, Identifier::fromUid(property->privateName()), getter, setter, attributes);
>+    defineAccessorProperty(exec, vm, base, Identifier::fromUid(&vm, &property->symbolImpl()), getter, setter, attributes);
> }
> 
> char* JIT_OPERATION operationNewArray(ExecState* exec, Structure* arrayStructure, void* buffer, size_t size)
>diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>index 7f9e381b755d264ca75e3e7dcaca80c8741dde66..37b1ffc2662adc15022952ef85152b4be7b77694 100644
>--- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>@@ -4407,6 +4407,7 @@ void SpeculativeJIT::compile(Node* node)
>         case SymbolUse: {
>             speculateSymbol(node->child2(), keyGPR);
>             m_jit.loadPtr(MacroAssembler::Address(keyGPR, Symbol::offsetOfSymbolImpl()), implGPR);
>+            m_jit.xorPtr(JITCompiler::TrustedImmPtr(SymbolPoison::key()), implGPR);
>             break;
>         }
>         case StringUse: {
>@@ -4431,6 +4432,7 @@ void SpeculativeJIT::compile(Node* node)
>             isNotString.link(&m_jit);
>             slowPath.append(m_jit.branchIfNotSymbol(keyGPR));
>             m_jit.loadPtr(MacroAssembler::Address(keyGPR, Symbol::offsetOfSymbolImpl()), implGPR);
>+            m_jit.xorPtr(JITCompiler::TrustedImmPtr(SymbolPoison::key()), implGPR);
> 
>             hasUniquedImpl.link(&m_jit);
>             break;
>diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>index 4f3a5a054b58b145c70c8a4a6a4af6cc0f5a0807..c7442d4f1b9a71abfebf952900058715a2122993 100644
>--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>@@ -10117,7 +10117,7 @@ class LowerDFGToB3 {
>         }
>         case SymbolUse: {
>             keyAsValue = lowSymbol(m_node->child2());
>-            uniquedStringImpl = m_out.loadPtr(keyAsValue, m_heaps.Symbol_symbolImpl);
>+            uniquedStringImpl = m_out.bitXor(m_out.loadPtr(keyAsValue, m_heaps.Symbol_symbolImpl), m_out.constInt64(SymbolPoison::key()));
>             lastNext = m_out.insertNewBlocksBefore(slowCase);
>             break;
>         }
>@@ -10148,7 +10148,7 @@ class LowerDFGToB3 {
>             m_out.branch(isSymbol(keyAsValue), unsure(isSymbolCase), unsure(slowCase));
> 
>             m_out.appendTo(isSymbolCase, hasUniquedStringImpl);
>-            ValueFromBlock symbolResult = m_out.anchor(m_out.loadPtr(keyAsValue, m_heaps.Symbol_symbolImpl));
>+            ValueFromBlock symbolResult = m_out.anchor(m_out.bitXor(m_out.loadPtr(keyAsValue, m_heaps.Symbol_symbolImpl), m_out.constInt64(SymbolPoison::key())));
>             m_out.jump(hasUniquedStringImpl);
> 
>             m_out.appendTo(hasUniquedStringImpl, slowCase);
>diff --git a/Source/JavaScriptCore/runtime/JSCJSValueInlines.h b/Source/JavaScriptCore/runtime/JSCJSValueInlines.h
>index 6cf3a223c2dabecd28c429bf0725415328d24d1a..412b073dab2f052f0890ed2a27199d1fcdedf0c0 100644
>--- a/Source/JavaScriptCore/runtime/JSCJSValueInlines.h
>+++ b/Source/JavaScriptCore/runtime/JSCJSValueInlines.h
>@@ -652,7 +652,7 @@ ALWAYS_INLINE Identifier JSValue::toPropertyKey(ExecState* exec) const
>     JSValue primitive = toPrimitive(exec, PreferString);
>     RETURN_IF_EXCEPTION(scope, vm.propertyNames->emptyIdentifier);
>     if (primitive.isSymbol())
>-        RELEASE_AND_RETURN(scope, Identifier::fromUid(asSymbol(primitive)->privateName()));
>+        RELEASE_AND_RETURN(scope, Identifier::fromUid(&vm, &asSymbol(primitive)->symbolImpl()));
> 
>     RELEASE_AND_RETURN(scope, primitive.toString(exec)->toIdentifier(exec));
> }
>diff --git a/Source/JavaScriptCore/runtime/JSCPoison.h b/Source/JavaScriptCore/runtime/JSCPoison.h
>index 1db4ac6d5463019d9fc6fa11883107c851da840d..ced826d875b7cee4fbe1625b13d3bef41ff42211 100644
>--- a/Source/JavaScriptCore/runtime/JSCPoison.h
>+++ b/Source/JavaScriptCore/runtime/JSCPoison.h
>@@ -51,6 +51,7 @@ namespace JSC {
>     v(NativeCode) \
>     v(ScopedArguments) \
>     v(StructureTransitionTable) \
>+    v(Symbol) \
>     v(UnlinkedSourceCode) \
>     v(WebAssemblyFunctionBase) \
>     v(WebAssemblyModuleRecord) \
>diff --git a/Source/JavaScriptCore/runtime/JSFunction.cpp b/Source/JavaScriptCore/runtime/JSFunction.cpp
>index 50568e846b6a7f0388c9ea469df9edfc77504ab3..94b639e8386c5970db03cc2af94f90badfaecd72 100644
>--- a/Source/JavaScriptCore/runtime/JSFunction.cpp
>+++ b/Source/JavaScriptCore/runtime/JSFunction.cpp
>@@ -669,7 +669,7 @@ void JSFunction::setFunctionName(ExecState* exec, JSValue value)
>     ASSERT(jsExecutable()->ecmaName().isNull());
>     String name;
>     if (value.isSymbol()) {
>-        SymbolImpl& uid = asSymbol(value)->privateName().uid();
>+        SymbolImpl& uid = asSymbol(value)->symbolImpl();
>         if (uid.isNullSymbol())
>             name = emptyString();
>         else
>diff --git a/Source/JavaScriptCore/runtime/Symbol.cpp b/Source/JavaScriptCore/runtime/Symbol.cpp
>index d83e31d1029c3ea29931c1de23b00bc4ecf6c109..380dabac19bf2366cf48ed73c21995af9f312f0d 100644
>--- a/Source/JavaScriptCore/runtime/Symbol.cpp
>+++ b/Source/JavaScriptCore/runtime/Symbol.cpp
>@@ -37,19 +37,19 @@ const ClassInfo Symbol::s_info = { "symbol", nullptr, nullptr, nullptr, CREATE_M
> 
> Symbol::Symbol(VM& vm)
>     : Base(vm, vm.symbolStructure.get())
>-    , m_privateName()
>+    , m_symbolImpl(SymbolImpl::createNullSymbol())
> {
> }
> 
>-Symbol::Symbol(VM& vm, const String& string)
>+Symbol::Symbol(VM& vm, const String& description)
>     : Base(vm, vm.symbolStructure.get())
>-    , m_privateName(PrivateName::Description, string)
>+    , m_symbolImpl(SymbolImpl::create(*description.impl()))
> {
> }
> 
> Symbol::Symbol(VM& vm, SymbolImpl& uid)
>     : Base(vm, vm.symbolStructure.get())
>-    , m_privateName(uid)
>+    , m_symbolImpl(uid)
> {
> }
> 
>@@ -58,7 +58,7 @@ void Symbol::finishCreation(VM& vm)
>     Base::finishCreation(vm);
>     ASSERT(inherits(vm, info()));
> 
>-    vm.symbolImplToSymbolMap.set(&m_privateName.uid(), this);
>+    vm.symbolImplToSymbolMap.set(m_symbolImpl.ptr(), this);
> }
> 
> inline SymbolObject* SymbolObject::create(VM& vm, JSGlobalObject* globalObject, Symbol* symbol)
>@@ -100,12 +100,12 @@ void Symbol::destroy(JSCell* cell)
> 
> String Symbol::descriptiveString() const
> {
>-    return makeString("Symbol(", String(privateName().uid()), ')');
>+    return makeString("Symbol(", String(symbolImpl()), ')');
> }
> 
> String Symbol::description() const
> {
>-    auto& uid = privateName().uid();
>+    auto& uid = symbolImpl();
>     return uid.isNullSymbol() ? String() : uid;
> }
> 
>diff --git a/Source/JavaScriptCore/runtime/Symbol.h b/Source/JavaScriptCore/runtime/Symbol.h
>index 71023e61abf00d44edcc851ba92264369710ecc6..403a3c17bf621ff8ec458fa24382c4ef564070cf 100644
>--- a/Source/JavaScriptCore/runtime/Symbol.h
>+++ b/Source/JavaScriptCore/runtime/Symbol.h
>@@ -26,8 +26,9 @@
> 
> #pragma once
> 
>+#include "JSCPoison.h"
> #include "JSString.h"
>-#include "PrivateName.h"
>+#include <wtf/text/SymbolImpl.h>
> 
> namespace JSC {
> 
>@@ -36,12 +37,6 @@ class Symbol final : public JSCell {
>     typedef JSCell Base;
>     static const unsigned StructureFlags = Base::StructureFlags | StructureIsImmortal | OverridesToThis;
> 
>-    template<typename>
>-    static CompleteSubspace* subspaceFor(VM& vm)
>-    {
>-        return &vm.cellJSValueOOBSpace;
>-    }
>-
>     DECLARE_EXPORT_INFO;
> 
>     static const bool needsDestruction = true;
>@@ -55,7 +50,7 @@ class Symbol final : public JSCell {
>     static Symbol* create(ExecState*, JSString* description);
>     JS_EXPORT_PRIVATE static Symbol* create(VM&, SymbolImpl& uid);
> 
>-    const PrivateName& privateName() const { return m_privateName; }
>+    SymbolImpl& symbolImpl() const { return m_symbolImpl.get(); }
>     String descriptiveString() const;
>     String description() const;
> 
>@@ -66,8 +61,7 @@ class Symbol final : public JSCell {
> 
>     static ptrdiff_t offsetOfSymbolImpl()
>     {
>-        // PrivateName is just a Ref<SymbolImpl> which can just be used as a SymbolImpl*.
>-        return OBJECT_OFFSETOF(Symbol, m_privateName);
>+        return OBJECT_OFFSETOF(Symbol, m_symbolImpl);
>     }
> 
> protected:
>@@ -79,7 +73,7 @@ class Symbol final : public JSCell {
> 
>     void finishCreation(VM&);
> 
>-    PrivateName m_privateName;
>+    PoisonedRef<SymbolPoison, SymbolImpl> m_symbolImpl;
> };
> 
> Symbol* asSymbol(JSValue);
>diff --git a/Source/JavaScriptCore/runtime/SymbolConstructor.cpp b/Source/JavaScriptCore/runtime/SymbolConstructor.cpp
>index ac6fe851f5154f5f0d1ad8009054cd64fdad2a90..999d5655b98c62315fbb305e345b3a2e0de18c26 100644
>--- a/Source/JavaScriptCore/runtime/SymbolConstructor.cpp
>+++ b/Source/JavaScriptCore/runtime/SymbolConstructor.cpp
>@@ -109,7 +109,7 @@ EncodedJSValue JSC_HOST_CALL symbolConstructorKeyFor(ExecState* exec)
>     if (!symbolValue.isSymbol())
>         return JSValue::encode(throwTypeError(exec, scope, SymbolKeyForTypeError));
> 
>-    SymbolImpl& uid = asSymbol(symbolValue)->privateName().uid();
>+    SymbolImpl& uid = asSymbol(symbolValue)->symbolImpl();
>     if (!uid.symbolRegistry())
>         return JSValue::encode(jsUndefined());
>

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194082: 360694 | 360695 | 360696