[MERGE #4591 @kfarnung] Fixing TTD regressions from JsObject* function refactor

Merge pull request #4591 from kfarnung:ttdobjects

When the JsObject* functions were added to JSRT there was an attempt
to refactor the TTD instrumentation into a common method. The
refactor caused a regression in cross-context scenarios where the
record was capturing both the result of the function as well as any
marshalling necessary to get the result.

This change reverts the behavior of the existing methods (e.g.
JsSetProperty and friends) to capture the record before any marshalling
can occur (VALIDATE_INCOMING_OBJECT will marshal the object if
necessary). For the new JsObject* functions I've added an assert to
ensure that once they are used they will cause TTD record to fail with
an actionable message.

Author: kfarnung

lib/Jsrt/Jsrt.cpp

@@ -1422,11 +1422,8 @@ CHAKRA_API JsPreventExtension(_In_ JsValueRef object)
 }
 
 CHAKRA_API JsHasOwnPropertyCommon(Js::ScriptContext * scriptContext, _In_ JsValueRef object,
-    _In_ const Js::PropertyRecord * propertyRecord, _Out_ bool *hasOwnProperty,
-    TTDRecorder& _actionEntryPopper)
+    _In_ const Js::PropertyRecord * propertyRecord, _Out_ bool *hasOwnProperty)
 {
-    PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTHasOwnProperty, propertyRecord, object);
-
     *hasOwnProperty = Js::JavascriptOperators::OP_HasOwnProperty(object,
         propertyRecord->GetPropertyId(), scriptContext) != 0;
 
@@ -1438,14 +1435,15 @@ CHAKRA_API JsHasOwnProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propert
 {
     return ContextAPIWrapper<true>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTHasOwnProperty, (const Js::PropertyRecord *)propertyId, object);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_PROPERTYID(propertyId);
         PARAM_NOT_NULL(hasOwnProperty);
         *hasOwnProperty = false;
 
         return JsHasOwnPropertyCommon(scriptContext, object,
-            (const Js::PropertyRecord *)propertyId, hasOwnProperty, _actionEntryPopper);
+            (const Js::PropertyRecord *)propertyId, hasOwnProperty);
     });
 }
 
@@ -1476,6 +1474,7 @@ CHAKRA_API JsObjectHasOwnProperty(_In_ JsValueRef object, _In_ JsValueRef proper
 {
     return ContextAPIWrapper<true>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_RECYCLABLE(propertyId, scriptContext);
@@ -1491,40 +1490,41 @@ CHAKRA_API JsObjectHasOwnProperty(_In_ JsValueRef object, _In_ JsValueRef proper
             return errorValue;
         }
 
-        return JsHasOwnPropertyCommon(scriptContext, object, propertyRecord, hasOwnProperty, _actionEntryPopper);
+        return JsHasOwnPropertyCommon(scriptContext, object, propertyRecord, hasOwnProperty);
     });
 }
 #endif
 
 static JsErrorCode JsGetPropertyCommon(Js::ScriptContext * scriptContext,
     _In_ Js::RecyclableObject * object,
-    _In_ const Js::PropertyRecord * propertyRecord, _Out_ JsValueRef *value,
-    TTDRecorder& _actionEntryPopper)
+    _In_ const Js::PropertyRecord * propertyRecord, _Out_ JsValueRef *value)
 {
     AssertMsg(scriptContext->GetThreadContext()->IsScriptActive(), "Caller is expected to be under ContextAPIWrapper!");
-    PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTGetProperty, propertyRecord, object);
 
     *value = Js::JavascriptOperators::GetPropertyNoCache(object, propertyRecord->GetPropertyId(), scriptContext);
     Assert(*value == nullptr || !Js::CrossSite::NeedMarshalVar(*value, scriptContext));
 
-    PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, value);
-
     return JsNoError;
 }
 
 CHAKRA_API JsGetProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId, _Out_ JsValueRef *value)
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTGetProperty, (const Js::PropertyRecord *)propertyId, object);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_PROPERTYID(propertyId);
         PARAM_NOT_NULL(value);
         *value = nullptr;
 
         Js::RecyclableObject * instance = Js::RecyclableObject::FromVar(object);
