[JSC] Strict, Sloppy and Arrow functions should have different classInfo
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jun 2019 07:48:08 +0000 (07:48 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jun 2019 07:48:08 +0000 (07:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197631

Reviewed by Saam Barati.

JSTests:

* stress/has-own-property-arguments.js: Added.
(shouldBe):
(A):

Source/JavaScriptCore:

If a constructor inherits a builtin class, it creates a Structure which is subclassing the builtin class.
This is done by using InternalFunction::createSubclassStructure. But to accelerate the common cases, we
cache the created structure in InternalFunctionAllocationProfile. Whether the cache is valid is checked
by comparing classInfo of the cached structure and the given base structure. This implicitly assume that
each builtin class's InternalFunction creates an instance based on one structure.

However, Function constructor is an exception: Function constructor creates an instance which has different
structures based on a parameter. If a strict code is given (e.g. "'use strict'"), it creates a function
instance with strict function structure.

As a result, InternalFunctionAllocationProfile incorrectly caches the structure. Consider the following code.

    class A extends Function { };
    let a = new A("'use strict'");
    let b = new A("");

While `a` and `b` should have different structures, `A` caches the structure for `a`, and reuse it even the given
code is not a strict code. This is problematic: We are separating structures of strict, sloppy, and arrow functions
because they have different properties. However, in the above case, a and b have the same structure while they have
different properties. So it causes incorrect structure-based caching in JSC. One of the example is HasOwnPropertyCache.

In this patch, we introduce JSStrictFunction, JSSloppyFunction, and JSArrowFunction classes and classInfos. This design
works well and already partially accepted for JSGeneratorFunction, JSAsyncGeneratorFunction, and JSAsyncFunction. Each
structure now has a different classInfo so that InternalFunctionAllocationProfile correctly caches and invalidates the
cached one based on the classInfo. Since we already have different structures for these instances, and DFG and FTL
optimizations are based on JSFunctionType (not classInfo), introducing these three classInfo do not break the optimization.

Note that structures on ArrayConstructor does not cause the same problem. It only uses Undecided indexing typed array
structure in InternalFunctionAllocationProfile, and once haveABadTime happens, it clears InternalFunctionAllocationProfile.

* runtime/JSAsyncFunction.h: This subspaceFor is not necessary since it is defined in JSFunction. And we already ensure that
sizeof(JSAsyncFunction) == sizeof(JSFunction).
* runtime/JSAsyncGeneratorFunction.cpp:
* runtime/JSAsyncGeneratorFunction.h: Ditto.
* runtime/JSFunction.cpp:
* runtime/JSFunction.h:
* runtime/JSGeneratorFunction.h: Ditto.
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):

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

JSTests/ChangeLog
JSTests/stress/has-own-property-arguments.js [new file with mode: 0644]
JSTests/stress/inline-cache-arguments.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSAsyncFunction.h
Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.cpp
Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.h
Source/JavaScriptCore/runtime/JSFunction.cpp
Source/JavaScriptCore/runtime/JSFunction.h
Source/JavaScriptCore/runtime/JSGeneratorFunction.h
Source/JavaScriptCore/runtime/JSGlobalObject.cpp

index 51df831..209b898 100644 (file)
@@ -1,5 +1,16 @@
 2019-06-22  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] Strict, Sloppy and Arrow functions should have different classInfo
+        https://bugs.webkit.org/show_bug.cgi?id=197631
+
+        Reviewed by Saam Barati.
+
+        * stress/has-own-property-arguments.js: Added.
+        (shouldBe):
+        (A):
+
+2019-06-22  Yusuke Suzuki  <ysuzuki@apple.com>
+
         [JSC] ClassExpr should not store result in the middle of evaluation
         https://bugs.webkit.org/show_bug.cgi?id=199106
 
