Bug 1427908: Never reenter synchronously into frame construction. r?bz draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 03 Jan 2018 05:51:12 +0100
changeset 716633 381f6e4fdbabf8c50986ca96a9244cd4a24bbbaf
parent 716619 b84fe2ad1ca27fc30c2e3f609b8f766185652560
child 745065 eed9d582e730d7ad3e509515063f7b723bb7a1c5
push id94470
push userbmo:emilio@crisal.io
push dateSat, 06 Jan 2018 00:10:57 +0000
reviewersbz
bugs1427908, 1389743
milestone59.0a1
Bug 1427908: Never reenter synchronously into frame construction. r?bz We remove async from the DOM all the time now since bug 1389743. We could, before this patch, recurse into frame construction in a sync way, due to the way we handle the weird insertion cases for <fieldset>, <details>, and <mathml>. This patch makes those also async, making the IssueSingleInsertNotifications condition unnecessary. MozReview-Commit-ID: LujPaYPwA4G
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -7319,46 +7319,37 @@ nsCSSFrameConstructor::CreateNeededFrame
     CreateNeededFrames(rootElement, treeMatchContext);
     EndUpdate();
   }
 }
 
 void
 nsCSSFrameConstructor::IssueSingleInsertNofications(nsIContent* aContainer,
                                                     nsIContent* aStartChild,
-                                                    nsIContent* aEndChild,
-                                                    InsertionKind aInsertionKind)
+                                                    nsIContent* aEndChild)
 {
   for (nsIContent* child = aStartChild;
        child != aEndChild;
        child = child->GetNextSibling()) {
-    if ((child->GetPrimaryFrame() || GetDisplayNoneStyleFor(child) ||
-         GetDisplayContentsStyleFor(child))
-#ifdef MOZ_XUL
-        //  Except listboxes suck, so do NOT skip anything here if
-        //  we plan to notify a listbox.
-        && !MaybeGetListBoxBodyFrame(aContainer, child)
-#endif
-        ) {
-      // Already have a frame or undisplayed entry for this content; a
-      // previous ContentRangeInserted in this loop must have reconstructed
-      // its insertion parent.  Skip it.
-      continue;
-    }
+    // listboxes suck.
+    MOZ_ASSERT(MaybeGetListBoxBodyFrame(aContainer, child) ||
+               (!child->GetPrimaryFrame() &&
+                !GetDisplayNoneStyleFor(child) &&
+                !GetDisplayContentsStyleFor(child)));
+
     // Call ContentRangeInserted with this node.
     ContentRangeInserted(aContainer, child, child->GetNextSibling(),
-                         mTempFrameTreeState, aInsertionKind, nullptr);
+                         mTempFrameTreeState, InsertionKind::Sync, nullptr);
   }
 }
 
 nsCSSFrameConstructor::InsertionPoint
 nsCSSFrameConstructor::GetRangeInsertionPoint(nsIContent* aContainer,
                                               nsIContent* aStartChild,
-                                              nsIContent* aEndChild,
-                                              InsertionKind aInsertionKind)
+                                              nsIContent* aEndChild)
 {
   // 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.
   InsertionPoint insertionPoint = GetInsertionPoint(aContainer, nullptr);
   if (!insertionPoint.mParentFrame && !insertionPoint.mMultiple) {
     return insertionPoint; // Don't build the frames.
   }
@@ -7367,18 +7358,17 @@ nsCSSFrameConstructor::GetRangeInsertion
     // If we have multiple insertion points or if we have an insertion point
     // and the operation is not a true append or if the insertion point already
     // has explicit children, then we must fall back.
     if (insertionPoint.mMultiple || aEndChild ||
         insertionPoint.mParentFrame->GetContent()->HasChildren()) {
       // Now comes the fun part.  For each inserted child, make a
       // ContentInserted call as if it had just gotten inserted and
       // let ContentInserted handle the mess.
-      IssueSingleInsertNofications(aContainer, aStartChild, aEndChild,
-                                   aInsertionKind);
+      IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
       insertionPoint.mParentFrame = nullptr;
     }
   }
 
   return insertionPoint;
 }
 
 bool
