Changeset 233722 in webkit


Ignore:
Timestamp:
Jul 10, 2018, 11:21:22 PM (7 years ago)
Author:
mark.lam@apple.com
Message:

constructArray() should always allocate the requested length.
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):

Location:
trunk
Files:
3 added
9 edited

Legend:

Unmodified
Added
Removed
  • TabularUnified trunk/JSTests/ChangeLog

    r233718 r233722  
     12018-07-10  Mark Lam  <mark.lam@apple.com>
     2
     3        constructArray() should always allocate the requested length.
     4        https://bugs.webkit.org/show_bug.cgi?id=187543
     5        <rdar://problem/41947884>
     6
     7        Reviewed by Saam Barati.
     8
     9        * stress/regress-187543-2.js: Added.
     10        * stress/regress-187543-3.js: Added.
     11        * stress/regress-187543.js: Added.
     12
    1132018-07-10  Keith Miller  <keith_miller@apple.com>
    214
  • TabularUnified trunk/Source/JavaScriptCore/ChangeLog

    r233721 r233722  
     12018-07-10  Mark Lam  <mark.lam@apple.com>
     2
     3        constructArray() should always allocate the requested length.
     4        https://bugs.webkit.org/show_bug.cgi?id=187543
     5        <rdar://problem/41947884>
     6
     7        Reviewed by Saam Barati.
     8
     9        Currently, it does not when we're having a bad time.  We fix this by switching
     10        back to using tryCreateUninitializedRestricted() exclusively in constructArray().
     11        If we detect that a structure transition is possible before we can initialize
     12        the butterfly, we'll go ahead and eagerly initialize the rest of the butterfly.
     13        We will introduce JSArray::eagerlyInitializeButterfly() to handle this.
     14
     15        Also enhanced the DisallowScope and ObjectInitializationScope to support this
     16        eager initialization when needed.
     17
     18        * dfg/DFGOperations.cpp:
     19        - the client of operationNewArrayWithSizeAndHint() (in FTL generated code) expects
     20          the array allocation to always succeed.  Adding this RELEASE_ASSERT here makes
     21          it clearer that we encountered an OutOfMemory condition instead of failing in FTL
     22          generated code, which will appear as a generic null pointer dereference.
     23
     24        * runtime/ArrayPrototype.cpp:
     25        (JSC::concatAppendOne):
     26        - the code here clearly wants to check for an allocation failure.  Switched to
     27          using JSArray::tryCreate() instead of JSArray::create().
     28
     29        * runtime/DisallowScope.h:
     30        (JSC::DisallowScope::disable):
     31        * runtime/JSArray.cpp:
     32        (JSC::JSArray::tryCreateUninitializedRestricted):
     33        (JSC::JSArray::eagerlyInitializeButterfly):
     34        (JSC::constructArray):
     35        * runtime/JSArray.h:
     36        * runtime/ObjectInitializationScope.cpp:
     37        (JSC::ObjectInitializationScope::notifyInitialized):
     38        * runtime/ObjectInitializationScope.h:
     39        (JSC::ObjectInitializationScope::notifyInitialized):
     40
    1412018-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
    242
  • TabularUnified trunk/Source/JavaScriptCore/dfg/DFGOperations.cpp

    r233122 r233722  
    15331533    else {
    15341534        result = JSArray::tryCreate(vm, arrayStructure, size, vectorLengthHint);
    1535         ASSERT(result);
     1535        RELEASE_ASSERT(result);
    15361536    }
    15371537    return bitwise_cast<char*>(result);
  • TabularUnified trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp

    r233245 r233722  
    12701270
    12711271    Structure* resultStructure = exec->lexicalGlobalObject()->arrayStructureForIndexingTypeDuringAllocation(type);
    1272     JSArray* result = JSArray::create(vm, resultStructure, resultSize);
     1272    JSArray* result = JSArray::tryCreate(vm, resultStructure, resultSize);
    12731273    if (UNLIKELY(!result)) {
    12741274        throwOutOfMemoryError(exec, scope);
  • TabularUnified trunk/Source/JavaScriptCore/runtime/DisallowScope.h

    r215885 r233722  
    11/*
    2  * Copyright (C) 2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
     
    6767    }
    6868
     69    void disable()
     70    {
     71        m_isEnabled = false;
     72        exitScope();
     73    }
     74
    6975private:
    7076    void enterScope()
  • TabularUnified trunk/Source/JavaScriptCore/runtime/JSArray.cpp

    r233167 r233722  
    4949
    5050    if (UNLIKELY(initialLength > MAX_STORAGE_VECTOR_LENGTH))
    51         return 0;
     51        return nullptr;
    5252
    5353    unsigned outOfLineStorage = structure->outOfLineCapacity();
     
    7979        }
    8080    } else {
     81        ASSERT(
     82            indexingType == ArrayWithSlowPutArrayStorage
     83            || indexingType == ArrayWithArrayStorage);
    8184        static const unsigned indexBias = 0;
    8285        unsigned vectorLength = ArrayStorage::optimalVectorLength(indexBias, structure, initialLength);
     
    102105    scope.notifyAllocated(result, createUninitialized);
    103106    return result;
     107}
     108
     109void JSArray::eagerlyInitializeButterfly(ObjectInitializationScope& scope, JSArray* array, unsigned initialLength)
     110{
     111    Structure* structure = array->structure(scope.vm());
     112    IndexingType indexingType = structure->indexingType();
     113    Butterfly* butterfly = array->butterfly();
     114
     115    // This function only serves as a companion to tryCreateUninitializedRestricted()
     116    // in the event that we really can't defer initialization of the butterfly after all.
     117    // tryCreateUninitializedRestricted() already initialized the elements between
     118    // initialLength and vector length. We just need to do 0 - initialLength.
     119    // ObjectInitializationScope::notifyInitialized() will verify that all elements are
     120    // initialized.
     121    if (LIKELY(!hasAnyArrayStorage(indexingType))) {
     122        if (hasDouble(indexingType)) {
     123            for (unsigned i = 0; i < initialLength; ++i)
     124                butterfly->contiguousDouble().atUnsafe(i) = PNaN;
     125        } else {
     126            for (unsigned i = 0; i < initialLength; ++i)
     127                butterfly->contiguous().atUnsafe(i).clear();
     128        }
     129    } else {
     130        ArrayStorage* storage = butterfly->arrayStorage();
     131        for (unsigned i = 0; i < initialLength; ++i)
     132            storage->m_vector[i].clear();
     133    }
     134    scope.notifyInitialized(array);
    104135}
    105136
     
    13411372inline JSArray* constructArray(ObjectInitializationScope& scope, Structure* arrayStructure, unsigned length)
    13421373{
    1343     // FIXME: We only need this for subclasses of Array because we might need to allocate a new structure to change
    1344     // indexing types while initializing. If this triggered a GC then we might scan our currently uninitialized
    1345     // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
    1346     JSArray* array;
    1347     if (arrayStructure->globalObject()->isOriginalArrayStructure(arrayStructure))
    1348         array = JSArray::tryCreateUninitializedRestricted(scope, arrayStructure, length);
    1349     else {
    1350         array = JSArray::create(scope.vm(), arrayStructure, length);
    1351 
    1352         // Our client will initialize the storage using initializeIndex() up to
    1353         // length values, and expects that we've already set m_numValuesInVector
    1354         // to length. This matches the behavior of tryCreateUninitializedRestricted().
    1355         IndexingType indexingType = arrayStructure->indexingType();
    1356         if (UNLIKELY(hasAnyArrayStorage(indexingType)))
    1357             array->butterfly()->arrayStorage()->m_numValuesInVector = length;
    1358     }
     1374    JSArray* array = JSArray::tryCreateUninitializedRestricted(scope, arrayStructure, length);
    13591375
    13601376    // FIXME: we should probably throw an out of memory error here, but
     
    13631379    // https://bugs.webkit.org/show_bug.cgi?id=169786
    13641380    RELEASE_ASSERT(array);
     1381
     1382    // FIXME: We only need this for subclasses of Array because we might need to allocate a new structure to change
     1383    // indexing types while initializing. If this triggered a GC then we might scan our currently uninitialized
     1384    // array and crash. https://bugs.webkit.org/show_bug.cgi?id=186811
     1385    if (!arrayStructure->globalObject()->isOriginalArrayStructure(arrayStructure))
     1386        JSArray::eagerlyInitializeButterfly(scope, array, length);
     1387
    13651388    return array;
    13661389}
  • TabularUnified trunk/Source/JavaScriptCore/runtime/JSArray.h

    r233122 r233722  
    8080        return tryCreateUninitializedRestricted(scope, nullptr, structure, initialLength);
    8181    }
     82
     83    static void eagerlyInitializeButterfly(ObjectInitializationScope&, JSArray*, unsigned initialLength);
    8284
    8385    JS_EXPORT_PRIVATE static bool defineOwnProperty(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool throwException);
  • TabularUnified trunk/Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp

    r233697 r233722  
    11/*
    2  * Copyright (C) 2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
     
    5858}
    5959
     60void ObjectInitializationScope::notifyInitialized(JSObject* object)
     61{
     62    if (m_object) {
     63        m_disallowGC.disable();
     64        m_disallowVMReentry.disable();
     65        m_object = nullptr;
     66    }
     67    verifyPropertiesAreInitialized(object);
     68}
     69
    6070void ObjectInitializationScope::verifyPropertiesAreInitialized(JSObject* object)
    6171{
  • TabularUnified trunk/Source/JavaScriptCore/runtime/ObjectInitializationScope.h

    r215885 r233722  
    11/*
    2  * Copyright (C) 2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
     
    4444    ALWAYS_INLINE VM& vm() const { return m_vm; }
    4545    ALWAYS_INLINE void notifyAllocated(JSObject*, bool) { }
     46    ALWAYS_INLINE void notifyInitialized(JSObject*) { }
    4647
    4748private:
     
    5859    VM& vm() const { return m_vm; }
    5960    void notifyAllocated(JSObject*, bool wasCreatedUninitialized);
     61    void notifyInitialized(JSObject*);
    6062
    6163private:
Note: See TracChangeset for help on using the changeset viewer.