-        return JsGetPropertyCommon(scriptContext, instance, (const Js::PropertyRecord *)propertyId,
-             value, _actionEntryPopper);
+        JsErrorCode err = JsGetPropertyCommon(scriptContext, instance, (const Js::PropertyRecord *)propertyId,
+             value);
+
+        PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, value);
+
+        return err;
     });
 }
 
@@ -1533,6 +1533,7 @@ CHAKRA_API JsObjectGetProperty(_In_ JsValueRef object, _In_ JsValueRef propertyI
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_RECYCLABLE(propertyId, scriptContext);
@@ -1551,17 +1552,15 @@ CHAKRA_API JsObjectGetProperty(_In_ JsValueRef object, _In_ JsValueRef propertyI
         Assert(propertyRecord != nullptr);
 
         Js::RecyclableObject * instance = Js::RecyclableObject::FromVar(object);
-        return JsGetPropertyCommon(scriptContext, instance, propertyRecord, value, _actionEntryPopper);
+        return JsGetPropertyCommon(scriptContext, instance, propertyRecord, value);
     });
 }
 #endif
 
 static JsErrorCode JsGetOwnPropertyDescriptorCommon(Js::ScriptContext * scriptContext,
-    _In_ JsValueRef object, _In_ const Js::PropertyRecord * propertyRecord, _Out_ JsValueRef *propertyDescriptor,
-    TTDRecorder& _actionEntryPopper)
+    _In_ JsValueRef object, _In_ const Js::PropertyRecord * propertyRecord, _Out_ JsValueRef *propertyDescriptor)
 {
     AssertMsg(scriptContext->GetThreadContext()->IsScriptActive(), "Caller is expected to be under ContextAPIWrapper!");
-    PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTGetOwnPropertyInfo, propertyRecord, object);
 
     Js::PropertyDescriptor propertyDescriptorValue;
     if (Js::JavascriptOperators::GetOwnPropertyDescriptor(Js::RecyclableObject::FromVar(object),
@@ -1575,21 +1574,25 @@ static JsErrorCode JsGetOwnPropertyDescriptorCommon(Js::ScriptContext * scriptCo
     }
     Assert(*propertyDescriptor == nullptr || !Js::CrossSite::NeedMarshalVar(*propertyDescriptor, scriptContext));
 
-    PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, propertyDescriptor);
-
     return JsNoError;
 }
 
 CHAKRA_API JsGetOwnPropertyDescriptor(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId, _Out_ JsValueRef *propertyDescriptor)
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTGetOwnPropertyInfo, (const Js::PropertyRecord *)propertyId, object);
+
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_PROPERTYID(propertyId);
         PARAM_NOT_NULL(propertyDescriptor);
         *propertyDescriptor = nullptr;
 
-        return JsGetOwnPropertyDescriptorCommon(scriptContext, object, (const Js::PropertyRecord *)propertyId,
-            propertyDescriptor, _actionEntryPopper);
+        JsErrorCode err = JsGetOwnPropertyDescriptorCommon(scriptContext, object, (const Js::PropertyRecord *)propertyId,
+            propertyDescriptor);
+
+        PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, propertyDescriptor);
+
+        return err;
     });
 }
 
