Skip to content

Commit 6a8c6df

Browse files
tcaretcare
committed
[MERGE chakra-core#717] ES6 Spread over zealous optimization for Arrays ignores user changes to %ArrayIteratorPrototype%.next property
Merge pull request chakra-core#717 from tcare:spreadIterator This is a similar bug that was reported in chakra-core#608 and fixed by @suwc for TypedArray in chakra-core#635. The core problem is that we do not check for changes to %ArrayIteratorPrototype%.next that could have side effects when optimizing spread item copying. Using a function Suwei added, we attempt to get the value without invoking implicit calls, e.g. a getter. In this case we will fall back to calling the iterator for each element. I moved out the ArrayIteratorPrototypeHasUserDefinedNext function from TypedArray to JavascriptLibrary since we now use it in JavascriptOperators.
2 parents 9cb45a9 + 7338c89 commit 6a8c6df

5 files changed

Lines changed: 60 additions & 40 deletions

File tree

‎lib/Runtime/Language/JavascriptOperators.cpp‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1347,12 +1347,11 @@ namespace Js
13471347
{
13481348
RecyclableObject* function = GetIteratorFunction(aRight, scriptContext);
13491349
JavascriptMethod method = function->GetEntryPoint();
1350-
if ((JavascriptArray::Is(aRight) && method == JavascriptArray::EntryInfo::Values.GetOriginalEntryPoint()) ||
1351-
(TypedArrayBase::Is(aRight) && method == TypedArrayBase::EntryInfo::Values.GetOriginalEntryPoint()))
1350+
if (((JavascriptArray::Is(aRight) && method == JavascriptArray::EntryInfo::Values.GetOriginalEntryPoint())
1351+
|| (TypedArrayBase::Is(aRight) && method == TypedArrayBase::EntryInfo::Values.GetOriginalEntryPoint()))
1352+
// We can't optimize away the iterator if the array iterator prototype is user defined.
1353+
&& !JavascriptLibrary::ArrayIteratorPrototypeHasUserDefinedNext(scriptContext))
13521354
{
1353-
// TODO: There is a compliance bug here in the case where the user has changed %ArrayIteratorPrototype%.next(); we won't call it.
1354-
// Checking if the property has been modified is currently not possible without doing a Get on it which might call user code.
1355-
// Fixing this bug will require a way to get the value stored in the property without doing the evaluation semantics of a Get.
13561355
return aRight;
13571356
}
13581357

‎lib/Runtime/Library/JavascriptLibrary.cpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3716,6 +3716,15 @@ namespace Js
37163716
typesEnsuredToHaveOnlyWritableDataPropertiesInItAndPrototypeChain->ClearAndZero();
37173717
}
37183718

3719+
bool JavascriptLibrary::ArrayIteratorPrototypeHasUserDefinedNext(ScriptContext *scriptContext)
3720+
{
3721+
Var arrayIteratorPrototypeNext = nullptr;
3722+
ImplicitCallFlags flags = scriptContext->GetThreadContext()->TryWithDisabledImplicitCall(
3723+
[&]() { arrayIteratorPrototypeNext = JavascriptOperators::GetProperty(scriptContext->GetLibrary()->GetArrayIteratorPrototype(), PropertyIds::next, scriptContext); });
3724+
3725+
return (flags != ImplicitCall_None) || arrayIteratorPrototypeNext != scriptContext->GetLibrary()->GetArrayIteratorPrototypeBuiltinNextFunction();
3726+
}
3727+
37193728
void JavascriptLibrary::InitializeNumberConstructor(DynamicObject* numberConstructor, DeferredTypeHandlerBase * typeHandler, DeferredInitializeMode mode)
37203729
{
37213730
typeHandler->Convert(numberConstructor, mode, 17);

‎lib/Runtime/Library/JavascriptLibrary.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1042,6 +1042,8 @@ namespace Js
10421042
void TypeAndPrototypesAreEnsuredToHaveOnlyWritableDataProperties(Type *const type);
10431043
void NoPrototypeChainsAreEnsuredToHaveOnlyWritableDataProperties();
10441044

1045+
static bool ArrayIteratorPrototypeHasUserDefinedNext(ScriptContext *scriptContext);
1046+
10451047
HRESULT EnsureReadyIfHybridDebugging(bool isScriptEngineReady = true);
10461048

10471049
CharStringCache& GetCharStringCache() { return charStringCache; }

‎lib/Runtime/Library/TypedArray.cpp‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -93,15 +93,6 @@ namespace Js
9393
return newArr;
9494
}
9595

96-
bool TypedArrayBase::ArrayIteratorPrototypeHasUserDefinedNext(ScriptContext *scriptContext)
97-
{
98-
Var arrayIteratorPrototypeNext = nullptr;
99-
ImplicitCallFlags flags = scriptContext->GetThreadContext()->TryWithDisabledImplicitCall(
100-
[&] () { arrayIteratorPrototypeNext = JavascriptOperators::GetProperty(scriptContext->GetLibrary()->GetArrayIteratorPrototype(), PropertyIds::next, scriptContext); });
101-
102-
return (flags != ImplicitCall_None) || arrayIteratorPrototypeNext != scriptContext->GetLibrary()->GetArrayIteratorPrototypeBuiltinNextFunction();
103-
}
104-
10596
Var TypedArrayBase::CreateNewInstance(Arguments& args, ScriptContext* scriptContext, uint32 elementSize, PFNCreateTypedArray pfnCreateTypedArray)
10697
{
10798
uint32 byteLength = 0;
@@ -155,7 +146,7 @@ namespace Js
155146
RecyclableObject* iteratorFn = JavascriptOperators::GetIteratorFunction(firstArgument, scriptContext, true /* optional */);
156147
if (iteratorFn != nullptr &&
157148
(iteratorFn != scriptContext->GetLibrary()->GetArrayPrototypeValuesFunction() ||
158-
!JavascriptArray::Is(firstArgument) || ArrayIteratorPrototypeHasUserDefinedNext(scriptContext) ))
149+
!JavascriptArray::Is(firstArgument) || JavascriptLibrary::ArrayIteratorPrototypeHasUserDefinedNext(scriptContext)))
159150
{
160151
Var iterator = iteratorFn->GetEntryPoint()(iteratorFn, CallInfo(Js::CallFlags_Value, 1), RecyclableObject::FromVar(firstArgument));
161152

‎test/es6/spread.js‎

Lines changed: 44 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -253,37 +253,56 @@ var tests = [
253253
assert.throws(function () { eval('++...window, z;'); }, SyntaxError, "Invalid use of the ... operator");
254254
}
255255
},
256-
/* This tests is broken and tracked by bug OS 5204357
257256
{
258-
name: "Corner case: user changes %ArrayIteratorPrototype%.next property and we should call it",
257+
name: "OS 5204357: Corner case: user changes %ArrayIteratorPrototype%.next property and we should call it",
259258
body: function () {
260259
var overrideCalled = false;
261-
var aip = Object.getPrototypeOf([].values());
262-
var aipNext = aip.next;
263-
aip.next = function () {
264-
overrideCalled = true;
265-
return aipNext.apply(this, arguments);
260+
var arrayIteratorProto = Object.getPrototypeOf([][Symbol.iterator]());
261+
var arrayIteratorProtoNext = arrayIteratorProto.next;
262+
263+
function testArrayIteratorProto() {
264+
var a = [1];
265+
function f() {}
266+
f(...a);
267+
assert.isTrue(overrideCalled, "Spread of a in call to f should have invoked the overridden %ArrayIteratorPrototype%.next method");
268+
overrideCalled = false;
269+
270+
var b = [...a];
271+
assert.isTrue(overrideCalled, "Spread of a in array initializer should have invoked the overridden %ArrayIteratorPrototype%.next method");
272+
overrideCalled = false;
273+
}
274+
275+
var overrideFunc = function () {
276+
overrideCalled = true;
277+
return arrayIteratorProtoNext.apply(this, arguments);
278+
};
279+
280+
arrayIteratorProto.next = overrideFunc;
281+
testArrayIteratorProto();
282+
// Restore the original prototype .next
283+
arrayIteratorProto.next = arrayIteratorProtoNext;
284+
285+
function getIterableObjNextDesc() {
286+
return {
287+
get: function next() {
288+
overrideCalled = true;
289+
return function () {
290+
return {
291+
value: 0,
292+
done: 1
293+
};
294+
};
295+
}
296+
};
266297
}
267298

268-
var a = [1];
269-
270-
function f() { }
271-
272-
f(...a);
273-
274-
assert.isTrue(overrideCalled, "Spread of a in call to f should have invoked the overridden %ArrayIteratorPrototype%.next method");
275-
276-
overrideCalled = false;
277-
278-
var b = [...a];
279-
280-
assert.isTrue(overrideCalled, "Spread of a in array initializer should have invoked the overridden %ArrayIteratorPrototype%.next method");
281-
282-
// restore after test so we don't muck this up for others
283-
aip.next = aipNext;
299+
// Change built-in array iterator's next getter function
300+
var builtinArrayPrototypeIteratorNextDesc = Object.getOwnPropertyDescriptor(arrayIteratorProto, "next");
301+
Object.defineProperty(arrayIteratorProto, "next", getIterableObjNextDesc());
302+
testArrayIteratorProto();
303+
Object.defineProperty(arrayIteratorProto, "next", builtinArrayPrototypeIteratorNextDesc);
284304
}
285305
},
286-
*/
287306
{
288307
name: "Corner case: Spread of an array with an accessor property (ES5Array) should call that getter and recognize a change in length during iteration",
289308
body: function () {
@@ -363,7 +382,7 @@ var tests = [
363382
assert.areEqual(7, result[2], "Spread for the array initializer called the getter of the third element of a and copied the result to the third element of b");
364383
assert.areEqual(8, result[3], "Spread for the array initializer recognized the new length of four and copied the fourth element of a to the fourth element of b");
365384
}
366-
},
385+
}
367386
];
368387

369388
testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

0 commit comments

Comments
 (0)