Skip to content

Commit 60a8a82

Browse files
author
Kevin Smith
committed
[MERGE #6189 @zenparsing] Don't copy object directly if internal properties are set
Merge pull request #6189 from zenparsing:weakmap-assign-bug Fixes #6186. A fast path for Object.assign will directly copy object properties. Currently, this fast path does not detect when internal properties have been added to an object and will copy those properties as well. This change tracks whether internal properties have been added for a path type handler and uses that information during object copying. A similar flag is being used for `ArraySpeciesCreate`. This change also moves that flag into `PathTypeHandlerBase`.
2 parents 462869c + 86333c2 commit 60a8a82

File tree

8 files changed

+73
-42
lines changed

8 files changed

+73
-42
lines changed

‎lib/Runtime/Library/JavascriptArray.cpp

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12278,15 +12278,23 @@ using namespace Js;
1227812278
JS_REENTRANCY_LOCK(jsReentLock, scriptContext->GetThreadContext());
1227912279
SETOBJECT_FOR_MUTATION(jsReentLock, originalArray);
1228012280

12281-
if (JavascriptArray::IsNonES5Array(originalArray)
12282-
&& !UnsafeVarTo<DynamicObject>(originalArray)->GetDynamicType()->GetTypeHandler()->GetIsNotPathTypeHandlerOrHasUserDefinedCtor()
12283-
&& UnsafeVarTo<DynamicObject>(originalArray)->GetPrototype() == scriptContext->GetLibrary()->GetArrayPrototype()
12284-
&& !scriptContext->GetLibrary()->GetArrayObjectHasUserDefinedSpecies())
12281+
auto* library = scriptContext->GetLibrary();
12282+
12283+
if (JavascriptArray::IsNonES5Array(originalArray))
1228512284
{
12286-
return nullptr;
12285+
auto* dynamicObject = UnsafeVarTo<DynamicObject>(originalArray);
12286+
auto* typeHandler = dynamicObject->GetDynamicType()->GetTypeHandler();
12287+
12288+
if (typeHandler->IsPathTypeHandler()
12289+
&& !PathTypeHandlerBase::FromTypeHandler(typeHandler)->HasUserDefinedCtor()
12290+
&& dynamicObject->GetPrototype() == library->GetArrayPrototype()
12291+
&& !library->GetArrayObjectHasUserDefinedSpecies())
12292+
{
12293+
return nullptr;
12294+
}
1228712295
}
1228812296

12289-
Var constructor = scriptContext->GetLibrary()->GetUndefined();
12297+
Var constructor = library->GetUndefined();
1229012298

1229112299
JS_REENTRANT(jsReentLock, BOOL isArray = JavascriptOperators::IsArray(originalArray));
1229212300
if (isArray)
@@ -12304,7 +12312,7 @@ using namespace Js;
1230412312
{
1230512313
if (constructorScriptContext->GetLibrary()->GetArrayConstructor() == constructor)
1230612314
{
12307-
constructor = scriptContext->GetLibrary()->GetUndefined();
12315+
constructor = library->GetUndefined();
1230812316
}
1230912317
}
1231012318
}
@@ -12320,14 +12328,14 @@ using namespace Js;
1232012328
}
1232112329
return nullptr;
1232212330
}
12323-
if (constructor == scriptContext->GetLibrary()->GetNull())
12331+
if (constructor == library->GetNull())
1232412332
{
12325-
constructor = scriptContext->GetLibrary()->GetUndefined();
12333+
constructor = library->GetUndefined();
1232612334
}
1232712335
}
1232812336
}
1232912337

