Bug 1303605: Make LazyFC assertions actually hold. r?bz draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 29 Mar 2018 14:04:40 +0200
changeset 775949 c1c50627d70eb97d57be56af984e7a5164a33a65
parent 775948 fd33bfa6bab482db4cd942481f21b7eb84fad6e4
child 775950 40f1d6b8def5561e696f8d7d4101d2fc4f1c9769
push id104762
push userbmo:emilio@crisal.io
push dateMon, 02 Apr 2018 03:56:19 +0000
reviewersbz
bugs1303605, 1410020, 1450027
milestone61.0a1
Bug 1303605: Make LazyFC assertions actually hold. r?bz So while removing that wallpaper I started hitting the !noPrimaryFrame assertions on XBL + HMTL stuff in display-contents-xbl-3.xbl. The code was trying to assert that we had frames constructed for all the nodes in the parent chain, but we don't bail out in the !GetContentInsertionFrameFor(aContainer) in the case that it's a children element, because they actually have no insertion frame, though their children do. Move the LazyFC check after the insertion point check. That makes the previous check work on the insertion point of the child, which makes it sound. This also fixes bug 1410020, and with it a Shadow DOM test-case that was failing because the content other slot wasn't getting properly flagged, and thus the slotted content never got a frame. The other two test failures in this test are an event dispatch failure, where the position of the target is not what the test expects (we don't account for margin and padding). Filed that as bug 1450027. Also, added a test for which we have wrong layout without these patches, and that crashes with "Called Servo_Element_IsDisplayNone" with the first applied but not this one, due to the bogus check mentioned above. MozReview-Commit-ID: 6OeaVrZhTDv
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
testing/web-platform/meta/MANIFEST.json
testing/web-platform/meta/shadow-dom/MouseEvent-prototype-offsetX-offsetY.html.ini
testing/web-platform/tests/css/css-scoping/shadow-assign-dynamic-001.html
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -6853,19 +6853,19 @@ nsCSSFrameConstructor::CheckBitsForLazyF
     }
 
     content = content->GetFlattenedTreeParent();
   }
   if (content && content->GetPrimaryFrame() &&
       content->GetPrimaryFrame()->IsLeaf()) {
     noPrimaryFrame = needsFrameBitSet = false;
   }
