Bug 1461749: Fix slow path for finding the next sibling frame for appending in presence of Shadow DOM. r?mats draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 23 May 2018 18:20:33 +0200
changeset 798993 861e8cb149b239af55542edd48bf36cc83297a94
parent 798853 b82436267d2b685944da87b81b5d8a89a555d0d2
child 798994 79adf271faddba9e2446659d6ac3e728adf49c61
push id110917
push userbmo:emilio@crisal.io
push dateWed, 23 May 2018 22:21:30 +0000
reviewersmats
bugs1461749
milestone62.0a1
Bug 1461749: Fix slow path for finding the next sibling frame for appending in presence of Shadow DOM. r?mats In this case we have a shadow hoot with display: contents, with no children. Those children wouldn't be flattened tree children of the shadow host. Instead of using the last light dom child and seek to it, use FlattenedChildIterator's reverse iteration. MozReview-Commit-ID: 18XL5Ong7ww
layout/base/crashtests/1461749.html
layout/base/crashtests/crashtests.list
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
new file mode 100644
--- /dev/null
+++ b/layout/base/crashtests/1461749.html
@@ -0,0 +1,19 @@
+<style>
+:last-of-type {
+  display: contents;
+}
+</style>
+<script>
+function start() {
+  o1 = document.createElement('footer')
+  o2 = document.createElement('t')
+  document.documentElement.appendChild(o1)
+  document.documentElement.appendChild(o2)
+  o3 = o1.attachShadow({
+      mode: "open"
+  })
+  o2.getClientRects()
+  o3.innerHTML = ">"
+}
+window.addEventListener('load', start)
+</script>
--- a/layout/base/crashtests/crashtests.list
+++ b/layout/base/crashtests/crashtests.list
@@ -529,8 +529,9 @@ load 1443027-1.html
 load 1448841-1.html
 load 1452839.html
 load 1453702.html
 load 1453342.html
 load 1453196.html
 pref(dom.webcomponents.shadowdom.enabled,true) load 1414303.html
 load 1461812.html
 load 1462412.html
+pref(dom.webcomponents.shadowdom.enabled,true) load 1461749.html
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -7033,16 +7033,36 @@ nsCSSFrameConstructor::StyleNewChildRang
                  "GetFlattenedTreeParent and ChildIterator don't agree, fix this!");
     }
 #endif
 
     styleSet->StyleNewSubtree(childElement);
   }
 }
 
+nsIFrame*
+nsCSSFrameConstructor::FindNextSiblingForAppend(const InsertionPoint& aInsertion)
+{
+  auto SlowPath = [&]() -> nsIFrame* {
+    FlattenedChildIterator iter(aInsertion.mContainer,
+                                /* aStartAtBeginning = */ false);
+    iter.GetPreviousChild(); // Prime the iterator.
+    StyleDisplay unused = UNSET_DISPLAY;
+    return FindNextSibling(iter, unused);
+  };
+
+  if (!IsDisplayContents(aInsertion.mContainer) &&
+      !nsLayoutUtils::GetAfterFrame(aInsertion.mContainer)) {
+    MOZ_ASSERT(!SlowPath());
+    return nullptr;
+  }
+
+  return SlowPath();
+}
+
 void
 nsCSSFrameConstructor::ContentAppended(nsIContent* aFirstNewContent,
                                        InsertionKind aInsertionKind)
 {
   MOZ_ASSERT(aInsertionKind == InsertionKind::Sync ||
              !RestyleManager()->IsInStyleRefresh());
 
   AUTO_LAYOUT_PHASE_ENTRY_POINT(mPresShell->GetPresContext(), FrameC);
@@ -7127,27 +7147,17 @@ nsCSSFrameConstructor::ContentAppended(n
   }
 #endif
 
   // We should never get here with fieldsets or details, since they have
   // multiple insertion points.
   MOZ_ASSERT(!parentFrame->IsFieldSetFrame() && !parentFrame->IsDetailsFrame(),
              "Parent frame should not be fieldset or details!");
 
-  // Deal with possible :after generated content on the parent, or display:
-  // contents.
-  nsIFrame* nextSibling = nullptr;
-  if (IsDisplayContents(insertion.mContainer) ||
-      nsLayoutUtils::GetAfterFrame(insertion.mContainer)) {
-    FlattenedChildIterator iter(insertion.mContainer);
-    iter.Seek(insertion.mContainer->GetLastChild());
-    StyleDisplay unused = UNSET_DISPLAY;
-    nextSibling = FindNextSibling(iter, unused);
-  }
-
+  nsIFrame* nextSibling = FindNextSiblingForAppend(insertion);
   if (nextSibling) {
     parentFrame = nextSibling->GetParent()->GetContentInsertionFrame();
   } else {
     parentFrame =
       ::ContinuationToAppendTo(parentFrame);
   }
 
   nsContainerFrame* containingBlock = GetFloatContainingBlock(parentFrame);
@@ -7166,20 +7176,17 @@ nsCSSFrameConstructor::ContentAppended(n
 
     // Before we get going, remove the current letter frames
     RemoveLetterFrames(mPresShell, containingBlock);
 
     // Reget nextSibling, since we may have killed it.
     //
     // FIXME(emilio): This kinda sucks! :(
     if (nextSibling && !wf) {
-      FlattenedChildIterator iter(insertion.mContainer);
-      iter.Seek(insertion.mContainer->GetLastChild());
-      StyleDisplay unused = UNSET_DISPLAY;
-      nextSibling = FindNextSibling(iter, unused);
+      nextSibling = FindNextSiblingForAppend(insertion);
       if (nextSibling) {
         parentFrame = nextSibling->GetParent()->GetContentInsertionFrame();
         containingBlock = GetFloatContainingBlock(parentFrame);
       }
     }
   }
 
   // Create some new frames
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -2011,16 +2011,24 @@ private:
    * resolved for aFrameItems are wrong (they don't take ::first-line into
    * account), and we should fix them up, which is what this method does.
    *
    * This method does not mutate aFrameItems.
    */
   void CheckForFirstLineInsertion(nsIFrame* aParentFrame,
                                   nsFrameItems& aFrameItems);
 
+  /**
+   * Find the next frame for appending to a given insertion point.
+   *
+   * We're appending, so this is almost always null, except for a few edge
+   * cases.
+   */
+  nsIFrame* FindNextSiblingForAppend(const InsertionPoint&);
+
   // The direction in which we should look for siblings.
   enum class SiblingDirection
   {
     Forward,
     Backward,
   };
 
   /**