Skip to content

Clean up error handling in declarative shadow DOM code#47558

Merged
chromium-wpt-export-bot merged 1 commit intomasterfrom
chromium-export-cl-5750587
Aug 10, 2024
Merged

Clean up error handling in declarative shadow DOM code#47558
chromium-wpt-export-bot merged 1 commit intomasterfrom
chromium-export-cl-5750587

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Aug 9, 2024

This CL started as an implementation of exception throwing from
the parsing code for DSD. However, as discussed in the issue:

whatwg/html#10527

the desire is not to add such exceptions, mostly for fear of
opening new XSS endpoints. So that bit has been ripped out of
the CL, and what's left is a bit of a cleanup that at least
moves all errors (including wrong-mode-string) to a central
place in ErrorMessageForAttachShadow. This also allowed me
to remove the DeclarativeShadowRootMode enum and some
related code.

I also added a few small explicit tests that exceptions are
not thrown/fired during HTML parsing.

Since the spec says exceptions should be fired in this case,
I will put up a spec PR soon. For now, though, this is not
a behavioral change for Chrome, so no need to do an I2S or
anything.

The spec, currently:
-https://html.spec.whatwg.org/#parsing-main-inhead

Start at A start tag whose tag name is "template", then Otherwise,
and then point 8, which is:

Attach a shadow root with declarative shadow host element, mode,
clonable, serializable, delegatesFocus, and "named". If an exception
is thrown, then catch it, report it for declarative shadow host
element's relevant global object, insert an element at the adjusted
insertion location with template, and return.

Change-Id: I0a8e2c8fa1301efdd28f9bb786f9ca8d3596c2fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5750587
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Reviewed-by: Di Zhang <dizhangg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1339961}

This CL started as an implementation of exception throwing from
the parsing code for DSD. However, as discussed in the issue:

  whatwg/html#10527

the desire is not to add such exceptions, mostly for fear of
opening new XSS endpoints. So that bit has been ripped out of
the CL, and what's left is a bit of a cleanup that at least
moves all errors (including wrong-mode-string) to a central
place in `ErrorMessageForAttachShadow`. This also allowed me
to remove the `DeclarativeShadowRootMode` enum and some
related code.

I also added a few small explicit tests that exceptions are
not thrown/fired during HTML parsing.

Since the spec says exceptions should be fired in this case,
I will put up a spec PR soon. For now, though, this is not
a behavioral change for Chrome, so no need to do an I2S or
anything.

The spec, currently:
 -https://html.spec.whatwg.org/#parsing-main-inhead

Start at `A start tag whose tag name is "template"`, then Otherwise,
and then point 8, which is:

> Attach a shadow root with declarative shadow host element, mode,
> clonable, serializable, delegatesFocus, and "named". If an exception
> is thrown, then catch it, report it for declarative shadow host
> element's relevant global object, insert an element at the adjusted
> insertion location with template, and return.

Change-Id: I0a8e2c8fa1301efdd28f9bb786f9ca8d3596c2fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5750587
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Reviewed-by: Di Zhang <dizhangg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1339961}
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants