1683992 - [wpt-sync] Sync PR 26993 - Revert "Fix several crashes when nested slots are removed from a ShadowRoot"

Status: RESOLVED FIXED

(Testing :: web-platform-tests, task, P4)

Description

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

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

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

b'Findit <[e-mail]>' wrote:

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

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 \<[e-mail]>
Reviewed-by: Yu Han \<[e-mail]>
Reviewed-by: Joey Arhar \<[e-mail]>
Auto-Submit: Mason Freed \<[e-mail]>
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}

Comment 1

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

PR 26993 applied with additional changes from upstream: f5e7e1f070892dd8257d3dce7d5f26746d27092c

Comment 2

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d983cb3fc7e46cab740a373fb8709113f72db59

Comment 3

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Test result changes from PR not available.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05ddb0864295
[wpt PR 26993] - Revert "Fix several crashes when nested slots are removed from a ShadowRoot", a=testonly

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/05ddb0864295