constructArray() should always allocate the requested length.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Jul 2018 06:21:22 +0000 (06:21 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Jul 2018 06:21:22 +0000 (06:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187543
<rdar://problem/41947884>

Reviewed by Saam Barati.

JSTests:

* stress/regress-187543-2.js: Added.
* stress/regress-187543-3.js: Added.
* stress/regress-187543.js: Added.

Source/JavaScriptCore:

Currently, it does not when we're having a bad time.  We fix this by switching
back to using tryCreateUninitializedRestricted() exclusively in constructArray().
If we detect that a structure transition is possible before we can initialize
the butterfly, we'll go ahead and eagerly initialize the rest of the butterfly.
We will introduce JSArray::eagerlyInitializeButterfly() to handle this.

Also enhanced the DisallowScope and ObjectInitializationScope to support this
eager initialization when needed.

* dfg/DFGOperations.cpp:
- the client of operationNewArrayWithSizeAndHint() (in FTL generated code) expects
  the array allocation to always succeed.  Adding this RELEASE_ASSERT here makes
  it clearer that we encountered an OutOfMemory condition instead of failing in FTL
  generated code, which will appear as a generic null pointer dereference.

* runtime/ArrayPrototype.cpp:
(JSC::concatAppendOne):
- the code here clearly wants to check for an allocation failure.  Switched to
  using JSArray::tryCreate() instead of JSArray::create().

* runtime/DisallowScope.h:
(JSC::DisallowScope::disable):
* runtime/JSArray.cpp:
(JSC::JSArray::tryCreateUninitializedRestricted):
(JSC::JSArray::eagerlyInitializeButterfly):
(JSC::constructArray):
* runtime/JSArray.h:
* runtime/ObjectInitializationScope.cpp:
(JSC::ObjectInitializationScope::notifyInitialized):
* runtime/ObjectInitializationScope.h:
(JSC::ObjectInitializationScope::notifyInitialized):

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

12 files changed:
JSTests/ChangeLog
JSTests/stress/regress-187543-2.js [new file with mode: 0644]
JSTests/stress/regress-187543-3.js [new file with mode: 0644]
JSTests/stress/regress-187543.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/runtime/DisallowScope.h
Source/JavaScriptCore/runtime/JSArray.cpp
Source/JavaScriptCore/runtime/JSArray.h
Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp
Source/JavaScriptCore/runtime/ObjectInitializationScope.h

index efa7784..f8007ee 100644 (file)
@@ -1,3 +1,15 @@
+2018-07-10  Mark Lam  <mark.lam@apple.com>
+
+        constructArray() should always allocate the requested length.
+        https://bugs.webkit.org/show_bug.cgi?id=187543
+        <rdar://problem/41947884>
+
+        Reviewed by Saam Barati.
+
+        * stress/regress-187543-2.js: Added.
+        * stress/regress-187543-3.js: Added.
+        * stress/regress-187543.js: Added.
+
 2018-07-10  Keith Miller  <keith_miller@apple.com>
 
         hasOwnProperty returns true for out of bounds property index on TypedArray
diff --git a/JSTests/stress/regress-187543-2.js b/JSTests/stress/regress-187543-2.js
new file mode 100644 (file)
index 0000000..2764548
--- /dev/null
@@ -0,0 +1,10 @@
+// This test should not crash.
+
+Object.defineProperty(Object.prototype, 0, { set() {} });
+
+var foo = Function.bind.call(new Proxy(Array, {}));
+for (var i = 10; i < 50; ++i) {
+    var args = Array(i).fill(i);
+    new foo(...args);
+    gc()
+}
diff --git a/JSTests/stress/regress-187543-3.js b/JSTests/stress/regress-187543-3.js
new file mode 100644 (file)
index 0000000..25d1b30
--- /dev/null
@@ -0,0 +1,14 @@
+//@ requireOptions("--useDollarVM=false")
+
+// This test should not crash.
+Date.prototype.valueOf;
+Math.abs;
+
+Object.prototype.__defineGetter__(0, function () {});
+
+class Test extends Array {}
+
+for (let i = 0; i < 100; i++)
+    new Test(1, 2, 3, -4, 5, 6, 7, 8, 9).splice();
+
+gc();
diff --git a/JSTests/stress/regress-187543.js b/JSTests/stress/regress-187543.js
new file mode 100644 (file)
index 0000000..b47fb55
--- /dev/null
@@ -0,0 +1,7 @@
+// This test should not crash.
+
+Object.defineProperty(Object.prototype, 0, { set() { } });
+var foo = Function.bind.call(new Proxy(Array, {}));
+gc();
+new foo(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25);
+gc();
index 89ca52e..81021db 100644 (file)
@@ -1,3 +1,43 @@
+2018-07-10  Mark Lam  <mark.lam@apple.com>
+
+        constructArray() should always allocate the requested length.
+        https://bugs.webkit.org/show_bug.cgi?id=187543
+        <rdar://problem/41947884>
+
+        Reviewed by Saam Barati.
+
+        Currently, it does not when we're having a bad time.  We fix this by switching
+        back to using tryCreateUninitializedRestricted() exclusively in constructArray().
+        If we detect that a structure transition is possible before we can initialize
+        the butterfly, we'll go ahead and eagerly initialize the rest of the butterfly.
+        We will introduce JSArray::eagerlyInitializeButterfly() to handle this.
+
+        Also enhanced the DisallowScope and ObjectInitializationScope to support this
+        eager initialization when needed.
+
+        * dfg/DFGOperations.cpp:
+        - the client of operationNewArrayWithSizeAndHint() (in FTL generated code) expects
+          the array allocation to always succeed.  Adding this RELEASE_ASSERT here makes
+          it clearer that we encountered an OutOfMemory condition instead of failing in FTL
+          generated code, which will appear as a generic null pointer dereference.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::concatAppendOne):
+        - the code here clearly wants to check for an allocation failure.  Switched to
+          using JSArray::tryCreate() instead of JSArray::create().
+
+        * runtime/DisallowScope.h:
+        (JSC::DisallowScope::disable):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::tryCreateUninitializedRestricted):
+        (JSC::JSArray::eagerlyInitializeButterfly):
+        (JSC::constructArray):
+        * runtime/JSArray.h:
+        * runtime/ObjectInitializationScope.cpp:
+        (JSC::ObjectInitializationScope::notifyInitialized):
+        * runtime/ObjectInitializationScope.h:
+        (JSC::ObjectInitializationScope::notifyInitialized):
+
 2018-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Remove getTypedArrayImpl
