Audit and fix incorrect uses of JSArray::tryCreateForInitializationPrivate().
https://bugs.webkit.org/show_bug.cgi?id=171344
<rdar://problem/31352667>
Reviewed by Filip Pizlo.
JSArray::tryCreateForInitializationPrivate() should only be used in performance
critical paths, and should always be used with care because it creates an
uninitialized object that needs to be initialized by its client before the object
can be released into the system. Before the object is fully initialized:
a. the client should not re-enter the VM to execute JS code, and
b. GC should not run.
This is because until the object is fully initialized, it is an inconsistent
state that the GC and JS code will not be happy about.
In this patch, we do the following:
1. Renamed JSArray::tryCreateForInitializationPrivate() to
JSArray::tryCreateUninitializedRestricted() because "private" is a bit ambiguous
and can be confused with APIs that are called freely within WebKit but are
not meant for clients of WebKit. In this case, we intend for use of this API
to be restricted to only a few carefully considered and crafted cases.
2. Introduce the ObjectInitializationScope RAII object which covers the period
when the uninitialized object is created and gets initialized.
ObjectInitializationScope will asserts that either the object is created
fully initialized (in the case where the object structure is not an "original"
structure) or if created uninitialized, is fully initialized at the end of
the scope.
If the object is created uninitialized, the ObjectInitializationScope also
ensures that we do not GC nor re-enter the VM to execute JS code. This is
achieved by enabling DisallowGC and DisallowVMReentry scopes.
tryCreateUninitializedRestricted() and initializeIndex() now requires an
ObjectInitializationScope instance. The ObjectInitializationScope replaces
the VM& argument because it can be used to pass the VM& itself. This is a
small optimization that makes passing the ObjectInitializationScope free even
on release builds.
3. Factored a DisallowScope out of DisallowGC, and make DisallowGC extend it.
Introduce a DisallowVMReentry class that extends DisallowScope.
4. Fixed a bug found by the ObjectInitializationScope. The bug is that there are
scenarios where the structure passed to tryCreateUninitializedRestricted()
that may not be an "original" structure. As a result, initializeIndex() would
end up allocating new structures, and therefore trigger a GC.
The fix is to detect that the structure passed to tryCreateUninitializedRestricted()
is not an "original" one, and pre-initialize the array with 0s.
This bug was detected by existing tests. Hence, no new test needed.
5. Replaced all inappropriate uses of tryCreateUninitializedRestricted() with
tryCreate(). Inappropriate uses here means code that is not in performance
critical paths.
Similarly, replaced accompanying uses of initializeIndex() with putDirectIndex().
This patch is performance neutral (according to the JSC command line benchmarks).
* CMakeLists.txt:
* JavaScriptCore.xcodeproj/project.pbxproj:
* dfg/DFGOperations.cpp:
* ftl/FTLOperations.cpp:
(JSC::FTL::operationMaterializeObjectInOSR):
* heap/DeferGC.cpp:
* heap/DeferGC.h:
(JSC::DisallowGC::DisallowGC):
(JSC::DisallowGC::initialize):
(JSC::DisallowGC::scopeReentryCount):
(JSC::DisallowGC::setScopeReentryCount):
(JSC::DisallowGC::~DisallowGC): Deleted.
(JSC::DisallowGC::isGCDisallowedOnCurrentThread): Deleted.
* heap/GCDeferralContextInlines.h:
(JSC::GCDeferralContext::~GCDeferralContext):
* heap/Heap.cpp:
(JSC::Heap::collectIfNecessaryOrDefer):
* runtime/ArrayPrototype.cpp:
(JSC::arrayProtoPrivateFuncConcatMemcpy):
* runtime/ClonedArguments.cpp:
(JSC::ClonedArguments::createWithInlineFrame):
(JSC::ClonedArguments::createByCopyingFrom):
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL):
* runtime/DisallowScope.h: Added.
(JSC::DisallowScope::DisallowScope):
(JSC::DisallowScope::~DisallowScope):
(JSC::DisallowScope::isInEffectOnCurrentThread):
(JSC::DisallowScope::enable):
(JSC::DisallowScope::enterScope):
(JSC::DisallowScope::exitScope):
* runtime/DisallowVMReentry.cpp: Added.
* runtime/DisallowVMReentry.h: Added.
(JSC::DisallowVMReentry::DisallowVMReentry):
(JSC::DisallowVMReentry::initialize):
(JSC::DisallowVMReentry::scopeReentryCount):
(JSC::DisallowVMReentry::setScopeReentryCount):
* runtime/InitializeThreading.cpp:
(JSC::initializeThreading):
* runtime/JSArray.cpp:
(JSC::JSArray::tryCreateUninitializedRestricted):
(JSC::JSArray::fastSlice):
(JSC::JSArray::tryCreateForInitializationPrivate): Deleted.
* runtime/JSArray.h:
(JSC::JSArray::tryCreateUninitializedRestricted):
(JSC::JSArray::tryCreate):
(JSC::constructArray):
(JSC::constructArrayNegativeIndexed):
(JSC::JSArray::tryCreateForInitializationPrivate): Deleted.
(JSC::createArrayButterfly): Deleted.
* runtime/JSCellInlines.h:
(JSC::allocateCell):
* runtime/JSObject.h:
(JSC::JSObject::initializeIndex):
(JSC::JSObject::initializeIndexWithoutBarrier):
* runtime/ObjectInitializationScope.cpp: Added.
(JSC::ObjectInitializationScope::ObjectInitializationScope):
(JSC::ObjectInitializationScope::~ObjectInitializationScope):
(JSC::ObjectInitializationScope::notifyAllocated):
(JSC::ObjectInitializationScope::verifyPropertiesAreInitialized):
* runtime/ObjectInitializationScope.h: Added.
(JSC::ObjectInitializationScope::ObjectInitializationScope):
(JSC::ObjectInitializationScope::vm):
(JSC::ObjectInitializationScope::notifyAllocated):
* runtime/Operations.h:
(JSC::isScribbledValue):
(JSC::scribble):
* runtime/RegExpMatchesArray.cpp:
(JSC::createEmptyRegExpMatchesArray):
* runtime/RegExpMatchesArray.h:
(JSC::tryCreateUninitializedRegExpMatchesArray):
(JSC::createRegExpMatchesArray):
* runtime/VMEntryScope.cpp:
(JSC::VMEntryScope::VMEntryScope):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@215885 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/runtime/JSArray.h b/Source/JavaScriptCore/runtime/JSArray.h
index f7284dd..ff9ed96 100644
--- a/Source/JavaScriptCore/runtime/JSArray.h
+++ b/Source/JavaScriptCore/runtime/JSArray.h
@@ -56,18 +56,24 @@
static JSArray* create(VM&, Structure*, unsigned initialLength = 0);
static JSArray* createWithButterfly(VM&, GCDeferralContext*, Structure*, Butterfly*);
- // tryCreateForInitializationPrivate is used for fast construction of arrays whose size and
- // contents are known at time of creation. This should be considered a private API.
+ // tryCreateUninitializedRestricted is used for fast construction of arrays whose size and
+ // contents are known at time of creation. This is a restricted API for careful use only in
+ // performance critical code paths. If you don't have a good reason to use it, you probably
+ // shouldn't use it. Instead, you should go with
+ // - JSArray::tryCreate() or JSArray::create() instead of tryCreateUninitializedRestricted(), and
+ // - putDirectIndex() instead of initializeIndex().
+ //
// Clients of this interface must:
// - null-check the result (indicating out of memory, or otherwise unable to allocate vector).
// - call 'initializeIndex' for all properties in sequence, for 0 <= i < initialLength.
// - Provide a valid GCDefferalContext* if they might garbage collect when initializing properties,
// otherwise the caller can provide a null GCDefferalContext*.
+ // - Provide a local stack instance of ObjectInitializationScope at the call site.
//
- JS_EXPORT_PRIVATE static JSArray* tryCreateForInitializationPrivate(VM&, GCDeferralContext*, Structure*, unsigned initialLength);
- static JSArray* tryCreateForInitializationPrivate(VM& vm, Structure* structure, unsigned initialLength)
+ JS_EXPORT_PRIVATE static JSArray* tryCreateUninitializedRestricted(ObjectInitializationScope&, GCDeferralContext*, Structure*, unsigned initialLength);
+ static JSArray* tryCreateUninitializedRestricted(ObjectInitializationScope& scope, Structure* structure, unsigned initialLength)
{
- return tryCreateForInitializationPrivate(vm, nullptr, structure, initialLength);
+ return tryCreateUninitializedRestricted(scope, nullptr, structure, initialLength);
}
JS_EXPORT_PRIVATE static bool defineOwnProperty(JSObject*, ExecState*, PropertyName, const PropertyDescriptor&, bool throwException);
@@ -204,13 +210,6 @@
return butterfly;
}
-inline Butterfly* createArrayButterfly(VM& vm, JSCell* intendedOwner, unsigned initialLength)
-{
- Butterfly* result = tryCreateArrayButterfly(vm, intendedOwner, initialLength);
- RELEASE_ASSERT(result);
- return result;
-}
-
Butterfly* createArrayButterflyInDictionaryIndexingMode(
VM&, JSCell* intendedOwner, unsigned initialLength);
@@ -227,8 +226,8 @@
|| hasDouble(indexingType)
|| hasContiguous(indexingType));
- if (initialLength > MAX_STORAGE_VECTOR_LENGTH)
- return 0;
+ if (UNLIKELY(initialLength > MAX_STORAGE_VECTOR_LENGTH))
+ return nullptr;
unsigned vectorLength = Butterfly::optimalContiguousVectorLength(structure, initialLength);
void* temp = vm.auxiliarySpace.tryAllocate(nullptr, Butterfly::totalSize(0, outOfLineStorage, true, vectorLength * sizeof(EncodedJSValue)));
@@ -245,7 +244,7 @@
ASSERT(
indexingType == ArrayWithSlowPutArrayStorage
|| indexingType == ArrayWithArrayStorage);
- butterfly = tryCreateArrayButterfly(vm, 0, initialLength);
+ butterfly = tryCreateArrayButterfly(vm, nullptr, initialLength);
if (!butterfly)
return nullptr;
for (unsigned i = 0; i < BASE_ARRAY_STORAGE_VECTOR_LEN; ++i)
@@ -295,7 +294,8 @@
{
VM& vm = exec->vm();
unsigned length = values.size();
- JSArray* array = JSArray::tryCreateForInitializationPrivate(vm, arrayStructure, length);
+ ObjectInitializationScope scope(vm);
+ 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
@@ -304,14 +304,15 @@
RELEASE_ASSERT(array);
for (unsigned i = 0; i < length; ++i)
- array->initializeIndex(vm, i, values.at(i));
+ array->initializeIndex(scope, i, values.at(i));
return array;
}
inline JSArray* constructArray(ExecState* exec, Structure* arrayStructure, const JSValue* values, unsigned length)
{
VM& vm = exec->vm();
- JSArray* array = JSArray::tryCreateForInitializationPrivate(vm, arrayStructure, length);
+ ObjectInitializationScope scope(vm);
+ 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
@@ -320,14 +321,15 @@
RELEASE_ASSERT(array);
for (unsigned i = 0; i < length; ++i)
- array->initializeIndex(vm, i, values[i]);
+ array->initializeIndex(scope, i, values[i]);
return array;
}
inline JSArray* constructArrayNegativeIndexed(ExecState* exec, Structure* arrayStructure, const JSValue* values, unsigned length)
{
VM& vm = exec->vm();
- JSArray* array = JSArray::tryCreateForInitializationPrivate(vm, arrayStructure, length);
+ ObjectInitializationScope scope(vm);
+ 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
@@ -336,7 +338,7 @@
RELEASE_ASSERT(array);
for (int i = 0; i < static_cast<int>(length); ++i)
- array->initializeIndex(vm, i, values[-i]);
+ array->initializeIndex(scope, i, values[-i]);
return array;
}
