[wrangler] fix: don't crash wrangler dev when source-mapping a truncated error chunk#14316
[wrangler] fix: don't crash wrangler dev when source-mapping a truncated error chunk#14316matingathani wants to merge 9 commits into
Conversation
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
- Move for-loop inside try block so sourceMappedStackTrace can be const - Capture caught error and log at debug level instead of silently dropping it
🦋 Changeset detectedLatest commit: 52ecf74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
Wraps the prepareStack call in getSourceMappedString in a try-catch so that if the source map library throws (e.g. because a truncated stderr chunk produced a call site with an invalid column number), the original un-source-mapped string is returned instead of crashing wrangler dev.
0ee4112 to
e7325f9
Compare
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
The 'Tests (Windows, packages-and-tools)' failure is a pre-existing flake in miniflare's browser tests unrelated to this PR. The Chrome DLL gets locked between parallel test processes on the Windows runner. The same failure appears on main without our changes. |
petebacondarwin
left a comment
There was a problem hiding this comment.
I think it would be helpful to have an automated regression test if possible.
Please can you see if you can create one?
…and fix null check - Narrow try-catch to wrap only `prepareStack()` and early-return on failure, so the for-loop replacements are never partially applied then silently dropped - Fix `getFileName() === undefined` → `=== null` (`CallSite.getFileName()` returns `string | null`, never `undefined`, so the old guard always evaluated false) - Add regression test verifying `getSourceMappedString` returns the original value when source mapping fails instead of throwing
|
Addressed all the review feedback in the latest commit: Narrowed try-catch to only wrap Fixed Added regression test in |
When Chrome fails to start with "Failed to launch the browser process" (e.g. Windows CI runners serving a DLL that the OS rejects with 0xC1 "%1 is not a valid Win32 application"), delete the stale build directory and retry install + launch once. Complements the existing installWithCorruptedCacheRecovery which handles the "executable missing from cache" case.
| "miniflare": patch | ||
| --- | ||
|
|
||
| fix: recover from corrupt Chrome binary when launching browser for local Browser Rendering |
There was a problem hiding this comment.
🟡 Changeset title uses prohibited conventional commit prefix
The changeset title on line 5 starts with fix:, which violates the REVIEW.md rule: "Do NOT prefix changeset titles with a 'type' (e.g. fix:, feat:, chore:). The changeset title should be a plain description without conventional commit prefixes." It should read something like "Recover from corrupt Chrome binary when launching browser for local Browser Rendering".
| fix: recover from corrupt Chrome binary when launching browser for local Browser Rendering | |
| Recover from corrupt Chrome binary when launching browser for local Browser Rendering | |
Was this helpful? React with 👍 or 👎 to provide feedback.
…able let Factors the try-catch into a dedicated `tryPrepareStack` helper that returns `null` on failure instead of throwing, letting `getSourceMappedString` use `const` for the result and check `=== null` before entering the for-loop. Addresses Copilot review feedback (prefer const over let-bridging a try block) and aligns with petebacondarwin's suggestion to make prepareStack non-throwing.
When the cached Chrome binary exists but is corrupt, the retry path calls install() again, which fires the downloadProgressCallback and starts the spinner. Without a matching s.stop(), the spinner runs indefinitely through the subsequent launch() and waitForBrowserReady() calls, leaving the terminal in a broken state. Reset startedDownloading before the retry so the callback re-arms cleanly, then stop the spinner after install() returns (success path) or throws (failure path), mirroring the pattern on lines 163-175. Addresses Devin review comment on PR cloudflare#14316.
…umn' into fix/sourcemap-crash-on-invalid-column
|
@matingathani - this seems to have picked up the changes from another PR too? |
Fixes #9919.
When a worker throws errors in rapid succession, the stderr chunks received by wrangler dev can arrive mid-frame — so a call site ends up with a negative column number. The
@jridgewell/trace-mappinglibrary throwsError: \column` must be greater than or equal to 0` in that case, which was bubbling up uncaught and crashing the wrangler process.The fix wraps the
prepareStack(...)call ingetSourceMappedStringin atry/catch. If source mapping throws for any reason, we fall back to returning the original (un-source-mapped) string. The worker process keeps running and the user still sees the error message, just without source map translation.I reproduced this with the snippet from the issue:
Hitting the worker endpoint a few times caused
wrangler devto crash before this fix. After the fix, it logs the raw stack and keeps running.