@@ -1598,6 +1601,7 @@ CHAKRA_API JsObjectGetOwnPropertyDescriptor(_In_ JsValueRef object, _In_ JsValue
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_RECYCLABLE(propertyId, scriptContext);
@@ -1615,18 +1619,15 @@ CHAKRA_API JsObjectGetOwnPropertyDescriptor(_In_ JsValueRef object, _In_ JsValue
 
         Assert(propertyRecord != nullptr);
 
-        return JsGetOwnPropertyDescriptorCommon(scriptContext, object, propertyRecord, propertyDescriptor, _actionEntryPopper);
+        return JsGetOwnPropertyDescriptorCommon(scriptContext, object, propertyRecord, propertyDescriptor);
     });
 }
 #endif
 
 static JsErrorCode JsSetPropertyCommon(Js::ScriptContext * scriptContext, _In_ JsValueRef object,
-    _In_ const Js::PropertyRecord * propertyRecord, _In_ JsValueRef value, _In_ bool useStrictRules,
-    TTDRecorder& _actionEntryPopper)
+    _In_ const Js::PropertyRecord * propertyRecord, _In_ JsValueRef value, _In_ bool useStrictRules)
 {
     AssertMsg(scriptContext->GetThreadContext()->IsScriptActive(), "Caller is expected to be under ContextAPIWrapper!");
-    PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTSetProperty, object,
-        propertyRecord, value, useStrictRules);
 
     Js::JavascriptOperators::OP_SetProperty(object, propertyRecord->GetPropertyId(),
         value, scriptContext, nullptr, useStrictRules ? Js::PropertyOperation_StrictMode : Js::PropertyOperation_None);
@@ -1638,13 +1639,14 @@ CHAKRA_API JsSetProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTSetProperty, object, (const Js::PropertyRecord *)propertyId, value, useStrictRules);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_PROPERTYID(propertyId);
         VALIDATE_INCOMING_REFERENCE(value, scriptContext);
 
         return JsSetPropertyCommon(scriptContext, object, (const Js::PropertyRecord *)propertyId,
-            value, useStrictRules, _actionEntryPopper);
+            value, useStrictRules);
     });
 }
 
@@ -1653,6 +1655,7 @@ CHAKRA_API JsObjectSetProperty(_In_ JsValueRef object, _In_ JsValueRef propertyI
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_RECYCLABLE(propertyId, scriptContext);
@@ -1669,7 +1672,7 @@ CHAKRA_API JsObjectSetProperty(_In_ JsValueRef object, _In_ JsValueRef propertyI
 
         Assert(propertyRecord != nullptr);
 
-        return JsSetPropertyCommon(scriptContext, object, propertyRecord, value, useStrictRules, _actionEntryPopper);
+        return JsSetPropertyCommon(scriptContext, object, propertyRecord, value, useStrictRules);
     });
 }
 #endif
@@ -1718,6 +1721,7 @@ CHAKRA_API JsObjectHasProperty(_In_ JsValueRef object, _In_ JsValueRef propertyI
     if (!Js::JavascriptOperators::IsObject(object)) return JsErrorArgumentNotObject;
 
     auto internalHasProperty = [&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_RECYCLABLE(propertyId, scriptContext);
         PARAM_NOT_NULL(hasProperty);
@@ -1732,8 +1736,6 @@ CHAKRA_API JsObjectHasProperty(_In_ JsValueRef object, _In_ JsValueRef propertyI
             return errorValue;
         }
 
-        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTHasProperty, propertyRecord, object);
-
         Js::RecyclableObject * instance = Js::RecyclableObject::FromVar(object);
         *hasProperty = Js::JavascriptOperators::HasProperty(instance, propertyRecord->GetPropertyId()) != 0;
 
@@ -1760,21 +1762,16 @@ CHAKRA_API JsObjectHasProperty(_In_ JsValueRef object, _In_ JsValueRef propertyI
 #endif
 
 static JsErrorCode JsDeletePropertyCommon(Js::ScriptContext * scriptContext, _In_ JsValueRef object,
-    _In_ const Js::PropertyRecord * propertyRecord, _In_ bool useStrictRules, _Out_ JsValueRef *result,
-    TTDRecorder& _actionEntryPopper)
+    _In_ const Js::PropertyRecord * propertyRecord, _In_ bool useStrictRules, _Out_ JsValueRef *result)
 {
     AssertMsg(scriptContext->GetThreadContext()->IsScriptActive(), "Caller is expected to be under ContextAPIWrapper!");
-    PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTDeleteProperty, object,
-        propertyRecord, useStrictRules);
 
     *result = Js::JavascriptOperators::OP_DeleteProperty((Js::Var)object,
         propertyRecord->GetPropertyId(),
         scriptContext, useStrictRules ? Js::PropertyOperation_StrictMode : Js::PropertyOperation_None);
 
     Assert(*result == nullptr || !Js::CrossSite::NeedMarshalVar(*result, scriptContext));
 
-    PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, result);
-
     return JsNoError;
 }
 
