Skip to content

feat: WHATWG performance API (hr-time, user timing, performance timeline) - #2001

Open
edusperoni wants to merge 3 commits into
mainfrom
feat/performance
Open

feat: WHATWG performance API (hr-time, user timing, performance timeline)#2001
edusperoni wants to merge 3 commits into
mainfrom
feat/performance

Conversation

@edusperoni

@edusperoni edusperoni commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Description

Replaces the bare native {now, timeOrigin} object with a spec-shaped implementation of hr-time, User Timing Level 3 and the performance timeline with PerformanceObserver. performance, Performance, PerformanceEntry, PerformanceMark, PerformanceMeasure, PerformanceObserver and PerformanceObserverEntryList are globals in main and worker isolates alike, with Performance extends EventTarget and WebIDL-shaped descriptors/brands.

Stacked on #2000 — review only the last two commits.

Architecture

  • All spec logic lives in the internal/performance.js builtin. The native side hands it exactly two things — binding.now() (double ms, monotonic) and binding.timeOrigin (wall-clock ms) — so the file is shared verbatim with the iOS runtime (it differs from iOS's copy by two comment lines only, both naming the Android init entry point).
  • tns::Performance native shim (Performance.h/.cpp) runs post-context in PrepareV8Runtime, after Events::Init/ErrorEvents::Init/StructuredClone::Init. Performance::NowMillis(isolate) is the single native clock hook: future requestAnimationFrame work must read the clock through it (V8 platform monotonic clock, CLOCK_MONOTONIC base) so every JS-visible timestamp shares performance.timeOrigin.
  • Per-worker time origins: origins stay captured per Runtime right after Isolate::New (members renamed to m_timeOriginMonotonic/m_timeOriginRealtimeMs, with public PerformanceNowMillis()/TimeOriginMillis() accessors replacing the private PerformanceNowCallback), so each worker keeps its own timeOrigin — asserted by the worker specs.
  • performance.now() keeps full double precision (no coarsening, Node's choice).
  • New primordials: SymbolToStringTag, NumberIsFinite, ArrayPrototypeSort (plus the matching Number.isFinite entry in the ESLint capturedStatics rule).

Frame callbacks are the first consumer (2nd commit)

The clock hook exists so JS-visible timestamps share one base, so this PR wires up the runtime's existing frame callbacks rather than leaving the hook unused.

  • Two-argument contract: __postFrameCallback now calls back with (frameTimeNanos, performanceMillis). The first argument is unchanged — raw CLOCK_MONOTONIC nanos, which @nativescript/core divides by 1e6 — so this is additive and nothing breaks; the second is the same instant on the performance timeline, comparable with performance.now(). Core can migrate to the second argument at its own pace.
  • Exact, not resampled: Choreographer stamps frames on the very clock the time origin is captured on, so Performance::MonotonicNanosToTimelineMillis() just subtracts the new Runtime::TimeOriginMonotonicMillis(). Verified on device: the origin implied by the two arguments matches the one implied by (System.nanoTime(), performance.now()).
  • The whole minSdk range is covered now. AChoreographer only exists from API 24, so on API 21–23 __postFrameCallback existed but silently never fired. The machinery moves out of CallbackHandlers into FrameCallbacks.{h,cpp}, and API 21–23 goes through android.view.Choreographer via a new com.tns.FrameCallbacks (same entry, same two arguments, per-thread so workers schedule against their own looper).
  • Cleanups carried by the move: entries live behind unique_ptr so both implementations can hand the platform a stable pointer; isolate teardown no longer erases while iterating; and the pre-API-29 AChoreographer entry point's frame time is widened to 64 bits, which it is not on the 32-bit ABIs (the reason postFrameCallback64 exists).
  • Debug runtimes expose __setFrameCallbackImpl (absent from release) so the Java bridge is selectable on a modern device — otherwise it would ship with no device coverage.

detail is structured-cloned

Building on #2000: mark/measure detail is cloned once at entry creation through the structuredClone global, so entries hold snapshots and an uncloneable detail throws the DataCloneError-named error. The builtin keeps an identity fallback so the file stays portable to a runtime shipping the Performance API before structuredClone. This is the Android side of the iOS follow-up commit 6dd55238d.

Deviations (documented in docs/performance.md)

  • User-timing buffers are unbounded (per spec); clearMarks()/clearMeasures() is the release valve.
  • Observer callbacks are delivered from a microtask (still asynchronous relative to mark()/measure()); callback exceptions route to reportError, so one throwing observer does not starve the others.
  • No DOMException: the spec'd SyntaxError/InvalidModificationError are Error instances with name patched.
  • Browser-only surface absent: no resource/navigation timing, no eventCounts.

Does your commit message include the wording below to reference a specific issue in this repo?

No — this is not tracked by an issue in this repo.

Related Pull Requests

Does your pull request have unit tests?

Yes. The cross-runtime shared suite (test-app/app/src/main/assets/app/shared/Performance, already at the pin this branch inherits) is registered from mainpage.js, contributing 56 specs — hr-time invariants, the full measure() options algebra, the timeline queries, observer semantics including buffered replay and takeRecords, detail snapshotting plus the DataCloneError case, and worker time-origin/buffer isolation. The suite gates itself on the API being present, so an unguarded Performance API canary was added to testRuntimeImplementedAPIs.js (mirroring iOS) to make a regression fail rather than silently skip. testPerformanceNow.js is removed — the shared suite supersedes it, as on iOS.

The frame-callback contract is covered by 4 new specs in testPostFrameCallback.js, run twice over — once per implementation — asserting the raw first argument stays boot-scale nanoseconds, that the second argument sits on the performance base (never ahead of a performance.now() read taken inside the callback, within a frame of it), that both advance across consecutive frames, and that the origin the two arguments imply matches the one (System.nanoTime(), performance.now()) implies. The results XML confirms the Java implementation suite ran with 2 tests / 0 skipped / 0 failures, so the API 21–23 bridge is device-tested rather than review-only.

Verified on an arm64-v8a API 35 emulator:

specs failures
base (feat/structured-clone, 975e724) 701 0
after the Performance API commit 756 0
this branch 760 0

(+55 = 56 shared Performance specs + 1 canary − 1 self-skip gate spec − 2 superseded testPerformanceNow specs; +4 = the frame-callback specs × 2 implementations.) check_if_tests_passed.js on the pulled results XML: 749 executed, 749 passed, 0 failed. ESLint clean (npm run lint).

Summary by CodeRabbit

  • New Features

    • Added a WHATWG-compatible Performance API with timing, marks, measures, entries, observers, and shared clock support.
    • Added performance.now() and timeOrigin for consistent runtime timestamps.
    • Added frame callback scheduling with native and Java Android support, including delayed callbacks.
  • Documentation

    • Added comprehensive Performance API and frame callback documentation, including platform behavior and specification deviations.
  • Tests

    • Expanded coverage for performance APIs, frame timestamps, clock consistency, ordering, and callback progression.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The runtime adds a WHATWG Performance API, native timing integration, and a dedicated Android frame-callback bridge. The test app validates performance timestamps and frame implementations. Documentation describes supported APIs, platform behavior, and specification deviations.

Changes

Performance runtime

Layer / File(s) Summary
JavaScript Performance API
test-app/runtime/src/main/cpp/js/performance.js, test-app/runtime/src/main/cpp/js/primordials.js, eslint.config.mjs
Adds performance entries, marks, measures, timeline queries, PerformanceObserver, constructors, and the global performance object.
Native performance timing integration
test-app/runtime/src/main/cpp/Performance.*, test-app/runtime/src/main/cpp/Runtime.*, test-app/runtime/CMakeLists.txt
Adds monotonic timing and time-origin accessors. Initializes the builtin Performance API and includes its native implementation.
Dedicated frame callback bridge
test-app/runtime/src/main/cpp/FrameCallbacks.*, test-app/runtime/src/main/java/com/tns/FrameCallbacks.java, test-app/runtime/src/main/cpp/CallbackHandlers.*, test-app/runtime/src/main/cpp/Runtime.cpp
Moves frame callback scheduling into FrameCallbacks, with native and Java Choreographer backends, timestamp dispatch, removal, exception translation, and isolate cleanup.
Validation and runtime documentation
test-app/app/src/main/assets/app/*, docs/README.md, docs/performance.md
Runs shared performance tests, adds Performance API and frame timestamp checks, and documents API behavior and specification deviations.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant JavaScript
  participant FrameCallbacks
  participant AndroidChoreographer
  participant V8
  JavaScript->>FrameCallbacks: postFrameCallback()
  FrameCallbacks->>AndroidChoreographer: schedule callback
  AndroidChoreographer->>FrameCallbacks: deliver frame timestamp
  FrameCallbacks->>V8: invoke callback with timing values
Loading

Possibly related PRs

Suggested reviewers: nathanwalker

Poem

I’m a rabbit timing each little hop,
Native frames arrive and stop.
Marks and measures fill the air,
Observers wait with careful care.
Clocks align from root to sky—
I twitch my nose and watch time fly.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the PR's main change: adding a WHATWG Performance API across timing, user timing, and performance timeline features.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Base automatically changed from feat/structured-clone to main August 11, 2026 18:56
…ine)

Replaces the bare native {now, timeOrigin} object with a spec-shaped
implementation of High Resolution Time, User Timing Level 3 and the
Performance Timeline with PerformanceObserver. performance, Performance,
PerformanceEntry, PerformanceMark, PerformanceMeasure, PerformanceObserver
and PerformanceObserverEntryList are globals in the main and worker isolates
alike, with Performance extends EventTarget and WebIDL-shaped descriptors
and brands.

All spec logic lives in the internal/performance.js builtin, which the
native side feeds exactly two values -- binding.now() and
binding.timeOrigin -- so the file is shared verbatim with the iOS runtime.
tns::Performance::NowMillis(isolate) is the single native clock hook a
future requestAnimationFrame must read, so every JS-visible timestamp
shares performance.timeOrigin as its base. Time origins stay per-Runtime,
captured in PrepareV8Runtime, so each worker keeps its own.

mark/measure detail is structured-cloned at entry creation through the
structuredClone global installed by StructuredClone::Init, so entries hold
snapshots and an uncloneable detail throws the DataCloneError-named error;
the builtin keeps an identity fallback for a runtime that ships the
Performance API before structuredClone.

Mirrors NativeScript/ios#430 and 6dd55238d.
__postFrameCallback now hands its callback two arguments,
(frameTimeNanos, performanceMillis). The first is unchanged -- the
platform's raw CLOCK_MONOTONIC frame time, which shipped app code divides
by 1e6 -- and the second is that same instant on the isolate's performance
timeline, so it compares directly with performance.now(). Choreographer
stamps frames on the clock the time origin is captured on, so the
conversion (Performance::MonotonicNanosToTimelineMillis, subtracting the
new Runtime::TimeOriginMonotonicMillis) is exact rather than a resampling.

The machinery moves out of CallbackHandlers into FrameCallbacks.{h,cpp},
which now covers the whole minSdk range: AChoreographer only exists from
API 24, so below it __postFrameCallback silently never fired. API 21-23 now
goes through android.view.Choreographer via com.tns.FrameCallbacks, holding
the same entry and producing the same two arguments. Entries are stored
behind unique_ptr so both implementations can hand the platform a stable
pointer, isolate teardown no longer erases while iterating, and the frame
time from the pre-API-29 AChoreographer entry point is widened to 64 bits,
which it is not on the 32-bit ABIs.

Debug runtimes expose __setFrameCallbackImpl so the Java bridge is
selectable on a modern device; both implementations are covered by specs.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (3)
test-app/runtime/src/main/cpp/Performance.cpp (1)

25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Read the isolate from the context.

context->GetIsolate() returns the isolate that owns the context. It removes the dependence on the current-isolate thread-local.

♻️ Proposed refactor
 void Performance::Init(Local<Context> context) {
-    Isolate* isolate = Isolate::GetCurrent();
+    Isolate* isolate = context->GetIsolate();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/runtime/src/main/cpp/Performance.cpp` around lines 25 - 27, Update
Performance::Init to obtain the isolate via context->GetIsolate() instead of
Isolate::GetCurrent(), ensuring it uses the isolate that owns the provided
context.
test-app/runtime/src/main/cpp/js/performance.js (1)

599-605: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Install the globals as non-enumerable properties.

Plain assignment makes these properties enumerable. WebIDL defines interface objects and the performance attribute as non-enumerable, writable, and configurable. The rest of this file already reproduces the WebIDL shape through finishInterface, so the globals should match.

♻️ Proposed refactor for the global installation
-g.Performance = Performance;
-g.PerformanceEntry = PerformanceEntry;
-g.PerformanceMark = PerformanceMark;
-g.PerformanceMeasure = PerformanceMeasure;
-g.PerformanceObserver = PerformanceObserver;
-g.PerformanceObserverEntryList = PerformanceObserverEntryList;
-g.performance = new Performance(kInternal);
+function defineGlobal(name, value) {
+  ObjectDefineProperty(g, name, {
+    value: value,
+    writable: true,
+    enumerable: false,
+    configurable: true,
+  });
+}
+defineGlobal("Performance", Performance);
+defineGlobal("PerformanceEntry", PerformanceEntry);
+defineGlobal("PerformanceMark", PerformanceMark);
+defineGlobal("PerformanceMeasure", PerformanceMeasure);
+defineGlobal("PerformanceObserver", PerformanceObserver);
+defineGlobal("PerformanceObserverEntryList", PerformanceObserverEntryList);
+defineGlobal("performance", new Performance(kInternal));
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/runtime/src/main/cpp/js/performance.js` around lines 599 - 605,
Update the global installations for Performance, PerformanceEntry,
PerformanceMark, PerformanceMeasure, PerformanceObserver,
PerformanceObserverEntryList, and performance to use non-enumerable, writable,
configurable property definitions instead of plain assignment, matching the
WebIDL property shape established by finishInterface.
docs/performance.md (1)

78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the debug-only __setFrameCallbackImpl override.

FrameCallbacks::Init installs __setFrameCallbackImpl under APPLICATION_IN_DEBUG, and testPostFrameCallback.js depends on it to reach the Java bridge. The section describes both implementations but does not mention that the selection can be forced in debug builds. Add a sentence so the next reader does not rediscover it from the source.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/performance.md` around lines 78 - 82, Update the documentation section
describing the NDK and Java Choreographer implementations to mention that
FrameCallbacks::Init installs the debug-only __setFrameCallbackImpl override
under APPLICATION_IN_DEBUG, allowing testPostFrameCallback.js to force the Java
bridge path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/performance.md`:
- Around line 37-39: Update the PerformanceObserver description to state that
supportedEntryTypes contains “mark” and “measure” rather than asserting array
identity with `===`; preserve the surrounding API summary.

In `@test-app/app/src/main/assets/app/tests/testPostFrameCallback.js`:
- Around line 191-197: Update the reference-clock calculation near
originFromClock to sample performance.now() immediately before and after
java.lang.System.nanoTime(), then use the performance timestamp midpoint when
computing the origin. Preserve the existing origin comparison and tolerance
while ensuring the two clock reads are bracketed.

In `@test-app/runtime/src/main/cpp/FrameCallbacks.cpp`:
- Around line 196-201: Protect all accesses to the shared entries_ map with one
std::mutex, including emplace, find, erase, and RemoveIsolateEntries iteration;
release the lock before invoking cb->Call while retaining the needed callback
data safely. Also replace lazy initialization in ResolveChoreographer for
choreographerResolved_ and its dlsym pointers, and in PostJava for
FRAME_CALLBACKS_CLASS, FRAME_CALLBACKS_CTOR, FRAME_CALLBACKS_POST, and
FRAME_CALLBACKS_RELEASE, with std::call_once or function-local statics.
- Around line 238-250: Wrap the Dispatch calls in OnNativeFrame32 and
OnNativeFrame64 with exception handling so NativeScriptException cannot escape
the AChoreographer C callbacks. Catch the exception inside each callback and
report it using the existing exception-reporting mechanism, while preserving the
current timestamp conversion and callback-entry dispatch behavior.

In `@test-app/runtime/src/main/cpp/js/performance.js`:
- Around line 484-537: Update the measure() argument normalization around
isOptionsObject so null startOrMeasureOptions and endMark are treated as
omitted, matching WebIDL dictionary conversion. Ensure null does not reach
convertMarkToTimestamp: performance.measure("a", null) must use startTime 0, and
performance.measure("a", "m", null) must use the two-argument behavior with
endTime now().

In `@test-app/runtime/src/main/java/com/tns/FrameCallbacks.java`:
- Line 15: Declare the released field in FrameCallbacks as volatile so doFrame
observes updates made by release() even when teardown occurs on another thread;
leave the existing release and callback logic unchanged.

---

Nitpick comments:
In `@docs/performance.md`:
- Around line 78-82: Update the documentation section describing the NDK and
Java Choreographer implementations to mention that FrameCallbacks::Init installs
the debug-only __setFrameCallbackImpl override under APPLICATION_IN_DEBUG,
allowing testPostFrameCallback.js to force the Java bridge path.

In `@test-app/runtime/src/main/cpp/js/performance.js`:
- Around line 599-605: Update the global installations for Performance,
PerformanceEntry, PerformanceMark, PerformanceMeasure, PerformanceObserver,
PerformanceObserverEntryList, and performance to use non-enumerable, writable,
configurable property definitions instead of plain assignment, matching the
WebIDL property shape established by finishInterface.

In `@test-app/runtime/src/main/cpp/Performance.cpp`:
- Around line 25-27: Update Performance::Init to obtain the isolate via
context->GetIsolate() instead of Isolate::GetCurrent(), ensuring it uses the
isolate that owns the provided context.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ccd90a72-d7e5-4854-8dd9-4f2f7f2f2087

📥 Commits

Reviewing files that changed from the base of the PR and between f284059 and 479dd74.

📒 Files selected for processing (19)
  • docs/README.md
  • docs/performance.md
  • eslint.config.mjs
  • test-app/app/src/main/assets/app/mainpage.js
  • test-app/app/src/main/assets/app/tests/testPerformanceNow.js
  • test-app/app/src/main/assets/app/tests/testPostFrameCallback.js
  • test-app/app/src/main/assets/app/tests/testRuntimeImplementedAPIs.js
  • test-app/runtime/CMakeLists.txt
  • test-app/runtime/src/main/cpp/CallbackHandlers.cpp
  • test-app/runtime/src/main/cpp/CallbackHandlers.h
  • test-app/runtime/src/main/cpp/FrameCallbacks.cpp
  • test-app/runtime/src/main/cpp/FrameCallbacks.h
  • test-app/runtime/src/main/cpp/Performance.cpp
  • test-app/runtime/src/main/cpp/Performance.h
  • test-app/runtime/src/main/cpp/Runtime.cpp
  • test-app/runtime/src/main/cpp/Runtime.h
  • test-app/runtime/src/main/cpp/js/performance.js
  • test-app/runtime/src/main/cpp/js/primordials.js
  • test-app/runtime/src/main/java/com/tns/FrameCallbacks.java
💤 Files with no reviewable changes (3)
  • test-app/app/src/main/assets/app/tests/testPerformanceNow.js
  • test-app/runtime/src/main/cpp/CallbackHandlers.cpp
  • test-app/runtime/src/main/cpp/CallbackHandlers.h

Comment thread docs/performance.md Outdated
Comment thread test-app/app/src/main/assets/app/tests/testPostFrameCallback.js Outdated
Comment thread test-app/runtime/src/main/cpp/FrameCallbacks.cpp Outdated
Comment thread test-app/runtime/src/main/cpp/FrameCallbacks.cpp
Comment on lines +484 to +537
const isOptionsObject =
startOrMeasureOptions !== null && typeof startOrMeasureOptions === "object";
let startTime;
let endTime;
let detail = null;
if (
isOptionsObject &&
(startOrMeasureOptions.start !== undefined ||
startOrMeasureOptions.end !== undefined ||
startOrMeasureOptions.duration !== undefined ||
startOrMeasureOptions.detail !== undefined)
) {
const o = startOrMeasureOptions;
if (endMark !== undefined) {
throw new TypeError("measure: endMark cannot be combined with a measure options object");
}
if (o.start === undefined && o.end === undefined) {
throw new TypeError("measure: the options object must specify start and/or end");
}
if (o.start !== undefined && o.end !== undefined && o.duration !== undefined) {
throw new TypeError("measure: cannot specify start, end and duration together");
}
let duration;
if (o.duration !== undefined) {
duration = Number(o.duration);
if (!NumberIsFinite(duration)) {
throw new TypeError("measure: duration must be a finite number");
}
}
if (o.end !== undefined) {
endTime = convertMarkToTimestamp(o.end);
} else if (o.start !== undefined && duration !== undefined) {
endTime = convertMarkToTimestamp(o.start) + duration;
} else {
endTime = now();
}
if (o.start !== undefined) {
startTime = convertMarkToTimestamp(o.start);
} else if (duration !== undefined) {
startTime = endTime - duration;
} else {
startTime = 0;
}
if (o.detail !== undefined && o.detail !== null) {
detail = cloneDetail(o.detail);
}
} else {
endTime = endMark !== undefined ? convertMarkToTimestamp(endMark) : now();
// A members-free options object means "no start given", not a mark name.
startTime =
startOrMeasureOptions !== undefined && !isOptionsObject
? convertMarkToTimestamp(startOrMeasureOptions)
: 0;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Treat null arguments as omitted in measure().

Line 485 excludes null from isOptionsObject. Line 534 then passes null to convertMarkToTimestamp, which converts it to the string "null" and throws the SyntaxError stand-in. Line 497 and line 531 do the same for a null endMark.

WebIDL converts null for the (DOMString or PerformanceMeasureOptions) union to an empty dictionary, so performance.measure("a", null) must return a measure from 0 to now(). performance.measure("a", "m", null) must behave like the two-argument form.

🐛 Proposed fix for null start and end arguments
     const name = String(measureName);
     const isOptionsObject =
-      startOrMeasureOptions !== null && typeof startOrMeasureOptions === "object";
+      startOrMeasureOptions === null ||
+      (startOrMeasureOptions !== undefined && typeof startOrMeasureOptions === "object");
-      if (endMark !== undefined) {
+      if (endMark !== undefined && endMark !== null) {
         throw new TypeError("measure: endMark cannot be combined with a measure options object");
       }
     } else {
-      endTime = endMark !== undefined ? convertMarkToTimestamp(endMark) : now();
+      endTime =
+        endMark !== undefined && endMark !== null ? convertMarkToTimestamp(endMark) : now();
       // A members-free options object means "no start given", not a mark name.
       startTime =
         startOrMeasureOptions !== undefined && !isOptionsObject
           ? convertMarkToTimestamp(startOrMeasureOptions)
           : 0;
     }

With isOptionsObject true for null, the member probe at lines 489-495 is false for a null value, so control reaches the else branch and yields startTime === 0.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/runtime/src/main/cpp/js/performance.js` around lines 484 - 537,
Update the measure() argument normalization around isOptionsObject so null
startOrMeasureOptions and endMark are treated as omitted, matching WebIDL
dictionary conversion. Ensure null does not reach convertMarkToTimestamp:
performance.measure("a", null) must use startTime 0, and
performance.measure("a", "m", null) must use the two-argument behavior with
endTime now().

Comment thread test-app/runtime/src/main/java/com/tns/FrameCallbacks.java Outdated
The registry holds entries for every isolate in the process, so its
lookups now take a mutex -- never held across the JS call, which a
self-rescheduling callback re-enters -- and the two lazy-init blocks (the
AChoreographer dlsym, the FrameCallbacks method ids) go through call_once.
Entries are identified to the platform by id rather than by address, so a
frame arriving after its entry was retired resolves to nothing instead of
to freed memory, and teardown detaches entries under the mutex and
destroys them after releasing it, since the destructor calls into Java.
The registry was shared and unguarded before it moved out of
CallbackHandlers; this is not a regression from the move.

Dispatch no longer throws. On the NDK path it runs inside a C callback in
libandroid, which a C++ exception may not unwind through, so a JS
exception the runtime still owns goes to Java the way Timers::FireTimer
does. com.tns.FrameCallbacks.released becomes volatile: it is set from
runtime teardown, which is not necessarily the frame thread.

measure() treats a null startOrMeasureOptions as absent. WebIDL converts
null for a (DOMString or PerformanceMeasureOptions) union to an empty
dictionary, so it means "no options", not the mark name "null". A null
endMark keeps throwing: that parameter is a plain DOMString, neither a
union nor nullable, so null stringifies per WebIDL.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test-app/app/src/main/assets/app/tests/testPerformance.js`:
- Around line 37-46: Add performance.mark("the-start") before the
performance.measure call in the “Should reject a null end mark” test, ensuring
the existing start mark is present so the assertion specifically validates null
endMark handling.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 76065f26-8f35-4c20-9490-589b986d461d

📥 Commits

Reviewing files that changed from the base of the PR and between 479dd74 and 9b20dd2.

📒 Files selected for processing (7)
  • docs/performance.md
  • test-app/app/src/main/assets/app/mainpage.js
  • test-app/app/src/main/assets/app/tests/testPerformance.js
  • test-app/app/src/main/assets/app/tests/testPostFrameCallback.js
  • test-app/runtime/src/main/cpp/FrameCallbacks.cpp
  • test-app/runtime/src/main/cpp/js/performance.js
  • test-app/runtime/src/main/java/com/tns/FrameCallbacks.java
🚧 Files skipped from review as they are similar to previous changes (6)
  • test-app/app/src/main/assets/app/mainpage.js
  • docs/performance.md
  • test-app/runtime/src/main/java/com/tns/FrameCallbacks.java
  • test-app/app/src/main/assets/app/tests/testPostFrameCallback.js
  • test-app/runtime/src/main/cpp/FrameCallbacks.cpp
  • test-app/runtime/src/main/cpp/js/performance.js

Comment on lines +37 to +46
it("Should reject a null end mark", function () {
let thrown = null;
try {
performance.measure("null-end", "the-start", null);
} catch (e) {
thrown = e;
}
expect(thrown).not.toBeNull();
expect(thrown && thrown.name).toBe("SyntaxError");
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Create the start mark before testing a null end mark.

The test never creates the-start. If null were incorrectly treated as an omitted endMark, the missing start mark could still produce SyntaxError. Add performance.mark("the-start") so the test isolates null endMark handling.

Proposed fix
   it("Should reject a null end mark", function () {
+    performance.mark("the-start");
     let thrown = null;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("Should reject a null end mark", function () {
let thrown = null;
try {
performance.measure("null-end", "the-start", null);
} catch (e) {
thrown = e;
}
expect(thrown).not.toBeNull();
expect(thrown && thrown.name).toBe("SyntaxError");
});
it("Should reject a null end mark", function () {
performance.mark("the-start");
let thrown = null;
try {
performance.measure("null-end", "the-start", null);
} catch (e) {
thrown = e;
}
expect(thrown).not.toBeNull();
expect(thrown && thrown.name).toBe("SyntaxError");
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test-app/app/src/main/assets/app/tests/testPerformance.js` around lines 37 -
46, Add performance.mark("the-start") before the performance.measure call in the
“Should reject a null end mark” test, ensuring the existing start mark is
present so the assertion specifically validates null endMark handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant