Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix several crashes when nested slots are removed from a ShadowRoot #26951

Merged
merged 1 commit into from Dec 22, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 18, 2020

There were some crashes caused by nested slots (e.g.
<slot><slot>Content</slot></slot>) being removed from the tree.
These crashes were triggered by [1], which removed Shadow DOM v0, but
my theory is that due to the old V0 shadow root code, more calls were
being made to SlotAssignment::RecalcAssignment(). Now that the V0 code
is gone, it has exposed some missing code.

Three issues are being fixed here:

  1. In Node::CheckSlotChange(), while removing the inner nested slot,
    the parent_slot will have already been removed from the tree, so we
    only need to call DidSlotChange if not. This used to be a DCHECK.
  2. In TreeOrderedMap::Get(), while removing a key that previously had
    more than one element, we may walk the tree and find that none of
    the pre-existing elements are present. I.e. we're in a RemoveScope.
    In this case, the key should be removed from the map.
  3. In SlotAssignment::DidRemoveSlotInternal(), given Apple's submission #2 above, we can
    just early-out if the slot isn't present in the map.

I added a test for the crash conditions (variations on removing nested
named and unnamed slots), plus I added a test for the TreeOrderedMap
class, since there was none previously. The last test in the set
documents the new Get() behavior. I also tried to improve some of the
comments along the way. Finally, this CL rolls back a mitigation [2]
previously landed for this crash.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2586019
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2595967

Bug: 1159328, 1159727
Change-Id: I47fbf33b2313b9ae2efe229443af6e8c9a1920a9
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597040
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Yu Han <yuzhehan@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838974}

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.

@chromium-wpt-export-bot chromium-wpt-export-bot changed the title Fix a crash when nested slots are removed from a ShadowRoot Fix several crashes when nested slots are removed from a ShadowRoot Dec 22, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2597040 branch 5 times, most recently from e0f27af to 65cdd81 Compare December 22, 2020 21:25
There were some crashes caused by nested slots (e.g.
<slot><slot>Content</slot></slot>) being removed from the tree.
These crashes were triggered by [1], which removed Shadow DOM v0, but
my theory is that due to the old V0 shadow root code, more calls were
being made to SlotAssignment::RecalcAssignment(). Now that the V0 code
is gone, it has exposed some missing code.

Three issues are being fixed here:
 1. In Node::CheckSlotChange(), while removing the inner nested slot,
    the parent_slot will have already been removed from the tree, so we
    only need to call DidSlotChange if not. This used to be a DCHECK.
 2. In TreeOrderedMap::Get(), while removing a key that previously had
    more than one element, we may walk the tree and find that none of
    the pre-existing elements are present. I.e. we're in a RemoveScope.
    In this case, the key should be removed from the map.
 3. In SlotAssignment::DidRemoveSlotInternal(), given #2 above, we can
    just early-out if the slot isn't present in the map.

I added a test for the crash conditions (variations on removing nested
named and unnamed slots), plus I added a test for the TreeOrderedMap
class, since there was none previously. The last test in the set
documents the new Get() behavior. I also tried to improve some of the
comments along the way. Finally, this CL rolls back a mitigation [2]
previously landed for this crash.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/2586019
[2] https://chromium-review.googlesource.com/c/chromium/src/+/2595967

Bug: 1159328, 1159727
Change-Id: I47fbf33b2313b9ae2efe229443af6e8c9a1920a9
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597040
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Yu Han <yuzhehan@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838974}
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit f5e7e1f into master Dec 22, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-2597040 branch December 22, 2020 23:18
tetsuharuohzeki added a commit to tetsuharuohzeki/wpt that referenced this pull request Feb 5, 2021
…` instead of `const` to declare variables

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests#26951
   - web-platform-tests#27001

So I think this `root` should be declare by `let` instead of `const`.
rniwa pushed a commit that referenced this pull request Feb 9, 2021
…` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - #26951
   - #27001

So I think this `root` should be declare by `let` instead of `const`.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 12, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 12, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Feb 16, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510

UltraBlame original commit: 231e9e2b8cdc2dda685ec5b06c27d504ca7f74f1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Feb 16, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510

UltraBlame original commit: eda94059722d529ebe7c53bc516b0ac747f0bcfa
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 16, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510

UltraBlame original commit: 231e9e2b8cdc2dda685ec5b06c27d504ca7f74f1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Feb 16, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510

UltraBlame original commit: eda94059722d529ebe7c53bc516b0ac747f0bcfa
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 16, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510

UltraBlame original commit: 231e9e2b8cdc2dda685ec5b06c27d504ca7f74f1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Feb 16, 2021
…ve-crash.html should use `let` instead of `const` to declare variables, a=testonly

Automatic update from web-platform-tests
[shadow-dom] shadow-dom/nested-slot-remove-crash.html should use `let` instead of `const` to declare variables (#27510)

1. If this code runs on Chromium/Firefox/WebKit,
  [this test](https://github.com/web-platform-tests/wpt/blob/master/shadow-dom/nested-slot-remove-crash.html) does not crash.
2. But by the browser's developer tools console, the execution is stopped at
   https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L30 by `SyntaxError`.
    - By the comment, I seem this test is expected to reach to
     https://github.com/web-platform-tests/wpt/blob/0fa6dc568c006811e1fc0035d3b9c70dd7efadf5/shadow-dom/nested-slot-remove-crash.html#L39-L40.
3. By pull requests which imported this tests, I feel their original bug is related to DOM.
   - web-platform-tests/wpt#26951
   - web-platform-tests/wpt#27001

So I think this `root` should be declare by `let` instead of `const`.
--

wpt-commits: 90d8d30d6ccc96cec77aa66619e085ca4e6f11df
wpt-pr: 27510

UltraBlame original commit: eda94059722d529ebe7c53bc516b0ac747f0bcfa
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.

None yet

3 participants