12330-
if (constructor == scriptContext->GetLibrary()->GetUndefined() || constructor == scriptContext->GetLibrary()->GetArrayConstructor())
12338+
if (constructor == library->GetUndefined() || constructor == library->GetArrayConstructor())
1233112339
{
1233212340
if (length > UINT_MAX)
1233312341
{
@@ -12336,7 +12344,7 @@ using namespace Js;
1233612344

1233712345
if (nullptr == pIsIntArray)
1233812346
{
12339-
return scriptContext->GetLibrary()->CreateArray(static_cast<uint32>(length));
12347+
return library->CreateArray(static_cast<uint32>(length));
1234012348
}
1234112349
else
1234212350
{

‎lib/Runtime/Types/DynamicObject.cpp

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -866,11 +866,11 @@ namespace Js
866866
}
867867
return false;
868868
}
869-
if (!from->GetTypeHandler()->IsPathTypeHandler())
869+
if (!from->GetTypeHandler()->IsObjectCopyable())
870870
{
871871
if (PHASE_TRACE1(ObjectCopyPhase))
872872
{
873-
Output::Print(_u("ObjectCopy: Can't copy: Don't have PathTypeHandler\n"));
873+
Output::Print(_u("ObjectCopy: Can't copy: from obj does not have copyable type handler\n"));
874874
}
875875
return false;
876876
}
@@ -890,14 +890,6 @@ namespace Js
890890
}
891891
return false;
892892
}
893-
if (PathTypeHandlerBase::FromTypeHandler(from->GetTypeHandler())->HasAccessors())
894-
{
895-
if (PHASE_TRACE1(ObjectCopyPhase))
896-
{
897-
Output::Print(_u("ObjectCopy: Can't copy: type handler has accessors\n"));
898-
}
899-
return false;
900-
}
901893
if (this->GetPrototype() != from->GetPrototype())
902894
{
903895
if (PHASE_TRACE1(ObjectCopyPhase))
@@ -906,14 +898,6 @@ namespace Js
906898
}
907899
return false;
908900
}
909-
if (!from->GetTypeHandler()->AllPropertiesAreEnumerable())
910-
{
911-
if (PHASE_TRACE1(ObjectCopyPhase))
912-
{
913-
Output::Print(_u("ObjectCopy: Can't copy: from obj has non-enumerable properties\n"));
914-
}
915-
return false;
916-
}
917901
if (from->IsExternal())
918902
{
919903
if (PHASE_TRACE1(ObjectCopyPhase))

‎lib/Runtime/Types/PathTypeHandler.cpp

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -235,12 +235,20 @@ namespace Js
235235
DynamicTypeHandler(slotCapacity, inlineSlotCapacity, offsetOfInlineSlots, DefaultFlags | (isLocked ? IsLockedFlag : 0) | (isShared ? (MayBecomeSharedFlag | IsSharedFlag) : 0)),
236236
typePath(typePath),
237237
predecessorType(predecessorType),
238-
successorInfo(nullptr)
238+
successorInfo(nullptr),
239+
hasUserDefinedCtor(false),
240+
hasInternalProperty(false)
239241
{
240242
Assert(pathLength <= slotCapacity);
241243
Assert(inlineSlotCapacity <= slotCapacity);
242244
SetUnusedBytesValue(pathLength);
243-
isNotPathTypeHandlerOrHasUserDefinedCtor = predecessorType == nullptr ? false : predecessorType->GetTypeHandler()->GetIsNotPathTypeHandlerOrHasUserDefinedCtor();
245+
246+
if (predecessorType != nullptr && predecessorType->GetTypeHandler()->IsPathTypeHandler())
247+
{
248+
auto* handler = PathTypeHandlerBase::FromTypeHandler(predecessorType->GetTypeHandler());
249+
hasUserDefinedCtor = handler->hasUserDefinedCtor;
250+
hasInternalProperty = handler->hasInternalProperty;
251+
}
244252
}
245253

246254
int PathTypeHandlerBase::GetPropertyCount()
@@ -2166,7 +2174,12 @@ namespace Js
21662174

21672175
if (key.GetPropertyId() == PropertyIds::constructor)
21682176
{
2169-
nextPath->isNotPathTypeHandlerOrHasUserDefinedCtor = true;
2177+
nextPath->hasUserDefinedCtor = true;
2178+
}
2179+
2180+
if (IsInternalPropertyId(key.GetPropertyId()))
2181+
{
2182+
nextPath->hasInternalProperty = true;
21702183
}
21712184

21722185
#ifdef PROFILE_TYPES

‎lib/Runtime/Types/PathTypeHandler.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,8 @@ namespace Js
102102
Field(DynamicType*) predecessorType; // Strong reference to predecessor type so that predecessor types remain in the cache even though they might not be used
103103
Field(TypePath*) typePath;
104104
Field(PathTypeSuccessorInfo*) successorInfo;
105+
Field(bool) hasUserDefinedCtor;
106+
Field(bool) hasInternalProperty;
105107

106108
public:
107109
DEFINE_GETCPPNAME();
@@ -119,6 +121,8 @@ namespace Js
119121
return nullptr;
120122
}
121123

