fix: prevent memory leaks by explicitly disposing WASM contexts and d…#2457
Conversation
…etaching event listeners during component teardown
🦋 Changeset detectedLatest commit: 9802421 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 |
📝 WalkthroughWalkthroughThis PR adds coordinated async disposal across the web-core client: serialized component disposal, explicit WASM binding/context freeing, event-listener tracking and removal, and guards to prevent operations after disposal to eliminate WASM memory leaks and use-after-free risks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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 |
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: 4
🤖 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/elementAPIs/createElementAPI.ts`:
- Around line 77-89: The dispose function (mtsBinding.disposeWasmContext) nulls
shared bindings (wasmContext, mtsBinding, rootDom, page, timingFlags) while
async callbacks queued by __FlushElementTree (via requestIdleCallbackImpl which
calls mtsBinding.postTimingFlags(...) and wasmContext.gc()) still close over
those variables; this can cause TypeError. Fix by replacing the nullification
with a disposed flag (e.g., set disposed = true) and avoid mutating the captured
bindings, or capture stable locals for any work you enqueue (create const
localMts = mtsBinding, localWasm = wasmContext before scheduling callbacks) and
in queued callbacks check disposed before using them; also clear/zero
timingFlags safely (e.g., timingFlags.length = 0 only after ensuring no queued
work will read it). Ensure changes touch mtsBinding.disposeWasmContext and any
code paths in __FlushElementTree/requestIdleCallbackImpl that rely on those
bindings.
In `@packages/web-platform/web-core/ts/client/mainthread/LynxView.ts`:
- Around line 419-420: The awaited call to oldInstance[Symbol.asyncDispose]() in
LynxView blocks renders when disposal can hang (as noted for backgroundThread
when startBTS() wasn't called); change LynxView to not await the asyncDispose on
oldInstance (e.g., fire-and-forget the Promise), or run it with a bounded
timeout/guard and catch errors so disposal cannot block subsequent renders;
locate the disposal call in the LynxView class (oldInstance and its
Symbol.asyncDispose usage) and update it to invoke asyncDispose without awaiting
(or wrap it in a cancellable/timed helper) and ensure any rejection is
caught/logged.
- Around line 407-425: The teardown race occurs because `#disposeInstance` clears
this.#instance and then awaits oldInstance[Symbol.asyncDispose](), and
disconnectedCallback doesn't await it, so a fast reconnect can create a new
instance that gets wiped when the old dispose resumes; fix by serializing
disposes with an in-flight token or promise (e.g., add this.#disposePromise or
this.#disposeGeneration) so `#disposeInstance` records a unique generation/promise
before awaiting Symbol.asyncDispose and only clears or mutates shadowRoot if the
generation matches, and ensure the render/attach logic (where a new instance is
created) awaits any existing this.#disposePromise (or checks generation) before
reusing shadowRoot or assigning this.#instance to avoid stale teardown wiping a
new render.
In `@packages/web-platform/web-core/ts/client/mainthread/LynxViewInstance.ts`:
- Around line 299-301: The disposal order is wrong: free the background thread
before disposing the main-thread WASM binding to avoid use-after-free during RPC
teardown; in the async disposal method (async [Symbol.asyncDispose]()) call
await this.backgroundThread[Symbol.asyncDispose]() first, then call
this.mtsWasmBinding.dispose() (and clear any references to mtsWasmBinding
afterward) so backgroundThread teardown can complete while the WASM context is
still valid.
🪄 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: 5aac50a2-8475-4c37-a81c-6c6ba84c30f2
📒 Files selected for processing (5)
.changeset/fix-wasmcontext-memory-leak.mdpackages/web-platform/web-core/ts/client/mainthread/LynxView.tspackages/web-platform/web-core/ts/client/mainthread/LynxViewInstance.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/WASMJSBinding.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-core/ts/client/mainthread/LynxView.ts (1)
469-510:⚠️ Potential issue | 🟡 Minor
#renderingflag may not reset when#urlis falsy.If
this.#urlis not set when#render()is called, theifblock (lines 469-510) is skipped, but#renderingremainstrue(set on line 450). This blocks all subsequent#render()calls even afterurlis later assigned.🐛 Proposed fix
const mtsRealm = await mtsRealmPromise; if (this.#url) { // ... existing code ... this.#instance = await lynxViewInstance; - this.#rendering = false; } + this.#rendering = false; });🤖 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/LynxView.ts` around lines 469 - 510, The `#rendering` flag can remain true when this.#url is falsy because the if (this.#url) block is skipped; modify the render method (the `#render` / render function where this.#rendering is set true) to ensure this.#rendering is reset to false whenever rendering cannot proceed: either add an early branch that sets this.#rendering = false and returns when this.#url is falsy, or wrap the async work in a try/finally and set this.#rendering = false in the finally block, referencing the same private field names (`#rendering`, `#url`) and the lynxViewInstance/this.#instance logic so the flag is always cleared on exit.
🧹 Nitpick comments (1)
packages/web-platform/web-core/ts/client/mainthread/LynxView.ts (1)
507-508: Consider movinglynxGroupIddeclaration before its closure capture.The
lynxGroupIdvariable is used on line 488 inside the.then()callback but declared on line 507. While this works correctly (the declaration executes before the callback runs due to theawaiton line 508), it's confusing. Moving the declaration before line 470 would improve readability.♻️ Suggested reordering
+ const lynxGroupId = this.lynxGroupId; const lynxViewInstance = import( /* webpackChunkName: "web-core-main-chunk" */ /* webpackFetchPriority: "high" */ './LynxViewInstance.js' ).then(({ LynxViewInstance }) => { ... }); templateManager.fetchBundle( ... ); - const lynxGroupId = this.lynxGroupId; this.#instance = await lynxViewInstance;🤖 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/LynxView.ts` around lines 507 - 508, The variable lynxGroupId is declared after its use inside the .then() callback that resolves lynxViewInstance which is confusing; move the declaration of lynxGroupId so it appears before the closure that references it (i.e., above the code that calls or awaits lynxViewInstance and before the .then() callback), ensuring the .then() closure reads the already-declared lynxGroupId; keep the assignment to this.#instance = await lynxViewInstance unchanged but relocate the lynxGroupId declaration to precede that await/.then() usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/web-platform/web-core/ts/client/mainthread/LynxView.ts`:
- Around line 469-510: The `#rendering` flag can remain true when this.#url is
falsy because the if (this.#url) block is skipped; modify the render method (the
`#render` / render function where this.#rendering is set true) to ensure
this.#rendering is reset to false whenever rendering cannot proceed: either add
an early branch that sets this.#rendering = false and returns when this.#url is
falsy, or wrap the async work in a try/finally and set this.#rendering = false
in the finally block, referencing the same private field names (`#rendering`,
`#url`) and the lynxViewInstance/this.#instance logic so the flag is always
cleared on exit.
---
Nitpick comments:
In `@packages/web-platform/web-core/ts/client/mainthread/LynxView.ts`:
- Around line 507-508: The variable lynxGroupId is declared after its use inside
the .then() callback that resolves lynxViewInstance which is confusing; move the
declaration of lynxGroupId so it appears before the closure that references it
(i.e., above the code that calls or awaits lynxViewInstance and before the
.then() callback), ensuring the .then() closure reads the already-declared
lynxGroupId; keep the assignment to this.#instance = await lynxViewInstance
unchanged but relocate the lynxGroupId declaration to precede that await/.then()
usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d8f69c3-3cca-4607-b2a0-14bab045a9ec
📒 Files selected for processing (3)
packages/web-platform/web-core/ts/client/mainthread/Background.tspackages/web-platform/web-core/ts/client/mainthread/LynxView.tspackages/web-platform/web-core/ts/client/mainthread/elementAPIs/createElementAPI.ts
Merging this PR will degrade performance by 26.66%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | 002-hello-reactLynx-destroyBackground |
681.9 µs | 929.7 µs | -26.66% |
| ⚡ | transform 1000 view elements |
43.7 ms | 40.7 ms | +7.47% |
| ❌ | basic-performance-scroll-view-100 |
8.1 ms | 8.7 ms | -6.33% |
Comparing PupilTong:p/hw/free-lynx-view-instance (9802421) with main (a9f8d05)
Footnotes
-
21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React MTF Example#402 Bundle Size — 206.12KiB (0%).9802421(current) vs a9f8d05 main#384(baseline) Bundle metrics
|
| Current #402 |
Baseline #384 |
|
|---|---|---|
 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.79% |
45.79% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #402 |
Baseline #384 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
94.89KiB |
94.89KiB |
Bundle analysis report Branch PupilTong:p/hw/free-lynx-view-in... Project dashboard
Generated by RelativeCI Documentation Report issue
React External#387 Bundle Size — 591.76KiB (0%).9802421(current) vs a9f8d05 main#369(baseline) Bundle metrics
|
| Current #387 |
Baseline #369 |
|
|---|---|---|
| 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 Example#7269 Bundle Size — 236.83KiB (0%).9802421(current) vs a9f8d05 main#7251(baseline) Bundle metrics
|
| Current #7269 |
Baseline #7251 |
|
|---|---|---|
| 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 |
46.13% |
46.13% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #7269 |
Baseline #7251 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.07KiB |
91.07KiB |
Bundle analysis report Branch PupilTong:p/hw/free-lynx-view-in... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#8843 Bundle Size — 749.26KiB (+0.08%).9802421(current) vs a9f8d05 main#8825(baseline) Bundle metrics
|
| Current #8843 |
Baseline #8825 |
|
|---|---|---|
 Initial JS |
44.45KiB(+0.39%) |
44.27KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
 Cache Invalidation |
15.35% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
148(-1.33%) |
150 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
35.02%(+0.03%) |
35.01% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #8843 |
Baseline #8825 |
|
|---|---|---|
| Other |
401.63KiB |
401.63KiB |
| JS |
345.47KiB (+0.17%) |
344.87KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch PupilTong:p/hw/free-lynx-view-in... Project dashboard
Generated by RelativeCI Documentation Report issue
|
@coderabbitai reconsider your comments with latest PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 72649 Let me fetch the current code to properly reassess each comment. 🏁 Script executed: Length of output: 53571
✅ Resolved / Withdrawn Comments1. 2. 3. 4.
|
2 participants
…etaching event listeners during component teardown
Summary by CodeRabbit
Checklist