[edusperoni]

What's wrong

• Handle scope. WorkerWrapper's drain dispatches a whole batch of queued messages under one HandleScope, and Worker::OnMessageCallback opened none of its own. So every handle made while delivering each message in a batch (the deserialized payload, its ports, the event, and whatever the listeners allocate) stayed alive until the batch ended. The comment on SerializedValue::Deserialize already says each caller opens a scope per message. The parent-side post and NativeMessagePort::Drain do; the worker-side drain didn't.
• Flaky spec. "lets the same listener be on() and once() at the same time" started its settle timer at the third reply but expects four, so it failed whenever the fourth reply took longer than SETTLE. Its comment also had the two registrations swapped.
• Weak assertion. The DOMException structuredClone canary only checked the stack the constructor generates, never a stack set by the caller.

The fix

• Worker::OnMessageCallback opens a HandleScope per message.
• The spec waits for the fourth reply, then settles to catch a fifth.
• The canary sets a sentinel stack and asserts that the clone carries it.

Tests

Full suite on an iPhone 16 Pro simulator (./run_tests.sh): 1824 specs, 0 failures. That run was before rebasing onto #544, which adds one spec.

Same fixes on Android: NativeScript/android#2090

Summary by CodeRabbit

• Bug Fixes
  • Improved worker message handling so message data is scoped to each delivery.
  • DOMException copies created through structured cloning now retain the original stack.
  • Updated worker messaging checks to verify the expected sequence of received values.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The worker message callback now opens a handle scope for each delivered message. The messaging test expects four received values. The DOMException clone test checks that the clone retains a stack sentinel.

Changes: Worker message handling

Layer / File(s)	Summary
Per-message handle scope and message count NativeScript/runtime/Worker.mm, TestRunner/app/tests/MessagingTests.js	Each message callback opens a handle scope. The messaging test expects four values and describes the listener counts and settle window.

DOMException clone test

Layer / File(s)	Summary
Stack preservation assertion TestRunner/app/tests/RuntimeImplementedAPIs.js	The test assigns a stack sentinel to a DOMException and checks that the clone retains it.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: adrian-niculescu, nathanwalker

Merge Risk: Minimal · up to 32980

The change scopes worker message handles and strengthens two tests. No merge-blocking risk is established; normal checks should run on the rebased head.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the main change: Worker::OnMessageCallback now opens a HandleScope per delivered message, so handles do not outlive each message's callback. It also mentions the two messaging spec fix…
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: Worker::OnMessageCallback now opens a HandleScope per delivered message, so handles do not outlive each message's callback. It also mentions the two messaging spec fixes (the on()/once() listener test and the DOMException structuredClone test), which the changeset does cover. The title is specific, concise, and single-sentence, and it matches the raw summary and PR objectives.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.)

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit watched each message pass,
A fresh scope kept its handles fast.
Four values came, then waited still,
A cloned exception kept its fill.
The rabbit twitched and hopped away,
With tests in tow to greet the day.}

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: b9ef997a-acfb-4cd2-b9e5-a6dc902c4e69

Commits

Reviewing files that changed from the base of the PR and between 77d89a4 and 32980be.

Files selected for processing: 3

• NativeScript/runtime/Worker.mm
• TestRunner/app/tests/MessagingTests.js
• TestRunner/app/tests/RuntimeImplementedAPIs.js

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.