Skip to content

Commit c1623a6

Browse files
obastemurobastemur
committed
[MERGE chakra-core#2337 @obastemur] bug-fix: 9575461 - IsConcatSpreadable is called twice
Merge pull request chakra-core#2337 from obastemur:9575461
2 parents b15a61a + 9443bab commit c1623a6

File tree

4 files changed

+84
-25
lines changed

4 files changed

+84
-25
lines changed

‎lib/Runtime/Library/JavascriptArray.cpp

Lines changed: 48 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2997,7 +2997,9 @@ namespace Js
29972997
}
29982998

29992999
template<typename T>
3000-
void JavascriptArray::ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start, BigIndex startIdxDest, BOOL FirstPromotedItemIsSpreadable, BigIndex FirstPromotedItemLength)
3000+
void JavascriptArray::ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds,
3001+
Js::Arguments& args, ScriptContext* scriptContext, uint start, BigIndex startIdxDest,
3002+
BOOL FirstPromotedItemIsSpreadable, BigIndex FirstPromotedItemLength, bool spreadableCheckedAndTrue)
30013003
{
30023004
// This never gets called.
30033005
Throw::InternalError();
@@ -3006,7 +3008,9 @@ namespace Js
30063008
// Helper for EntryConcat. Concat args or elements of arg arrays into dest array.
30073009
//
30083010
template<typename T>
3009-
void JavascriptArray::ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start, uint startIdxDest, BOOL firstPromotedItemIsSpreadable, BigIndex firstPromotedItemLength)
3011+
void JavascriptArray::ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds,
3012+
Js::Arguments& args, ScriptContext* scriptContext, uint start, uint startIdxDest,
3013+
BOOL firstPromotedItemIsSpreadable, BigIndex firstPromotedItemLength, bool spreadableCheckedAndTrue)
30103014
{
30113015
JavascriptArray* pDestArray = nullptr;
30123016

@@ -3019,8 +3023,8 @@ namespace Js
30193023
for (uint idxArg = start; idxArg < args.Info.Count; idxArg++)
30203024
{
30213025
Var aItem = args[idxArg];
3022-
BOOL spreadable = false;
3023-
if (scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled())
3026+
bool spreadable = spreadableCheckedAndTrue;
3027+
if (!spreadable && scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled())
30243028
{
30253029
// firstPromotedItemIsSpreadable is ONLY used to resume after a type promotion from uint32 to uint64
30263030
// we do this because calls to IsConcatSpreadable are observable (a big deal for proxies) and we don't
@@ -3034,6 +3038,10 @@ namespace Js
30343038
continue;
30353039
}
30363040
}
3041+
else
3042+
{
3043+
spreadableCheckedAndTrue = false; // if it was `true`, reset after the first use
3044+
}
30373045

30383046
if (pDestArray && JavascriptArray::IsDirectAccessArray(aItem) && JavascriptArray::IsDirectAccessArray(pDestArray)
30393047
&& BigIndex(idxDest + JavascriptArray::FromVar(aItem)->length).IsSmallIndex()) // Fast path
@@ -3151,6 +3159,7 @@ namespace Js
31513159
pDestArray->SetLength(ConvertToIndex<T, uint32>(idxDest, scriptContext));
31523160
}
31533161
}
3162+
31543163
bool JavascriptArray::PromoteToBigIndex(BigIndex lhs, BigIndex rhs)
31553164
{
31563165
return false; // already a big index
@@ -3173,17 +3182,25 @@ namespace Js
31733182
for (uint idxArg = 0; idxArg < args.Info.Count; idxArg++)
31743183
{
31753184
Var aItem = args[idxArg];
3185+
bool spreadableCheckedAndTrue = false;
31763186

3177-
if (scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled() && !JavascriptOperators::IsConcatSpreadable(aItem))
3187+
if (scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled())
31783188
{
3179-
pDestArray->SetItem(idxDest, aItem, PropertyOperation_ThrowIfNotExtensible);
3180-
idxDest = idxDest + 1;
3181-
if (!JavascriptNativeIntArray::Is(pDestArray)) // SetItem could convert pDestArray to a var array if aItem is not an integer if so fall back
3189+
if (JavascriptOperators::IsConcatSpreadable(aItem))
31823190
{
3183-
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
3184-
return pDestArray;
3191+
spreadableCheckedAndTrue = true;
3192+
}
3193+
else
3194+
{
3195+
pDestArray->SetItem(idxDest, aItem, PropertyOperation_ThrowIfNotExtensible);
3196+
idxDest = idxDest + 1;
3197+
if (!JavascriptNativeIntArray::Is(pDestArray)) // SetItem could convert pDestArray to a var array if aItem is not an integer if so fall back
3198+
{
3199+
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
3200+
return pDestArray;
3201+
}
3202+
continue;
31853203
}
3186-
continue;
31873204
}
31883205

31893206
if (JavascriptNativeIntArray::Is(aItem)) // Fast path
@@ -3220,7 +3237,7 @@ namespace Js
32203237
else
32213238
{
32223239
JavascriptArray *pVarDestArray = JavascriptNativeIntArray::ConvertToVarArray(pDestArray);
3223-
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest);
3240+
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest, spreadableCheckedAndTrue);
32243241
return pVarDestArray;
32253242
}
32263243
}
@@ -3238,18 +3255,26 @@ namespace Js
32383255
{
32393256
Var aItem = args[idxArg];
32403257

3241-
if (scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled() && !JavascriptOperators::IsConcatSpreadable(aItem))
3242-
{
3243-
3244-
pDestArray->SetItem(idxDest, aItem, PropertyOperation_ThrowIfNotExtensible);
3258+
bool spreadableCheckedAndTrue = false;
32453259

3246-
idxDest = idxDest + 1;
3247-
if (!JavascriptNativeFloatArray::Is(pDestArray)) // SetItem could convert pDestArray to a var array if aItem is not an integer if so fall back
3260+
if (scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled())
3261+
{
3262+
if (JavascriptOperators::IsConcatSpreadable(aItem))
32483263
{
3249-
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
3250-
return pDestArray;
3264+
spreadableCheckedAndTrue = true;
3265+
}
3266+
else
3267+
{
3268+
pDestArray->SetItem(idxDest, aItem, PropertyOperation_ThrowIfNotExtensible);
3269+
3270+
idxDest = idxDest + 1;
3271+
if (!JavascriptNativeFloatArray::Is(pDestArray)) // SetItem could convert pDestArray to a var array if aItem is not an integer if so fall back
3272+
{
3273+
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg + 1, idxDest);
3274+
return pDestArray;
3275+
}
3276+
continue;
32513277
}
3252-
continue;
32533278
}
32543279

32553280
bool converted;
@@ -3270,9 +3295,10 @@ namespace Js
32703295
else
32713296
{
32723297
JavascriptArray *pVarDestArray = JavascriptNativeFloatArray::ConvertToVarArray(pDestArray);
3273-
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest);
3298+
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest, spreadableCheckedAndTrue);
32743299
return pVarDestArray;
32753300
}
3301+
32763302
if (converted)
32773303
{
32783304
// Copying the last array forced a conversion, so switch over to the var version

‎lib/Runtime/Library/JavascriptArray.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ namespace Js
123123
static const uint8 AllocationBucketIndex = 0;
124124
// 1st column in allocationBuckets that stores no. of missing elements to initialize for given bucket
125125
static const uint8 MissingElementsCountIndex = 1;
126-
// 2nd column in allocationBuckets that stores allocation size for given bucket
126+
// 2nd column in allocationBuckets that stores allocation size for given bucket
127127
static const uint8 AllocationSizeIndex = 2;
128128
#if defined(_M_X64_OR_ARM64)
129129
static const uint8 AllocationBucketsCount = 3;
@@ -780,9 +780,9 @@ namespace Js
780780
static void SetConcatItem(Var aItem, uint idxArg, JavascriptArray* pDestArray, RecyclableObject* pDestObj, T idxDest, ScriptContext *scriptContext);
781781

782782
template<typename T>
783-
static void ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start, BigIndex startIdxDest, BOOL firstPromotedItemIsSpreadable, BigIndex firstPromotedItemLength);
783+
static void ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start, BigIndex startIdxDest, BOOL firstPromotedItemIsSpreadable, BigIndex firstPromotedItemLength, bool spreadableCheckedAndTrue = false);
784784
template<typename T>
785-
static void ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start = 0, uint startIdxDest = 0u, BOOL FirstPromotedItemIsSpreadable = false, BigIndex FirstPromotedItemLength = 0u);
785+
static void ConcatArgs(RecyclableObject* pDestObj, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext, uint start = 0, uint startIdxDest = 0u, BOOL FirstPromotedItemIsSpreadable = false, BigIndex FirstPromotedItemLength = 0u, bool spreadableCheckedAndTrue = false);
786786
static JavascriptArray* ConcatIntArgs(JavascriptNativeIntArray* pDestArray, TypeId* remoteTypeIds, Js::Arguments& args, ScriptContext* scriptContext);
787787
static bool PromoteToBigIndex(BigIndex lhs, BigIndex rhs);
788788
static bool PromoteToBigIndex(BigIndex lhs, uint32 rhs);

‎test/Array/bug_9575461.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
var obj = [1, 2, 3];
7+
var cc_base = [-2, -1, 0];
8+
var isCS = false;
9+
var counter = 0;
10+
11+
Object.defineProperty(obj, Symbol.isConcatSpreadable, {
12+
get : function () {
13+
counter++;
14+
obj[2] = isCS ? "Some String inserted" : 123;
15+
isCS = !isCS;
16+
return isCS;
17+
}
18+
});
19+
20+
var MAY_THROW = function(n, result) {
21+
if (!result) throw new Error(n + ". FAILED");
22+
};
23+
24+
MAY_THROW(0, cc_base.concat(obj).length == 6);
25+
MAY_THROW(1, cc_base.concat(obj).length == 4);
26+
MAY_THROW(2, counter == 2 && !isCS);
27+
28+
print("PASS");

‎test/Array/rlexe.xml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -728,4 +728,9 @@
728728
<tags>BugFix</tags>
729729
</default>
730730
</test>
731+
<test>
732+
<default>
733+
<files>bug_9575461.js</files>
734+
</default>
735+
</test>
731736
</regress-exe>

0 commit comments

Comments
 (0)