Skip to content

Commit 833c7f5

Browse files
MikeHolmanMikeHolman
committed
[MERGE #5965 @MikeHolman] make JsCloneObject able to handle all objects, instead of just path type objects
Merge pull request #5965 from MikeHolman:bettercopy This improves the JsCloneObject API to work with more object types
2 parents 1096864 + 2e4978c commit 833c7f5

File tree

8 files changed

+111
-80
lines changed

8 files changed

+111
-80
lines changed

‎lib/Jsrt/Core/JsrtCore.cpp

Lines changed: 9 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -735,49 +735,23 @@ CHAKRA_API JsCloneObject(_In_ JsValueRef source, _Out_ JsValueRef* newObject)
735735
VALIDATE_JSREF(source);
736736

737737
return ContextAPINoScriptWrapper([&](Js::ScriptContext* scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
738-
if (Js::VarIs<Js::JavascriptProxy>(source))
738+
739+
while (Js::VarIs<Js::JavascriptProxy>(source))
739740
{
740741
source = Js::UnsafeVarTo<Js::JavascriptProxy>(source)->GetTarget();
741742
}
742-
JsrtExternalObject* externalSource = Js::JavascriptOperators::TryFromVar<JsrtExternalObject>(source);
743-
if (externalSource)
744-
{
745-
JsrtExternalType* externalType = externalSource->GetExternalType();
746-
JsrtExternalObject* target = JsrtExternalObject::Create(
747-
externalSource->GetSlotData(),
748-
externalSource->GetInlineSlotSize(),
749-
externalType->GetJsTraceCallback(),
750-
externalType->GetJsFinalizeCallback(),
751-
externalSource->GetPrototype(),
752-
scriptContext,
753-
externalType);
754-
bool success = target->TryCopy(externalSource, true);
755-
AssertOrFailFast(success);
756-
*newObject = target;
757-
return JsNoError;
758-
}
759-
else
760-
{
761-
Js::CustomExternalWrapperObject * externalWrapper = Js::JavascriptOperators::TryFromVar<Js::CustomExternalWrapperObject>(source);
762-
if (externalWrapper != nullptr) {
763-
Js::CustomExternalWrapperObject * target = Js::CustomExternalWrapperObject::Clone(externalWrapper, scriptContext);
764-
Assert(target);
765-
*newObject = target;
766-
return JsNoError;
767-
}
768-
}
769743

770-
Js::DynamicObject* objSource = Js::JavascriptOperators::TryFromVar<Js::DynamicObject>(source);
771-
if (objSource)
744+
// We can currently only clone certain types of dynamic objects
745+
// TODO: support other object types
746+
if (Js::DynamicObject::IsBaseDynamicObject(source) ||
747+
Js::VarIs<JsrtExternalObject>(source) ||
748+
Js::VarIs<Js::CustomExternalWrapperObject>(source))
772749
{
773-
if (!objSource->ShareType())
774-
{
775-
AssertOrFailFast(UNREACHED); // TODO
776-
return JsErrorInvalidArgument;
777-
}
750+
Js::DynamicObject* objSource = Js::UnsafeVarTo<Js::DynamicObject>(source);
778751
*newObject = objSource->Copy(true);
779752
return JsNoError;
780753
}
754+
781755
return JsErrorInvalidArgument;
782756
});
783757
}

‎lib/Jsrt/Jsrt.cpp

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2723,9 +2723,9 @@ CHAKRA_API JsHasExternalData(_In_ JsValueRef object, _Out_ bool *value)
27232723

