fix(profiler): verbose errors when target app cannot be profiled#201
Open
latekvo wants to merge 6 commits into
Open
fix(profiler): verbose errors when target app cannot be profiled#201latekvo wants to merge 6 commits into
latekvo wants to merge 6 commits into
Conversation
Profiler and inspector tools used to crash with generic "Cannot read property 'X' of undefined" TypeErrors whenever they hit a runtime without `__REACT_DEVTOOLS_GLOBAL_HOOK__` (release/production builds where DevTools is stripped). Surface the diagnosis up front instead, and stop leaving a half-initialised session for downstream tools to trip over. - inspect-at-point.ts: guard the injected script for missing hook / renderer / fiber root and report through the existing __argent_callback error channel - react-profiler-start.ts: dispose the in-flight ReactProfilerSession before each early-validation throw so subsequent stop/analyze calls see a clean "no session" state; wrap the READ_STATE_SCRIPT eval so a thrown CDP error also rolls back - react-profiler-stop.ts: refuse to stop when `api.profilingActive` is false, validate the Hermes profile shape (samples/nodes/timeDeltas), and defer cacheProfilerPaths until after the response is built so a partial dump cannot poison react-profiler-analyze - 00-cpu-correlate.ts: new `assertHermesCpuProfile` shape guard; called by buildCpuSampleIndex and at every tool boundary (analyze, cpu-summary, cpu-query) - react-profiler-renders.ts / react-profiler-fiber-tree.ts: align HOOK_MISSING_MESSAGE with the canonical NO_DEVTOOLS_HOOK_ERROR so every entry point gives the same remediation
Drop assertHermesCpuProfile + 3 boundary call sites + its test: stop.ts already validates the Hermes profile shape inline at the producer, so downstream validators only catch dumps loaded by profiler-load from older versions (a separate concern). Drop disposeSessionQuietly and its try/catch wrappers around cdp.evaluate in react-profiler-start: the stop.ts !api.profilingActive precheck is the load-bearing check for "session never reached the sampler"; the helper and per-eval wrappers added no behaviour beyond what propagating throws already provided. Revert the cacheProfilerPaths reorder in react-profiler-stop: the defer-on-throw rationale is unrelated to the "verbose errors when unprofileable" PR goal and belongs in its own change. Also drop unused NO_RENDERER_INTERFACE_ERROR export. Preserves the user-facing behaviour: NO_DEVTOOLS_HOOK_ERROR is still thrown on missing hook, inspect-at-point guards remain, stop.ts gives the verbose explanation when sampling never started.
readCpuProfile is the only entry point into analyze/cpu-summary/cpu-query for sessions reloaded via profiler-load (older or partial dumps), and the previous trim removed the every-tool-boundary assertHermesCpuProfile that guarded these consumers. A malformed cpuProfile.json reloaded that way would crash buildCpuSampleIndex with "TypeError: Cannot read properties of undefined (reading 'length')" — the exact failure mode this PR exists to prevent. Move the shape check inside readCpuProfile so the load path is covered at one site (no producer-side duplication; react-profiler-stop still validates inline before writing the dump).
react-profiler-renders and react-profiler-fiber-tree previously collapsed both "no __REACT_DEVTOOLS_GLOBAL_HOOK__" and "no renderers attached to hook" into NO_DEVTOOLS_HOOK_ERROR. The hook-missing message tells the user to rebuild in dev mode, which is the wrong remediation when the hook IS present and only the renderer hasn't registered (typical on bridgeless RN dev builds before the first commit, or before react-profiler-start has had a chance to bootstrap the DevTools backend). Add NO_RENDERERS_ATTACHED_ERROR in react-profiler-start and a small messageForHookError() helper in each tool that branches on the actual error code from the in-runtime script. The retry-via-FIBER_ROOT_TRACKER path still considers both codes "hook not present" so it keeps trying once before throwing.
The two single-line comment deletions in profiler-cpu-query.ts and react-profiler-analyze.ts were leftover from the original belt-and-suspenders plan that 5211c30 reverted. They don't serve this PR's "verbose errors when target app cannot be profiled" purpose; put them back so the diff stays surgical.
The guards added for missing hook / renderers / fiber root cover the common production-build path, but any throw later in the script (e.g. the pre-existing Fabric L115 crash on stateNode.canonical, or a runtime-junk getFiberRoots) still surfaces as a generic CDP exception, which is exactly the failure mode this PR was meant to eliminate. Wrap the IIFE body in try/catch and route the message through __argent_fail so the operator always sees a structured "Inspect script crashed: ..." instead.
latekvo
force-pushed
the
ignacylatka/verbose-profiler-errors
branch
from
54474b5 to
04b6202
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If you point the profiler or inspect-element tool at an app that can't actually be profiled (usually a release build with React DevTools stripped out), the tools currently blow up with a cryptic
Cannot read property 'X' of undefinedand leave the session in a weird half-started state. This PR makes them say what went wrong and what to do about it instead.Profiler and inspector tools used to crash with generic "Cannot read property 'X' of undefined" TypeErrors whenever they hit a runtime without
__REACT_DEVTOOLS_GLOBAL_HOOK__(release/production builds where DevTools is stripped) or against a partial/malformed Hermes CPU profile. Surface the diagnosis up front instead, and stop leaving a half-initialised session for downstream tools to trip over.Technical details
inspect-at-point.ts: guard the injected script for missing hook / renderer / fiber root and report through the existing__argent_callbackerror channel; exportINSPECT_NO_DEVTOOLS_HOOK_ERROR,INSPECT_NO_RENDERER_ERROR,INSPECT_NO_FIBER_ROOT_ERRORso callers can recognise the same diagnosis without string-matching bespoke phrasings.react-profiler-start.ts: replace the terse "production build" one-liner with the verboseNO_DEVTOOLS_HOOK_ERROR(rebuild in dev mode, three likely causes, concreterun-ios/ Expo dev client hints). ExportNO_RENDERERS_ATTACHED_ERRORfor the distinct "hook present but no renderer registered" state.react-profiler-stop.ts: refuse to stop whenapi.profilingActiveis false (start failed before reaching the sampler), and validate the Hermes profile shape (samples/nodes/timeDeltasarrays) before handing it to the analyse pipeline.dump.ts(readCpuProfile): validate shape inline at load soprofiler-loadof an older / partial dump fails fast with an actionable message instead of crashingbuildCpuSampleIndexlater.react-profiler-renders.ts/react-profiler-fiber-tree.ts: branch on the in-runtime error code so "no__REACT_DEVTOOLS_GLOBAL_HOOK__" gets the rebuild-in-dev advice while "no renderers attached to hook" gets the wait-for-render / let-react-profiler-start-bootstrap advice — separating two failure modes that the oldHOOK_MISSING_MESSAGEcollapsed into one misleading message.Tests
inspect-at-point-guards.test.ts— eval the injected IIFE against a controlledglobalThisto verify each guard fires the verbose diagnostic and routes through__argent_callback.