124+
bool HasUserDefinedCtor() { return this->hasUserDefinedCtor; }
125+
122126
virtual BOOL IsLockable() const override { return true; }
123127
virtual BOOL IsSharable() const override { return true; }
124128

@@ -459,6 +463,8 @@ namespace Js
459463
DEFINE_VTABLE_CTOR_NO_REGISTER(PathTypeHandlerNoAttr, PathTypeHandlerBase);
460464

461465
public:
466+
virtual bool IsObjectCopyable() const override { return !this->hasInternalProperty; }
467+
462468
static PathTypeHandlerNoAttr * New(ScriptContext * scriptContext, TypePath* typePath, uint16 pathLength, uint16 inlineSlotCapacity, uint16 offsetOfInlineSlots, bool isLocked = false, bool isShared = false, DynamicType* predecessorType = nullptr);
463469
static PathTypeHandlerNoAttr * New(ScriptContext * scriptContext, TypePath* typePath, uint16 pathLength, const PropertyIndex slotCapacity, uint16 inlineSlotCapacity, uint16 offsetOfInlineSlots, bool isLocked = false, bool isShared = false, DynamicType* predecessorType = nullptr);
464470
static PathTypeHandlerNoAttr * New(ScriptContext * scriptContext, PathTypeHandlerNoAttr * typeHandler, bool isLocked, bool isShared);
@@ -534,6 +540,7 @@ namespace Js
534540
return FindNextPropertyHelper(scriptContext, this->attributes, index, propertyString, propertyId, attributes, type, typeToEnumerate, flags, instance, info);
535541
}
536542
virtual BOOL AllPropertiesAreEnumerable() sealed override { return false; }
543+
virtual bool IsObjectCopyable() const override { return false; }
537544
#if ENABLE_NATIVE_CODEGEN
538545
virtual bool IsObjTypeSpecEquivalent(const Type* type, const TypeEquivalenceRecord& record, uint& failedPropertyIndex) override;
539546
virtual bool IsObjTypeSpecEquivalent(const Type* type, const EquivalentPropertyEntry* entry) override;

‎lib/Runtime/Types/TypeHandler.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ using namespace Js;
7777
? RoundUpObjectHeaderInlinedInlineSlotCapacity(inlineSlotCapacity)
7878
: RoundUpInlineSlotCapacity(inlineSlotCapacity);
7979
this->slotCapacity = RoundUpSlotCapacity(slotCapacity, inlineSlotCapacity);
80-
this->isNotPathTypeHandlerOrHasUserDefinedCtor = true;
8180

8281
Assert(IsObjectHeaderInlinedTypeHandler() == IsObjectHeaderInlined(offsetOfInlineSlots));
8382
}
@@ -884,6 +883,5 @@ using namespace Js;
884883
Output::Print(_u("%*sslotCapacity: %d\n"), fieldIndent, padding, this->slotCapacity);
885884
Output::Print(_u("%*sunusedBytes: %u\n"), fieldIndent, padding, this->unusedBytes);
886885
Output::Print(_u("%*sinlineSlotCapacty: %u\n"), fieldIndent, padding, this->inlineSlotCapacity);
887-
Output::Print(_u("%*sisNotPathTypeHandlerOrHasUserDefinedCtor: %d\n"), fieldIndent, padding, static_cast<int>(this->isNotPathTypeHandlerOrHasUserDefinedCtor));
888886
}
889887
#endif

