Fixing TTD regressions from JsObject* function refactor #4591
Conversation
|
what do you mean by "more nuance to supporting these methods"? Did these never work? |
| { | ||
| PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTHasOwnProperty, propertyRecord, object); |
There was a problem hiding this comment.
what was broken w/ having this here?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks Kyle, a few questions:
- What problem does this fix today? Is something broken?
- With this change, what are the implications on someone trying to use TTD?
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK, is there a bug opened to track getting this fixed? Do any existing tests fail with these asserts now?
There was a problem hiding this comment.
Answers to your questions:
- These were causing
vmmodule scenarios to fail in TTD, it regressed from v1.7.1 which was the last supported build with TTD. - 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.
- 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.
There was a problem hiding this comment.
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.
|
I've updated the commit message to better explain. These methods were refactored recently and regressed the cross-context TTD scenarios around them. |
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.
|
@obastemur Can you take a look? |
|
LGTM |
1 similar comment
|
LGTM |
…n 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.
…ject* 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.
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.