Title: [239325] trunk
- Revision
- 239325
- Author
- mark....@apple.com
- Date
- 2018-12-17 22:56:51 -0800 (Mon, 17 Dec 2018)
Log Message
Array unshift/shift should not race against the AI in the compiler thread.
https://bugs.webkit.org/show_bug.cgi?id=192795
<rdar://problem/46724263>
Reviewed by Saam Barati.
JSTests:
* stress/array-unshift-should-not-race-against-compiler-thread.js: Added.
Source/_javascript_Core:
The Array unshift and shift operations for ArrayStorage type arrays are protected
using the cellLock. The AbstractInterpreter's foldGetByValOnConstantProperty()
function does grab the cellLock before reading a value from the array's ArrayStorage,
but does not get the array butterfly under the protection of the cellLock.
This is insufficient and racy. For ArrayStorage type arrays, the fetching of the
butterfly also needs to be protected by the cellLock. The unshift / shift
operations can move values around in the butterfly. Hence, the fact that AI has
fetched a butterfly pointer (while ensuring no structure change) is insufficient
to guarantee that the values in the butterfly haven't shifted.
Having AI hold the cellLock the whole time (from before fetching the butterfly
till after reading the value from it) eliminates this race. Note: we only need
to do this for ArrayStorage type arrays.
Note also that though AI is holding the cellLock in this case, we still need to
ensure that the array structure hasn't changed around the fetching of the butterfly.
This is because operations other than unshift and shift are guarded by this
protocol, and not the cellLock.
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* runtime/JSArray.cpp:
(JSC::JSArray::unshiftCountSlowCase):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (239324 => 239325)
--- trunk/JSTests/ChangeLog 2018-12-18 06:54:49 UTC (rev 239324)
+++ trunk/JSTests/ChangeLog 2018-12-18 06:56:51 UTC (rev 239325)
@@ -1,3 +1,13 @@
+2018-12-17 Mark Lam <mark....@apple.com>
+
+ Array unshift/shift should not race against the AI in the compiler thread.
+ https://bugs.webkit.org/show_bug.cgi?id=192795
+ <rdar://problem/46724263>
+
+ Reviewed by Saam Barati.
+
+ * stress/array-unshift-should-not-race-against-compiler-thread.js: Added.
+
2018-12-16 Yusuke Suzuki <yusukesuz...@slowstart.org>
[JSC] Optimize Object.keys by caching own keys results in StructureRareData
Added: trunk/JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js (0 => 239325)
--- trunk/JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js (rev 0)
+++ trunk/JSTests/stress/array-unshift-should-not-race-against-compiler-thread.js 2018-12-18 06:56:51 UTC (rev 239325)
@@ -0,0 +1,7 @@
+let x = [];
+for (let i = 0; i < 30; ++i) {
+ for (let j = 0; j < 20000; ++j) {
+ x[0]
+ x.unshift(undefined);
+ }
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (239324 => 239325)
--- trunk/Source/_javascript_Core/ChangeLog 2018-12-18 06:54:49 UTC (rev 239324)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-12-18 06:56:51 UTC (rev 239325)
@@ -1,3 +1,36 @@
+2018-12-17 Mark Lam <mark....@apple.com>
+
+ Array unshift/shift should not race against the AI in the compiler thread.
+ https://bugs.webkit.org/show_bug.cgi?id=192795
+ <rdar://problem/46724263>
+
+ Reviewed by Saam Barati.
+
+ The Array unshift and shift operations for ArrayStorage type arrays are protected
+ using the cellLock. The AbstractInterpreter's foldGetByValOnConstantProperty()
+ function does grab the cellLock before reading a value from the array's ArrayStorage,
+ but does not get the array butterfly under the protection of the cellLock.
+
+ This is insufficient and racy. For ArrayStorage type arrays, the fetching of the
+ butterfly also needs to be protected by the cellLock. The unshift / shift
+ operations can move values around in the butterfly. Hence, the fact that AI has
+ fetched a butterfly pointer (while ensuring no structure change) is insufficient
+ to guarantee that the values in the butterfly haven't shifted.
+
+ Having AI hold the cellLock the whole time (from before fetching the butterfly
+ till after reading the value from it) eliminates this race. Note: we only need
+ to do this for ArrayStorage type arrays.
+
+ Note also that though AI is holding the cellLock in this case, we still need to
+ ensure that the array structure hasn't changed around the fetching of the butterfly.
+ This is because operations other than unshift and shift are guarded by this
+ protocol, and not the cellLock.
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * runtime/JSArray.cpp:
+ (JSC::JSArray::unshiftCountSlowCase):
+
2018-12-16 Yusuke Suzuki <yusukesuz...@slowstart.org>
[JSC] Optimize Object.keys by caching own keys results in StructureRareData
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (239324 => 239325)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2018-12-18 06:54:49 UTC (rev 239324)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2018-12-18 06:56:51 UTC (rev 239325)
@@ -1901,17 +1901,19 @@
if (isNuked(structureIDEarly))
return false;
- WTF::loadLoadFence();
- Butterfly* butterfly = array->butterfly();
+ if (node->arrayMode().arrayClass() == Array::OriginalCopyOnWriteArray) {
- WTF::loadLoadFence();
- StructureID structureIDLate = array->structureID();
+ WTF::loadLoadFence();
+ Butterfly* butterfly = array->butterfly();
- if (structureIDEarly != structureIDLate)
- return false;
+ WTF::loadLoadFence();
+ StructureID structureIDLate = array->structureID();
- Structure* structure = m_vm.getStructure(structureIDLate);
- if (node->arrayMode().arrayClass() == Array::OriginalCopyOnWriteArray) {
+ if (structureIDEarly != structureIDLate)
+ return false;
+
+ Structure* structure = m_vm.getStructure(structureIDLate);
+
if (!isCopyOnWrite(structure->indexingMode()))
return false;
@@ -1944,17 +1946,27 @@
}
if (node->arrayMode().type() == Array::ArrayStorage || node->arrayMode().type() == Array::SlowPutArrayStorage) {
- if (!hasAnyArrayStorage(structure->indexingMode()))
- return false;
-
- if (structure->typeInfo().interceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero())
- return false;
-
JSValue value;
{
// ArrayStorage's Butterfly can be half-broken state.
auto locker = holdLock(array->cellLock());
+ WTF::loadLoadFence();
+ Butterfly* butterfly = array->butterfly();
+
+ WTF::loadLoadFence();
+ StructureID structureIDLate = array->structureID();
+
+ if (structureIDEarly != structureIDLate)
+ return false;
+
+ Structure* structure = m_vm.getStructure(structureIDLate);
+ if (!hasAnyArrayStorage(structure->indexingMode()))
+ return false;
+
+ if (structure->typeInfo().interceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero())
+ return false;
+
ArrayStorage* storage = butterfly->arrayStorage();
if (index >= storage->length())
return false;
Modified: trunk/Source/_javascript_Core/runtime/JSArray.cpp (239324 => 239325)
--- trunk/Source/_javascript_Core/runtime/JSArray.cpp 2018-12-18 06:54:49 UTC (rev 239324)
+++ trunk/Source/_javascript_Core/runtime/JSArray.cpp 2018-12-18 06:56:51 UTC (rev 239325)
@@ -334,6 +334,8 @@
// This method makes room in the vector, but leaves the new space for count slots uncleared.
bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool addToFront, unsigned count)
{
+ ASSERT(cellLock().isLocked());
+
ArrayStorage* storage = ensureArrayStorage(vm);
Butterfly* butterfly = storage->butterfly();
Structure* structure = this->structure(vm);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