‎lib/Runtime/Types/TypeHandler.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ namespace Js
4848
Field(int) slotCapacity;
4949
Field(uint16) unusedBytes; // This always has it's lowest bit set to avoid false references
5050
Field(uint16) inlineSlotCapacity;
51-
Field(bool) isNotPathTypeHandlerOrHasUserDefinedCtor;
5251
Field(bool) protoCachesWereInvalidated;
5352

5453
public:
@@ -58,7 +57,6 @@ namespace Js
5857
propertyTypes(typeHandler->propertyTypes),
5958
slotCapacity(typeHandler->slotCapacity),
6059
offsetOfInlineSlots(typeHandler->offsetOfInlineSlots),
61-
isNotPathTypeHandlerOrHasUserDefinedCtor(typeHandler->isNotPathTypeHandlerOrHasUserDefinedCtor),
6260
unusedBytes(typeHandler->unusedBytes),
6361
protoCachesWereInvalidated(false),
6462
inlineSlotCapacity(typeHandler->inlineSlotCapacity)
@@ -427,7 +425,6 @@ namespace Js
427425
}
428426

429427
BOOL Freeze(DynamicObject *instance, bool isConvertedType = false) { return FreezeImpl(instance, isConvertedType); }
430-
bool GetIsNotPathTypeHandlerOrHasUserDefinedCtor() const { return this->isNotPathTypeHandlerOrHasUserDefinedCtor; }
431428

432429
virtual BOOL IsStringTypeHandler() const { return false; }
433430

@@ -560,6 +557,8 @@ namespace Js
560557
virtual BOOL IsSimpleDictionaryTypeHandler() const {return FALSE; }
561558
virtual BOOL IsDictionaryTypeHandler() const {return FALSE;}
562559

560+
virtual bool IsObjectCopyable() const { return false; }
561+
563562
static bool IsolatePrototypes() { return CONFIG_FLAG(IsolatePrototypes); }
564563
static bool ChangeTypeOnProto() { return CONFIG_FLAG(ChangeTypeOnProto); }
565564
static bool ShouldFixMethodProperties() { return !PHASE_OFF1(FixMethodPropsPhase); }

‎test/Object/assign.baseline

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
ObjectCopy succeeded
2-
ObjectCopy: Can't copy: Don't have PathTypeHandler
3-
ObjectCopy: Can't copy: type handler has accessors
4-
ObjectCopy: Can't copy: type handler has accessors
2+
ObjectCopy: Can't copy: from obj does not have copyable type handler
3+
ObjectCopy: Can't copy: from obj does not have copyable type handler
4+
ObjectCopy: Can't copy: from obj does not have copyable type handler
55
ObjectCopy: Can't copy: Prototypes don't match
6-
ObjectCopy: Can't copy: from obj has non-enumerable properties
6+
ObjectCopy: Can't copy: from obj does not have copyable type handler
77
ObjectCopy succeeded
88
ObjectCopy succeeded
99
ObjectCopy: Can't copy: to obj has object array
10-
ObjectCopy: Can't copy: Don't have PathTypeHandler
10+
ObjectCopy: Can't copy: from obj does not have copyable type handler
1111
pass

‎test/es6/weakmap_functionality.js

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,28 @@ var tests = [
501501
}
502502
},
503503

504+
{
505+
name: "WeakMap internal property data is not copied by Object.assign",
506+
body: function () {
507+
var key1 = {};
508+
var key2 = {};
509+
var map = new WeakMap();
510+
511+
map.set(key1, 1);
512+
map.delete(Object.assign(key2, key1));
513+
assert.isFalse(map.has(key2));
514+
515+
key1 = {};
516+
key2 = {};
517+
map = new WeakMap();
518+
519+
map.set(key1, 1);
520+
key1.a = 1;
521+
map.delete(Object.assign(key2, key1));
522+
assert.isFalse(map.has(key2));
523+
}
524+
}
525+
504526
];
505527

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

0 commit comments

Comments
 (0)