Separate getPropertyNames return record

Summary:
Original Author: mattblagden@meta.com
Original Git: b192d63
Original Reviewed By: neildhar
Original Revision: D59113879

Emit separate records for the invocation of getPropertyNames and its
return value, allowing other records to exist in between.
Keep the return value on the getPropertyNames record for now so old
traces can be used, but don't emit it in any new traces.

Reviewed By: neildhar

Differential Revision: D59403433

fbshipit-source-id: 092764bd01c14e842e24123ed2e6d0424203672c

API/hermes/SynthTrace.cpp

@@ -430,7 +430,6 @@ void SynthTrace::GetPropertyNamesRecord::toJSONInternal(
     JSONEmitter &json) const {
   Record::toJSONInternal(json);
   json.emitKeyValue("objID", objID_);
-  json.emitKeyValue("propNamesID", propNamesID_);
 }
 
 void SynthTrace::CreateArrayRecord::toJSONInternal(JSONEmitter &json) const {

API/hermes/SynthTrace.h

@@ -882,20 +882,24 @@ class SynthTrace {
     // Since getPropertyNames always returns an array, this can be an object id
     // rather than a TraceValue.
     /// The ObjectID of the array that was returned by getPropertyNames().
-    const ObjectID propNamesID_;
+    // TODO: delete once no traces contain this field
+    const std::optional<ObjectID> propNamesID_;
 
     explicit GetPropertyNamesRecord(
         TimeSinceStart time,
         ObjectID objID,
-        ObjectID propNamesID)
+        std::optional<ObjectID> propNamesID = std::nullopt)
         : Record(time), objID_(objID), propNamesID_(propNamesID) {}
 
     void toJSONInternal(::hermes::JSONEmitter &json) const override;
     RecordType getType() const override {
       return type;
     }
     std::vector<ObjectID> defs() const override {
-      return {propNamesID_};
+      if (!propNamesID_) {
+        return {};
+      }
+      return {*propNamesID_};
     }
     std::vector<ObjectID> uses() const override {
       return {objID_};

API/hermes/SynthTraceParser.cpp

@@ -434,12 +434,16 @@ SynthTrace getTrace(
 #endif
         );
         break;
-      case RecordType::GetPropertyNames:
+      case RecordType::GetPropertyNames: {
+        JSONValue *propNamesIDVal = obj->get("propNamesID");
+        std::optional<SynthTrace::ObjectID> propNamesId;
+        if (propNamesIDVal) {
+          propNamesId = getNumberAs<SynthTrace::ObjectID>(propNamesIDVal);
+        }
         trace.emplace_back<SynthTrace::GetPropertyNamesRecord>(
-            timeFromStart,
-            objID->getValue(),
-            getNumberAs<SynthTrace::ObjectID>(obj->get("propNamesID")));
+            timeFromStart, objID->getValue(), propNamesId);
         break;
+      }
       case RecordType::CreateArray:
         trace.emplace_back<SynthTrace::CreateArrayRecord>(
             timeFromStart,

API/hermes/TraceInterpreter.cpp

@@ -912,7 +912,12 @@ void TraceInterpreter::executeRecords() {
           jsi::Array arr = getJSIValueForUse(gpnr.objID_)
                                .asObject(rt_)
                                .getPropertyNames(rt_);
-          addToObjectMap(gpnr.propNamesID_, std::move(arr), currentExecIndex);
+          if (gpnr.propNamesID_) {
+            addToObjectMap(
+                *gpnr.propNamesID_, std::move(arr), currentExecIndex);
+          } else {
+            retval = std::move(arr);
+          }
           break;
         }
         case RecordType::CreateArray: {

API/hermes/TracingRuntime.cpp

@@ -627,9 +627,11 @@ void TracingRuntime::setPropertyValue(
 }
 
 jsi::Array TracingRuntime::getPropertyNames(const jsi::Object &o) {
-  jsi::Array arr = RD::getPropertyNames(o);
   trace_.emplace_back<SynthTrace::GetPropertyNamesRecord>(
-      getTimeSinceStart(), useObjectID(o), defObjectID(arr));
+      getTimeSinceStart(), useObjectID(o));
+  jsi::Array arr = RD::getPropertyNames(o);
+  trace_.emplace_back<SynthTrace::ReturnToNativeRecord>(
+      getTimeSinceStart(), SynthTrace::encodeObject(defObjectID(arr)));
   return arr;
 }
 

unittests/API/SynthTraceSerializationTest.cpp

@@ -188,8 +188,8 @@ TEST_F(SynthTraceSerializationTest, HasProperty) {
 
 TEST_F(SynthTraceSerializationTest, GetPropertyNames) {
   EXPECT_EQ(
-      R"({"type":"GetPropertyNamesRecord","time":0,"objID":1,"propNamesID":2})",
-      to_string(SynthTrace::GetPropertyNamesRecord(dummyTime, 1, 2)));
+      R"({"type":"GetPropertyNamesRecord","time":0,"objID":1})",
+      to_string(SynthTrace::GetPropertyNamesRecord(dummyTime, 1)));
 }
 
 TEST_F(SynthTraceSerializationTest, CreateArray) {

unittests/API/SynthTraceTest.cpp

@@ -483,12 +483,16 @@ TEST_F(SynthTraceTest, GetPropertyNames) {
     propNamesID = rt->useObjectID(names);
   }
   const auto &records = rt->trace().records();
-  EXPECT_EQ(2, records.size());
+  EXPECT_EQ(3, records.size());
   EXPECT_EQ_RECORD(
       SynthTrace::CreateObjectRecord(records[0]->time_, objID), *records[0]);
   EXPECT_EQ_RECORD(
-      SynthTrace::GetPropertyNamesRecord(records[1]->time_, objID, propNamesID),
+      SynthTrace::GetPropertyNamesRecord(records[1]->time_, objID),
       *records[1]);
+  EXPECT_EQ_RECORD(
+      SynthTrace::ReturnToNativeRecord(
+          records[2]->time_, SynthTrace::encodeObject(propNamesID)),
+      *records[2]);
 }
 
 TEST_F(SynthTraceTest, CreateArray) {
@@ -1231,7 +1235,7 @@ struct SynthTraceRuntimeTest : public ::testing::Test {
             std::make_unique<llvh::raw_string_ostream>(traceResult),
             /* forReplay */ false)) {}
 
-  jsi::Value eval(jsi::Runtime &rt, std::string code) {
+  static jsi::Value eval(jsi::Runtime &rt, std::string code) {
     return rt.global().getPropertyAsFunction(rt, "eval").call(rt, code);
   }
 };
@@ -1924,11 +1928,10 @@ TEST_F(SynthTraceReplayTest, HostFunctionAsGetter) {
         rt,
         jsi::PropNameID::forAscii(rt, "foo"),
         0,
-        [this](
-            jsi::Runtime &rt,
-            const jsi::Value &thisVal,
-            const jsi::Value *args,
-            size_t count) { return eval(rt, "++flag"); });
+        [](jsi::Runtime &rt,
+           const jsi::Value &thisVal,
+           const jsi::Value *args,
+           size_t count) { return eval(rt, "++flag"); });
 
     // Attach the host function to the property getter of an object
     auto obj = jsi::Object(rt);
@@ -1956,6 +1959,45 @@ TEST_F(SynthTraceReplayTest, HostFunctionAsGetter) {
   }
 }
 