@@ -1783,14 +1780,19 @@ CHAKRA_API JsDeleteProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propert
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTDeleteProperty, object, (const Js::PropertyRecord *)propertyId, useStrictRules);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_PROPERTYID(propertyId);
         PARAM_NOT_NULL(result);
         *result = nullptr;
 
-        return JsDeletePropertyCommon(scriptContext, object, (const Js::PropertyRecord *)propertyId,
-            useStrictRules, result, _actionEntryPopper);
+        JsErrorCode err = JsDeletePropertyCommon(scriptContext, object, (const Js::PropertyRecord *)propertyId,
+            useStrictRules, result);
+
+        PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, result);
+
+        return err;
     });
 }
 
@@ -1800,6 +1802,7 @@ CHAKRA_API JsObjectDeleteProperty(_In_ JsValueRef object, _In_ JsValueRef proper
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_RECYCLABLE(propertyId, scriptContext);
@@ -1818,18 +1821,16 @@ CHAKRA_API JsObjectDeleteProperty(_In_ JsValueRef object, _In_ JsValueRef proper
         Assert(propertyRecord != nullptr);
 
         return JsDeletePropertyCommon(scriptContext, object, propertyRecord,
-            useStrictRules, result, _actionEntryPopper);
+            useStrictRules, result);
     });
 }
 #endif
 
 static JsErrorCode JsDefinePropertyCommon(Js::ScriptContext * scriptContext, _In_ JsValueRef object,
     _In_ const Js::PropertyRecord *propertyRecord, _In_ JsValueRef propertyDescriptor,
-    _Out_ bool *result, TTDRecorder& _actionEntryPopper)
+    _Out_ bool *result)
 {
     AssertMsg(scriptContext->GetThreadContext()->IsScriptActive(), "Caller is expected to be under ContextAPIWrapper!");
-    PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTDefineProperty, object,
-        propertyRecord, propertyDescriptor);
 
     Js::PropertyDescriptor propertyDescriptorValue;
     if (!Js::JavascriptOperators::ToPropertyDescriptor(propertyDescriptor, &propertyDescriptorValue, scriptContext))
@@ -1849,6 +1850,7 @@ CHAKRA_API JsDefineProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propert
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTDefineProperty, object, (const Js::PropertyRecord *)propertyId, propertyDescriptor);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_PROPERTYID(propertyId);
@@ -1857,7 +1859,7 @@ CHAKRA_API JsDefineProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propert
         *result = false;
 
         return JsDefinePropertyCommon(scriptContext, object, (const Js::PropertyRecord *)propertyId,
-            propertyDescriptor, result, _actionEntryPopper);
+            propertyDescriptor, result);
     });
 }
 
@@ -1867,6 +1869,7 @@ CHAKRA_API JsObjectDefineProperty(_In_ JsValueRef object, _In_ JsValueRef proper
 {
     return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext,
         TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+        PERFORM_JSRT_TTD_RECORD_ACTION_NOT_IMPLEMENTED(scriptContext);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
         VALIDATE_INCOMING_RECYCLABLE(propertyId, scriptContext);
@@ -1883,7 +1886,7 @@ CHAKRA_API JsObjectDefineProperty(_In_ JsValueRef object, _In_ JsValueRef proper
             return errorValue;
         }
 
-        return JsDefinePropertyCommon(scriptContext, object, propertyRecord, propertyDescriptor, result, _actionEntryPopper);
+        return JsDefinePropertyCommon(scriptContext, object, propertyRecord, propertyDescriptor, result);
     });
 }
 #endif