Merged PR 156729: Fixes asserts in Speedometer

Fixes asserts in Speedometer
- Some asserts in inline caches confused Null and Missing. CustomExternalWrapperObjects may produce the latter.
- "Missing" property results should not be cached by inline caches if comes from CEWO when implicit calls are disabled - that may change without type changes.
- rationalizing the marshaling story in CEWO. - The idea is to _not_ marshal on Get and the like, and really not be in marshalling business at the level of CEWO - it is responsibility of CrossSite context wrappers.
However - on Set/Define operations marshal the values to the script context of the containing CEWO.

The latter scenario arises when initialization is deferred and needs to be done in a context different from when the object was created. We just want to maintain that object and its property data have the same context - like if deferral never happened. Otherwise we have inconsistencies in what comes from inline caches via direct accesses to data and from actual property accesses (causes asserts, at least, perhaps worse).

Corresponding PR in Shim layer - https://devdiv.visualstudio.com/DevDiv/_git/ChakraCore/pullrequest/156729?_a=overview

Author: VSadov

lib/Runtime/Language/InlineCache.h

@@ -461,8 +461,8 @@ namespace Js
                 DebugOnly(Var getPropertyValue = JavascriptOperators::GetProperty(propertyObject, propertyId, requestContext));
                 Assert(*propertyValue == getPropertyValue ||
                     (VarIs<RootObjectBase>(propertyObject) && *propertyValue == JavascriptOperators::GetRootProperty(propertyObject, propertyId, requestContext))||
-                    // In some cases, such as CustomExternalObject, if implicit calls are disabled GetPropertyQuery may return null. See CustomExternalObject::GetPropertyQuery for an example.
-                    (getPropertyValue == requestContext->GetLibrary()->GetNull() && requestContext->GetThreadContext()->IsDisableImplicitCall() && propertyObject->GetType()->IsExternal()));
+                    // In some cases, such as CustomExternalObject, if implicit calls are disabled GetPropertyQuery may return missing. See CustomExternalWrapperObject::GetPropertyQuery for an example.
+                    (getPropertyValue == requestContext->GetMissingPropertyResult() && requestContext->GetThreadContext()->IsDisableImplicitCall() && propertyObject->GetType()->IsJsrtExternal()));
             }
         };
     };

lib/Runtime/Language/JavascriptOperators.cpp

@@ -1883,13 +1883,19 @@ using namespace Js;
     void JavascriptOperators::TryCacheMissingProperty(Var instance, Var cacheInstance, bool isRoot, PropertyId propertyId, ScriptContext* requestContext, _Inout_ PropertyValueInfo * info)
     {
         // Here, any well-behaved subclasses of DynamicObject can opt in to getting included in the missing property cache.
-        // For now, we only include basic objects and arrays. CustomExternalObject in particular is problematic because in
-        // some cases it can add new properties without transitioning its type handler.
+        // For now, we only include basic objects and arrays. 
         if (PHASE_OFF1(MissingPropertyCachePhase) || isRoot || !(DynamicObject::IsBaseDynamicObject(instance) || DynamicObject::IsAnyArray(instance)))
         {
             return;
         }
 
+        // CustomExternalObject in particular is problematic because in some cases it can report missing when implicit callsare disabled.
+        // See CustomExternalObject::GetPropertyQuery for an example.
+        if (UnsafeVarTo<DynamicObject>(instance)->GetType()->IsJsrtExternal() && requestContext->GetThreadContext()->IsDisableImplicitCall())
+        {
+            return;
+        }
+
         DynamicTypeHandler* handler = UnsafeVarTo<DynamicObject>(instance)->GetDynamicType()->GetTypeHandler();
 
         // Only cache missing property lookups for non-root field loads on objects that have PathTypeHandlers, because only these types have the right behavior

lib/Runtime/Library/CustomExternalWrapperObject.cpp

@@ -308,14 +308,17 @@ void CustomExternalWrapperObject::SetPrototype(RecyclableObject* newPrototype)
         return;
     }
 