-  NS_ASSERTION(!noPrimaryFrame, "Ancestors of nodes with frames to be "
+  MOZ_ASSERT(!noPrimaryFrame, "Ancestors of nodes with frames to be "
     "constructed lazily should have frames");
-  NS_ASSERTION(!needsFrameBitSet, "Ancestors of nodes with frames to be "
+  MOZ_ASSERT(!needsFrameBitSet, "Ancestors of nodes with frames to be "
     "constructed lazily should not have NEEDS_FRAME bit set");
 }
 #endif
 
 // Returns true if this operation can be lazy, false if not.
 //
 // FIXME(emilio, bug 1410020): This function assumes that the flattened tree
 // parent of all the appended children is the same, which, afaict, is not
@@ -6934,28 +6934,29 @@ nsCSSFrameConstructor::MaybeConstructLaz
 
   return true;
 }
 
 
 void
 nsCSSFrameConstructor::IssueSingleInsertNofications(nsIContent* aContainer,
                                                     nsIContent* aStartChild,
-                                                    nsIContent* aEndChild)
+                                                    nsIContent* aEndChild,
+                                                    InsertionKind aInsertionKind)
 {
   for (nsIContent* child = aStartChild;
        child != aEndChild;
        child = child->GetNextSibling()) {
     // listboxes suck.
     MOZ_ASSERT(MaybeGetListBoxBodyFrame(aContainer, child) ||
                !child->GetPrimaryFrame());
 
     // Call ContentRangeInserted with this node.
     ContentRangeInserted(aContainer, child, child->GetNextSibling(),
-                         mTempFrameTreeState, InsertionKind::Sync);
+                         mTempFrameTreeState, aInsertionKind);
   }
 }
 
 bool
 nsCSSFrameConstructor::InsertionPoint::IsMultiple() const
 {
   if (!mParentFrame) {
     return false;
@@ -6974,25 +6975,27 @@ nsCSSFrameConstructor::InsertionPoint::I
   }
 
   return false;
 }
 
 nsCSSFrameConstructor::InsertionPoint
 nsCSSFrameConstructor::GetRangeInsertionPoint(nsIContent* aContainer,
                                               nsIContent* aStartChild,
-                                              nsIContent* aEndChild)
+                                              nsIContent* aEndChild,
+                                              InsertionKind aInsertionKind)
 {
   MOZ_ASSERT(aStartChild);
 
   // If the children of the container may be distributed to different insertion
   // points, insert them separately and bail out, letting ContentInserted handle
   // the mess.
   if (aContainer->GetShadowRoot() || aContainer->GetXBLBinding()) {
-    IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
+    IssueSingleInsertNofications(
+      aContainer, aStartChild, aEndChild, aInsertionKind);
     return { };
   }
 
 #ifdef DEBUG
   {
     nsIContent* expectedParent = aStartChild->GetFlattenedTreeParent();
     for (nsIContent* child = aStartChild->GetNextSibling(); child;
          child = child->GetNextSibling()) {
@@ -7001,17 +7004,18 @@ nsCSSFrameConstructor::GetRangeInsertion
   }
 #endif
 
   // Now the flattened tree parent of all the siblings is the same, just use the
   // same insertion point and take the fast path, unless it's a multiple
   // insertion point.
   InsertionPoint ip = GetInsertionPoint(aStartChild);
   if (ip.IsMultiple()) {
-    IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
+    IssueSingleInsertNofications(
+      aContainer, aStartChild, aEndChild, aInsertionKind);
     return { };
   }
 
   return ip;
 }
 
 bool
 nsCSSFrameConstructor::MaybeRecreateForFrameset(nsIFrame* aParentFrame,
@@ -7126,19 +7130,22 @@ nsCSSFrameConstructor::ContentAppended(n
     // Just ignore tree tags, anyway we don't create any frames for them.
     if (tag == nsGkAtoms::treechildren ||
         tag == nsGkAtoms::treeitem ||
         tag == nsGkAtoms::treerow)
       return;
   }
 #endif // MOZ_XUL
 
-  // See comment in ContentRangeInserted for why this is necessary.
-  if (!GetContentInsertionFrameFor(aContainer) &&
-      !aContainer->IsActiveChildrenElement()) {
+  LAYOUT_PHASE_TEMP_EXIT();
+  InsertionPoint insertion =
+    GetRangeInsertionPoint(aContainer, aFirstNewContent, nullptr, aInsertionKind);
+  nsContainerFrame*& parentFrame = insertion.mParentFrame;
+  LAYOUT_PHASE_TEMP_REENTER();
+  if (!parentFrame) {
     // We're punting on frame construction because there's no container frame.
     // The Servo-backed style system handles this case like the lazy frame
     // construction case, except when we're already constructing frames, in
     // which case we shouldn't need to do anything else.
     if (aInsertionKind == InsertionKind::Async) {
       LazilyStyleNewChildRange(aFirstNewContent, nullptr);
     }
     return;
@@ -7153,25 +7160,16 @@ nsCSSFrameConstructor::ContentAppended(n
   // We couldn't construct lazily. Make Servo eagerly traverse the new content
   // if needed (when aInsertionKind == InsertionKind::Sync, we know that the
   // styles are up-to-date already).
   if (aInsertionKind == InsertionKind::Async) {
     StyleNewChildRange(aFirstNewContent, nullptr);
   }
 
   LAYOUT_PHASE_TEMP_EXIT();
-  InsertionPoint insertion =
-    GetRangeInsertionPoint(aContainer, aFirstNewContent, nullptr);
-  nsContainerFrame*& parentFrame = insertion.mParentFrame;
-  LAYOUT_PHASE_TEMP_REENTER();
-  if (!parentFrame) {
-    return;
-  }
-
-  LAYOUT_PHASE_TEMP_EXIT();
   if (MaybeRecreateForFrameset(parentFrame, aFirstNewContent, nullptr)) {
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
   LAYOUT_PHASE_TEMP_REENTER();
 
   if (parentFrame->IsLeaf()) {
     // Nothing to do here; we shouldn't be constructing kids of leaves
@@ -7532,17 +7530,18 @@ nsCSSFrameConstructor::ContentRangeInser
                             // doesn't use "old next sibling".
                             aStartChild, nullptr, nullptr, CONTENT_INSERTED)) {
         return;
       }
     } else {
       // We don't handle a range insert to a listbox parent, issue single
       // ContertInserted calls for each node inserted.
       LAYOUT_PHASE_TEMP_EXIT();
-      IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
+      IssueSingleInsertNofications(
+        aContainer, aStartChild, aEndChild, InsertionKind::Sync);
       LAYOUT_PHASE_TEMP_REENTER();
       return;
     }
   }
 #endif // MOZ_XUL
 
   // If we have a null parent, then this must be the document element being
   // inserted, or some other child of the document in the DOM (might be a PI,
@@ -7583,76 +7582,61 @@ nsCSSFrameConstructor::ContentRangeInser
       accService->ContentRangeInserted(mPresShell, aContainer,
                                        aStartChild, aEndChild);
     }
 #endif
 
     return;
   }
 
-  nsContainerFrame* parentFrame = GetContentInsertionFrameFor(aContainer);
-  // The xbl:children element won't have a frame, but default content can have the children as
-  // a parent. While its uncommon to change the structure of the default content itself, a label,
-  // for example, can be reframed by having its value attribute set or removed.
-  if (!parentFrame &&
-      !(aContainer->IsActiveChildrenElement() || aContainer->IsShadowRoot())) {
+  InsertionPoint insertion;
+  if (isSingleInsert) {
+    // See if we have an XBL insertion point. If so, then that's our
+    // real parent frame; if not, then the frame hasn't been built yet
+    // and we just bail.
+    insertion = GetInsertionPoint(aStartChild);
+  } else {
+    // Get our insertion point. If we need to issue single ContentInserted's
+    // GetRangeInsertionPoint will take care of that for us.
+    LAYOUT_PHASE_TEMP_EXIT();
+    insertion = GetRangeInsertionPoint(aContainer, aStartChild, aEndChild, aInsertionKind);
+    LAYOUT_PHASE_TEMP_REENTER();
+  }
+
+  if (!insertion.mParentFrame) {
     // We're punting on frame construction because there's no container frame.
     // The Servo-backed style system handles this case like the lazy frame
     // construction case, except when we're already constructing frames, in
     // which case we shouldn't need to do anything else.
     if (aInsertionKind == InsertionKind::Async) {
       LazilyStyleNewChildRange(aStartChild, aEndChild);
     }
     return;
   }
 
-  MOZ_ASSERT_IF(aContainer->IsShadowRoot(), !parentFrame);
-
-  // Otherwise, we've got parent content. Find its frame.
-  NS_ASSERTION(!parentFrame || parentFrame->GetContent() == aContainer ||
-               IsDisplayContents(aContainer),
-               "New XBL code is possibly wrong!");
-
   if (aInsertionKind == InsertionKind::Async &&
       MaybeConstructLazily(CONTENTINSERT, aStartChild)) {
     LazilyStyleNewChildRange(aStartChild, aEndChild);
     return;
   }
 
   // We couldn't construct lazily. Make Servo eagerly traverse the new content
   // if needed.
   styleNewChildRangeEagerly();
 
-  InsertionPoint insertion;
-  if (isSingleInsert) {
-    // See if we have an XBL insertion point. If so, then that's our
-    // real parent frame; if not, then the frame hasn't been built yet
-    // and we just bail.
-    insertion = GetInsertionPoint(aStartChild);
-  } else {
-    // Get our insertion point. If we need to issue single ContentInserted's
-    // GetRangeInsertionPoint will take care of that for us.
-    LAYOUT_PHASE_TEMP_EXIT();
-    insertion = GetRangeInsertionPoint(aContainer, aStartChild, aEndChild);
-    LAYOUT_PHASE_TEMP_REENTER();
-  }
-
-  if (!insertion.mParentFrame) {
-    return;
-  }
-
   bool isAppend, isRangeInsertSafe;
   nsIFrame* prevSibling = GetInsertionPrevSibling(&insertion, aStartChild,
                                                   &isAppend, &isRangeInsertSafe);
 
   // check if range insert is safe
   if (!isSingleInsert && !isRangeInsertSafe) {
     // must fall back to a single ContertInserted for each child in the range
     LAYOUT_PHASE_TEMP_EXIT();
-    IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
+    IssueSingleInsertNofications(
+        aContainer, aStartChild, aEndChild, InsertionKind::Sync);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
   LayoutFrameType frameType = insertion.mParentFrame->Type();
   LAYOUT_PHASE_TEMP_EXIT();
   if (MaybeRecreateForFrameset(insertion.mParentFrame, aStartChild, aEndChild)) {
     LAYOUT_PHASE_TEMP_REENTER();
@@ -7774,17 +7758,18 @@ nsCSSFrameConstructor::ContentRangeInser
 
       // Need check whether a range insert is still safe.
       if (!isSingleInsert && !isRangeInsertSafe) {
         // Need to recover the letter frames first.
         RecoverLetterFrames(state.mFloatedItems.containingBlock);
 
         // must fall back to a single ContertInserted for each child in the range
         LAYOUT_PHASE_TEMP_EXIT();
-        IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
+        IssueSingleInsertNofications(
+          aContainer, aStartChild, aEndChild, InsertionKind::Sync);
         LAYOUT_PHASE_TEMP_REENTER();
         return;
       }
 
       frameType = insertion.mParentFrame->Type();
     }
   }
 
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -120,17 +120,18 @@ private:
 #else
   void CheckBitsForLazyFrameConstruction(nsIContent*) {}
 #endif
 
   // Issues a single ContentInserted for each child of aContainer in the range
   // [aStartChild, aEndChild).
   void IssueSingleInsertNofications(nsIContent* aContainer,
                                     nsIContent* aStartChild,
-                                    nsIContent* aEndChild);
+                                    nsIContent* aEndChild,
+                                    InsertionKind);
 
   /**
    * Data that represents an insertion point for some child content.
    */
   struct InsertionPoint
   {
     InsertionPoint()
       : mParentFrame(nullptr)
@@ -168,17 +169,18 @@ private:
    * Checks if the children of aContainer in the range [aStartChild, aEndChild)
    * can be inserted/appended to one insertion point together. If so, returns
    * that insertion point. If not, returns with InsertionPoint.mFrame == nullptr
    * and issues single ContentInserted calls for each child.
    * aEndChild = nullptr indicates that we are dealing with an append.
    */
   InsertionPoint GetRangeInsertionPoint(nsIContent* aContainer,
                                         nsIContent* aStartChild,
-                                        nsIContent* aEndChild);
+                                        nsIContent* aEndChild,
+                                        InsertionKind);
 
   // Returns true if parent was recreated due to frameset child, false otherwise.
   bool MaybeRecreateForFrameset(nsIFrame* aParentFrame,
                                 nsIContent* aStartChild,
                                 nsIContent* aEndChild);
 
   /**
    * For each child in the aStartChild/aEndChild range, calls
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -124012,16 +124012,28 @@
       [
        "/css/css-scoping/reference/green-box.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/css-scoping/shadow-assign-dynamic-001.html": [
+    [
+     "/css/css-scoping/shadow-assign-dynamic-001.html",
+     [
+      [
+       "/css/css-scoping/reference/green-box.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/css-scoping/shadow-disabled-sheet-001.html": [
     [
      "/css/css-scoping/shadow-disabled-sheet-001.html",
      [
       [
        "/css/css-scoping/reference/green-box.html",
        "=="
       ]
@@ -500973,16 +500985,20 @@
   "css/css-scoping/css-scoping-shadow-with-rules.html": [
    "69774c51e6df49c891ecaaf2c384552106a1eb32",
    "reftest"
   ],
   "css/css-scoping/reference/green-box.html": [
    "a736f68dc602c0fccab56ec5cc6234cb3298c88d",
    "support"
   ],
+  "css/css-scoping/shadow-assign-dynamic-001.html": [
+   "c57e0fd5aa5be63e1cadf65a4e382798c5e05ec4",
+   "reftest"
+  ],
   "css/css-scoping/shadow-cascade-order-001.html": [
    "46913ea7e47811b11be898de5c3bd0a330ea6637",
    "testharness"
   ],
   "css/css-scoping/shadow-disabled-sheet-001.html": [
    "3de2d23c1b3339b964ec2c009832a3207a3b9dc4",
    "reftest"
   ],
--- a/testing/web-platform/meta/shadow-dom/MouseEvent-prototype-offsetX-offsetY.html.ini
+++ b/testing/web-platform/meta/shadow-dom/MouseEvent-prototype-offsetX-offsetY.html.ini
@@ -1,10 +1,7 @@
 [MouseEvent-prototype-offsetX-offsetY.html]
   [MouseEvent's offsetX and offsetY attributes must be relative to the target.]
     expected: FAIL
 
   [MouseEvent's offsetX and offsetY attributes must be relative to the shadow host when an event is dispatched inside its shadow tree.]
     expected: FAIL
 
-  [MouseEvent's offsetX and offsetY attributes must be relative to the target when an event is dispatched on a slotted content.]
-    expected: FAIL
-
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-scoping/shadow-assign-dynamic-001.html
@@ -0,0 +1,29 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSS Scoping Module Level 1 - Dynamic mutations to both shadow root and shadow host subtrees</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://drafts.csswg.org/css-scoping/#selectors-data-model">
+<link rel="help" href="https://bugzil.la/1303605">
+<link rel="match" href="reference/green-box.html"/>
+<p>Test passes if you see a single 100px by 100px green box below.</p>
+<style>
+  #host {
+    width: 100px;
+    height: 100px;
+    background: red;
+  }
+
+  #host > div {
+    width: 100px;
+    height: 50px;
+    background: green;
+  }
+</style>
+<div id="host"></div>
+<script>
+  let root = host.attachShadow({ mode: "open" });
+  document.body.offsetTop;
+
+  root.innerHTML = `<slot name="slot1"></slot><slot name="slot2"></slot>`;
+  host.innerHTML = `<div slot="slot1"></div><div slot="slot2"></div>`;
+</script>