IntlObject.cpp uses JSArray::tryCreateUninitialized() in three locations in a unsafe way. If the caller of tryCreateUninitialized allocates object when initializing the array contents, it needs to have an active GCDeferralContext. This is responsible for a GC related crash in LayoutTests/js/intl.html, see attached crash dump. It really isn’t appropriate for IntlObject to be using tryCreateUninitialized() as it is intended for performance sensitive Array allocation sites. The appropriate fix is to provide a JSArray::tryCreate() method for code like IntlObject to use. <rdar://problem/30134434>
Created attachment 299458 [details] Patch
Comment on attachment 299458 [details] Patch Attachment 299458 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2929097 New failing tests: sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A2.1_T3.html js/array-splice.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.3_T4.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.2/15.4.2.2_new_Array_len/S15.4.2.2_A2.1_T1.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.1_T3.html
Created attachment 299460 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 299458 [details] Patch Attachment 299458 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2929100 New failing tests: sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A2.1_T3.html js/array-splice.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.3_T4.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.2/15.4.2.2_new_Array_len/S15.4.2.2_A2.1_T1.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.1_T3.html
Created attachment 299461 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 299458 [details] Patch Attachment 299458 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2929078 New failing tests: sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A2.1_T3.html js/array-splice.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.3_T4.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.2/15.4.2.2_new_Array_len/S15.4.2.2_A2.1_T1.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.1_T3.html
Created attachment 299462 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 299458 [details] Patch Attachment 299458 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2929109 New failing tests: sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A2.1_T3.html js/array-splice.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.3_T4.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.2/15.4.2.2_new_Array_len/S15.4.2.2_A2.1_T1.html sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.10_Array_prototype_slice/S15.4.4.10_A1.1_T3.html
Created attachment 299463 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
The bots look quite upset: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000107653747 WTFCrash + 39 (Assertions.cpp:323) 1 com.apple.JavaScriptCore 0x0000000106608af9 JSC::JSArray::create(JSC::VM&, JSC::Structure*, unsigned int) + 89 (JSArray.h:266) 2 com.apple.JavaScriptCore 0x00000001066079ce JSC::constructEmptyArray(JSC::ExecState*, JSC::ArrayAllocationProfile*, JSC::JSGlobalObject*, unsigned int, JSC::JSValue) + 350 (JSGlobalObject.h:870) 3 com.apple.JavaScriptCore 0x0000000106606eac JSC::constructArrayWithSizeQuirk(JSC::ExecState*, JSC::ArrayAllocationProfile*, JSC::JSGlobalObject*, JSC::JSValue, JSC::JSValue) + 508 (ArrayConstructor.cpp:82) 4 com.apple.JavaScriptCore 0x00000001072177c7 llint_slow_path_new_array_with_size + 311 (LLIntSlowPaths.cpp:537)
Comment on attachment 299458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299458&action=review > Source/JavaScriptCore/runtime/JSArray.h:221 > + if (initialLength > MAX_STORAGE_VECTOR_LENGTH) > + return 0; This doesn't belong except on the contiguous path. > Source/JavaScriptCore/runtime/JSArray.h:257 > - ASSERT( > - structure->indexingType() == ArrayWithSlowPutArrayStorage > - || structure->indexingType() == ArrayWithArrayStorage); > - butterfly = createArrayButterfly(vm, 0, initialLength); > - for (unsigned i = 0; i < BASE_ARRAY_STORAGE_VECTOR_LEN; ++i) > - butterfly->arrayStorage()->m_vector[i].clear(); > + unsigned vectorLength = ArrayStorage::optimalVectorLength(0, structure, initialLength); > + void* temp = vm.auxiliarySpace.tryAllocate(nullptr, Butterfly::totalSize(0, outOfLineStorage, true, ArrayStorage::sizeFor(vectorLength))); > + if (!temp) > + return nullptr; > + butterfly = Butterfly::fromBase(temp, 0, outOfLineStorage); > + *butterfly->indexingHeader() = indexingHeaderForArrayStorage(initialLength, vectorLength); > + ArrayStorage* storage = butterfly->arrayStorage(); > + storage->m_indexBias = 0; > + storage->m_sparseMap.clear(); > + storage->m_numValuesInVector = initialLength; > + for (unsigned i = 0; i < vectorLength; ++i) > + storage->m_vector[i].clear(); This is wrong because you're changing the logic too much. The goal was to make this code do an attempted allocation and return null if it failed. But this changes a fundamental behavior of the code. Prior to this change, this code did createArrayButterfly(), which always allocates a fixed-size butterfly. It can do this because ArrayStorage can have array.length be larger than the amount of allocated space. In fact, the caller of this function would have deliberately selected an ArrayStorage indexing type specifically in order to get this effect. Your code changes this to instead allocate an ArrayStorage butterfly that is big enough to hold the entire array.length. There is probably no way to make this behavior change pass tests. You're crashing on a test that does new Array(huge number), which is totally valid in trunk because we go down the ArrayStorage path and only allocate a tiny array. Your change causes us to allocate an initialLength array, but initialLength is failing the MAX_STORAGE_VECTOR_LENGTH test. This means that the size of the butterfly would be bigger than 2^32. We don't want that, since that can't succeed on some platforms, and on other platforms it's a security minefield because I don't know if all of our butterfly size math is safe for that. What you should do here is refactor createArrayButterfly into tryCreateArrayButterfly, but otherwise keep the logic intact.
My vote for this API would be to require a DeferGC& or DeferGCForAWhile& as a required argument to the function. (I'm not really sure what the difference is between DeferGC/DeferGCForAWhile and GCDeferralContext. We could also have a version requiring GCDeferralContext&.) I audited all the places inside JavaScriptCore that use this API (there are probably some places in WebCore I'm guessing since it's exported) and found incorrect uses of it. Some use this API under a deferral context, but fail to pass in the deferral context. Here are the uses I found that I think are incorrect since they use initializeIndex which could do a structure transition for a change in indexing type: ------------------------------------------------------------------ DFGOperations.h operationNewArrayWithSpreadSlow looks wrong to me: ``` ... JSArray* result = JSArray::tryCreateUninitialized(vm, structure, length); RETURN_IF_EXCEPTION(scope, nullptr); unsigned index = 0; for (unsigned i = 0; i < numItems; i++) { JSValue value = JSValue::decode(values[i]); if (JSFixedArray* array = jsDynamicCast<JSFixedArray*>(value)) { // We are spreading. for (unsigned i = 0; i < array->size(); i++) { result->initializeIndex(vm, index, array->get(i)); ++index; } } else { // We are not spreading. result->initializeIndex(vm, index, value); ++index; } } ... ``` FTLOperations.h operationMaterializeObjectInOSR does use the API under a DeferGCForAWhile context. ArrayPrototype.h arrayProtoFuncSplice: ``` ... result = JSArray::tryCreateUninitialized(vm, exec->lexicalGlobalObject()->arrayStructureForIndexingTypeDuringAllocation(ArrayWithUndecided), actualDeleteCount); if (!result) return JSValue::encode(throwOutOfMemoryError(exec, scope)); for (unsigned k = 0; k < actualDeleteCount; ++k) { JSValue v = getProperty(exec, thisObj, k + actualStart); RETURN_IF_EXCEPTION(scope, encodedJSValue()); if (UNLIKELY(!v)) continue; result->initializeIndex(vm, k, v); } ... ``` ArrayPrototype.h arrayProtoPrivateFuncConcatMemcpy uses the API correctly because it doesn't initialize index. It uses memcpy because it guarantees what the result array's indexing type will be by looking at the values that would feed into the result. CommonSlowPaths.cpp slow_path_new_array_with_spread: ``` ... JSArray* result = JSArray::tryCreateUninitialized(vm, structure, arraySize); CHECK_EXCEPTION(); unsigned index = 0; for (int i = 0; i < numItems; i++) { JSValue value = values[-i]; if (bitVector.get(i)) { // We are spreading. JSFixedArray* array = jsCast<JSFixedArray*>(value); for (unsigned i = 0; i < array->size(); i++) { RELEASE_ASSERT(array->get(i)); result->initializeIndex(vm, index, array->get(i)); ++index; } } else { // We are not spreading. result->initializeIndex(vm, index, value); ++index; } } ... ``` JSArray.cpp fastSlice also uses the API correctly because it does a memcpy. JSArray.h constructArray: ``` inline JSArray* constructArray(ExecState* exec, Structure* arrayStructure, const ArgList& values) { VM& vm = exec->vm(); unsigned length = values.size(); JSArray* array = JSArray::tryCreateUninitialized(vm, 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. RELEASE_ASSERT(array); for (unsigned i = 0; i < length; ++i) array->initializeIndex(vm, i, values.at(i)); return array; } ``` JSArray.h constructArrayNegativeIndexed ``` inline JSArray* constructArrayNegativeIndexed(ExecState* exec, Structure* arrayStructure, const JSValue* values, unsigned length) { VM& vm = exec->vm(); JSArray* array = JSArray::tryCreateUninitialized(vm, 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. RELEASE_ASSERT(array); for (int i = 0; i < static_cast<int>(length); ++i) array->initializeIndex(vm, i, values[-i]); return array; } ``` RegExpMatchesArray.cpp and RegExpMatchesArray.h uses it correctly by passing in a GCDeferralContext ------------------------------------------------------------------------------- If we decide to still allow the deferral context to be null, my vote would be to remove the version of the function that doesn't have GCDeferralContext* as a parameter. At the very least, callers should have to think about this issue. My guess would be though, that requiring DeferGC& or DeferGCForAWhile& as a parameter wouldn't actually cause any slow downs.
(In reply to comment #12) > My vote for this API would be to require a DeferGC& or DeferGCForAWhile& as > a required argument to the function. (I'm not really sure what the > difference is between DeferGC/DeferGCForAWhile and GCDeferralContext. We > could also have a version requiring GCDeferralContext&.) I'm opposed to changes that would regress the performance of array allocation. DeferGC is not fast, and neither is DeferGCForAWhile. They increase code size and introduce unnecessary checks. GCDeferralContext is what you use when you need DeferGC, but you want fewer checks. I'm opposed to introducing uses of GCDeferralContext, DeferGC, or DeferGCForAWhile in code that is already using tryCreateUninitialized correctly, because any of those functions would introduce new code. > I audited all the > places inside JavaScriptCore that use this API (there are probably some > places in WebCore I'm guessing since it's exported) and found incorrect uses > of it. Some use this API under a deferral context, but fail to pass in the > deferral context. Here are the uses I found that I think are incorrect since > they use initializeIndex which could do a structure transition for a change > in indexing type: > > ------------------------------------------------------------------ > DFGOperations.h operationNewArrayWithSpreadSlow looks wrong to me: > ``` > ... > JSArray* result = JSArray::tryCreateUninitialized(vm, structure, length); > RETURN_IF_EXCEPTION(scope, nullptr); > > unsigned index = 0; > for (unsigned i = 0; i < numItems; i++) { > JSValue value = JSValue::decode(values[i]); > if (JSFixedArray* array = jsDynamicCast<JSFixedArray*>(value)) { > // We are spreading. > for (unsigned i = 0; i < array->size(); i++) { > result->initializeIndex(vm, index, array->get(i)); > ++index; > } > } else { > // We are not spreading. > result->initializeIndex(vm, index, value); > ++index; > } > } > ... > ``` This code is almost correct, since it does not allocate after allocating the array until after this code runs. It does need a mutatorFence after initializing the elements though. DeferGC wouldn't give you that. > > FTLOperations.h operationMaterializeObjectInOSR does use the API under a > DeferGCForAWhile context. > > > ArrayPrototype.h arrayProtoFuncSplice: > ``` > ... > result = JSArray::tryCreateUninitialized(vm, > exec->lexicalGlobalObject()- > >arrayStructureForIndexingTypeDuringAllocation(ArrayWithUndecided), > actualDeleteCount); > if (!result) > return JSValue::encode(throwOutOfMemoryError(exec, scope)); > > for (unsigned k = 0; k < actualDeleteCount; ++k) { > JSValue v = getProperty(exec, thisObj, k + actualStart); > RETURN_IF_EXCEPTION(scope, encodedJSValue()); > if (UNLIKELY(!v)) > continue; > result->initializeIndex(vm, k, v); > } > ... > ``` This code is correct, since it does not allocate after allocating the array until after this code runs. It does need a mutatorFence after initializing the elements though. DeferGC wouldn't give you that. > > ArrayPrototype.h arrayProtoPrivateFuncConcatMemcpy uses the API correctly > because it doesn't initialize index. It uses memcpy because it guarantees > what the result array's indexing type will be by looking at the values that > would feed into the result. > > > CommonSlowPaths.cpp slow_path_new_array_with_spread: > ``` > ... > JSArray* result = JSArray::tryCreateUninitialized(vm, structure, > arraySize); > CHECK_EXCEPTION(); > > unsigned index = 0; > for (int i = 0; i < numItems; i++) { > JSValue value = values[-i]; > if (bitVector.get(i)) { > // We are spreading. > JSFixedArray* array = jsCast<JSFixedArray*>(value); > for (unsigned i = 0; i < array->size(); i++) { > RELEASE_ASSERT(array->get(i)); > result->initializeIndex(vm, index, array->get(i)); > ++index; > } > } else { > // We are not spreading. > result->initializeIndex(vm, index, value); > ++index; > } > } > ... > ``` This code is correct, since it does not allocate after allocating the array until after this code runs. It does need a mutatorFence after initializing the elements though. DeferGC wouldn't give you that. > > JSArray.cpp fastSlice also uses the API correctly because it does a memcpy. This code is correct, since it does not allocate after allocating the array until after this code runs. It might need a mutatorFence after initializing the elments. > > JSArray.h constructArray: > ``` > inline JSArray* constructArray(ExecState* exec, Structure* arrayStructure, > const ArgList& values) > { > VM& vm = exec->vm(); > unsigned length = values.size(); > JSArray* array = JSArray::tryCreateUninitialized(vm, 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. > RELEASE_ASSERT(array); > > for (unsigned i = 0; i < length; ++i) > array->initializeIndex(vm, i, values.at(i)); > return array; > } > ``` This code is correct, since it does not allocate after allocating the array until after this code runs. It does need a mutatorFence after initializing the elements though. DeferGC wouldn't give you that. > > JSArray.h constructArrayNegativeIndexed > ``` > inline JSArray* constructArrayNegativeIndexed(ExecState* exec, Structure* > arrayStructure, const JSValue* values, unsigned length) > { > VM& vm = exec->vm(); > JSArray* array = JSArray::tryCreateUninitialized(vm, 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. > RELEASE_ASSERT(array); > > for (int i = 0; i < static_cast<int>(length); ++i) > array->initializeIndex(vm, i, values[-i]); > return array; > } > ``` This code is correct, since it does not allocate after allocating the array until after this code runs. It does need a mutatorFence after initializing the elements though. DeferGC wouldn't give you that. > > RegExpMatchesArray.cpp and RegExpMatchesArray.h uses it correctly by passing > in a GCDeferralContext > ----------------------------------------------------------------------------- > -- > > If we decide to still allow the deferral context to be null, my vote would > be to remove the version of the function that doesn't have > GCDeferralContext* as a parameter. At the very least, callers should have to > think about this issue. My guess would be though, that requiring DeferGC& or > DeferGCForAWhile& as a parameter wouldn't actually cause any slow downs. The code you cite is already correct, provided it's got that mutatorFence. Please be careful about introducing DeferGC everywhere. I had performance and correctness nightmares as a result of this: - DeferGC is not necessarily correct, because it introduces a GC point at a different location than before. It's only correct if moving the GC to that point is correct. - DeferGCForAWhile should be used extremely rarely and never for something like a user-triggered array allocation, since that risks not having any GC points. Imagine allocating with DeferGCForAWhile in a loop. In order to understand how to write correct code for our GC, you need to know where your GC points are. The code above is correct because there is no GC point that could possibly observe the partially-initialized array. Introducing DeferGC moves the GC point, and is only correct if the return points in your function are safe for GC. That's not necessarily always true. Introducing DeferGC everywhere is like to create a bug due to this. We know this, because that's exactly what the bug tail has looked like every time that someone had the idea to put DeferGC everywhere. If you are already thinking about GC points, then you should be able to see that the code snippets above are already correct.
(In reply to comment #12) > My vote for this API would be to require a DeferGC& or DeferGCForAWhile& as > a required argument to the function. (I'm not really sure what the > difference is between DeferGC/DeferGCForAWhile and GCDeferralContext. We > could also have a version requiring GCDeferralContext&.) I audited all the > places inside JavaScriptCore that use this API (there are probably some > places in WebCore I'm guessing since it's exported) There should never be WebCore uses of this API since this is a private API. It was only exported because it's called from inline functions. Michael's patch introduces a comment to say that this function is a private internal API. I think it would further elaborate that this is to be used for the VM's internal allocation hot paths and nothing more. In particular, it was wrong for Intl code to use this API. No library code or even runtime code outside of critical allocation paths should use this API. This is also the reason why I'm strongly opposed to introducing DeferGC into this API. We should encourage each other to optimize critical array allocation hot paths by leveraging whatever information we have available to us, and that includes taking advantage of the fact that an object may remain uninitialized from the time it is allocated until just before the next safepoint.
(In reply to comment #13) > (In reply to comment #12) > > My vote for this API would be to require a DeferGC& or DeferGCForAWhile& as > > a required argument to the function. (I'm not really sure what the > > difference is between DeferGC/DeferGCForAWhile and GCDeferralContext. We > > could also have a version requiring GCDeferralContext&.) > > I'm opposed to changes that would regress the performance of array > allocation. DeferGC is not fast, and neither is DeferGCForAWhile. They > increase code size and introduce unnecessary checks. > > GCDeferralContext is what you use when you need DeferGC, but you want fewer > checks. > > I'm opposed to introducing uses of GCDeferralContext, DeferGC, or > DeferGCForAWhile in code that is already using tryCreateUninitialized > correctly, because any of those functions would introduce new code. > > > I audited all the > > places inside JavaScriptCore that use this API (there are probably some > > places in WebCore I'm guessing since it's exported) and found incorrect uses > > of it. Some use this API under a deferral context, but fail to pass in the > > deferral context. Here are the uses I found that I think are incorrect since > > they use initializeIndex which could do a structure transition for a change > > in indexing type: > > > > ------------------------------------------------------------------ > > DFGOperations.h operationNewArrayWithSpreadSlow looks wrong to me: > > ``` > > ... > > JSArray* result = JSArray::tryCreateUninitialized(vm, structure, length); > > RETURN_IF_EXCEPTION(scope, nullptr); > > > > unsigned index = 0; > > for (unsigned i = 0; i < numItems; i++) { > > JSValue value = JSValue::decode(values[i]); > > if (JSFixedArray* array = jsDynamicCast<JSFixedArray*>(value)) { > > // We are spreading. > > for (unsigned i = 0; i < array->size(); i++) { > > result->initializeIndex(vm, index, array->get(i)); > > ++index; > > } > > } else { > > // We are not spreading. > > result->initializeIndex(vm, index, value); > > ++index; > > } > > } > > ... > > ``` > > This code is almost correct, since it does not allocate after allocating the > array until after this code runs. > > It does need a mutatorFence after initializing the elements though. DeferGC > wouldn't give you that. > > > > > FTLOperations.h operationMaterializeObjectInOSR does use the API under a > > DeferGCForAWhile context. > > > > > > ArrayPrototype.h arrayProtoFuncSplice: > > ``` > > ... > > result = JSArray::tryCreateUninitialized(vm, > > exec->lexicalGlobalObject()- > > >arrayStructureForIndexingTypeDuringAllocation(ArrayWithUndecided), > > actualDeleteCount); > > if (!result) > > return JSValue::encode(throwOutOfMemoryError(exec, scope)); > > > > for (unsigned k = 0; k < actualDeleteCount; ++k) { > > JSValue v = getProperty(exec, thisObj, k + actualStart); > > RETURN_IF_EXCEPTION(scope, encodedJSValue()); > > if (UNLIKELY(!v)) > > continue; > > result->initializeIndex(vm, k, v); > > } > > ... > > ``` > > This code is correct, since it does not allocate after allocating the array > until after this code runs. > > It does need a mutatorFence after initializing the elements though. DeferGC > wouldn't give you that. > > > > > ArrayPrototype.h arrayProtoPrivateFuncConcatMemcpy uses the API correctly > > because it doesn't initialize index. It uses memcpy because it guarantees > > what the result array's indexing type will be by looking at the values that > > would feed into the result. > > > > > > CommonSlowPaths.cpp slow_path_new_array_with_spread: > > ``` > > ... > > JSArray* result = JSArray::tryCreateUninitialized(vm, structure, > > arraySize); > > CHECK_EXCEPTION(); > > > > unsigned index = 0; > > for (int i = 0; i < numItems; i++) { > > JSValue value = values[-i]; > > if (bitVector.get(i)) { > > // We are spreading. > > JSFixedArray* array = jsCast<JSFixedArray*>(value); > > for (unsigned i = 0; i < array->size(); i++) { > > RELEASE_ASSERT(array->get(i)); > > result->initializeIndex(vm, index, array->get(i)); > > ++index; > > } > > } else { > > // We are not spreading. > > result->initializeIndex(vm, index, value); > > ++index; > > } > > } > > ... > > ``` > > This code is correct, since it does not allocate after allocating the array > until after this code runs. > > It does need a mutatorFence after initializing the elements though. DeferGC > wouldn't give you that. > > > > > JSArray.cpp fastSlice also uses the API correctly because it does a memcpy. > > This code is correct, since it does not allocate after allocating the array > until after this code runs. > > It might need a mutatorFence after initializing the elments. > > > > > JSArray.h constructArray: > > ``` > > inline JSArray* constructArray(ExecState* exec, Structure* arrayStructure, > > const ArgList& values) > > { > > VM& vm = exec->vm(); > > unsigned length = values.size(); > > JSArray* array = JSArray::tryCreateUninitialized(vm, 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. > > RELEASE_ASSERT(array); > > > > for (unsigned i = 0; i < length; ++i) > > array->initializeIndex(vm, i, values.at(i)); > > return array; > > } > > ``` > > This code is correct, since it does not allocate after allocating the array > until after this code runs. > > It does need a mutatorFence after initializing the elements though. DeferGC > wouldn't give you that. > > > > > JSArray.h constructArrayNegativeIndexed > > ``` > > inline JSArray* constructArrayNegativeIndexed(ExecState* exec, Structure* > > arrayStructure, const JSValue* values, unsigned length) > > { > > VM& vm = exec->vm(); > > JSArray* array = JSArray::tryCreateUninitialized(vm, 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. > > RELEASE_ASSERT(array); > > > > for (int i = 0; i < static_cast<int>(length); ++i) > > array->initializeIndex(vm, i, values[-i]); > > return array; > > } > > ``` > > This code is correct, since it does not allocate after allocating the array > until after this code runs. > > It does need a mutatorFence after initializing the elements though. DeferGC > wouldn't give you that. > > > > > RegExpMatchesArray.cpp and RegExpMatchesArray.h uses it correctly by passing > > in a GCDeferralContext > > ----------------------------------------------------------------------------- > > -- > > > > If we decide to still allow the deferral context to be null, my vote would > > be to remove the version of the function that doesn't have > > GCDeferralContext* as a parameter. At the very least, callers should have to > > think about this issue. My guess would be though, that requiring DeferGC& or > > DeferGCForAWhile& as a parameter wouldn't actually cause any slow downs. > > The code you cite is already correct, provided it's got that mutatorFence. My argument is that initializeIndex is a GC point because can allocate Structures for indexing type transitions. So all the code I cited that uses initializeIndex is wrong if it doesn't have a deferral context. I'm ok not requiring a DeferGC(ForAWhile), but we should really remove the function variant that doesn't require the caller to think about the GCDeferralContext. > > Please be careful about introducing DeferGC everywhere. I had performance > and correctness nightmares as a result of this: > > - DeferGC is not necessarily correct, because it introduces a GC point at a > different location than before. It's only correct if moving the GC to that > point is correct. > > - DeferGCForAWhile should be used extremely rarely and never for something > like a user-triggered array allocation, since that risks not having any GC > points. Imagine allocating with DeferGCForAWhile in a loop. > > In order to understand how to write correct code for our GC, you need to > know where your GC points are. The code above is correct because there is no > GC point that could possibly observe the partially-initialized array. > Introducing DeferGC moves the GC point, and is only correct if the return > points in your function are safe for GC. That's not necessarily always true. > Introducing DeferGC everywhere is like to create a bug due to this. We know > this, because that's exactly what the bug tail has looked like every time > that someone had the idea to put DeferGC everywhere. > > If you are already thinking about GC points, then you should be able to see > that the code snippets above are already correct.
(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #12) > > The code you cite is already correct, provided it's got that mutatorFence. > My argument is that initializeIndex is a GC point because can allocate > Structures for indexing type transitions. So all the code I cited that uses > initializeIndex is wrong if it doesn't have a deferral context. > I'm ok not requiring a DeferGC(ForAWhile), but we should really remove the > function variant that doesn't require the caller to think about the > GCDeferralContext. They can't allocate structures because they are allocating an array with an "original" structure, and all of the possible original array structures should have already been allocated by the JSGlobalObject. We could introduce a DisallowGC into initializeIndex to catch this in debug. Note that RegExpMatchesArray *could* have had this bug since it's not allocating with an original array structure, but it saves itself by cooking its structure itself and then also using GCDeferralContext. Also, a note about the safety of initialization: it's safe to be uninitialized until the next GC point or the next escape. As soon as you escape the object into something the GC might find out about without a safepoint (like an object), it had better be initialized and the initialization should have been flushed with a mutatorFence.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > > (In reply to comment #12) > > > The code you cite is already correct, provided it's got that mutatorFence. > > My argument is that initializeIndex is a GC point because can allocate > > Structures for indexing type transitions. So all the code I cited that uses > > initializeIndex is wrong if it doesn't have a deferral context. > > I'm ok not requiring a DeferGC(ForAWhile), but we should really remove the > > function variant that doesn't require the caller to think about the > > GCDeferralContext. > > They can't allocate structures because they are allocating an array with an > "original" structure, and all of the possible original array structures > should have already been allocated by the JSGlobalObject. Ah, right. This makes sense. I hadn't read the implementation of Structure::nonPropertyTransition. Since this is one of the main ways this API used, and the reason it's correct is really subtle, maybe the comment should mention why this is correct. I still think it'd be nice to do two things: 1. No longer mark the function as exported. 2. Make the GCDeferralContext required. > > We could introduce a DisallowGC into initializeIndex to catch this in debug. > > Note that RegExpMatchesArray *could* have had this bug since it's not > allocating with an original array structure, but it saves itself by cooking > its structure itself and then also using GCDeferralContext. > > Also, a note about the safety of initialization: it's safe to be > uninitialized until the next GC point or the next escape. As soon as you > escape the object into something the GC might find out about without a > safepoint (like an object), it had better be initialized and the > initialization should have been flushed with a mutatorFence.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > (In reply to comment #13) > > > > (In reply to comment #12) > > > > The code you cite is already correct, provided it's got that mutatorFence. > > > My argument is that initializeIndex is a GC point because can allocate > > > Structures for indexing type transitions. So all the code I cited that uses > > > initializeIndex is wrong if it doesn't have a deferral context. > > > I'm ok not requiring a DeferGC(ForAWhile), but we should really remove the > > > function variant that doesn't require the caller to think about the > > > GCDeferralContext. > > > > They can't allocate structures because they are allocating an array with an > > "original" structure, and all of the possible original array structures > > should have already been allocated by the JSGlobalObject. > Ah, right. This makes sense. I hadn't read the implementation of > Structure::nonPropertyTransition. Since this is one of the main ways this > API used, and the reason it's correct is really subtle, maybe the comment > should mention why this is correct. > > I still think it'd be nice to do two things: > 1. No longer mark the function as exported. Whether or not it is exported should depend entirely on whether this is necessary for linking since inline functions could call it. > 2. Make the GCDeferralContext required. This is definitely a code size regression and possibly a performance regression. It's also not necessary, so it's just pointless code that people have to maintain. It's not even testable! There is no test that would fail if you removed it. Worse it makes it hard to reason about what the JIT does. Back when code like this used DeferGC it would be a pain to understand why the JIT does not emit anything like that. Finally, GCDeferralcontext doesn't do anything for code that it's not threaded through. You'd still have to trust that initializeIndex and everything else you're loop does doesn't GC. And if you close the GCDeferralContezt scope before finishing initializatipj then you've definitely introduced a bug and the solution would be to simply remove the GCDeferralContext. Some of the places where I removed DeferGC were like that. So, it's not like requiring the context is an assurance of anything. Sometimes it's right. Sometimes it's wrong! The bug in Intl is that it shouldn't have been using tryCreateUninitialized. All other uses are already correct because they have always followed the "initialize it before safepoint or escape" rule. > > > > > We could introduce a DisallowGC into initializeIndex to catch this in debug. > > > > Note that RegExpMatchesArray *could* have had this bug since it's not > > allocating with an original array structure, but it saves itself by cooking > > its structure itself and then also using GCDeferralContext. > > > > Also, a note about the safety of initialization: it's safe to be > > uninitialized until the next GC point or the next escape. As soon as you > > escape the object into something the GC might find out about without a > > safepoint (like an object), it had better be initialized and the > > initialization should have been flushed with a mutatorFence.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > (In reply to comment #15) > > > > (In reply to comment #13) > > > > > (In reply to comment #12) > > > > > The code you cite is already correct, provided it's got that mutatorFence. > > > > My argument is that initializeIndex is a GC point because can allocate > > > > Structures for indexing type transitions. So all the code I cited that uses > > > > initializeIndex is wrong if it doesn't have a deferral context. > > > > I'm ok not requiring a DeferGC(ForAWhile), but we should really remove the > > > > function variant that doesn't require the caller to think about the > > > > GCDeferralContext. > > > > > > They can't allocate structures because they are allocating an array with an > > > "original" structure, and all of the possible original array structures > > > should have already been allocated by the JSGlobalObject. > > Ah, right. This makes sense. I hadn't read the implementation of > > Structure::nonPropertyTransition. Since this is one of the main ways this > > API used, and the reason it's correct is really subtle, maybe the comment > > should mention why this is correct. > > > > I still think it'd be nice to do two things: > > 1. No longer mark the function as exported. > > Whether or not it is exported should depend entirely on whether this is > necessary for linking since inline functions could call it. Indeed. I wasn't thinking about that. It is called from inlined functions. I'm not sure if they're used from WebCore/JSC.cpp or not. They probably are. > > > 2. Make the GCDeferralContext required. > > This is definitely a code size regression and possibly a performance > regression. It's also not necessary, so it's just pointless code that people > have to maintain. It's not even testable! There is no test that would fail > if you removed it. I don't mean actually having to use a context. I meant having to pass something in for GCDeferralContext* should be required. That said, this probably doesn't buy much. Basically, I'm trying to make this API hard to get wrong. Maybe that's not easily possible. > > Worse it makes it hard to reason about what the JIT does. Back when code > like this used DeferGC it would be a pain to understand why the JIT does not > emit anything like that. > > Finally, GCDeferralcontext doesn't do anything for code that it's not > threaded through. You'd still have to trust that initializeIndex and > everything else you're loop does doesn't GC. And if you close the > GCDeferralContezt scope before finishing initializatipj then you've > definitely introduced a bug and the solution would be to simply remove the > GCDeferralContext. Some of the places where I removed DeferGC were like > that. So, it's not like requiring the context is an assurance of anything. > Sometimes it's right. Sometimes it's wrong! > > The bug in Intl is that it shouldn't have been using tryCreateUninitialized. > All other uses are already correct because they have always followed the > "initialize it before safepoint or escape" rule. > > > > > > > > > > > We could introduce a DisallowGC into initializeIndex to catch this in debug. > > > > > > Note that RegExpMatchesArray *could* have had this bug since it's not > > > allocating with an original array structure, but it saves itself by cooking > > > its structure itself and then also using GCDeferralContext. > > > > > > Also, a note about the safety of initialization: it's safe to be > > > uninitialized until the next GC point or the next escape. As soon as you > > > escape the object into something the GC might find out about without a > > > safepoint (like an object), it had better be initialized and the > > > initialization should have been flushed with a mutatorFence.
I think I need to provide some more context. We use tryCreateUnitialized only in: - Code paths that we optimize like crazy. We routinely do very shady things in these paths to get perf, and this should be encouraged. In particular, removing an unnecessary safety mitigation from a hot path is something we usually do without even perf testing it - we just do it because we know in the long run removing code adds up to perf. - Code paths that are internal to JSArray/JSObject. All of these paths are part of a tight circle of close friends of JSObject. Intl calling into this code was always wrong. I think it's very important for this patch to clearly spell out the internal nature of tryCreateUninitialized, and we should reject patches that introduce new calls to this function unless there is some truly spectacular reason to call it. To me, this function is the GC equivalent of inline assembly! We should not make it a tradition in JSC to take existing, known-correct, known-to-be-hot pieces of code and introduce safety mitigations that are not known to fix any bug. In this case, the mitigations also risk introducing new bugs, because this is always wrong: JSArray* array; { GCDeferralContext ctx; array = tryCreateUninitialized(ctx, ...); } // This is wrong because you have just introduced a GC point here. The GC will see the array, but it won't be initialized. array->initializeIndex(); Intriguingly, if initializeIndex() does not GC, then all you need to do to make this code safe is to *remove* GCDeferralContext: JSArray* array = tryCreateUninitialized(...); array->initializeIndex(); Even if you don't make that mistake, the GCDeferralContext mitigation is still not sure to prevent bugs, since this is wrong: GCDeferralContext ctx; array = tryCreateUninitialized(ctx, ...); array->initializeIndex(..., JSFoo::create(vm, thingy)); // Wrong because you didn't thread ctx through JSFoo::create(). GCDeferralContext was never meant to be a safety mitigation. It's an optimized version of DeferGC for when there's no way to make the code correct and fast without some deferral. So, using GCDeferralContext when all you want is safety is wrong. It isn't a good safety mitigation since there is so much that can go wrong. I agree that introducing mitigations makes sense for code that is either new, not known to be correct, or not known to be hot, and those mitigations stand a good chance of catching or preventing a bug. That's not the case here - the code is hot, old, correct, and GCDeferralContext is an optimization, not a safety mitigation. That brings us to DeferGC, which is not meant to be used on hot paths because it adds to memory in DeferGC::DeferGC and subtracts from memory and branches in ~DeferGC. That's more overhead than we want in most hot paths. Also, DeferGC is not a safety mitigation because it's not guaranteed to make the code correct. DeferGC can easily introduce new bugs. The last time we did the drill of introducing DeferGC everywhere it led to bugs because of complex versions of the ~DeferGC-runs-too-soon bug. Like any crappy GC bug, we didn't find put about those bugs until some time after the changes landed. Because DeferGC is super likely to introduce bugs based on our own experiences with it, we should only use it or mandate it if we know for sure that it fixes a bug. For example, it makes sense for ConcurrentJSLocker to have a version that requires it because there is no way to correctly grab such a lock and allocate without first deferring GC. But there are other places that are only correct if you remove DeferGC. One example of where DeferGC is wrong is when you scope it around the allocation but not the initialization. Another example of DeferGC being wrong is when you DeferGC and do a lot of allocation. The GC cannot run during those allocations, this is sure to increase memory footprint. It could increase memory footprint unboundedly. It's also wrong to DeferGC and then run JS, since then you could have an unbounded number of allocations without GC. It's wrong to DeferGC and call anything that could wait on GC, since waiting on GC requires safepoints. Anyway, the point is: I get that you viewed DeferGC and GCDeferralContext as safety mitigations. Maybe I viewed them this way at some point also. But that's just not the case - these are super dangerous features that should be used with great care.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (In reply to comment #16) > > > > (In reply to comment #15) > > > > > (In reply to comment #13) > > > > > > (In reply to comment #12) > > > > > > The code you cite is already correct, provided it's got that mutatorFence. > > > > > My argument is that initializeIndex is a GC point because can allocate > > > > > Structures for indexing type transitions. So all the code I cited that uses > > > > > initializeIndex is wrong if it doesn't have a deferral context. > > > > > I'm ok not requiring a DeferGC(ForAWhile), but we should really remove the > > > > > function variant that doesn't require the caller to think about the > > > > > GCDeferralContext. > > > > > > > > They can't allocate structures because they are allocating an array with an > > > > "original" structure, and all of the possible original array structures > > > > should have already been allocated by the JSGlobalObject. > > > Ah, right. This makes sense. I hadn't read the implementation of > > > Structure::nonPropertyTransition. Since this is one of the main ways this > > > API used, and the reason it's correct is really subtle, maybe the comment > > > should mention why this is correct. > > > > > > I still think it'd be nice to do two things: > > > 1. No longer mark the function as exported. > > > > Whether or not it is exported should depend entirely on whether this is > > necessary for linking since inline functions could call it. > Indeed. I wasn't thinking about that. It is called from inlined functions. > I'm not sure if they're used from WebCore/JSC.cpp or not. They probably are. > > > > > > 2. Make the GCDeferralContext required. > > > > This is definitely a code size regression and possibly a performance > > regression. It's also not necessary, so it's just pointless code that people > > have to maintain. It's not even testable! There is no test that would fail > > if you removed it. > I don't mean actually having to use a context. I meant having to pass > something in for GCDeferralContext* should be required. That said, this > probably doesn't buy much. Basically, I'm trying to make this API hard to > get wrong. Maybe that's not easily possible. Here's a really glib way of summarizing my feelings: you have no business using tryCreateUninitialized if you need a nanny. Also, any new use of tryCreateUninitialized should be treated with extreme suspicion, since to me this is logically a private function of JSArray. I hate that 'friend' in C++ is so annoying to use, otherwise we would probably make all users of tryCreateUninitialized friends of JSArray.
(In reply to comment #21) > (In reply to comment #19) > > (In reply to comment #18) > > > (In reply to comment #17) > > > > (In reply to comment #16) > > > > > (In reply to comment #15) > > > > > > (In reply to comment #13) > > > > > > > (In reply to comment #12) > > > > > > > The code you cite is already correct, provided it's got that mutatorFence. > > > > > > My argument is that initializeIndex is a GC point because can allocate > > > > > > Structures for indexing type transitions. So all the code I cited that uses > > > > > > initializeIndex is wrong if it doesn't have a deferral context. > > > > > > I'm ok not requiring a DeferGC(ForAWhile), but we should really remove the > > > > > > function variant that doesn't require the caller to think about the > > > > > > GCDeferralContext. > > > > > > > > > > They can't allocate structures because they are allocating an array with an > > > > > "original" structure, and all of the possible original array structures > > > > > should have already been allocated by the JSGlobalObject. > > > > Ah, right. This makes sense. I hadn't read the implementation of > > > > Structure::nonPropertyTransition. Since this is one of the main ways this > > > > API used, and the reason it's correct is really subtle, maybe the comment > > > > should mention why this is correct. > > > > > > > > I still think it'd be nice to do two things: > > > > 1. No longer mark the function as exported. > > > > > > Whether or not it is exported should depend entirely on whether this is > > > necessary for linking since inline functions could call it. > > Indeed. I wasn't thinking about that. It is called from inlined functions. > > I'm not sure if they're used from WebCore/JSC.cpp or not. They probably are. > > > > > > > > > 2. Make the GCDeferralContext required. > > > > > > This is definitely a code size regression and possibly a performance > > > regression. It's also not necessary, so it's just pointless code that people > > > have to maintain. It's not even testable! There is no test that would fail > > > if you removed it. > > I don't mean actually having to use a context. I meant having to pass > > something in for GCDeferralContext* should be required. That said, this > > probably doesn't buy much. Basically, I'm trying to make this API hard to > > get wrong. Maybe that's not easily possible. > > Here's a really glib way of summarizing my feelings: you have no business > using tryCreateUninitialized if you need a nanny. > > Also, any new use of tryCreateUninitialized should be treated with extreme > suspicion, since to me this is logically a private function of JSArray. I > hate that 'friend' in C++ is so annoying to use, otherwise we would probably > make all users of tryCreateUninitialized friends of JSArray. Yeah I was going to suggest that, but then came to the same conclusion that using 'friend' would just be a pain
Created attachment 299517 [details] Updated patch with suggested changes
Attachment 299517 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/Butterfly.h:114: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/runtime/Butterfly.h:114: The parameter name "indexingHeader" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 299517 [details] Updated patch with suggested changes View in context: https://bugs.webkit.org/attachment.cgi?id=299517&action=review > Source/JavaScriptCore/runtime/JSArray.h:250 > ASSERT( > - hasUndecided(structure->indexingType()) > - || hasInt32(structure->indexingType()) > - || hasDouble(structure->indexingType()) > - || hasContiguous(structure->indexingType())); > - unsigned vectorLength; > - butterfly = createContiguousArrayButterfly(vm, 0, initialLength, vectorLength); > - if (hasDouble(structure->indexingType())) > + hasUndecided(indexingType) > + || hasInt32(indexingType) > + || hasDouble(indexingType) > + || hasContiguous(indexingType)); > + > + if (initialLength > MAX_STORAGE_VECTOR_LENGTH) > + return 0; > + > + unsigned vectorLength = Butterfly::optimalContiguousVectorLength(structure, initialLength); > + void* temp = vm.auxiliarySpace.tryAllocate(nullptr, Butterfly::totalSize(0, outOfLineStorage, true, vectorLength * sizeof(EncodedJSValue))); > + if (!temp) > + return nullptr; > + butterfly = Butterfly::fromBase(temp, 0, outOfLineStorage); > + butterfly->setVectorLength(vectorLength); > + butterfly->setPublicLength(initialLength); > + if (hasDouble(indexingType)) Does this remove the last use of createContiguousArrayButterfly? If so, you should probably delete it. If not, you should probably find a way to share code with it.
(In reply to comment #25) > Comment on attachment 299517 [details] > Updated patch with suggested changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299517&action=review > > > Source/JavaScriptCore/runtime/JSArray.h:250 > > ASSERT( > > - hasUndecided(structure->indexingType()) > > - || hasInt32(structure->indexingType()) > > - || hasDouble(structure->indexingType()) > > - || hasContiguous(structure->indexingType())); > > - unsigned vectorLength; > > - butterfly = createContiguousArrayButterfly(vm, 0, initialLength, vectorLength); > > - if (hasDouble(structure->indexingType())) > > + hasUndecided(indexingType) > > + || hasInt32(indexingType) > > + || hasDouble(indexingType) > > + || hasContiguous(indexingType)); > > + > > + if (initialLength > MAX_STORAGE_VECTOR_LENGTH) > > + return 0; > > + > > + unsigned vectorLength = Butterfly::optimalContiguousVectorLength(structure, initialLength); > > + void* temp = vm.auxiliarySpace.tryAllocate(nullptr, Butterfly::totalSize(0, outOfLineStorage, true, vectorLength * sizeof(EncodedJSValue))); > > + if (!temp) > > + return nullptr; > > + butterfly = Butterfly::fromBase(temp, 0, outOfLineStorage); > > + butterfly->setVectorLength(vectorLength); > > + butterfly->setPublicLength(initialLength); > > + if (hasDouble(indexingType)) > > Does this remove the last use of createContiguousArrayButterfly? > > If so, you should probably delete it. > > If not, you should probably find a way to share code with it. It is no longer used. I deleted it locally.
Committed r211043: <http://trac.webkit.org/changeset/211043>