Closed Bug 1684066 Opened 4 years ago Closed 4 years ago

[wpt-sync] Sync PR 27001 - Reland "Fix several crashes when nested slots are removed from a ShadowRoot"

Categories

(Core :: DOM: Core & HTML, task, P4)

task

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: wpt-sync, Unassigned)

References

()

Details

(Whiteboard: [wptsync downstream])

Sync web-platform-tests PR 27001 into mozilla-central (this bug is closed when the sync is complete).

PR: https://github.com/web-platform-tests/wpt/pull/27001
Details from upstream follow.

b'Mason Freed <masonfreed@chromium.org>' wrote:

Reland "Fix several crashes when nested slots are removed from a ShadowRoot"

This is a reland of dbfed21f94881a2918223792ebde3476b8fd69e6

Original change's description:

Fix several crashes when nested slots are removed from a ShadowRoot

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}

Bug: 1159328
Bug: 1159727
Change-Id: I0025c0f00d6b3876de8f40a60fdc34f726ddc85c

Reviewed-on: https://chromium-review.googlesource.com/2601051
WPT-Export-Revision: abb8b3ee796dba93fbdfc6a7b8191c4e16009bec

Component: web-platform-tests → DOM: Core & HTML
Product: Testing → Core

CI Results

Ran 14 Firefox configurations based on mozilla-central, and Firefox, Chrome, and Safari on GitHub CI

Total 1 tests

Status Summary

Firefox

PASS: 1

Chrome

PASS: 1

Safari

PASS: 1

Links

Gecko CI (Treeherder)
GitHub PR Head
GitHub PR Base

Pushed by wptsync@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/424acc59eebb [wpt PR 27001] - Reland "Fix several crashes when nested slots are removed from a ShadowRoot", a=testonly
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.