@@ -7529,18 +7519,17 @@ nsCSSFrameConstructor::ContentAppended(n
   // if needed (when aInsertionKind == InsertionKind::Sync, we know that the
   // styles are up-to-date already).
   if (aInsertionKind == InsertionKind::Async && aContainer->IsStyledByServo()) {
     StyleNewChildRange(aFirstNewContent, nullptr);
   }
 
   LAYOUT_PHASE_TEMP_EXIT();
   InsertionPoint insertion =
-    GetRangeInsertionPoint(aContainer, aFirstNewContent, nullptr,
-                           aInsertionKind);
+    GetRangeInsertionPoint(aContainer, aFirstNewContent, nullptr);
   nsContainerFrame*& parentFrame = insertion.mParentFrame;
   LAYOUT_PHASE_TEMP_REENTER();
   if (!parentFrame) {
     return;
   }
 
   LAYOUT_PHASE_TEMP_EXIT();
   if (MaybeRecreateForFrameset(parentFrame, aFirstNewContent, nullptr)) {
@@ -7553,17 +7542,17 @@ nsCSSFrameConstructor::ContentAppended(n
     // Nothing to do here; we shouldn't be constructing kids of leaves
     // Clear lazy bits so we don't try to construct again.
     ClearLazyBits(aFirstNewContent, nullptr);
     return;
   }
 
   if (parentFrame->IsFrameOfType(nsIFrame::eMathML)) {
     LAYOUT_PHASE_TEMP_EXIT();
-    RecreateFramesForContent(parentFrame->GetContent(), aInsertionKind);
+    RecreateFramesForContent(parentFrame->GetContent(), InsertionKind::Async);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
 #ifdef DEBUG
   if (gNoisyContentUpdates && IsFramePartOfIBSplit(parentFrame)) {
     printf("nsCSSFrameConstructor::ContentAppended: parentFrame=");
     nsFrame::ListTag(stdout, parentFrame);
@@ -7924,18 +7913,17 @@ 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,
-                                   aInsertionKind);
+      IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
       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,
@@ -8021,35 +8009,33 @@ nsCSSFrameConstructor::ContentRangeInser
     // 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(aContainer, 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);
+    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,
-                                 aInsertionKind);
+    IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
   LayoutFrameType frameType = insertion.mParentFrame->Type();
   LAYOUT_PHASE_TEMP_EXIT();
   if (MaybeRecreateForFrameset(insertion.mParentFrame, aStartChild, aEndChild)) {
     LAYOUT_PHASE_TEMP_REENTER();
@@ -8066,47 +8052,49 @@ nsCSSFrameConstructor::ContentRangeInser
     // Just reframe the parent, since figuring out whether this
     // should be the new legend and then handling it is too complex.
     // We could do a little better here --- check if the fieldset already
     // has a legend which occurs earlier in its child list than this node,
     // and if so, proceed. But we'd have to extend nsFieldSetFrame
     // to locate this legend in the inserted frames and extract it.
     LAYOUT_PHASE_TEMP_EXIT();
     RecreateFramesForContent(insertion.mParentFrame->GetContent(),
-                             aInsertionKind);
+                             InsertionKind::Async);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
   // We should only get here with details when doing a single insertion because
   // we treat details frame as if it has multiple insertion points.
   MOZ_ASSERT(isSingleInsert || frameType != LayoutFrameType::Details);
   if (frameType == LayoutFrameType::Details) {
     // When inserting an element into <details>, just reframe the details frame
     // and let it figure out where the element should be laid out. It might seem
     // expensive to recreate the entire details frame, but it's the simplest way
     // to handle the insertion.
     LAYOUT_PHASE_TEMP_EXIT();
     RecreateFramesForContent(insertion.mParentFrame->GetContent(),
-                             aInsertionKind);
+                             InsertionKind::Async);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
   // Don't construct kids of leaves
   if (insertion.mParentFrame->IsLeaf()) {
     // Clear lazy bits so we don't try to construct again.
     ClearLazyBits(aStartChild, aEndChild);
     return;
   }
 
+  // FIXME(emilio): This looks terribly inefficient if you insert elements deep
+  // in a MathML subtree.
   if (insertion.mParentFrame->IsFrameOfType(nsIFrame::eMathML)) {
     LAYOUT_PHASE_TEMP_EXIT();
     RecreateFramesForContent(insertion.mParentFrame->GetContent(),
-                             aInsertionKind);
+                             InsertionKind::Async);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
   Maybe<TreeMatchContext> matchContext;
   if (!aProvidedTreeMatchContext && !aContainer->IsStyledByServo()) {
     // We use GetParentElementCrossingShadowRoot to handle the case where
     // aContainer is a ShadowRoot.
@@ -8177,18 +8165,17 @@ 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,
-                                     aInsertionKind);
+        IssueSingleInsertNofications(aContainer, aStartChild, aEndChild);
         LAYOUT_PHASE_TEMP_REENTER();
         return;
       }
 
       frameType = insertion.mParentFrame->Type();
     }
   }
 
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -128,18 +128,17 @@ 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,
-                                    InsertionKind);
+                                    nsIContent* aEndChild);
 
   /**
    * Data that represents an insertion point for some child content.
    */
   struct InsertionPoint
   {
     InsertionPoint()
       : mParentFrame(nullptr), mContainer(nullptr), mMultiple(false) {}
@@ -171,18 +170,17 @@ 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,
-                                        InsertionKind);
+                                        nsIContent* aEndChild);
 
   // 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