diff --git a/API/hermes/SynthTrace.cpp b/API/hermes/SynthTrace.cpp index 1f3bcfbb94c..7dba9767e9e 100644 --- a/API/hermes/SynthTrace.cpp +++ b/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 { diff --git a/API/hermes/SynthTrace.h b/API/hermes/SynthTrace.h index cd3538bab20..46110d7e011 100644 --- a/API/hermes/SynthTrace.h +++ b/API/hermes/SynthTrace.h @@ -882,12 +882,13 @@ 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 propNamesID_; explicit GetPropertyNamesRecord( TimeSinceStart time, ObjectID objID, - ObjectID propNamesID) + std::optional propNamesID = std::nullopt) : Record(time), objID_(objID), propNamesID_(propNamesID) {} void toJSONInternal(::hermes::JSONEmitter &json) const override; @@ -895,7 +896,10 @@ class SynthTrace { return type; } std::vector defs() const override { - return {propNamesID_}; + if (!propNamesID_) { + return {}; + } + return {*propNamesID_}; } std::vector uses() const override { return {objID_}; diff --git a/API/hermes/SynthTraceParser.cpp b/API/hermes/SynthTraceParser.cpp index 5de1707d75d..5c4f32e17d6 100644 --- a/API/hermes/SynthTraceParser.cpp +++ b/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 propNamesId; + if (propNamesIDVal) { + propNamesId = getNumberAs(propNamesIDVal); + } trace.emplace_back( - timeFromStart, - objID->getValue(), - getNumberAs(obj->get("propNamesID"))); + timeFromStart, objID->getValue(), propNamesId); break; + } case RecordType::CreateArray: trace.emplace_back( timeFromStart, diff --git a/API/hermes/TraceInterpreter.cpp b/API/hermes/TraceInterpreter.cpp index 8883ee6e609..801afa03648 100644 --- a/API/hermes/TraceInterpreter.cpp +++ b/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: { diff --git a/API/hermes/TracingRuntime.cpp b/API/hermes/TracingRuntime.cpp index 02436c67584..12ce0b37c7f 100644 --- a/API/hermes/TracingRuntime.cpp +++ b/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( - getTimeSinceStart(), useObjectID(o), defObjectID(arr)); + getTimeSinceStart(), useObjectID(o)); + jsi::Array arr = RD::getPropertyNames(o); + trace_.emplace_back( + getTimeSinceStart(), SynthTrace::encodeObject(defObjectID(arr))); return arr; } diff --git a/unittests/API/SynthTraceSerializationTest.cpp b/unittests/API/SynthTraceSerializationTest.cpp index bf5a54cae23..51ae333d01c 100644 --- a/unittests/API/SynthTraceSerializationTest.cpp +++ b/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) { diff --git a/unittests/API/SynthTraceTest.cpp b/unittests/API/SynthTraceTest.cpp index b1e86a33f50..371627797db 100644 --- a/unittests/API/SynthTraceTest.cpp +++ b/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(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 getPropertyNames(jsi::Runtime &rt) override { + SynthTraceRuntimeTest::eval(rt, "++flag"); + return std::vector{}; + } + }; + + { + 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()); + 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) {