27242724
BEGIN_JSRT_NO_EXCEPTION
27252725
{
2726-
if (Js::VarIs<Js::JavascriptProxy>(object))
2726+
while (Js::VarIs<Js::JavascriptProxy>(object))
27272727
{
2728-
object = Js::VarTo<Js::JavascriptProxy>(object);
2728+
object = Js::UnsafeVarTo<Js::JavascriptProxy>(object);
27292729
}
27302730
*value = (Js::VarIs<JsrtExternalObject>(object)
27312731
#ifdef _CHAKRACOREBUILD
@@ -2743,18 +2743,18 @@ CHAKRA_API JsGetExternalData(_In_ JsValueRef object, _Out_ void **data)
27432743

27442744
BEGIN_JSRT_NO_EXCEPTION
27452745
{
2746-
if (Js::VarIs<Js::JavascriptProxy>(object))
2746+
while (Js::VarIs<Js::JavascriptProxy>(object))
27472747
{
2748-
object = Js::VarTo<Js::JavascriptProxy>(object)->GetTarget();
2748+
object = Js::UnsafeVarTo<Js::JavascriptProxy>(object)->GetTarget();
27492749
}
27502750
if (Js::VarIs<JsrtExternalObject>(object))
27512751
{
2752-
*data = Js::VarTo<JsrtExternalObject>(object)->GetSlotData();
2752+
*data = Js::UnsafeVarTo<JsrtExternalObject>(object)->GetSlotData();
27532753
}
27542754
#ifdef _CHAKRACOREBUILD
27552755
else if (Js::VarIs<Js::CustomExternalWrapperObject>(object))
27562756
{
2757-
*data = Js::VarTo<Js::CustomExternalWrapperObject>(object)->GetSlotData();
2757+
*data = Js::UnsafeVarTo<Js::CustomExternalWrapperObject>(object)->GetSlotData();
27582758
}
27592759
#endif
27602760
else
@@ -2772,18 +2772,18 @@ CHAKRA_API JsSetExternalData(_In_ JsValueRef object, _In_opt_ void *data)
27722772

27732773
BEGIN_JSRT_NO_EXCEPTION
27742774
{
2775-
if (Js::VarIs<Js::JavascriptProxy>(object))
2775+
while (Js::VarIs<Js::JavascriptProxy>(object))
27762776
{
2777-
object = Js::VarTo<Js::JavascriptProxy>(object)->GetTarget();
2777+
object = Js::UnsafeVarTo<Js::JavascriptProxy>(object)->GetTarget();
27782778
}
27792779
if (Js::VarIs<JsrtExternalObject>(object))
27802780
{
2781-
Js::VarTo<JsrtExternalObject>(object)->SetSlotData(data);
2781+
Js::UnsafeVarTo<JsrtExternalObject>(object)->SetSlotData(data);
27822782
}
27832783
#ifdef _CHAKRACOREBUILD
27842784
else if (Js::VarIs<Js::CustomExternalWrapperObject>(object))
27852785
{
2786-
Js::VarTo<Js::CustomExternalWrapperObject>(object)->SetSlotData(data);
2786+
Js::UnsafeVarTo<Js::CustomExternalWrapperObject>(object)->SetSlotData(data);
27872787
}
27882788
#endif
27892789
else

‎lib/Jsrt/JsrtExternalObject.cpp

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,25 @@ JsrtExternalObject::JsrtExternalObject(JsrtExternalType * type, void *data, uint
5959
}
6060
}
6161

62+
JsrtExternalObject::JsrtExternalObject(JsrtExternalObject* instance, bool deepCopy) :
63+
Js::DynamicObject(instance, deepCopy)
64+
{
65+
if (instance->GetInlineSlotSize() != 0)
66+
{
67+
this->slotType = SlotType::Inline;
68+
this->u.inlineSlotSize = instance->GetInlineSlotSize();
69+
if (instance->GetInlineSlots())
70+
{
71+
memcpy_s(this->GetInlineSlots(), this->GetInlineSlotSize(), instance->GetInlineSlots(), instance->GetInlineSlotSize());
72+
}
73+
}
74+
else
75+
{
76+
this->slotType = SlotType::External;
77+
this->u.slot = instance->GetInlineSlots();
78+
}
79+
}
80+
6281
#ifdef _CHAKRACOREBUILD
6382
/* static */
6483
JsrtExternalObject* JsrtExternalObject::Create(void *data, uint inlineSlotSize, JsTraceCallback traceCallback, JsFinalizeCallback finalizeCallback, Js::RecyclableObject * prototype, Js::ScriptContext *scriptContext, JsrtExternalType * type)
@@ -138,6 +157,26 @@ JsrtExternalObject* JsrtExternalObject::Create(void *data, uint inlineSlotSize,
138157
return externalObject;
139158
}
140159

160+
JsrtExternalObject* JsrtExternalObject::Copy(bool deepCopy)
161+
{
162+
Recycler* recycler = this->GetRecycler();
163+
JsrtExternalType* type = this->GetExternalType();
164+
int inlineSlotSize = this->GetInlineSlotSize();
165+
166+
if (type->GetJsTraceCallback() != nullptr)
167+
{
168+
return RecyclerNewTrackedPlus(recycler, inlineSlotSize, JsrtExternalObject, this, deepCopy);
169+
}
170+
else if (type->GetJsFinalizeCallback() != nullptr)
171+
{
172+
return RecyclerNewFinalizedPlus(recycler, inlineSlotSize, JsrtExternalObject, this, deepCopy);
173+
}
174+
else
175+
{
176+
return RecyclerNewPlus(recycler, inlineSlotSize, JsrtExternalObject, this, deepCopy);
177+
}
178+
}
179+
141180
void JsrtExternalObject::Mark(Recycler * recycler)
142181
{
143182
#ifdef _CHAKRACOREBUILD

‎lib/Jsrt/JsrtExternalObject.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,15 @@ class JsrtExternalObject : public Js::DynamicObject
6060

6161
public:
6262
JsrtExternalObject(JsrtExternalType * type, void *data, uint inlineSlotSize);
63+
JsrtExternalObject(JsrtExternalObject* instance, bool deepCopy);
6364

6465
#ifdef _CHAKRACOREBUILD
6566
static JsrtExternalObject * Create(void *data, uint inlineSlotSize, JsTraceCallback traceCallback, JsFinalizeCallback finalizeCallback, Js::RecyclableObject * prototype, Js::ScriptContext *scriptContext, JsrtExternalType * type);
6667
#endif
6768
static JsrtExternalObject * Create(void *data, uint inlineSlotSize, JsFinalizeCallback finalizeCallback, Js::RecyclableObject * prototype, Js::ScriptContext *scriptContext, JsrtExternalType * type);
6869

70+
virtual JsrtExternalObject* Copy(bool deepCopy) override;
71+
6972
JsrtExternalType * GetExternalType() const { return (JsrtExternalType *)this->GetType(); }
7073

7174
void Mark(Recycler * recycler) override;

‎lib/Runtime/Library/CustomExternalWrapperObject.cpp

Lines changed: 37 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,26 @@ CustomExternalWrapperObject::CustomExternalWrapperObject(CustomExternalWrapperTy
4545
}
4646
}
4747

48+
CustomExternalWrapperObject::CustomExternalWrapperObject(CustomExternalWrapperObject * instance, bool deepCopy) :
49+
Js::DynamicObject(instance, deepCopy),
50+
initialized(instance->initialized)
51+
{
52+
if (instance->GetInlineSlotSize() != 0)
53+
{
54+
this->slotType = SlotType::Inline;
55+
this->u.inlineSlotSize = instance->GetInlineSlotSize();
56+
if (instance->GetInlineSlots())
57+
{
58+
memcpy_s(this->GetInlineSlots(), this->GetInlineSlotSize(), instance->GetInlineSlots(), instance->GetInlineSlotSize());
59+
}
60+
}
61+
else
62+
{
63+
this->slotType = SlotType::External;
64+
this->u.slot = instance->GetInlineSlots();
65+
}
66+
}
67+
4868
/* static */
4969
CustomExternalWrapperObject* CustomExternalWrapperObject::Create(void *data, uint inlineSlotSize, JsTraceCallback traceCallback, JsFinalizeCallback finalizeCallback, JsGetterSetterInterceptor ** getterSetterInterceptor, Js::RecyclableObject * prototype, Js::ScriptContext *scriptContext)
5070
{
@@ -120,32 +140,24 @@ BOOL CustomExternalWrapperObject::EnsureInitialized(ScriptContext* requestContex
120140
return TRUE;
121141
}
122142

123-
CustomExternalWrapperObject * CustomExternalWrapperObject::Clone(CustomExternalWrapperObject * source, ScriptContext * scriptContext)
143+
CustomExternalWrapperObject* CustomExternalWrapperObject::Copy(bool deepCopy)
124144
{
125-
Js::CustomExternalWrapperType * externalType = source->GetExternalType();
126-
Js::JsGetterSetterInterceptor * originalInterceptors = externalType->GetJsGetterSetterInterceptor();
127-
Js::JsGetterSetterInterceptor * newInterceptors = originalInterceptors;
128-
Js::CustomExternalWrapperObject * target = Js::CustomExternalWrapperObject::Create(
129-
source->GetSlotData(),
130-
source->GetInlineSlotSize(),
131-
externalType->GetJsTraceCallback(),
132-
externalType->GetJsFinalizeCallback(),
133-
&newInterceptors,
134-
source->GetPrototype(),
135-
scriptContext);
136-
target->initialized = source->initialized;
137-
138-
bool success = target->TryCopy(source, true);
139-
AssertOrFailFast(success);
140-
141-
//TODO:akatti: We will always used a cached type, so the following code can be removed.
142-
// If we are using type from the cache we don't need to copy the interceptors over.
143-
if (newInterceptors != originalInterceptors)
144-
{
145-
memcpy_s(newInterceptors, sizeof(Js::JsGetterSetterInterceptor), originalInterceptors, sizeof(Js::JsGetterSetterInterceptor));
146-
}
147-
148-
return target;
145+
Recycler* recycler = this->GetRecycler();
146+
CustomExternalWrapperType* externalType = this->GetExternalType();
147+
int inlineSlotSize = this->GetInlineSlotSize();
148+
149+
if (externalType->GetJsTraceCallback() != nullptr)
150+
{
151+
return RecyclerNewTrackedPlus(recycler, inlineSlotSize, CustomExternalWrapperObject, this, deepCopy);
152+
}
153+
else if (externalType->GetJsFinalizeCallback() != nullptr)
154+
{
155+
return RecyclerNewFinalizedPlus(recycler, inlineSlotSize, CustomExternalWrapperObject, this, deepCopy);
156+
}
157+
else
158+
{
159+
return RecyclerNewPlus(recycler, inlineSlotSize, CustomExternalWrapperObject, this, deepCopy);
160+
}
149161
}
150162

151163
BOOL CustomExternalWrapperObject::IsObjectAlive()

‎lib/Runtime/Library/CustomExternalWrapperObject.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,14 @@ namespace Js
6161
};
6262

6363
CustomExternalWrapperObject(CustomExternalWrapperType * type, void *data, uint inlineSlotSize);
64+
CustomExternalWrapperObject(CustomExternalWrapperObject* instance, bool deepCopy);
6465

6566
BOOL IsObjectAlive();
6667
BOOL VerifyObjectAlive();
6768

6869
static CustomExternalWrapperObject * Create(void *data, uint inlineSlotSize, JsTraceCallback traceCallback, JsFinalizeCallback finalizeCallback, JsGetterSetterInterceptor ** getterSetterInterceptor, RecyclableObject * prototype, ScriptContext *scriptContext);
69-
static CustomExternalWrapperObject * Clone(CustomExternalWrapperObject * source, ScriptContext * scriptContext);
70+
71+
virtual CustomExternalWrapperObject* Copy(bool deepCopy) override;
7072

7173
static BOOL GetOwnPropertyDescriptor(RecyclableObject * obj, PropertyId propertyId, ScriptContext* requestContext, PropertyDescriptor* propertyDescriptor);
7274
static BOOL DefineOwnPropertyDescriptor(RecyclableObject * obj, PropertyId propId, const PropertyDescriptor& descriptor, bool throwOnError, ScriptContext* requestContext);

‎lib/Runtime/Types/DynamicObject.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -840,7 +840,7 @@ namespace Js
840840
return reinterpret_cast<Field(Var)*>(reinterpret_cast<size_t>(this) + this->GetOffsetOfInlineSlots());
841841
}
842842

843-
bool DynamicObject::IsCompatibleForCopy(DynamicObject* from, bool ignoreSideEffects) const
843+
bool DynamicObject::IsCompatibleForCopy(DynamicObject* from) const
844844
{
845845
if (this->GetTypeHandler()->GetInlineSlotCapacity() != from->GetTypeHandler()->GetInlineSlotCapacity())
846846
{
@@ -876,7 +876,7 @@ namespace Js
876876
}
877877
return false;
878878
}
879-
if (PathTypeHandlerBase::FromTypeHandler(from->GetTypeHandler())->HasAccessors() && !ignoreSideEffects)
879+
if (PathTypeHandlerBase::FromTypeHandler(from->GetTypeHandler())->HasAccessors())
880880
{
881881
if (PHASE_TRACE1(ObjectCopyPhase))
882882
{
@@ -892,15 +892,15 @@ namespace Js
892892
}
893893
return false;
894894
}
895-
if (!from->GetTypeHandler()->AllPropertiesAreEnumerable() && !ignoreSideEffects)
895+
if (!from->GetTypeHandler()->AllPropertiesAreEnumerable())
896896
{
897897
if (PHASE_TRACE1(ObjectCopyPhase))
898898
{
899899
Output::Print(_u("ObjectCopy: Can't copy: from obj has non-enumerable properties\n"));
900900
}
901901
return false;
902902
}
903-
if (from->IsExternal() && !ignoreSideEffects)
903+
if (from->IsExternal())
904904
{
905905
if (PHASE_TRACE1(ObjectCopyPhase))
906906
{
@@ -920,7 +920,7 @@ namespace Js
920920
return true;
921921
}
922922

923-
bool DynamicObject::TryCopy(DynamicObject* from, bool ignoreSideEffects)
923+
bool DynamicObject::TryCopy(DynamicObject* from)
924924
{
925925
#if ENABLE_TTD
926926
if (from->GetScriptContext()->ShouldPerformRecordOrReplayAction())
@@ -934,7 +934,7 @@ namespace Js
934934
return false;
935935
}
936936
// Validate that objects are compatible
937-
if (!this->IsCompatibleForCopy(from, ignoreSideEffects))
937+
if (!this->IsCompatibleForCopy(from))
938938
{
939939
return false;
940940
}

‎lib/Runtime/Types/DynamicObject.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ namespace Js
172172
#endif
173173

174174
private:
175-
bool IsCompatibleForCopy(DynamicObject* from, bool ignoreSideEffects) const;
175+
bool IsCompatibleForCopy(DynamicObject* from) const;
176176

177177
bool IsObjectHeaderInlinedTypeHandlerUnchecked() const;
178178
public:
@@ -232,7 +232,7 @@ namespace Js
232232
void InvalidateHasOnlyWritableDataPropertiesInPrototypeChainCacheIfPrototype();
233233
void ResetObject(DynamicType* type, BOOL keepProperties);
234234

235-
bool TryCopy(DynamicObject* from, bool ignoreSideEffects = false);
235+
bool TryCopy(DynamicObject* from);
236236

237237
virtual void SetIsPrototype();
238238

@@ -334,7 +334,7 @@ namespace Js
334334

335335
void SetObjectArray(ArrayObject* objectArray);
336336

337-
DynamicObject * Copy(bool deepCopy);
337+
virtual DynamicObject* Copy(bool deepCopy);
338338
protected:
339339
BOOL GetEnumeratorWithPrefix(JavascriptEnumerator * prefixEnumerator, JavascriptStaticEnumerator * enumerator, EnumeratorFlags flags, ScriptContext * scriptContext, EnumeratorCache * enumeratorCache);
340340

@@ -350,6 +350,7 @@ namespace Js
350350
static DynamicObject * BoxStackInstance(DynamicObject * instance, bool deepCopy);
351351

352352
private:
353+
353354
ArrayObject* EnsureObjectArray();
354355
ArrayObject* GetObjectArrayOrFlagsAsArray() const { return objectArray; }
355356

0 commit comments

Comments
 (0)