diff --git a/JSTests/stress/has-own-property-arguments.js b/JSTests/stress/has-own-property-arguments.js
new file mode 100644 (file)
index 0000000..0b4d1cd
--- /dev/null
@@ -0,0 +1,8 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+class A extends Function {}
+shouldBe(new A("'use strict';").hasOwnProperty('arguments'), false);
+shouldBe(new A().hasOwnProperty('arguments'), true);
diff --git a/JSTests/stress/inline-cache-arguments.js b/JSTests/stress/inline-cache-arguments.js
new file mode 100644 (file)
index 0000000..09091db
--- /dev/null
@@ -0,0 +1,41 @@
+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)}`);
+}
+
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+
+class A extends Function {}
+let a = new A("'use strict';");
+let b = new A();
+
+function test(object) {
+    return object.arguments;
+}
+noInline(test);
+
+for (var i = 0; i < 1e6; ++i) {
+    shouldBe(typeof test(b), "object");
+    shouldBe(typeof test({ arguments: { } }), "object");
+    shouldBe(typeof test({ arguments: { }, hello: 42 }), "object");
+    shouldBe(typeof test({ arguments: { }, world: 42 }), "object");
+    shouldBe(typeof test({ arguments: { }, cocoa: 42 }), "object");
+    shouldBe(typeof test({ arguments: { }, cappuccino: 42 }), "object");
+}
+
+shouldThrow(() => {
+    test(a);
+}, `TypeError: 'arguments', 'callee', and 'caller' cannot be accessed in this context.`);
index e496160..23d774c 100644 (file)
@@ -1,5 +1,52 @@
 2019-06-22  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] Strict, Sloppy and Arrow functions should have different classInfo
+        https://bugs.webkit.org/show_bug.cgi?id=197631
+
+        Reviewed by Saam Barati.
+
+        If a constructor inherits a builtin class, it creates a Structure which is subclassing the builtin class.
+        This is done by using InternalFunction::createSubclassStructure. But to accelerate the common cases, we
+        cache the created structure in InternalFunctionAllocationProfile. Whether the cache is valid is checked
+        by comparing classInfo of the cached structure and the given base structure. This implicitly assume that
+        each builtin class's InternalFunction creates an instance based on one structure.
+
+        However, Function constructor is an exception: Function constructor creates an instance which has different
+        structures based on a parameter. If a strict code is given (e.g. "'use strict'"), it creates a function
+        instance with strict function structure.
+
+        As a result, InternalFunctionAllocationProfile incorrectly caches the structure. Consider the following code.
+
+            class A extends Function { };
+            let a = new A("'use strict'");
+            let b = new A("");
+
+        While `a` and `b` should have different structures, `A` caches the structure for `a`, and reuse it even the given
+        code is not a strict code. This is problematic: We are separating structures of strict, sloppy, and arrow functions
+        because they have different properties. However, in the above case, a and b have the same structure while they have
+        different properties. So it causes incorrect structure-based caching in JSC. One of the example is HasOwnPropertyCache.
+
+        In this patch, we introduce JSStrictFunction, JSSloppyFunction, and JSArrowFunction classes and classInfos. This design
+        works well and already partially accepted for JSGeneratorFunction, JSAsyncGeneratorFunction, and JSAsyncFunction. Each
+        structure now has a different classInfo so that InternalFunctionAllocationProfile correctly caches and invalidates the
+        cached one based on the classInfo. Since we already have different structures for these instances, and DFG and FTL
+        optimizations are based on JSFunctionType (not classInfo), introducing these three classInfo do not break the optimization.
+
+        Note that structures on ArrayConstructor does not cause the same problem. It only uses Undecided indexing typed array
+        structure in InternalFunctionAllocationProfile, and once haveABadTime happens, it clears InternalFunctionAllocationProfile.
+
+        * runtime/JSAsyncFunction.h: This subspaceFor is not necessary since it is defined in JSFunction. And we already ensure that
+        sizeof(JSAsyncFunction) == sizeof(JSFunction).
+        * runtime/JSAsyncGeneratorFunction.cpp:
+        * runtime/JSAsyncGeneratorFunction.h: Ditto.
+        * runtime/JSFunction.cpp:
+        * runtime/JSFunction.h:
+        * runtime/JSGeneratorFunction.h: Ditto.
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::init):
+
+2019-06-22  Yusuke Suzuki  <ysuzuki@apple.com>
+
         [JSC] ClassExpr should not store result in the middle of evaluation
         https://bugs.webkit.org/show_bug.cgi?id=199106
 
index ba80bab..cb198b5 100644 (file)
@@ -38,12 +38,6 @@ public:
 
     const static unsigned StructureFlags = Base::StructureFlags;
 
-    template<typename CellType, SubspaceAccess>
-    static IsoSubspace* subspaceFor(VM& vm)
-    {
-        return &vm.functionSpace;
-    }
-
     DECLARE_EXPORT_INFO;
 
     static JSAsyncFunction* create(VM&, FunctionExecutable*, JSScope*);
index 09fac05..ce85b2b 100644 (file)
@@ -37,7 +37,7 @@
 
 namespace JSC {
 
-const ClassInfo JSAsyncGeneratorFunction    ::s_info = { "JSAsyncGeneratorFunction",  &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSAsyncGeneratorFunction) };
+const ClassInfo JSAsyncGeneratorFunction::s_info = { "JSAsyncGeneratorFunction",  &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSAsyncGeneratorFunction) };
 
 JSAsyncGeneratorFunction::JSAsyncGeneratorFunction(VM& vm, FunctionExecutable* executable, JSScope* scope, Structure* structure)
     : Base(vm, executable, scope, structure)
index 70239b8..86ba371 100644 (file)
@@ -38,12 +38,6 @@ public:
 
     const static unsigned StructureFlags = Base::StructureFlags;
 
-    template<typename CellType, SubspaceAccess>
-    static IsoSubspace* subspaceFor(VM& vm)
-    {
-        return &vm.functionSpace;
-    }
-
     DECLARE_EXPORT_INFO;
 
     static JSAsyncGeneratorFunction* create(VM&, FunctionExecutable*, JSScope*);
index 34b4872..c5640cb 100644 (file)
@@ -60,6 +60,9 @@ EncodedJSValue JSC_HOST_CALL callHostFunctionAsConstructor(ExecState* exec)
 }
 
 const ClassInfo JSFunction::s_info = { "Function", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSFunction) };
+const ClassInfo JSStrictFunction::s_info = { "Function", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSStrictFunction) };
+const ClassInfo JSSloppyFunction::s_info = { "Function", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSSloppyFunction) };
+const ClassInfo JSArrowFunction::s_info = { "Function", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSArrowFunction) };
 
 bool JSFunction::isHostFunctionNonInline() const
 {
index 5371504..18d9cfb 100644 (file)
@@ -226,4 +226,52 @@ private:
     WriteBarrier<FunctionRareData> m_rareData;
 };
 
+class JSStrictFunction final : public JSFunction {
+public:
+    using Base = JSFunction;
+
+    DECLARE_EXPORT_INFO;
+
+    static constexpr unsigned StructureFlags = Base::StructureFlags;
+
+    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
+    {
+        ASSERT(globalObject);
+        return Structure::create(vm, globalObject, prototype, TypeInfo(JSFunctionType, StructureFlags), info());
+    }
+};
+static_assert(sizeof(JSStrictFunction) == sizeof(JSFunction), "Allocated in JSFunction IsoSubspace");
+
+class JSSloppyFunction final : public JSFunction {
+public:
+    using Base = JSFunction;
+
+    DECLARE_EXPORT_INFO;
+
+    static constexpr unsigned StructureFlags = Base::StructureFlags;
+
+    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
+    {
+        ASSERT(globalObject);
+        return Structure::create(vm, globalObject, prototype, TypeInfo(JSFunctionType, StructureFlags), info());
+    }
+};
+static_assert(sizeof(JSSloppyFunction) == sizeof(JSFunction), "Allocated in JSFunction IsoSubspace");
+
+class JSArrowFunction final : public JSFunction {
+public:
+    using Base = JSFunction;
+
+    DECLARE_EXPORT_INFO;
+
+    static constexpr unsigned StructureFlags = Base::StructureFlags;
+
+    static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
+    {
+        ASSERT(globalObject);
+        return Structure::create(vm, globalObject, prototype, TypeInfo(JSFunctionType, StructureFlags), info());
+    }
+};
+static_assert(sizeof(JSArrowFunction) == sizeof(JSFunction), "Allocated in JSFunction IsoSubspace");
+
 } // namespace JSC
index 99781c5..a454b95 100644 (file)
@@ -66,12 +66,6 @@ public:
 
     const static unsigned StructureFlags = Base::StructureFlags;
 
-    template<typename CellType, SubspaceAccess>
-    static IsoSubspace* subspaceFor(VM& vm)
-    {
-        return &vm.functionSpace;
-    }
-
     DECLARE_EXPORT_INFO;
 
     static JSGeneratorFunction* create(VM&, FunctionExecutable*, JSScope*);
index 2f948b2..3c04536 100644 (file)
@@ -472,9 +472,9 @@ void JSGlobalObject::init(VM& vm)
     m_hostFunctionStructure.set(vm, this, JSFunction::createStructure(vm, this, m_functionPrototype.get()));
 
     auto initFunctionStructures = [&] (FunctionStructures& structures) {
-        structures.strictFunctionStructure.set(vm, this, JSFunction::createStructure(vm, this, m_functionPrototype.get()));
-        structures.sloppyFunctionStructure.set(vm, this, JSFunction::createStructure(vm, this, m_functionPrototype.get()));
-        structures.arrowFunctionStructure.set(vm, this, JSFunction::createStructure(vm, this, m_functionPrototype.get()));
+        structures.strictFunctionStructure.set(vm, this, JSStrictFunction::createStructure(vm, this, m_functionPrototype.get()));
+        structures.sloppyFunctionStructure.set(vm, this, JSSloppyFunction::createStructure(vm, this, m_functionPrototype.get()));
+        structures.arrowFunctionStructure.set(vm, this, JSArrowFunction::createStructure(vm, this, m_functionPrototype.get()));
     };
     initFunctionStructures(m_builtinFunctions);
     initFunctionStructures(m_ordinaryFunctions);