Skip to content

Commit

Permalink
Revert "Fix several crashes when nested slots are removed from a Shad…
Browse files Browse the repository at this point in the history
…owRoot"

This reverts commit dbfed21f94881a2918223792ebde3476b8fd69e6.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 838974 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2RiZmVkMjFmOTQ4ODFhMjkxODIyMzc5MmViZGUzNDc2YjhmZDY5ZTYM

Sample Failed Build: https://ci.chromium.org/b/8860163671563368608

Sample Failed Step: webkit_unit_tests

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}

Change-Id: I97202c545f74df090124e82775fe79ce978d3d63
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1159328, 1159727
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2601758
Cr-Commit-Position: refs/heads/master@{#839038}
  • Loading branch information
Findit authored and chromium-wpt-export-bot committed Dec 23, 2020
1 parent e5a6179 commit 1c5816e
Showing 1 changed file with 0 additions and 41 deletions.
41 changes: 0 additions & 41 deletions shadow-dom/nested-slot-remove-crash.html

This file was deleted.

0 comments on commit 1c5816e

Please sign in to comment.