+    // setting the value could be deferred and now we are on a different context from
+    // make sure the value has the same context as the containing object.
+    newPrototype = VarTo<RecyclableObject>(Js::CrossSite::MarshalVar(this->GetScriptContext(), newPrototype));
     DynamicObject::SetPrototype(newPrototype);
 }
 
 Js::Var CustomExternalWrapperObject::GetValueFromDescriptor(Js::Var instance, Js::PropertyDescriptor propertyDescriptor, Js::ScriptContext * requestContext)
 {
     if (propertyDescriptor.ValueSpecified())
     {
-        return Js::CrossSite::MarshalVar(requestContext, propertyDescriptor.GetValue());
+        return propertyDescriptor.GetValue();
     }
     if (propertyDescriptor.GetterSpecified())
     {
@@ -588,11 +591,20 @@ BOOL CustomExternalWrapperObject::DefineOwnPropertyDescriptor(Js::RecyclableObje
     Assert(!requestContext->IsHeapEnumInProgress());
     if (nullptr == defineOwnPropertyMethod)
     {
+        Js::PropertyDescriptor desc = descriptor;
+
+        if(desc.ValueSpecified())
+        {
+            // setting the value could be deferred and now we are on a different context from
+            // make sure the value has the same context as the containing object.
+            desc.SetValue(Js::CrossSite::MarshalVar(targetObj->GetScriptContext(), descriptor.GetValue()));
+        }
+
         PropertyDescriptor currentDescriptor;
         BOOL isCurrentDescriptorDefined = JavascriptOperators::GetOwnPropertyDescriptor(obj, propId, requestContext, &currentDescriptor);
 
         bool isExtensible = !!obj->IsExtensible();
-        return Js::JavascriptOperators::ValidateAndApplyPropertyDescriptor<true>(obj, propId, descriptor, isCurrentDescriptorDefined ? &currentDescriptor : nullptr, isExtensible, throwOnError, requestContext);
+        return Js::JavascriptOperators::ValidateAndApplyPropertyDescriptor<true>(obj, propId, desc, isCurrentDescriptorDefined ? &currentDescriptor : nullptr, isExtensible, throwOnError, requestContext);
     }
 
     Js::Var descVar = descriptor.GetOriginal();
@@ -651,6 +663,9 @@ BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyT
     };
     auto fn = [&]()->BOOL
     {
+        // setting the value could be deferred and now we are on a different context from
+        // make sure the value has the same context as the containing object.
+        newValue = Js::CrossSite::MarshalVar(this->GetScriptContext(), newValue);
         return DynamicObject::SetProperty(propertyNameString, newValue, propertyOperationFlags, nullptr);
     };
     return SetPropertyTrap(receiver, setPropertyTrapKind, getPropertyName, newValue, requestContext, propertyOperationFlags, FALSE, fn);
@@ -708,7 +723,6 @@ BOOL CustomExternalWrapperObject::SetPropertyTrap(Js::Var receiver, SetPropertyT
 Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyQuery(Js::Var originalInstance, Js::PropertyId propertyId, Js::Var* value, Js::PropertyValueInfo* info, Js::ScriptContext* requestContext)
 {
     if (!this->VerifyObjectAlive()) return Js::PropertyQueryFlags::Property_NotFound;
-    originalInstance = Js::CrossSite::MarshalVar(GetScriptContext(), originalInstance);
     Js::PropertyDescriptor result;
 
     auto fn = [&](Js::RecyclableObject* object)-> BOOL {
@@ -735,7 +749,6 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyQuery(Js::Var ori
 Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyQuery(Js::Var originalInstance, Js::JavascriptString* propertyNameString, Js::Var* value, Js::PropertyValueInfo* info, Js::ScriptContext* requestContext)
 {
     if (!this->VerifyObjectAlive()) return Js::PropertyQueryFlags::Property_NotFound;
-    originalInstance = Js::CrossSite::MarshalVar(GetScriptContext(), originalInstance);
     Js::PropertyDescriptor result;
 
     auto fn = [&](Js::RecyclableObject* object)-> BOOL {
@@ -766,7 +779,6 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyQuery(Js::Var ori
 Js::PropertyQueryFlags CustomExternalWrapperObject::GetPropertyReferenceQuery(Js::Var originalInstance, Js::PropertyId propertyId, Js::Var* value, Js::PropertyValueInfo* info, Js::ScriptContext* requestContext)
 {
     if (!this->VerifyObjectAlive()) return Js::PropertyQueryFlags::Property_NotFound;
-    originalInstance = Js::CrossSite::MarshalVar(GetScriptContext(), originalInstance);
     Js::PropertyDescriptor result;
 
     auto fn = [&](Js::RecyclableObject* object)-> BOOL {
@@ -826,7 +838,6 @@ BOOL CustomExternalWrapperObject::HasOwnItem(uint32 index)
 Js::PropertyQueryFlags CustomExternalWrapperObject::GetItemQuery(Js::Var originalInstance, uint32 index, Js::Var* value, Js::ScriptContext * requestContext)
 {
     if (!this->VerifyObjectAlive()) return Js::PropertyQueryFlags::Property_NotFound;
-    originalInstance = Js::CrossSite::MarshalVar(GetScriptContext(), originalInstance);
     Js::PropertyDescriptor result;
 
     auto fn = [&](Js::RecyclableObject* object)-> BOOL {
@@ -857,7 +868,6 @@ Js::PropertyQueryFlags CustomExternalWrapperObject::GetItemQuery(Js::Var origina
 Js::PropertyQueryFlags CustomExternalWrapperObject::GetItemReferenceQuery(Js::Var originalInstance, uint32 index, Js::Var* value, Js::ScriptContext * requestContext)
 {
     if (!this->VerifyObjectAlive()) return Js::PropertyQueryFlags::Property_NotFound;
-    originalInstance = Js::CrossSite::MarshalVar(GetScriptContext(), originalInstance);
 
     Js::PropertyDescriptor result;
     auto fn = [&](Js::RecyclableObject* object)-> BOOL {
@@ -898,11 +908,17 @@ BOOL CustomExternalWrapperObject::SetItem(uint32 index, Js::Var value, Js::Prope
 
     auto fn = [&]()->BOOL
     {
+        // setting the value could be deferred and now we are on a different context from
+        // make sure the value has the same context as the containing object.
+        value = Js::CrossSite::MarshalVar(this->GetScriptContext(), value);
         return DynamicObject::SetItem(index, value, flags);
     };
     BOOL trapResult = SetPropertyTrap(this, CustomExternalWrapperObject::SetPropertyTrapKind::SetItemKind, getPropertyName, value, GetScriptContext(), flags, FALSE, fn);
     if (!trapResult)
     {
+        // setting the value could be deferred and now we are on a different context from
+        // make sure the value has the same context as the containing object.
+        value = Js::CrossSite::MarshalVar(this->GetScriptContext(), value);
         return Js::DynamicObject::SetItem(index, value, flags);
     }
 
@@ -1020,6 +1036,7 @@ BOOL CustomExternalWrapperObject::GetEnumerator(Js::JavascriptStaticEnumerator *
                                 // if (desc.enumerable) yield key;
                                 if (desc.IsEnumerable())
                                 {
+                                    //TODO: (vsadov) not sure if should marshal here, it is "getting"
                                     return VarTo<JavascriptString>(CrossSite::MarshalVar(
                                         scriptContext, propertyName, propertyName->GetScriptContext()));
                                 }
@@ -1040,7 +1057,6 @@ BOOL CustomExternalWrapperObject::GetEnumerator(Js::JavascriptStaticEnumerator *
 BOOL CustomExternalWrapperObject::SetProperty(Js::PropertyId propertyId, Js::Var value, Js::PropertyOperationFlags flags, Js::PropertyValueInfo* info)
 {
     if (!this->VerifyObjectAlive()) return FALSE;
-    //value = Js::CrossSite::MarshalVar(GetScriptContext(), value);
     PROBE_STACK(GetScriptContext(), Js::Constants::MinStackDefault);
 
     auto getPropertyName = [&](Js::ScriptContext * requestContext, Js::Var * isPropertyNameNumeric, Js::Var * propertyNameNumericValue)->Js::Var
@@ -1050,6 +1066,9 @@ BOOL CustomExternalWrapperObject::SetProperty(Js::PropertyId propertyId, Js::Var
 
     auto fn = [&]()->BOOL
     {
+        // setting the value could be deferred and now we are on a different context from
+        // make sure the value has the same context as the containing object.
+        value = Js::CrossSite::MarshalVar(this->GetScriptContext(), value);
         return DynamicObject::SetProperty(propertyId, value, flags, info);
     };
 
@@ -1068,6 +1087,9 @@ BOOL CustomExternalWrapperObject::SetProperty(Js::JavascriptString* propertyName
 
     auto fn = [&]()->BOOL
     {
+        // setting the value could be deferred and now we are on a different context from
+        // make sure the value has the same context as the containing object.
+        value = Js::CrossSite::MarshalVar(this->GetScriptContext(), value);
         return DynamicObject::SetProperty(propertyNameString, value, flags, info);
     };
 
@@ -1101,6 +1123,9 @@ BOOL CustomExternalWrapperObject::InitProperty(PropertyId propertyId, Var value,
         return FALSE;
     }
 
+    // setting the value could be deferred and now we are on a different context from
+    // make sure the value has the same context as the containing object.
+    value = Js::CrossSite::MarshalVar(this->GetScriptContext(), value);
     return DynamicObject::InitProperty(propertyId, value, flags, info);
 }
 

lib/Runtime/Types/TypePropertyCache.cpp

@@ -302,7 +302,7 @@ namespace Js
                 DebugOnly(Var getPropertyValue = JavascriptOperators::GetProperty(propertyObject, propertyId, requestContext));
                 Assert(*propertyValue == getPropertyValue ||
                     // In some cases, such as CustomExternalObject, if implicit calls are disabled GetPropertyQuery may return null. See CustomExternalObject::GetPropertyQuery for an example.
-                    (getPropertyValue == requestContext->GetLibrary()->GetNull() && requestContext->GetThreadContext()->IsDisableImplicitCall() && propertyObject->GetType()->IsExternal()));
+                    (getPropertyValue == requestContext->GetMissingPropertyResult() && requestContext->GetThreadContext()->IsDisableImplicitCall() && propertyObject->GetType()->IsJsrtExternal()));
             }
 
             if(propertyObject->GetScriptContext() != requestContext)