fix(dom): don't assign to error.message in handleErrors (getter-only errors) - #2372
Open
aryanku-dev wants to merge 1 commit into
Open
fix(dom): don't assign to error.message in handleErrors (getter-only errors)#2372aryanku-dev wants to merge 1 commit into
aryanku-dev wants to merge 1 commit into
Conversation
…errors)
`handleErrors` enriches an error by assigning to `error.message`. DOMException
declares `message` as a getter-only accessor (WebIDL `readonly attribute`), so
in this strict-mode bundle the assignment throws:
TypeError: Cannot set property message of #<DOMException> which has only a getter
DOMException is exactly what the guarded calls throw — `canvas.toDataURL()` on a
tainted canvas raises SecurityError. So the enrichment step replaces the real,
actionable error with a confusing TypeError, which propagates out of serializeDOM
and fails the whole snapshot ("Could not take DOM snapshot"). Snapshots then drop
out of builds non-deterministically, giving inconsistent snapshot counts.
All seven handleErrors call sites are affected (canvas, cssom, dialog, video,
inputs, clone-dom, styleSheetFromNode), not just canvas.
Enrich in place when the assignment succeeds; otherwise throw a new Error that
carries the enriched message plus the original's name and a `cause` reference.
Also covers frozen/sealed errors and sloppy-mode callers, where the assignment
fails silently rather than throwing.
Fixes PER-10368
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Fixes PER-10368.
Problem
Customer reports snapshots being dropped from builds with inconsistent snapshot counts between runs. Their log:
Root cause
packages/dom/src/utils.js—handleErrorsenriches an error by mutating it:DOMExceptiondeclaresmessageas a getter-only accessor (WebIDLreadonly attribute DOMString message). Assigning to it inside this strict-mode bundle throws aTypeError:That reproduction is character-for-character identical to the customer's log, including the double space where the object tag would render.
And
DOMExceptionis precisely what the guarded operations throw —canvas.toDataURL()raisesSecurityErroron a canvas tainted by cross-origin data. So the chain is:canvas.toDataURL()throwsSecurityError(a real, actionable, well-understood condition)handleErrorstries to enrich it → TypeError, masking the original errorserializeElements→serializeDOM→page.evaluaterejectsCould not take DOM snapshot— the entire snapshot is droppedBecause it depends on which canvases happen to be tainted on a given run, snapshots disappear non-deterministically — the reported inconsistent counts.
This affects all seven
handleErrorscall sites, not just canvas:serialize-cssom(cross-origin stylesheet access also throwsSecurityError),serialize-dialog,serialize-video,serialize-inputs,clone-dom, andstyleSheetFromNode.Fix
Enrich in place when the assignment succeeds; otherwise throw a new
Errorcarrying the enriched message plus the original'snameand acausereference. The original error is never lost.The
error.message !== messagere-check after thetryalso covers sloppy-mode callers, where the assignment fails silently instead of throwing, and frozen/sealed errors.Testing
yarn workspace @percy/dom test→ 500 tests pass. Five new specs, and there were previously no tests forhandleErrorsat all.Verified these are genuine regression tests — reverting only
src/utils.jsand re-running:All five pass with the fix applied.
Note
After this change, a tainted-canvas snapshot surfaces the real
SecurityErrorinstead of a TypeError. That is the intended outcome — the underlying condition is genuinely actionable, and users who want to tolerate it already have theignoreCanvasSerializationErrorsconfig option, which bypasseshandleErrorsentirely.🤖 Generated with Claude Code