fix: avoid use-after-free for rust instance#2461
Conversation
…etaching event listeners during component teardown
🦋 Changeset detectedLatest commit: 9964d7a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughChanged disposal and cleanup sequencing across web-core: deferred wasm disposal, explicit exposure services disposal, safe wasmContext nullification, background MessagePort closure, and event-listener removal signature adjustments to prevent a Rust use-after-free. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Signed-off-by: Haoyang Wang <12288479+PupilTong@users.noreply.github.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.ts (1)
86-87: Add a regression test around post-dispose PAPI calls.These lines are the fail-closed guard for the UAF fix, but there’s no coverage proving a late flush/PAPI call now stops in JS instead of touching freed WASM state.
Based on learnings: In TypeScript test files, use
WASMJSBinding.tsto mock the Wasm binding for testing DOM manipulation logic without loading the actual Wasm binary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.ts` around lines 86 - 87, Add a regression test that verifies PAPI calls are no-ops after the Wasm context is disposed: write a TypeScript test (using WASMJSBinding.ts to mock the Wasm binding) that initializes the module via the createElementAPI entrypoint, simulates disposal by triggering the same code path that sets wasmContext = null, then attempts a representative post-dispose PAPI call (the same public API you expect to guard, e.g., any create/manipulate/flush method exported by createElementAPI) and assert it does not throw and that the mocked WASM binding methods were not invoked; ensure the test imports WASMJSBinding.ts, spies/stubs the underlying wasm methods, calls the dispose path that nulls wasmContext, then calls the PAPI and asserts a safe no-op behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/tidy-clubs-wink.md:
- Line 5: Update the changelog entry text in .changeset/tidy-clubs-wink.md to
rephrase the awkward wording; replace the existing line "fix: avoid to do
use-after-free for rust instance" with a clearer phrase such as "fix: avoid
use-after-free for the Rust instance" so the generated changelog reads
naturally.
In `@packages/web-platform/web-core/ts/client/mainthread/LynxViewInstance.ts`:
- Around line 303-305: The idle callback currently calls
this.mtsWasmBinding.dispose() which duplicates an earlier dispose path
(disposeWasmContext -> wasmContext.free()) and frees the Rust context too early;
instead, remove the second dispose call and split the teardown into two phases:
1) in the idle callback only run listener/JS cleanup (e.g., unregister event
listeners and release JS-side handles) using a new method like cleanupListeners
or cleanupJSResources on LynxViewInstance, and 2) ensure wasmContext.free() is
invoked only after awaiting this.backgroundThread[Symbol.asyncDispose]() (move
the wasm free call out of disposeWasmContext or add a separate method
freeWasmContext that is called after backgroundThread async dispose). Update
references to requestIdleCallbackImpl, this.mtsWasmBinding.dispose(),
disposeWasmContext, and wasmContext.free() accordingly so there is no duplicate
dispose and the Rust context is freed after the background thread finishes.
---
Nitpick comments:
In
`@packages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.ts`:
- Around line 86-87: Add a regression test that verifies PAPI calls are no-ops
after the Wasm context is disposed: write a TypeScript test (using
WASMJSBinding.ts to mock the Wasm binding) that initializes the module via the
createElementAPI entrypoint, simulates disposal by triggering the same code path
that sets wasmContext = null, then attempts a representative post-dispose PAPI
call (the same public API you expect to guard, e.g., any create/manipulate/flush
method exported by createElementAPI) and assert it does not throw and that the
mocked WASM binding methods were not invoked; ensure the test imports
WASMJSBinding.ts, spies/stubs the underlying wasm methods, calls the dispose
path that nulls wasmContext, then calls the PAPI and asserts a safe no-op
behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a18ab21e-79d0-45fb-9420-f23614396d76
📒 Files selected for processing (3)
.changeset/tidy-clubs-wink.mdpackages/web-platform/web-core/ts/client/mainthread/LynxViewInstance.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.ts
Merging this PR will degrade performance by 42.96%
Performance Changes
Comparing 
Footnotes
|
React External#411 Bundle Size — 582.81KiB (0%).9964d7a(current) vs 7332eb4 main#401(baseline) Bundle metrics
|
| Current #411 |
Baseline #401 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle analysis report Branch PupilTong:p/hw/free-lynx-view-in... Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#426 Bundle Size — 192.85KiB (0%).9964d7a(current) vs 7332eb4 main#416(baseline) Bundle metrics
|
| Current #426 |
Baseline #416 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
45.36% |
45.36% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #426 |
Baseline #416 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
81.61KiB |
81.61KiB |
Bundle analysis report Branch PupilTong:p/hw/free-lynx-view-in... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8867 Bundle Size — 749.55KiB (+0.03%).9964d7a(current) vs 7332eb4 main#8857(baseline) Bundle metrics
|
| Current #8867 |
Baseline #8857 |
|
|---|---|---|
| Initial JS |
44.45KiB |
44.45KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
 Cache Invalidation |
9.49% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
148(-1.33%) |
150 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
34.99%(-0.03%) |
35% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #8867 |
Baseline #8857 |
|
|---|---|---|
| Other |
401.63KiB |
401.63KiB |
| JS |
345.76KiB (+0.07%) |
345.51KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch PupilTong:p/hw/free-lynx-view-in... Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7293 Bundle Size — 223.58KiB (0%).9964d7a(current) vs 7332eb4 main#7283(baseline) Bundle metrics
|
| Current #7293 |
Baseline #7283 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
70 |
70 |
| Duplicate Code |
45.76% |
45.76% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7293 |
Baseline #7283 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
77.82KiB |
77.82KiB |
Bundle analysis report Branch PupilTong:p/hw/free-lynx-view-in... Project dashboard
Generated by RelativeCI Documentation Report issue
…, and WASMJSBinding to ensure proper disposal
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web-platform/web-core/ts/client/mainthread/LynxViewInstance.ts`:
- Around line 300-306: The async disposal currently schedules
mtsWasmBinding.dispose via requestIdleCallbackImpl and returns immediately,
violating the async disposal contract; change async [Symbol.asyncDispose]()
(which calls exposureServices.dispose()) to await a Promise that wraps
requestIdleCallbackImpl so the function only resolves after
mtsWasmBinding.dispose() completes; use the same requestIdleCallbackImpl call
(including its 16ms fallback) but resolve/reject the Promise after calling await
this.mtsWasmBinding.dispose() (or handling any errors) so callers in LynxView.ts
can safely assume resources are fully released when the returned Promise
resolves.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d88492f4-65ef-4828-9ec9-666890afc31b
📒 Files selected for processing (4)
packages/web-platform/web-core/ts/client/mainthread/Background.tspackages/web-platform/web-core/ts/client/mainthread/ExposureServices.tspackages/web-platform/web-core/ts/client/mainthread/LynxViewInstance.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/WASMJSBinding.ts
PupilTong
force-pushed
the
p/hw/free-lynx-view-instance
branch
from
aff4635 to
9964d7a
Compare
2 participants
Summary by CodeRabbit
Checklist