[JSC] ErrorConstructor should not have own IsoSubspace
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jan 2019 02:47:58 +0000 (02:47 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 Jan 2019 02:47:58 +0000 (02:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193800

Reviewed by Saam Barati.

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:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/interpreter/Interpreter.h
Source/JavaScriptCore/runtime/Error.cpp
Source/JavaScriptCore/runtime/ErrorConstructor.cpp
Source/JavaScriptCore/runtime/ErrorConstructor.h
Source/JavaScriptCore/runtime/JSGlobalObject.cpp
Source/JavaScriptCore/runtime/JSGlobalObject.h
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h

index de140b7..f925715 100644 (file)
@@ -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 Saam Barati.
+
+        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
index 6b926b4..be5d5e9 100644 (file)
@@ -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*);
 
index d933440..1b8e4c2 100644 (file)
@@ -161,13 +161,12 @@ private:
 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;
index b09c058..e443ddb 100644 (file)
@@ -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);
 }
index 857d319..6edee73 100644 (file)
@@ -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 @@ public:
         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 @@ protected:
 private:
     ErrorConstructor(VM&, Structure*);
 
-    Optional<unsigned> m_stackTraceLimit;
 };
 
+static_assert(sizeof(ErrorConstructor) == sizeof(InternalFunction), "");
+
 } // namespace JSC
index cce56a2..0a0fbf1 100644 (file)
@@ -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);
index 15f0637..4462ab1 100644 (file)
@@ -259,7 +259,6 @@ public:
     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 @@ public:
     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 @@ public:
     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 @@ public:
 
     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(); }
index c087ca8..cb2dc47 100644 (file)
@@ -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)
index ee0b3a6..3f4accf 100644 (file)
@@ -372,7 +372,6 @@ public:
     IsoSubspace boundFunctionSpace;
     IsoSubspace callbackFunctionSpace;
     IsoSubspace customGetterSetterFunctionSpace;
-    IsoSubspace errorConstructorSpace;
     IsoSubspace executableToCodeBlockEdgeSpace;
     IsoSubspace functionSpace;
     IsoSubspace generatorFunctionSpace;