+// This test exercises the case where getting property names triggers
+// other records to be emitted before returning the property names.
+TEST_F(SynthTraceReplayTest, HostFunctionAsGetPropertyNames) {
+  // Host object to carry out some actions that generate records when property
+  // names are requested.
+  class TestHostObject : public jsi::HostObject {
+   public:
+    TestHostObject() {}
+
+    std::vector<jsi::PropNameID> getPropertyNames(jsi::Runtime &rt) override {
+      SynthTraceRuntimeTest::eval(rt, "++flag");
+      return std::vector<jsi::PropNameID>{};
+    }
+  };
+
+  {
+    auto &rt = *traceRt;
+
+    // Setup a flag to ensure the host object's getPropertyNames() was invoked.
+    rt.global().setProperty(rt, "flag", 0);
+
+    // Get the property names.
+    auto obj = jsi::Object::createFromHostObject(
+        rt, std::make_shared<TestHostObject>());
+    rt.getPropertyNames(obj);
+    rt.getPropertyNames(obj);
+  }
+
+  // Ensure the records were emitted in the order that correctly represents
+  // the actual execution.
+  replay();
+
+  // Ensure the host object's getPropertyNames() was invoked twice.
+  {
+    auto &rt = *replayRt;
+    EXPECT_EQ(eval(rt, "flag").getNumber(), 2);
+  }
+}
+
 using JobQueueReplayTest = SynthTraceReplayTest;
 
 TEST_F(JobQueueReplayTest, DrainSingleMicrotask) {