Skip to content

Commit 730e580

Browse files
MikeHolmanMikeHolman
committed
[MERGE #5967 @MikeHolman] prevent caching interceptor results; don't assume implicit call
Merge pull request #5967 from MikeHolman:ceocacheissues This fixes a couple of problems that can cause inline caches to have wrong info for CustomExternalWrapperObjects
2 parents ff38436 + d9178e1 commit 730e580

File tree

2 files changed

+26
-86
lines changed

2 files changed

+26
-86
lines changed

‎lib/Runtime/Library/CustomExternalWrapperObject.cpp

Lines changed: 22 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -185,15 +185,6 @@ BOOL CustomExternalWrapperObject::VerifyObjectAlive()
185185

186186
BOOL CustomExternalWrapperObject::Equals(__in Var other, __out BOOL* value, ScriptContext* requestContext)
187187
{
188-
// Reject implicit call
189-
ThreadContext* threadContext = requestContext->GetThreadContext();
190-
if (threadContext->IsDisableImplicitCall())
191-
{
192-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
193-
*value = FALSE;
194-
return FALSE;
195-
}
196-
197188
// We need to implement comparison to other by reference in case the object
198189
// is in the left side of the comparison, and does not call a toString
199190
// method (like different objects) when compared to string. For Node, such
@@ -204,15 +195,6 @@ BOOL CustomExternalWrapperObject::Equals(__in Var other, __out BOOL* value, Scri
204195

205196
BOOL CustomExternalWrapperObject::StrictEquals(__in Var other, __out BOOL* value, ScriptContext* requestContext)
206197
{
207-
*value = FALSE;
208-
// Reject implicit call
209-
ThreadContext* threadContext = requestContext->GetThreadContext();
210-
if (threadContext->IsDisableImplicitCall())
211-
{
212-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
213-
return FALSE;
214-
}
215-
216198
// We need to implement comparison to other by reference in case the object
217199
// is in the left side of the comparison, and does not call a toString
218200
// method (like different objects) when compared to string. For Node, such
@@ -329,17 +311,11 @@ Js::Var CustomExternalWrapperObject::GetName(Js::ScriptContext* requestContext,
329311
}
330312

331313
template <class Fn, class GetPropertyNameFunc>
332-
BOOL CustomExternalWrapperObject::GetPropertyTrap(Js::Var instance, Js::PropertyDescriptor * propertyDescriptor, Fn fn, GetPropertyNameFunc getPropertyName, Js::ScriptContext * requestContext)
314+
BOOL CustomExternalWrapperObject::GetPropertyTrap(Js::Var instance, Js::PropertyDescriptor * propertyDescriptor, Fn fn, GetPropertyNameFunc getPropertyName, Js::ScriptContext * requestContext, Js::PropertyValueInfo* info)
333315
{
334316
PROBE_STACK(GetScriptContext(), Js::Constants::MinStackDefault);
335317

336-
// Reject implicit call
337318
ThreadContext* threadContext = requestContext->GetThreadContext();
338-
if (threadContext->IsDisableImplicitCall())
339-
{
340-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
341-
return FALSE;
342-
}
343319

344320
if (!this->EnsureInitialized(requestContext))
345321
{
@@ -363,6 +339,8 @@ BOOL CustomExternalWrapperObject::GetPropertyTrap(Js::Var instance, Js::Property
363339
Js::Var propertyNameNumericValue;
364340
Js::Var propertyName = getPropertyName(requestContext, &isPropertyNameNumeric, &propertyNameNumericValue);
365341

342+
PropertyValueInfo::SetNoCache(info, targetObj);
343+
PropertyValueInfo::DisablePrototypeCache(info, targetObj);
366344
Js::Var getGetResult = threadContext->ExecuteImplicitCall(getGetMethod, Js::ImplicitCall_Accessor, [=]()->Js::Var
367345
{
368346
return CALL_FUNCTION(threadContext, getGetMethod, Js::CallInfo(Js::CallFlags_Value, 4), targetObj, propertyName, isPropertyNameNumeric, propertyNameNumericValue);
@@ -379,17 +357,11 @@ BOOL CustomExternalWrapperObject::GetPropertyTrap(Js::Var instance, Js::Property
379357
}
380358

381359
template <class Fn, class GetPropertyNameFunc>
382-
BOOL CustomExternalWrapperObject::HasPropertyTrap(Fn fn, GetPropertyNameFunc getPropertyName)
360+
BOOL CustomExternalWrapperObject::HasPropertyTrap(Fn fn, GetPropertyNameFunc getPropertyName, Js::PropertyValueInfo* info)
383361
{
384362
PROBE_STACK(GetScriptContext(), Js::Constants::MinStackDefault);
385363

386-
// Reject implicit call
387364
ThreadContext* threadContext = GetScriptContext()->GetThreadContext();
388-
if (threadContext->IsDisableImplicitCall())
389-
{
390-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
391-
return FALSE;
392-
}
393365

394366
// Caller does not pass requestContext. Retrieve from host scriptContext stack.
395367
Js::ScriptContext* requestContext =
@@ -417,6 +389,8 @@ BOOL CustomExternalWrapperObject::HasPropertyTrap(Fn fn, GetPropertyNameFunc get
417389
Js::Var propertyNameNumericValue;
418390
Js::Var propertyName = getPropertyName(requestContext, &isPropertyNameNumeric, &propertyNameNumericValue);
419391

392+
PropertyValueInfo::SetNoCache(info, targetObj);
393+
PropertyValueInfo::DisablePrototypeCache(info, targetObj);
420394
Js::Var getHasResult = threadContext->ExecuteImplicitCall(hasMethod, Js::ImplicitCall_Accessor, [=]()->Js::Var
421395
{
422396
return CALL_FUNCTION(threadContext, hasMethod, Js::CallInfo(Js::CallFlags_Value, 4), targetObj, propertyName, isPropertyNameNumeric, propertyNameNumericValue);
@@ -435,20 +409,14 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::HasPropertyQuery(Js::Propert
435409
{
436410
return GetName(requestContext, propertyId, isPropertyNameNumeric, propertyNameNumericValue);
437411
};
438-
return Js::JavascriptConversion::BooleanToPropertyQueryFlags(HasPropertyTrap(fn, getPropertyName));
412+
return Js::JavascriptConversion::BooleanToPropertyQueryFlags(HasPropertyTrap(fn, getPropertyName, info));
439413
}
440414

441415
BOOL CustomExternalWrapperObject::GetPropertyDescriptorTrap(Js::PropertyId propertyId, Js::PropertyDescriptor* resultDescriptor, Js::ScriptContext* requestContext)
442416
{
443417
PROBE_STACK(GetScriptContext(), Js::Constants::MinStackDefault);
444418

445-
// Reject implicit call
446419
ThreadContext* threadContext = requestContext->GetThreadContext();
447-
if (threadContext->IsDisableImplicitCall())
448-
{
449-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
450-
return FALSE;
451-
}
452420

453421
if (!this->EnsureInitialized(requestContext))
454422
{
@@ -538,13 +506,7 @@ BOOL CustomExternalWrapperObject::DefineOwnPropertyDescriptor(Js::RecyclableObje
538506
{
539507
PROBE_STACK(requestContext, Js::Constants::MinStackDefault);
540508

541-
// Reject implicit call
542509
ThreadContext* threadContext = requestContext->GetThreadContext();
543-
if (threadContext->IsDisableImplicitCall())
544-
{
545-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
546-
return FALSE;
547-
}
548510

549511
CustomExternalWrapperObject * customObject = VarTo<CustomExternalWrapperObject>(obj);
550512

@@ -627,7 +589,7 @@ BOOL CustomExternalWrapperObject::DefineOwnPropertyDescriptor(Js::RecyclableObje
627589
return hasProperty;
628590
}
629591

630-
BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyTrapKind setPropertyTrapKind, Js::JavascriptString * propertyNameString, Js::Var newValue, Js::ScriptContext * requestContext, Js::PropertyOperationFlags propertyOperationFlags)
592+
BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyTrapKind setPropertyTrapKind, Js::JavascriptString * propertyNameString, Js::Var newValue, Js::ScriptContext * requestContext, Js::PropertyOperationFlags propertyOperationFlags, Js::PropertyValueInfo* info)
631593
{
632594
auto getPropertyName = [&](Js::ScriptContext * requestContext, Js::Var * isPropertyNameNumeric, Js::Var * propertyNameNumericValue)->Js::Var
633595
{
@@ -642,21 +604,15 @@ BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyT
642604
newValue = Js::CrossSite::MarshalVar(this->GetScriptContext(), newValue);
643605
return DynamicObject::SetProperty(propertyNameString, newValue, propertyOperationFlags, nullptr);
644606
};
645-
return SetPropertyTrap(receiver, setPropertyTrapKind, getPropertyName, newValue, requestContext, propertyOperationFlags, FALSE, fn);
607+
return SetPropertyTrap(receiver, setPropertyTrapKind, getPropertyName, newValue, requestContext, propertyOperationFlags, FALSE, fn, info);
646608
}
647609

648610
template <class Fn, class GetPropertyNameFunc>
649-
BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyTrapKind setPropertyTrapKind, GetPropertyNameFunc getPropertyName, Js::Var newValue, Js::ScriptContext * requestContext, Js::PropertyOperationFlags propertyOperationFlags, BOOL skipPrototypeCheck, Fn fn)
611+
BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyTrapKind setPropertyTrapKind, GetPropertyNameFunc getPropertyName, Js::Var newValue, Js::ScriptContext * requestContext, Js::PropertyOperationFlags propertyOperationFlags, BOOL skipPrototypeCheck, Fn fn, Js::PropertyValueInfo* info)
650612
{
651613
PROBE_STACK(GetScriptContext(), Js::Constants::MinStackDefault);
652614

653-
// Reject implicit call
654615
ThreadContext* threadContext = requestContext->GetThreadContext();
655-
if (threadContext->IsDisableImplicitCall())
656-
{
657-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
658-
return FALSE;
659-
}
660616

661617
if (!this->EnsureInitialized(requestContext))
662618
{
@@ -680,6 +636,8 @@ BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyT
680636
Js::Var propertyNameNumericValue;
681637
Js::Var propertyName = getPropertyName(requestContext, &isPropertyNameNumeric, &propertyNameNumericValue);
682638

639+
PropertyValueInfo::SetNoCache(info, targetObj);
640+
PropertyValueInfo::DisablePrototypeCache(info, targetObj);
683641
if (nullptr != setMethod)
684642
{
685643
Js::Var setPropertyResult = threadContext->ExecuteImplicitCall(setMethod, Js::ImplicitCall_Accessor, [=]()->Js::Var
@@ -708,7 +666,7 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyQuery(Js::Var ori
708666
{
709667
return GetName(requestContext, propertyId, isPropertyNameNumeric, propertyNameNumericValue);
710668
};
711-
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext);
669+
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext, info);
712670
if (!foundProperty)
713671
{
714672
*value = requestContext->GetMissingPropertyResult();
@@ -738,7 +696,7 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyQuery(Js::Var ori
738696
return GetName(requestContext, propertyRecord->GetPropertyId(), isPropertyNameNumeric, propertyNameNumericValue);
739697
};
740698

741-
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext);
699+
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext, info);
742700
if (!foundProperty)
743701
{
744702
*value = requestContext->GetMissingPropertyResult();
@@ -766,7 +724,7 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyReferenceQuery(Js
766724
return GetName(requestContext, propertyId, isPropertyNameNumeric, propertyNameNumericValue);
767725
};
768726

769-
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext);
727+
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext, info);
770728
if (!foundProperty)
771729
{
772730
*value = requestContext->GetMissingPropertyResult();
@@ -791,7 +749,7 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::HasItemQuery(uint32 index)
791749
return nullptr;
792750
};
793751

794-
return Js::JavascriptConversion::BooleanToPropertyQueryFlags(HasPropertyTrap(fn, getPropertyName));
752+
return Js::JavascriptConversion::BooleanToPropertyQueryFlags(HasPropertyTrap(fn, getPropertyName, nullptr));
795753
}
796754

797755
BOOL CustomExternalWrapperObject::HasOwnItem(uint32 index)
@@ -806,7 +764,7 @@ BOOL CustomExternalWrapperObject::HasOwnItem(uint32 index)
806764
return nullptr;
807765
};
808766

809-
return HasPropertyTrap(fn, getPropertyName);
767+
return HasPropertyTrap(fn, getPropertyName, nullptr);
810768
}
811769

812770
Js::PropertyQueryFlags CustomExternalWrapperObject::GetItemQuery(Js::Var originalInstance, uint32 index, Js::Var* value, Js::ScriptContext * requestContext)
@@ -827,7 +785,7 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetItemQuery(Js::Var origina
827785
return nullptr;
828786
};
829787

830-
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext);
788+
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext, nullptr);
831789
if (!foundProperty)
832790
{
833791
*value = requestContext->GetMissingItemResult();
@@ -857,7 +815,7 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetItemReferenceQuery(Js::Va
857815
return nullptr;
858816
};
859817

860-
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext);
818+
BOOL foundProperty = GetPropertyTrap(originalInstance, &result, fn, getPropertyName, requestContext, nullptr);
861819
if (!foundProperty)
862820
{
863821
*value = requestContext->GetMissingItemResult();
@@ -887,7 +845,7 @@ BOOL CustomExternalWrapperObject::SetItem(uint32 index, Js::Var value, Js::Prope
887845
value = Js::CrossSite::MarshalVar(this->GetScriptContext(), value);
888846
return DynamicObject::SetItem(index, value, flags);
889847
};
890-
BOOL trapResult = SetPropertyTrap(this, CustomExternalWrapperObject::SetPropertyTrapKind::SetItemKind, getPropertyName, value, GetScriptContext(), flags, FALSE, fn);
848+
BOOL trapResult = SetPropertyTrap(this, CustomExternalWrapperObject::SetPropertyTrapKind::SetItemKind, getPropertyName, value, GetScriptContext(), flags, FALSE, fn, nullptr);
891849
if (!trapResult)
892850
{
893851
// setting the value could be deferred and now we are on a different context from
@@ -910,13 +868,7 @@ BOOL CustomExternalWrapperObject::DeleteItem(uint32 index, Js::PropertyOperation
910868
BOOL CustomExternalWrapperObject::GetEnumerator(Js::JavascriptStaticEnumerator * enumerator, Js::EnumeratorFlags flags, Js::ScriptContext* requestContext, Js::EnumeratorCache * enumeratorCache)
911869
{
912870
if (!this->VerifyObjectAlive()) return FALSE;
913-
// Reject implicit call
914871
ThreadContext* threadContext = requestContext->GetThreadContext();
915-
if (threadContext->IsDisableImplicitCall())
916-
{
917-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
918-
return FALSE;
919-
}
920872

921873
if (!this->EnsureInitialized(requestContext))
922874
{
@@ -1046,7 +998,7 @@ BOOL CustomExternalWrapperObject::SetProperty(Js::PropertyId propertyId, Js::Var
1046998
return DynamicObject::SetProperty(propertyId, value, flags, info);
1047999
};
10481000

1049-
return SetPropertyTrap(this, CustomExternalWrapperObject::SetPropertyTrapKind::SetPropertyKind, getPropertyName, value, GetScriptContext(), flags, FALSE, fn);
1001+
return SetPropertyTrap(this, CustomExternalWrapperObject::SetPropertyTrapKind::SetPropertyKind, getPropertyName, value, GetScriptContext(), flags, FALSE, fn, info);
10501002
}
10511003

10521004
BOOL CustomExternalWrapperObject::SetProperty(Js::JavascriptString* propertyNameString, Js::Var value, Js::PropertyOperationFlags flags, Js::PropertyValueInfo* info)
@@ -1067,7 +1019,7 @@ BOOL CustomExternalWrapperObject::SetProperty(Js::JavascriptString* propertyName
10671019
return DynamicObject::SetProperty(propertyNameString, value, flags, info);
10681020
};
10691021

1070-
return SetPropertyTrap(this, CustomExternalWrapperObject::SetPropertyTrapKind::SetPropertyKind, getPropertyName, value, GetScriptContext(), flags, FALSE, fn);
1022+
return SetPropertyTrap(this, CustomExternalWrapperObject::SetPropertyTrapKind::SetPropertyKind, getPropertyName, value, GetScriptContext(), flags, FALSE, fn, info);
10711023
}
10721024

10731025
BOOL CustomExternalWrapperObject::EnsureNoRedeclProperty(Js::PropertyId propertyId)
@@ -1080,13 +1032,7 @@ BOOL CustomExternalWrapperObject::InitProperty(PropertyId propertyId, Var value,
10801032
{
10811033
if (!this->VerifyObjectAlive()) return FALSE;
10821034

1083-
// Reject implicit call
10841035
ThreadContext* threadContext = GetScriptContext()->GetThreadContext();
1085-
if (threadContext->IsDisableImplicitCall())
1086-
{
1087-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
1088-
return FALSE;
1089-
}
10901036

10911037
// Caller does not pass requestContext. Retrieve from host scriptContext stack.
10921038
Js::ScriptContext* requestContext =
@@ -1109,13 +1055,7 @@ BOOL CustomExternalWrapperObject::DeleteProperty(Js::PropertyId propertyId, Js::
11091055

11101056
PROBE_STACK(GetScriptContext(), Js::Constants::MinStackDefault);
11111057

1112-
// Reject implicit call
11131058
ThreadContext* threadContext = GetScriptContext()->GetThreadContext();
1114-
if (threadContext->IsDisableImplicitCall())
1115-
{
1116-
threadContext->AddImplicitCallFlags(Js::ImplicitCall_External);
1117-
return FALSE;
1118-
}
11191059

11201060
// Caller does not pass requestContext. Retrieve from host scriptContext stack.
11211061
Js::ScriptContext* requestContext =

‎lib/Runtime/Library/CustomExternalWrapperObject.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,8 @@ namespace Js
110110
void PropertyIdFromInt(uint32 index, PropertyRecord const** propertyRecord);
111111

112112
template <class Fn, class GetPropertyNameFunc>
113-
BOOL SetPropertyTrap(Var receiver, SetPropertyTrapKind setPropertyTrapKind, GetPropertyNameFunc getPropertyName, Var newValue, ScriptContext * requestContext, PropertyOperationFlags propertyOperationFlags, BOOL skipPrototypeCheck, Fn fn);
114-
BOOL SetPropertyTrap(Var receiver, SetPropertyTrapKind setPropertyTrapKind, JavascriptString * propertyString, Var newValue, ScriptContext * requestContext, PropertyOperationFlags propertyOperationFlags);
113+
BOOL SetPropertyTrap(Var receiver, SetPropertyTrapKind setPropertyTrapKind, GetPropertyNameFunc getPropertyName, Var newValue, ScriptContext * requestContext, PropertyOperationFlags propertyOperationFlags, BOOL skipPrototypeCheck, Fn fn, Js::PropertyValueInfo* info);
114+
BOOL SetPropertyTrap(Var receiver, SetPropertyTrapKind setPropertyTrapKind, JavascriptString * propertyString, Var newValue, ScriptContext * requestContext, PropertyOperationFlags propertyOperationFlags, Js::PropertyValueInfo* info);
115115

116116
void * GetSlotData() const;
117117
void SetSlotData(void * data);
@@ -173,10 +173,10 @@ namespace Js
173173
BOOL GetPropertyDescriptorTrap(PropertyId propertyId, PropertyDescriptor * resultDescriptor, ScriptContext * requestContext);
174174

175175
template <class Fn, class GetPropertyNameFunc>
176-
BOOL GetPropertyTrap(Var instance, PropertyDescriptor * propertyDescriptor, Fn fn, GetPropertyNameFunc getPropertyName, ScriptContext * requestContext);
176+
BOOL GetPropertyTrap(Var instance, PropertyDescriptor * propertyDescriptor, Fn fn, GetPropertyNameFunc getPropertyName, ScriptContext * requestContext, Js::PropertyValueInfo* info);
177177

178178
template <class Fn, class GetPropertyNameFunc>
179-
BOOL HasPropertyTrap(Fn fn, GetPropertyNameFunc getPropertyName);
179+
BOOL HasPropertyTrap(Fn fn, GetPropertyNameFunc getPropertyName, Js::PropertyValueInfo* info);
180180

181181
template <class Fn>
182182
void GetOwnPropertyKeysHelper(ScriptContext * scriptContext, RecyclableObject * trapResultArray, uint32 len, JavascriptArray * trapResult,

0 commit comments

Comments
 (0)