Attachment 360066 Details for Bug 193800 – Patch

[patch] Patch

bug-193800-20190124183858.patch (text/plain), 16.98 KB, created by Yusuke Suzuki on 2019-01-24 18:38:59 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-24 18:38:59 PST

Size: 16.98 KB

patch

obsolete

>Subversion Revision: 240461
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index de140b790bf8e78ca2f263f886ae5b7ee13e5edc..66b6ea4915046802c51f26546af5309c893c2827 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,50 @@
>+2019-01-24  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] ErrorConstructor should not have own IsoSubspace
>+        https://bugs.webkit.org/show_bug.cgi?id=193800
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Similar to r240456, sizeof(ErrorConstructor) != sizeof(InternalFunction), and that is why we have
>+        IsoSubspace errorConstructorSpace in VM. But it is allocated only one-per-JSGlobalObject, and it is
>+        too costly to have IsoSubspace which allocates 16KB. Since stackTraceLimit information is per
>+        JSGlobalObject information, we should have m_stackTraceLimit in JSGlobalObject instead and put
>+        ErrorConstructor in InternalFunction's IsoSubspace. As r230813 (moving InternalFunction and subclasses
>+        into IsoSubspaces) described,
>+
>+            "subclasses that are the same size as InternalFunction share its subspace. I did this because the subclasses
>+            appear to just override methods, which are called dynamically via the structure or class of the object.
>+            So, I don't see a type confusion risk if UAF is used to allocate one kind of InternalFunction over another."
>+
>+        Then, putting ErrorConstructor in InternalFunction IsoSubspace is fine since it meets the above condition.
>+        This patch removes m_stackTraceLimit in ErrorConstructor, and drops IsoSubspace for errorConstructorSpace.
>+        This reduces the memory usage.
>+
>+        * interpreter/Interpreter.h:
>+        * runtime/Error.cpp:
>+        (JSC::getStackTrace):
>+        * runtime/ErrorConstructor.cpp:
>+        (JSC::ErrorConstructor::ErrorConstructor):
>+        (JSC::ErrorConstructor::finishCreation):
>+        (JSC::constructErrorConstructor):
>+        (JSC::callErrorConstructor):
>+        (JSC::ErrorConstructor::put):
>+        (JSC::ErrorConstructor::deleteProperty):
>+        (JSC::Interpreter::constructWithErrorConstructor): Deleted.
>+        (JSC::Interpreter::callErrorConstructor): Deleted.
>+        * runtime/ErrorConstructor.h:
>+        * runtime/JSGlobalObject.cpp:
>+        (JSC::JSGlobalObject::JSGlobalObject):
>+        (JSC::JSGlobalObject::init):
>+        (JSC::JSGlobalObject::visitChildren):
>+        * runtime/JSGlobalObject.h:
>+        (JSC::JSGlobalObject::stackTraceLimit const):
>+        (JSC::JSGlobalObject::setStackTraceLimit):
>+        (JSC::JSGlobalObject::errorConstructor const): Deleted.
>+        * runtime/VM.cpp:
>+        (JSC::VM::VM):
>+        * runtime/VM.h:
>+
> 2019-01-24  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: CPU Usage Timeline
>diff --git a/Source/JavaScriptCore/interpreter/Interpreter.h b/Source/JavaScriptCore/interpreter/Interpreter.h
>index 6b926b42c41a4abcb6c3bf6f8523b918f4ae45f8..be5d5e99905f9ac1b680f39aa39140c0fad7b57c 100644
>--- a/Source/JavaScriptCore/interpreter/Interpreter.h
>+++ b/Source/JavaScriptCore/interpreter/Interpreter.h
>@@ -117,8 +117,6 @@ namespace JSC {
>         NEVER_INLINE void debug(CallFrame*, DebugHookType);
>         static String stackTraceAsString(VM&, const Vector<StackFrame>&);
> 
>-        static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*);
>-        static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*);
>         static EncodedJSValue JSC_HOST_CALL constructWithNativeErrorConstructor(ExecState*);
>         static EncodedJSValue JSC_HOST_CALL callNativeErrorConstructor(ExecState*);
> 
>diff --git a/Source/JavaScriptCore/runtime/Error.cpp b/Source/JavaScriptCore/runtime/Error.cpp
>index d93344097a13e0330a50c1410e5cc75bbe5ed46d..1b8e4c281de269710c7a3752a21d143acdd42c10 100644
>--- a/Source/JavaScriptCore/runtime/Error.cpp
>+++ b/Source/JavaScriptCore/runtime/Error.cpp
>@@ -161,13 +161,12 @@ class FindFirstCallerFrameWithCodeblockFunctor {
> std::unique_ptr<Vector<StackFrame>> getStackTrace(ExecState* exec, VM& vm, JSObject* obj, bool useCurrentFrame)
> {
>     JSGlobalObject* globalObject = obj->globalObject(vm);
>-    ErrorConstructor* errorConstructor = globalObject->errorConstructor();
>-    if (!errorConstructor->stackTraceLimit())
>+    if (!globalObject->stackTraceLimit())
>         return nullptr;
> 
>     size_t framesToSkip = useCurrentFrame ? 0 : 1;
>     std::unique_ptr<Vector<StackFrame>> stackTrace = std::make_unique<Vector<StackFrame>>();
>-    vm.interpreter->getStackTrace(obj, *stackTrace, framesToSkip, errorConstructor->stackTraceLimit().value());
>+    vm.interpreter->getStackTrace(obj, *stackTrace, framesToSkip, globalObject->stackTraceLimit().value());
>     if (!stackTrace->isEmpty())
>         ASSERT_UNUSED(exec, exec == vm.topCallFrame || exec->isGlobalExec());
>     return stackTrace;
>diff --git a/Source/JavaScriptCore/runtime/ErrorConstructor.cpp b/Source/JavaScriptCore/runtime/ErrorConstructor.cpp
>index b09c05870cd2195fcb7b15fcaa7bda8e31572dca..e443ddb0867b592d79776ad75a9b071359fb0326 100644
>--- a/Source/JavaScriptCore/runtime/ErrorConstructor.cpp
>+++ b/Source/JavaScriptCore/runtime/ErrorConstructor.cpp
>@@ -33,8 +33,11 @@ STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(ErrorConstructor);
> 
> const ClassInfo ErrorConstructor::s_info = { "Function", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(ErrorConstructor) };
> 
>+static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*);
>+static EncodedJSValue JSC_HOST_CALL constructErrorConstructor(ExecState*);
>+
> ErrorConstructor::ErrorConstructor(VM& vm, Structure* structure)
>-    : InternalFunction(vm, structure, Interpreter::callErrorConstructor, Interpreter::constructWithErrorConstructor)
>+    : InternalFunction(vm, structure, callErrorConstructor, constructErrorConstructor)
> {
> }
> 
>@@ -44,15 +47,12 @@ void ErrorConstructor::finishCreation(VM& vm, ErrorPrototype* errorPrototype)
>     // ECMA 15.11.3.1 Error.prototype
>     putDirectWithoutTransition(vm, vm.propertyNames->prototype, errorPrototype, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly);
>     putDirectWithoutTransition(vm, vm.propertyNames->length, jsNumber(1), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);
>-
>-    unsigned defaultStackTraceLimit = Options::defaultErrorStackTraceLimit();
>-    m_stackTraceLimit = defaultStackTraceLimit;
>-    putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(defaultStackTraceLimit), static_cast<unsigned>(PropertyAttribute::None));
>+    putDirectWithoutTransition(vm, vm.propertyNames->stackTraceLimit, jsNumber(globalObject(vm)->stackTraceLimit().valueOr(Options::defaultErrorStackTraceLimit())), static_cast<unsigned>(PropertyAttribute::None));
> }
> 
> // ECMA 15.9.3
> 
>-EncodedJSValue JSC_HOST_CALL Interpreter::constructWithErrorConstructor(ExecState* exec)
>+EncodedJSValue JSC_HOST_CALL constructErrorConstructor(ExecState* exec)
> {
>     VM& vm = exec->vm();
>     auto scope = DECLARE_THROW_SCOPE(vm);
>@@ -62,7 +62,7 @@ EncodedJSValue JSC_HOST_CALL Interpreter::constructWithErrorConstructor(ExecStat
>     RELEASE_AND_RETURN(scope, JSValue::encode(ErrorInstance::create(exec, errorStructure, message, nullptr, TypeNothing, false)));
> }
> 
>-EncodedJSValue JSC_HOST_CALL Interpreter::callErrorConstructor(ExecState* exec)
>+EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState* exec)
> {
>     JSValue message = exec->argument(0);
>     Structure* errorStructure = jsCast<InternalFunction*>(exec->jsCallee())->globalObject(exec->vm())->errorStructure();
>@@ -79,9 +79,9 @@ bool ErrorConstructor::put(JSCell* cell, ExecState* exec, PropertyName propertyN
>             double effectiveLimit = value.asNumber();
>             effectiveLimit = std::max(0., effectiveLimit);
>             effectiveLimit = std::min(effectiveLimit, static_cast<double>(std::numeric_limits<unsigned>::max()));
>-            thisObject->m_stackTraceLimit = static_cast<unsigned>(effectiveLimit);
>+            thisObject->globalObject(vm)->setStackTraceLimit(static_cast<unsigned>(effectiveLimit));
>         } else
>-            thisObject->m_stackTraceLimit = { };
>+            thisObject->globalObject(vm)->setStackTraceLimit(WTF::nullopt);
>     }
> 
>     return Base::put(thisObject, exec, propertyName, value, slot);
>@@ -93,7 +93,7 @@ bool ErrorConstructor::deleteProperty(JSCell* cell, ExecState* exec, PropertyNam
>     ErrorConstructor* thisObject = jsCast<ErrorConstructor*>(cell);
> 
>     if (propertyName == vm.propertyNames->stackTraceLimit)
>-        thisObject->m_stackTraceLimit = { };
>+        thisObject->globalObject(vm)->setStackTraceLimit(WTF::nullopt);
> 
>     return Base::deleteProperty(thisObject, exec, propertyName);
> }
>diff --git a/Source/JavaScriptCore/runtime/ErrorConstructor.h b/Source/JavaScriptCore/runtime/ErrorConstructor.h
>index 857d31955cfe3719247a66d137f726b00110e07a..6edee730066f75baf5b6ac60739760e324b786bc 100644
>--- a/Source/JavaScriptCore/runtime/ErrorConstructor.h
>+++ b/Source/JavaScriptCore/runtime/ErrorConstructor.h
>@@ -29,13 +29,7 @@ class GetterSetter;
> 
> class ErrorConstructor final : public InternalFunction {
> public:
>-    typedef InternalFunction Base;
>-
>-    template<typename CellType>
>-    static IsoSubspace* subspaceFor(VM& vm)
>-    {
>-        return &vm.errorConstructorSpace;
>-    }
>+    using Base = InternalFunction;
> 
>     static ErrorConstructor* create(VM& vm, Structure* structure, ErrorPrototype* errorPrototype, GetterSetter*)
>     {
>@@ -51,8 +45,6 @@ class ErrorConstructor final : public InternalFunction {
>         return Structure::create(vm, globalObject, prototype, TypeInfo(InternalFunctionType, StructureFlags), info()); 
>     }
> 
>-    Optional<unsigned> stackTraceLimit() const { return m_stackTraceLimit; }
>-
> protected:
>     void finishCreation(VM&, ErrorPrototype*);
> 
>@@ -62,7 +54,8 @@ class ErrorConstructor final : public InternalFunction {
> private:
>     ErrorConstructor(VM&, Structure*);
> 
>-    Optional<unsigned> m_stackTraceLimit;
> };
> 
>+static_assert(sizeof(ErrorConstructor) == sizeof(InternalFunction), "");
>+
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>index cce56a21bbc0366b0ff89743b51e5c5244527117..0a0fbf1b3d0fbe21642bf2df36d914c558f154d9 100644
>--- a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>@@ -360,6 +360,7 @@ JSGlobalObject::JSGlobalObject(VM& vm, Structure* structure, const GlobalObjectM
>     , m_arraySpeciesWatchpoint(ClearWatchpoint)
>     , m_numberToStringWatchpoint(IsWatched)
>     , m_runtimeFlags()
>+    , m_stackTraceLimit(Options::defaultErrorStackTraceLimit())
>     , m_globalObjectMethodTable(globalObjectMethodTable ? globalObjectMethodTable : &s_globalObjectMethodTable)
> {
> }
>@@ -700,7 +701,6 @@ m_ ## lowerName ## Prototype->putDirectWithoutTransition(vm, vm.propertyNames->c
>     
> #undef CREATE_CONSTRUCTOR_FOR_SIMPLE_TYPE
> 
>-    m_errorConstructor.set(vm, this, errorConstructor);
>     m_promiseConstructor.set(vm, this, promiseConstructor);
>     m_internalPromiseConstructor.set(vm, this, internalPromiseConstructor);
>     
>@@ -878,7 +878,7 @@ putDirectWithoutTransition(vm, vm.propertyNames-> jsName, lowerName ## Construct
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().propertyIsEnumerablePrivateName(), privateFuncPropertyIsEnumerable, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().importModulePrivateName(), privateFuncImportModule, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().enqueueJobPrivateName(), JSFunction::create(vm, this, 0, String(), enqueueJob), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>-        GlobalPropertyInfo(vm.propertyNames->builtinNames().ErrorPrivateName(), m_errorConstructor.get(), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>+        GlobalPropertyInfo(vm.propertyNames->builtinNames().ErrorPrivateName(), errorConstructor, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().RangeErrorPrivateName(), m_rangeErrorConstructor.get(), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().TypeErrorPrivateName(), m_typeErrorConstructor.get(), PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>         GlobalPropertyInfo(vm.propertyNames->builtinNames().typedArrayLengthPrivateName(), privateFuncTypedArrayLength, PropertyAttribute::DontEnum | PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly),
>@@ -1548,7 +1548,6 @@ void JSGlobalObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
>     visitor.append(thisObject->m_globalCallee);
>     visitor.append(thisObject->m_stackOverflowFrameCallee);
>     visitor.append(thisObject->m_regExpConstructor);
>-    visitor.append(thisObject->m_errorConstructor);
>     visitor.append(thisObject->m_nativeErrorPrototypeStructure);
>     visitor.append(thisObject->m_nativeErrorStructure);
>     thisObject->m_evalErrorConstructor.visit(visitor);
>diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.h b/Source/JavaScriptCore/runtime/JSGlobalObject.h
>index 15f0637922ab4bfdcbe0c06163240d7c364c78cf..4462ab1861d42caa91a7f1ca3501dc36477d0ec9 100644
>--- a/Source/JavaScriptCore/runtime/JSGlobalObject.h
>+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.h
>@@ -259,7 +259,6 @@ class JSGlobalObject : public JSSegmentedVariableObject {
>     WriteBarrier<JSCallee> m_globalCallee;
>     WriteBarrier<JSCallee> m_stackOverflowFrameCallee;
>     WriteBarrier<RegExpConstructor> m_regExpConstructor;
>-    WriteBarrier<ErrorConstructor> m_errorConstructor;
>     WriteBarrier<Structure> m_nativeErrorPrototypeStructure;
>     WriteBarrier<Structure> m_nativeErrorStructure;
>     LazyProperty<JSGlobalObject, NativeErrorConstructor> m_evalErrorConstructor;
>@@ -498,6 +497,7 @@ class JSGlobalObject : public JSSegmentedVariableObject {
>     String m_webAssemblyDisabledErrorMessage;
>     RuntimeFlags m_runtimeFlags;
>     ConsoleClient* m_consoleClient { nullptr };
>+    Optional<unsigned> m_stackTraceLimit;
> 
> #if !ASSERT_DISABLED
>     const ExecState* m_callFrameAtDebuggerEntry { nullptr };
>@@ -530,6 +530,9 @@ class JSGlobalObject : public JSSegmentedVariableObject {
>     WatchpointSet& ensureReferencedPropertyWatchpointSet(UniquedStringImpl*);
> #endif
> 
>+    Optional<unsigned> stackTraceLimit() const { return m_stackTraceLimit; }
>+    void setStackTraceLimit(Optional<unsigned> value) { m_stackTraceLimit = value; }
>+
> protected:
>     JS_EXPORT_PRIVATE explicit JSGlobalObject(VM&, Structure*, const GlobalObjectMethodTable* = nullptr);
> 
>@@ -573,7 +576,6 @@ class JSGlobalObject : public JSSegmentedVariableObject {
> 
>     RegExpConstructor* regExpConstructor() const { return m_regExpConstructor.get(); }
> 
>-    ErrorConstructor* errorConstructor() const { return m_errorConstructor.get(); }
>     ArrayConstructor* arrayConstructor() const { return m_arrayConstructor.get(); }
>     ObjectConstructor* objectConstructor() const { return m_objectConstructor.get(); }
>     JSPromiseConstructor* promiseConstructor() const { return m_promiseConstructor.get(); }
>diff --git a/Source/JavaScriptCore/runtime/VM.cpp b/Source/JavaScriptCore/runtime/VM.cpp
>index c087ca8d6952fd78c974a773214ac478773e7964..cb2dc47cddd6429ecfd01e932ef8d9885ff10779 100644
>--- a/Source/JavaScriptCore/runtime/VM.cpp
>+++ b/Source/JavaScriptCore/runtime/VM.cpp
>@@ -297,7 +297,6 @@ VM::VM(VMType vmType, HeapType heapType)
>     , boundFunctionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSBoundFunction)
>     , callbackFunctionSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSCallbackFunction)
>     , customGetterSetterFunctionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSCustomGetterSetterFunction)
>-    , errorConstructorSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), ErrorConstructor)
>     , executableToCodeBlockEdgeSpace ISO_SUBSPACE_INIT(heap, cellDangerousBitsHeapCellType.get(), ExecutableToCodeBlockEdge)
>     , functionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSFunction)
>     , generatorFunctionSpace ISO_SUBSPACE_INIT(heap, cellJSValueOOBHeapCellType.get(), JSGeneratorFunction)
>diff --git a/Source/JavaScriptCore/runtime/VM.h b/Source/JavaScriptCore/runtime/VM.h
>index ee0b3a66dde1e74fb52596dd39e0929443337c19..3f4accf358bf8cae99aaecdf6d9da105c1c4cd33 100644
>--- a/Source/JavaScriptCore/runtime/VM.h
>+++ b/Source/JavaScriptCore/runtime/VM.h
>@@ -372,7 +372,6 @@ class VM : public ThreadSafeRefCounted<VM>, public DoublyLinkedListNode<VM> {
>     IsoSubspace boundFunctionSpace;
>     IsoSubspace callbackFunctionSpace;
>     IsoSubspace customGetterSetterFunctionSpace;
>-    IsoSubspace errorConstructorSpace;
>     IsoSubspace executableToCodeBlockEdgeSpace;
>     IsoSubspace functionSpace;
>     IsoSubspace generatorFunctionSpace;

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193800: 360061 | 360062 | 360066