Attachment 372672 Details for Bug 197631 – Patch

[patch] Patch

bug-197631-20190621220048.patch (text/plain), 12.16 KB, created by Yusuke Suzuki on 2019-06-21 22:00:49 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-06-21 22:00:49 PDT

Size: 12.16 KB

patch

obsolete

>Subversion Revision: 246704
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 93f15ed9a43e64d3c98785971a0eeb75f7d1a0f1..032c83a1223267e2c22e8e0a4451d340ef042b6b 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,50 @@
>+2019-06-21  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 NOBODY (OOPS!).
>+
>+        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-21  Alexey Shvayka  <shvaikalesh@gmail.com>
> 
>         Remove extra check in RegExp @matchSlow
>diff --git a/Source/JavaScriptCore/runtime/JSAsyncFunction.h b/Source/JavaScriptCore/runtime/JSAsyncFunction.h
>index ba80bab6c9c9fbb644aa72fc6f088d6e979b0772..cb198b5fa7140513e4cb261baed93349c487531c 100644
>--- a/Source/JavaScriptCore/runtime/JSAsyncFunction.h
>+++ b/Source/JavaScriptCore/runtime/JSAsyncFunction.h
>@@ -38,12 +38,6 @@ class JSAsyncFunction final : public JSFunction {
> 
>     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*);
>diff --git a/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.cpp b/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.cpp
>index 09fac058662feea2cfc1892c107e242d43739333..ce85b2bea823b63f5332977ade2c355a9c1b3912 100644
>--- a/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.cpp
>+++ b/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.cpp
>@@ -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)
>diff --git a/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.h b/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.h
>index 70239b8c03a03545e193c6781bf452a5b6af4e37..86ba371b0d11c60d78a21892220c6d2b3d29a90f 100644
>--- a/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.h
>+++ b/Source/JavaScriptCore/runtime/JSAsyncGeneratorFunction.h
>@@ -38,12 +38,6 @@ class JSAsyncGeneratorFunction final : public JSFunction {
> 
>     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*);
>diff --git a/Source/JavaScriptCore/runtime/JSFunction.cpp b/Source/JavaScriptCore/runtime/JSFunction.cpp
>index 9ab3c22c04b425a725d353764827b2b34b2ec332..f244fa92564ae8d7830eba742ae785fca0e763d7 100644
>--- a/Source/JavaScriptCore/runtime/JSFunction.cpp
>+++ b/Source/JavaScriptCore/runtime/JSFunction.cpp
>@@ -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
> {
>diff --git a/Source/JavaScriptCore/runtime/JSFunction.h b/Source/JavaScriptCore/runtime/JSFunction.h
>index 2248330d03fdfa9c527e6281200f563b72e22b12..95f547b5b4474234dfe457d61df25c4d1a1b9a90 100644
>--- a/Source/JavaScriptCore/runtime/JSFunction.h
>+++ b/Source/JavaScriptCore/runtime/JSFunction.h
>@@ -225,4 +225,52 @@ class JSFunction : public JSCallee {
>     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
>diff --git a/Source/JavaScriptCore/runtime/JSGeneratorFunction.h b/Source/JavaScriptCore/runtime/JSGeneratorFunction.h
>index 99781c57b97a303675203988f2b392bda82a0311..a454b9588061d48b57f56b30efe063c84f26b293 100644
>--- a/Source/JavaScriptCore/runtime/JSGeneratorFunction.h
>+++ b/Source/JavaScriptCore/runtime/JSGeneratorFunction.h
>@@ -66,12 +66,6 @@ class JSGeneratorFunction final : public JSFunction {
> 
>     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*);
>diff --git a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>index 2f948b201d1989542eadfd6e6948d3be3bf80044..3c04536449105f07665aa7728ff0a211654a0540 100644
>--- a/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>+++ b/Source/JavaScriptCore/runtime/JSGlobalObject.cpp
>@@ -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);
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 0abc105640ec1e3289a1e954c63d4b9c38874ec1..fd1e538f063a60be732604e9a438b8524fb80698 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-06-21  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 NOBODY (OOPS!).
>+
>+        * stress/has-own-property-arguments.js: Added.
>+        (shouldBe):
>+        (A):
>+
> 2019-06-20  Justin Michaud  <justin_michaud@apple.com>
> 
>         [WASM-References] Add extra tests for Wasm references + fix element parsing and subtyping bugs
>diff --git a/JSTests/stress/has-own-property-arguments.js b/JSTests/stress/has-own-property-arguments.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..0b4d1cdfcabb4aa850a7370f0a3a8c751de0233a
>--- /dev/null
>+++ b/JSTests/stress/has-own-property-arguments.js
>@@ -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);

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197631: 369178 | 372671 | 372672