fix(web-shell): prevent React measure detail OOM - #7596
Conversation
E2E test report
Windows and Linux browser behavior were not tested. |
|
Thanks for the PR! Template looks good ✓ Problem: Observed bug with evidence. The PR describes Chrome Direction: Aligned. This is a dev-mode memory stability fix for Web Shell — clearly within scope. The existing try/catch approach only handles clone failures after they happen; successful clones still retain memory and cause heap growth. Stripping the detail proactively is the right layer to fix this. Size: Not applicable — Approach: The scope feels right — one targeted change and one focused test. The Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题: 已观测到的 bug,有证据。PR 描述了本地 daemon 开发中 Chrome 方向: 对齐。这是 Web Shell 开发模式的内存稳定性修复,明确在范围内。现有的 try/catch 只处理克隆失败后的情况;成功的克隆仍会保留内存导致堆增长。主动剥离 detail 是正确的修复层级。 规模: 不适用—— 方案: 范围合理——一个针对性修改和一个聚焦测试。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: I'd monkey-patch Comparison: The PR matches this approach exactly. Clean implementation, no surprises. Findings: No critical blockers or convention violations.
Real-Scenario Testingtmux is not available in this CI environment, and this is a browser-specific fix (React Performance API in Chrome dev mode) — CLI tmux testing wouldn't add meaningful signal beyond what the unit tests and daemon smoke test provide. Here's what I verified: Unit tests (2/2 pass): Build + typecheck: Both pass cleanly. Daemon smoke test — started The patched guard is live in the served HTML. The author's before/after evidence (803 React measures with null detail, 20-second stable run, no crashes) is consistent with what this code does. 中文说明代码审查独立方案: 我会 monkey-patch 对比: PR 与此方案完全一致。实现干净,没有意外。 发现: 无关键阻塞项或规范违反。
真实场景测试此 CI 环境无 tmux,且这是浏览器特定修复(Chrome 开发模式下的 React Performance API)——CLI tmux 测试不会比单元测试和 daemon 冒烟测试提供更多有意义的信号。以下是我验证的内容: 单元测试 2/2 通过,构建和类型检查均通过,daemon 冒烟测试确认修补后的脚本正确提供。作者的修改前后证据(803 个 null-detail React 测量、20 秒稳定运行、无崩溃)与此代码的行为一致。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — Clean across every stage; would merge without hesitation. This is exactly what a good fix looks like. Small, focused, solves a real observed problem (Chrome renderer OOM from React dev-mode structured-cloning transcript props into hundreds of performance measures). The old try/catch was a band-aid that only caught clone failures after the fact — successful clones still retained memory. Stripping the detail proactively at the Two files, 15 production lines, solid test covering both paths. No scope creep, no over-engineering. The author's before/after evidence (803 null-detail measures, stable 20-second run) matches what the code does. If I had to maintain this in six months, I'd thank the author. 中文说明置信度:5/5 — 每个阶段都很干净,毫不犹豫可以合并。 这是一个优秀修复的典范。小而聚焦,解决了一个真实观测到的问题(Chrome 渲染进程因 React 开发模式将大型会话属性结构化克隆到数百个性能测量条目而 OOM)。旧的 try/catch 只是事后补救——成功的克隆仍会保留内存。在 两个文件,15 行生产代码,扎实的测试覆盖两条路径。无范围蔓延,无过度工程。作者的修改前后证据(803 个 null-detail 测量、稳定 20 秒运行)与代码行为一致。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| if (!script) throw new Error('Performance measure guard not found'); | ||
|
|
||
| const performance = { measure }; | ||
| Function('performance', 'DOMException', script)(performance, DOMException); |
There was a problem hiding this comment.
[Suggestion] The DOMException parameter is no longer used by the new inline script — it was needed by the old try/catch approach that checked e instanceof DOMException, but the new proactive guard never references it. A future maintainer seeing DOMException injected here may incorrectly assume the script handles DataCloneError, risking regression of this fix. — Concrete cost: misleading test harness that suggests the script should handle clone errors.
| Function('performance', 'DOMException', script)(performance, DOMException); | |
| Function('performance', script)(performance); |
— qwen3.7-max via Qwen Code /review
Review —
|
| guard | Components ⚛ measures retaining detail |
detail retained in the user-timing buffer | total measures |
|---|---|---|---|
| none | 1105 | 55.7 MB | 1156 |
main (try/catch) |
1155 | 58.2 MB | 1217 |
| this PR | 0 | 0.01 MB | 1241 |
Two things this confirms:
- The PR's premise is right, and stronger than the description claims. On
mainthe guard is a complete no-op for this failure mode — retention is identical to having no guard. The structured clone succeeds; it just never gets released, socatch (DataCloneError)never fires. This is exactly the "does not address successful clones" argument in the description, now with numbers. - Timing is genuinely preserved. Measure count is unchanged (1241 vs. 1217); only
detailis nulled. Lane-track measures (trackGroup: 'Scheduler ⚛') still keep theirpropertiesand cost ~10 KB across ~50 entries — correctly left alone.
I also confirmed the string literal is exactly Components + U+269B with no variation selector, matching COMPONENTS_TRACK = "Components ⚛" in react-dom-client.development.js; that <meta charset="UTF-8"> sits at line 4 so the emoji survives; that the daemon CSP uses 'unsafe-inline' rather than a script hash (packages/cli/src/serve/web-shell-static.ts:28), so editing the inline script can't get it blocked; and that the classic <script> still wins the race against the deferred module script.
1. The exact-string match has no fallback behind it — a React rename silently disables the guard
devtools.track === 'Components ⚛' is an exact match on a React-internal constant, and this PR simultaneously deletes the try/catch. That leaves a single point of failure with nothing behind it.
Demonstrated: I patched COMPONENTS_TRACK in the bundle to "Components ⚛️" (a plausible React-side change — adding the emoji variation selector) and re-ran:
pr-drift measures=1146 retained=55.20MB <- guard is a silent no-op, back to main's behaviour
Prefix-matching plus keeping the old catch as a backstop survives both. I ran this exact version through the browser matrix and through your two new unit tests:
performance.measure = function () {
var options = arguments[1];
var devtools =
options &&
typeof options === 'object' &&
options.detail &&
options.detail.devtools;
// React's COMPONENTS_TRACK is 'Components ⚛'. Match by prefix so a
// React-side rename degrades to "still stripped" rather than "silently off".
var track = devtools && devtools.track;
if (typeof track === 'string' && track.indexOf('Components') === 0) {
var args = Array.prototype.slice.call(arguments);
args[1] = Object.assign({}, options, { detail: null });
return m.apply(this, args);
}
try {
return m.apply(this, arguments);
} catch (e) {
if (e instanceof DOMException && e.name === 'DataCloneError') return;
throw e;
}
};hard retained=0.01MB nonClonableMeasure=swallowed
hard-drift retained=0.01MB nonClonableMeasure=swallowed
index-html.test.ts 2 passed <- your existing tests still pass unchanged
2. DataCloneError now propagates to the caller
Removing the catch is an observable behaviour change:
performance.measure('x', { start: 1, end: 2, detail: { fn: function () {} } });
// main: swallowed, returns undefined
// this PR: throws DOMException: DataCloneErrorTo be fair: I don't think this is reachable from React any more. Once the Components track is stripped, the only React measures still carrying detail are the lane tracks, whose properties are short strings (["Component name", …], ["Error", message]) built by addValueToProperties. So this is defense-in-depth, not a live bug — but it's ~6 lines, it was added for a reason, and under the drift scenario above it's the only thing left standing.
3. Test coverage gaps
The two new tests cover the stripped path and the plain non-React path. Not covered:
-
The
detail.devtoolspresent but non-Componentstrack branch — i.e. lane/Scheduler measures. That's the only branch that still forwardsdetailuntouched, and nothing pins it. -
measure(name)and the legacymeasure(name, startMark, endMark)form.typeof options === 'object'is what keeps the 3-arg form working (a stringstartMarkmust fall through); a refactor could drop it without failing CI. -
A pin on React's constant. This is the highest-value addition given finding 1: a
react-dombump that renamesCOMPONENTS_TRACKwould silently disable the guard with fully green CI. This version passes today insidepackages/web-shell:it('matches react-dom’s COMPONENTS_TRACK constant', () => { const require = createRequire(import.meta.url); const src = readFileSync( join(dirname(require.resolve('react-dom')), 'cjs/react-dom-client.development.js'), 'utf8', ); const raw = src.match(/COMPONENTS_TRACK = "((?:[^"\\]|\\.)*)"/)?.[1]; expect(raw).toBeDefined(); expect(JSON.parse(`"${raw}"`)).toBe('Components ⚛'); });
-
expect(options.detail.devtools.properties).toHaveLength(1)is doing real work and deserves its own named test plus a comment. React reuses a single module-levelreusableComponentOptions/reusableComponentDevToolDetailsobject for every component measure (react-dom-client.development.js:25533-25543), so mutating the caller's options in place — rather thanObject.assign-copying, as this PR correctly does — would corrupt every subsequent entry. Right now that invariant reads like an incidental assertion.
Nits
installMeasureGuardstill threadsDOMExceptionintoFunction('performance', 'DOMException', script), but the patched script no longer references it. Dead today; becomes live again if thecatchis restored.expect(measure).toHaveBeenCalledWith('custom-measure', options)is deep-equality, so it passes even if the guard had copied the object. If the intent is "forwarded untouched",expect(measure.mock.calls[0]?.[1]).toBe(options)is the assertion that actually says that.index-html.test.tssits next to the existingindex.test.tsx, which testsindex.tsx.index.html.test.tswould read less ambiguously.- Optional: stripping unconditionally removes the "Changed Props" panel for every component in the Chrome performance profiler, not just the expensive ones. A
devtools.properties.length > Nthreshold would keep that DX for small components. Defensible either way — in my run the average was ~50 KB per component measure, so unconditional is a reasonable call; just noting the DX cost lands on all dev profiling, not only large transcripts.
Checks run
vitest run index-html.test.tsat PR head — 2 passed.- Full
packages/web-shellunit suite at PR head: 8 failing files, all of which reproduce identically onmain(pre-existing, unrelated). eslint packages/web-shell/client/index-html.test.ts— clean.- Prettier reports
index.htmlas unformatted, but that is pre-existing onmain(the deliberately minified theme-init script); this PR doesn't make it worse.
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Clean proactive fix — strips React component detail before it reaches the native measure call, preventing structured-clone OOM on large transcripts. Original options object is not mutated. Test cleverly extracts and executes the inline script. No issues found.
— qwen3.8-max-preview via Qwen Code /review
What this PR does
This change prevents React development instrumentation from structured-cloning and retaining Web Shell component property details while preserving component timing measurements. Non-React performance measurements and their details continue to pass through unchanged. Regression coverage verifies both behaviors.
Why it's needed
In local daemon development, React records component render metadata through the browser Performance API. Large transcript properties can be cloned and retained for hundreds of component measures, causing renderer heap growth and intermittent Chrome
Aw, Snap!crashes with error code 5. Catching only clone exceptions does not address successful clones or allocation failures.Reviewer Test Plan
How to verify
Start Web Shell with
npm run dev:daemon, open a transcript, and confirm the page remains responsive while React component performance entries containnulldetail. Create a custom non-React performance measure and confirm its detail remains intact. The focused regression tests should confirm both forwarding behaviors.Evidence (Before & After)
Before: React component measures retained structured-cloned transcript property details, and local Chrome renderer crash dumps showed the Web Shell development origin during intermittent crashes.
After: In an isolated daemon development run, the Web Shell root rendered successfully and remained stable for 20 seconds with no page crash, console error, page error, or failed request. All 803 observed React component measures had
nulldetail, while a custom measure preserved its detail.Tested on
Environment (optional)
Local
npm run dev:daemonwith an isolated Chrome profile, plus focused unit tests, full build, typecheck, and Web Shell lint.Risk & Scope
Linked Issues
No linked issue.
中文说明
本 PR 做了什么
此变更阻止 React 开发模式的性能检测逻辑对 Web Shell 组件属性详情执行结构化克隆并长期保留,同时保留组件计时数据。非 React 的性能测量及其详情仍按原样传递。回归测试覆盖了这两种行为。
为什么需要此变更
在本地 daemon 开发模式中,React 会通过浏览器 Performance API 记录组件渲染元数据。大型会话记录属性可能被数百个组件测量条目反复克隆并保留,导致渲染进程堆内存持续增长,并偶发出现 Chrome
Aw, Snap!、错误码 5 的页面崩溃。仅捕获克隆异常无法处理成功完成的克隆或内存分配失败。Reviewer Test Plan
如何验证
通过
npm run dev:daemon启动 Web Shell,打开一个会话记录,确认页面保持响应,并且 React 组件性能条目的detail为null。创建一个自定义的非 React 性能测量,确认其详情仍被保留。专项回归测试应同时验证这两种转发行为。证据(修改前与修改后)
修改前:React 组件测量会保留经过结构化克隆的会话属性详情,本地 Chrome 渲染进程崩溃转储显示偶发崩溃时加载了 Web Shell 开发地址。
修改后:在隔离的 daemon 开发运行中,Web Shell 根节点成功渲染并稳定运行 20 秒,没有页面崩溃、控制台错误、页面错误或请求失败。观察到的 803 个 React 组件测量条目的详情全部为
null,同时自定义测量仍保留其详情。测试平台
环境(可选)
本地
npm run dev:daemon、隔离的 Chrome 配置,以及专项单元测试、完整构建、类型检查和 Web Shell lint。风险与范围
关联 Issue
无关联 Issue。