index 4d06e89..661369c 100644 (file)
@@ -1532,7 +1532,7 @@ char* JIT_OPERATION operationNewArrayWithSizeAndHint(ExecState* exec, Structure*
         result = JSArray::createWithButterfly(vm, nullptr, arrayStructure, butterfly);
     else {
         result = JSArray::tryCreate(vm, arrayStructure, size, vectorLengthHint);
-        ASSERT(result);
+        RELEASE_ASSERT(result);
     }
     return bitwise_cast<char*>(result);
 }
index 4774750..abe4929 100644 (file)
@@ -1269,7 +1269,7 @@ static EncodedJSValue concatAppendOne(ExecState* exec, VM& vm, JSArray* first, J
         type = first->indexingType();
 
     Structure* resultStructure = exec->lexicalGlobalObject()->arrayStructureForIndexingTypeDuringAllocation(type);
-    JSArray* result = JSArray::create(vm, resultStructure, resultSize);
+    JSArray* result = JSArray::tryCreate(vm, resultStructure, resultSize);
     if (UNLIKELY(!result)) {
         throwOutOfMemoryError(exec, scope);
         return encodedJSValue();
index a084e99..979cf18 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -66,6 +66,12 @@ public:
         enterScope();
     }
 
+    void disable()
+    {
+        m_isEnabled = false;
+        exitScope();
+    }
+
 private:
     void enterScope()
     {
index fcc1688..b66b7c9 100644 (file)
@@ -48,7 +48,7 @@ JSArray* JSArray::tryCreateUninitializedRestricted(ObjectInitializationScope& sc
     VM& vm = scope.vm();
 
     if (UNLIKELY(initialLength > MAX_STORAGE_VECTOR_LENGTH))
-        return 0;
+        return nullptr;
 
     unsigned outOfLineStorage = structure->outOfLineCapacity();
     Butterfly* butterfly;
@@ -78,6 +78,9 @@ JSArray* JSArray::tryCreateUninitializedRestricted(ObjectInitializationScope& sc
                 butterfly->contiguous().atUnsafe(i).clear();
         }
     } else {
+        ASSERT(
+            indexingType == ArrayWithSlowPutArrayStorage
+            || indexingType == ArrayWithArrayStorage);
         static const unsigned indexBias = 0;
         unsigned vectorLength = ArrayStorage::optimalVectorLength(indexBias, structure, initialLength);
         void* temp = vm.jsValueGigacageAuxiliarySpace.allocateNonVirtual(
@@ -103,6 +106,34 @@ JSArray* JSArray::tryCreateUninitializedRestricted(ObjectInitializationScope& sc
     return result;
 }
 
+void JSArray::eagerlyInitializeButterfly(ObjectInitializationScope& scope, JSArray* array, unsigned initialLength)
+{
+    Structure* structure = array->structure(scope.vm());
+    IndexingType indexingType = structure->indexingType();
+    Butterfly* butterfly = array->butterfly();
+
+    // This function only serves as a companion to tryCreateUninitializedRestricted()
+    // in the event that we really can't defer initialization of the butterfly after all.
+    // tryCreateUninitializedRestricted() already initialized the elements between
+    // initialLength and vector length. We just need to do 0 - initialLength.
+    // ObjectInitializationScope::notifyInitialized() will verify that all elements are
+    // initialized.
+    if (LIKELY(!hasAnyArrayStorage(indexingType))) {
+        if (hasDouble(indexingType)) {
+            for (unsigned i = 0; i < initialLength; ++i)
+                butterfly->contiguousDouble().atUnsafe(i) = PNaN;
+        } else {
+            for (unsigned i = 0; i < initialLength; ++i)
+                butterfly->contiguous().atUnsafe(i).clear();
+        }
+    } else {
+        ArrayStorage* storage = butterfly->arrayStorage();
+        for (unsigned i = 0; i < initialLength; ++i)
+            storage->m_vector[i].clear();
+    }
+    scope.notifyInitialized(array);
+}
+
 void JSArray::setLengthWritable(ExecState* exec, bool writable)
 {
     ASSERT(isLengthWritable() || !writable);
@@ -1340,28 +1371,20 @@ bool JSArray::isIteratorProtocolFastAndNonObservable()
 
 inline JSArray* constructArray(ObjectInitializationScope& scope, Structure* arrayStructure, unsigned length)
 {
-    // FIXME: We only need this for subclasses of Array because we might need to allocate a new structure to change
-    // indexing types while initializing. If this triggered a GC then we might scan our currently uninitialized
-    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
-    JSArray* array;
-    if (arrayStructure->globalObject()->isOriginalArrayStructure(arrayStructure))
-        array = JSArray::tryCreateUninitializedRestricted(scope, arrayStructure, length);
-    else {
-        array = JSArray::create(scope.vm(), arrayStructure, length);
-
-        // Our client will initialize the storage using initializeIndex() up to
-        // length values, and expects that we've already set m_numValuesInVector
-        // to length. This matches the behavior of tryCreateUninitializedRestricted().
-        IndexingType indexingType = arrayStructure->indexingType();
-        if (UNLIKELY(hasAnyArrayStorage(indexingType)))
-            array->butterfly()->arrayStorage()->m_numValuesInVector = length;
-    }
+    JSArray* array = JSArray::tryCreateUninitializedRestricted(scope, arrayStructure, length);
 
     // FIXME: we should probably throw an out of memory error here, but
     // when making this change we should check that all clients of this
     // function will correctly handle an exception being thrown from here.
     // https://bugs.webkit.org/show_bug.cgi?id=169786
     RELEASE_ASSERT(array);
+
+    // FIXME: We only need this for subclasses of Array because we might need to allocate a new structure to change
+    // indexing types while initializing. If this triggered a GC then we might scan our currently uninitialized
+    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
+    if (!arrayStructure->globalObject()->isOriginalArrayStructure(arrayStructure))
+        JSArray::eagerlyInitializeButterfly(scope, array, length);
+
     return array;
 }
 
index 8ec8f64..a6ee5d3 100644 (file)
@@ -80,6 +80,8 @@ public:
         return tryCreateUninitializedRestricted(scope, nullptr, structure, initialLength);
     }
 
+    static void eagerlyInitializeButterfly(ObjectInitializationScope&, JSArray*, unsigned initialLength);
+
     JS_EXPORT_PRIVATE static bool defineOwnProperty(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool throwException);
 
     JS_EXPORT_PRIVATE static bool getOwnPropertySlot(JSObject*, ExecState*, PropertyName, PropertySlot&);
index 2b49898..e19c8a9 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -57,6 +57,16 @@ void ObjectInitializationScope::notifyAllocated(JSObject* object, bool wasCreate
         verifyPropertiesAreInitialized(object);
 }
 
+void ObjectInitializationScope::notifyInitialized(JSObject* object)
+{
+    if (m_object) {
+        m_disallowGC.disable();
+        m_disallowVMReentry.disable();
+        m_object = nullptr;
+    }
+    verifyPropertiesAreInitialized(object);
+}
+
 void ObjectInitializationScope::verifyPropertiesAreInitialized(JSObject* object)
 {
     Butterfly* butterfly = object->butterfly();
index 07fcd6a..ea62190 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -43,6 +43,7 @@ public:
 
     ALWAYS_INLINE VM& vm() const { return m_vm; }
     ALWAYS_INLINE void notifyAllocated(JSObject*, bool) { }
+    ALWAYS_INLINE void notifyInitialized(JSObject*) { }
 
 private:
     VM& m_vm;
@@ -57,6 +58,7 @@ public:
 
     VM& vm() const { return m_vm; }
     void notifyAllocated(JSObject*, bool wasCreatedUninitialized);
+    void notifyInitialized(JSObject*);
 
 private:
     void verifyPropertiesAreInitialized(JSObject*);