Bug 1409088: Fix destination insertion point removal algorithm. r=bz

When an insertion point (a) is added to the document before another insertion point (b), and that insertion point matches nodes that used to match (b), the following happens in RedistributeAllNodes: * Loop through (a), and clear the existing insertion points on nodes distributed into it (none, since it was just inserted). * Go through the node pool and add the matched nodes. That makes the node (which already had (b) in the insertion point array) have [(b), (a)] as the insertion points. * Go through (b), and clear the existing insertion points on the nodes distributed to it. That used to do IndexOf() + SetLength(), but since (b) was the first node by then in the insertion point array, we'll leave the insertion point array empty, while (a) would still think that the node is distributed to it. This causes the bloom filter code, which loops through the flattened tree parents, to not insert any (because the node doesn't know about where it's inserted). Also, add a debug phase to verify the flat tree before restyling that would've caught this more clearly (happy to remove it if you don't think it's worth). We still can't assert that the insertion point is properly referenced due to the hacky way mInsertionPoints is cleared in HTMLContentElement::UpdateFallbackDistribution, but we'll still clear the insertion points either there, or on the rest of insertion point removal code in ShadowRoot::DistributeAllNodes. MozReview-Commit-ID: 9k2gnsAKMEe --HG-- extra : rebase_source : 7e8371199bde8148d77cb69417a8dd8b1ee77078
2017-10-19 14:45:16 +02:00 · 2017-10-19 14:45:16 +02:00 · bce283e551
--- a/dom/base/ShadowRoot.cpp
+++ b/dom/base/ShadowRoot.cpp
@ -244,9 +244,10 @@ ShadowRoot::RemoveDestInsertionPoint(nsIContent* aInsertionPoint,
  int32_t index = aDestInsertionPoints.IndexOf(aInsertionPoint);

  // It's possible that we already removed the insertion point while processing
-  // other insertion point removals.
+  // other insertion point removals / fallback content redistribution (which
+  // does DestInsertionPoints().Clear()).
  if (index >= 0) {
-    aDestInsertionPoints.SetLength(index);
+    aDestInsertionPoints.RemoveElementAt(index);
  }
 }

--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@ -1207,9 +1207,30 @@ ServoRestyleManager::DoProcessPendingRestyles(ServoTraversalFlags aFlags)
  mAnimationsWithDestroyedFrame->StopAnimationsForElementsWithoutFrames();
 }

+#ifdef DEBUG
+static void
+VerifyFlatTree(const nsIContent& aContent)
+{
+  StyleChildrenIterator iter(&aContent);
+
+  for (auto* content = iter.GetNextChild();
+       content;
+       content = iter.GetNextChild()) {
+    MOZ_ASSERT(content->GetFlattenedTreeParentNodeForStyle() == &aContent);
+    VerifyFlatTree(*content);
+  }
+}
+#endif
+
 void
 ServoRestyleManager::ProcessPendingRestyles()
 {
+#ifdef DEBUG
+  if (auto* root = mPresContext->Document()->GetRootElement()) {
+    VerifyFlatTree(*root);
+  }
+#endif
+
  DoProcessPendingRestyles(ServoTraversalFlags::Empty);
 }

--- a/layout/base/crashtests/1409088.html
+++ b/layout/base/crashtests/1409088.html
@ -0,0 +1,16 @@
+<!doctype html>
+<div></div>
+<script>
+let host = document.querySelector('div');
+let shadow = host.createShadowRoot();
+
+// Append two divs and three spans into host.
+host.innerHTML = '<div></div><span></span><div></div><span></span><span></span>';
+shadow.innerHTML = '<content select="div" id="divpoint"></content><content select="div, span" id="allpoint"></content>';
+
+let divPoint = shadow.getElementById("divpoint");
+let allPoint = shadow.getElementById("allpoint");
+
+shadow.removeChild(allPoint);
+shadow.insertBefore(allPoint, divPoint);
+</script>
--- a/layout/base/crashtests/crashtests.list
+++ b/layout/base/crashtests/crashtests.list
@ -508,4 +508,5 @@ load 1401739.html
 load 1401840.html
 load 1402476.html
 load 1406562.html
+load 1409088.html
 load 1409147.html