Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing TTD regressions from JsObject* function refactor #4591

Merged
merged 1 commit into from
Jan 23, 2018
Merged

Fixing TTD regressions from JsObject* function refactor #4591

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 47 additions & 44 deletions lib/Jsrt/Jsrt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was broken w/ having this here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concern here is that every caller of JsHasOwnPropertyCommon will now have to "do the right thing" for TTD, namely either assert of record. Is there some way to have the right logic consolidated in JsHasOwnPropertyCommaon , (record if we can, assert if not), independent of caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any instances of VALIDATE_INCOMING_OBJECT can also marshal the object to the current context. The record actions need to be the value prior any of these calls to capture the original pointer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kyle, a few questions:

  1. What problem does this fix today? Is something broken?
  2. With this change, what are the implications on someone trying to use TTD?
  3. Is it possible to capture the value before VALIDATE_INCOMING_OBJECT, and include that as a param into the *Common* methods? This way, the right value will get recorded, and the API signature provides some assurance that any future callers will "do the right thing".

LMK if I'm mis-understanding something here. Happy to talk in on phone or in office.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attempted to update the commit description again:

Any instances of VALIDATE_INCOMING_OBJECT can also marshal the object
to the current context. The record actions need to be the value prior
any of these calls to capture the original pointer.

These methods were recently added and in the process regressed the
existing methods. Partially backing out the change so we can properly
address the TTD implementation for these methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, is there a bug opened to track getting this fixed? Do any existing tests fail with these asserts now?

Copy link
Contributor Author

@kfarnung kfarnung Jan 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answers to your questions:

  1. These were causing vm module scenarios to fail in TTD, it regressed from v1.7.1 which was the last supported build with TTD.
  2. It causes the record to fail if those JsObject* functions are called from a native module. They are basically just fast-path implementations and introduced later in the v1.7.* cycle, so they aren't currently used in node-chakracore.
  3. The real fix is not that simple, the record will need to be different for the JsObject* calls because they need to capture the JsValueRef during record and generate a JsPropertyIdRef during replay. This also requires that we introduce some new logic during replay so it's not a trivial fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarified this offline with @kfarnung. This change fixes a regression in TTD that was causing some internal tests to fail. The new "assert" code paths aren't hit in these tests.


*hasOwnProperty = Js::JavascriptOperators::OP_HasOwnProperty(object,
propertyRecord->GetPropertyId(), scriptContext) != 0;

Expand All @@ -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);
});
}

Expand Down Expand Up @@ -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);
Expand All @@ -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;
});
}

Expand All @@ -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);
Expand All @@ -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),
Expand All @@ -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;
});
}

Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
});
}

Expand All @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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;

Expand All @@ -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;
}

Expand All @@ -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;
});
}

Expand All @@ -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);
Expand All @@ -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))
Expand All @@ -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);
Expand All @@ -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);
});
}

Expand All @@ -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);
Expand All @@ -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
Expand Down