Less fishyness when resolving the style of the document element. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 10 Jul 2017 14:32:39 +0200
changeset 606880 5fbf3ecf37903a7f61dd352ba636146140ed0412
parent 606879 cc041d32e158ca22446e1cf27a03222e6b8476d3
child 606881 db1cac4f74bf73a675ad62b1961a47ada8dd59fb
push id67817
push userbmo:emilio+bugs@crisal.io
push dateTue, 11 Jul 2017 14:32:38 +0000
milestone56.0a1
Less fishyness when resolving the style of the document element. Previous to these patches, the style resolution happening on [1] made the style data stick on the element, so we'd never think it was the initial style, even if it was. This was wallpapering the fact that, if that was the initial style, we'd never have another chance of traversing the document when [2] kicked in. This somehow just happened to work, but is a very fishy way to get it to work. Instead, call StyleDocument() properly _before_, and rely on the fact that it will stop when it has a non-null binding, and only if it fails explicitly style the children. This fixes the few XBL test-cases with this patch series that exercise -moz-bindings on the root element without the root being styled, and makes the -moz-binding code more consistent in both places of the frame constructor. [1]: http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/base/nsCSSFrameConstructor.cpp#2526 [2]: https://github.com/servo/servo/blob/3330653dc80cc8947af17f1989fb7fbf390c4d2f/components/style/traversal.rs#L439 MozReview-Commit-ID: HbjsD6nYsvX
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsPresContext.cpp
servo/components/style/traversal.rs
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -2514,23 +2514,30 @@ nsCSSFrameConstructor::ConstructDocEleme
   // the future.  We need this, because the document element might
   // have stale restyle bits from a previous frame constructor for
   // this document.  Unlike in AddFrameConstructionItems, it's safe to
   // unset all element restyle flags, since we don't have any
   // siblings.
   aDocElement->UnsetRestyleFlagsIfGecko();
 
   // --------- CREATE AREA OR BOX FRAME -------
+  if (ServoStyleSet* set = mPresShell->StyleSet()->GetAsServo()) {
+    // NOTE(emilio): If the root has a non-null binding, we'll stop at the
+    // document element and won't process any children, loading the bindings (or
+    // failing to do so) will take care of the rest.
+    set->StyleDocument(TraversalRestyleBehavior::Normal);
+  }
+
   // FIXME: Should this use ResolveStyleContext?  (The calls in this
   // function are the only case in nsCSSFrameConstructor where we don't
   // do so for the construction of a style context for an element.)
-  RefPtr<nsStyleContext> styleContext;
-  styleContext = mPresShell->StyleSet()->ResolveStyleFor(aDocElement,
-                                                         nullptr,
-                                                         LazyComputeBehavior::Allow);
+  RefPtr<nsStyleContext> styleContext =
+    mPresShell->StyleSet()->ResolveStyleFor(aDocElement,
+                                            nullptr,
+                                            LazyComputeBehavior::Assert);
 
   const nsStyleDisplay* display = styleContext->StyleDisplay();
 
   // Ensure that our XBL bindings are installed.
   if (display->mBinding) {
     // Get the XBL loader.
     nsresult rv;
     bool resolveStyle;
@@ -2539,42 +2546,48 @@ nsCSSFrameConstructor::ConstructDocEleme
     if (!xblService) {
       return nullptr;
     }
 
     RefPtr<nsXBLBinding> binding;
     rv = xblService->LoadBindings(aDocElement, display->mBinding->GetURI(),
                                   display->mBinding->mExtraData->GetPrincipal(),
                                   getter_AddRefs(binding), &resolveStyle);
-    if (NS_FAILED(rv) && rv != NS_ERROR_XBL_BLOCKED)
-      return nullptr; // Binding will load asynchronously.
+    if (NS_FAILED(rv) && rv != NS_ERROR_XBL_BLOCKED) {
+      // Binding will load asynchronously.
+      if (aDocElement->IsStyledByServo()) {
+        ServoRestyleManager::ClearServoDataFromSubtree(aDocElement);
+      }
+      return nullptr;
+    }
 
     if (binding) {
       // For backwards compat, keep firing the root's constructor
       // after all of its kids' constructors.  So tell the binding
       // manager about it right now.
       mDocument->BindingManager()->AddToAttachedQueue(binding);
     }
 
     if (resolveStyle) {
       // FIXME: Should this use ResolveStyleContext?  (The calls in this
       // function are the only case in nsCSSFrameConstructor where we
       // don't do so for the construction of a style context for an
       // element.)
-      styleContext = mPresShell->StyleSet()->ResolveStyleFor(aDocElement,
-                                                             nullptr,
-                                                             LazyComputeBehavior::Allow);
+      //
+      // FIXME(emilio): This looks fishy. It really wants to fully re-resolve
+      // the style, but it wont if the element is already styled afaict... It
+      // seems we handle it on a subsequent restyle?
+      styleContext = mPresShell->StyleSet()->ResolveStyleFor(
+          aDocElement, nullptr, LazyComputeBehavior::Assert);
       display = styleContext->StyleDisplay();
     }
-  }
-
-  // We delay traversing the entire document until here, since we per above we
-  // may invalidate the root style when we load doc stylesheets.
-  if (ServoStyleSet* set = mPresShell->StyleSet()->GetAsServo()) {
-    set->StyleDocument(TraversalRestyleBehavior::Normal);
+  } else if (display->mBinding.ForceGet() && aDocElement->IsStyledByServo()) {
+    // See the comment in AddFrameConstructionItemsInternal for why this is
+    // needed.
+    mPresShell->StyleSet()->AsServo()->StyleNewChildren(aDocElement);
   }
 
   // --------- IF SCROLLABLE WRAP IN SCROLLFRAME --------
 
   NS_ASSERTION(!display->IsScrollableOverflow() ||
                state.mPresContext->IsPaginated() ||
                propagatedScrollFrom == aDocElement,
                "Scrollbars should have been propagated to the viewport");
@@ -5868,19 +5881,18 @@ nsCSSFrameConstructor::AddFrameConstruct
           // XXX: We should have a better way to restyle ourselves.
           ServoRestyleManager::ClearServoDataFromSubtree(element);
           styleSet->StyleNewSubtree(element);
 
           // Servo's should_traverse_children() in traversal.rs skips
           // styling descendants of elements with a -moz-binding the
           // first time. Thus call StyleNewChildren() again.
           styleSet->StyleNewChildren(element);
-
           styleContext =
-            styleSet->ResolveStyleFor(element, nullptr, LazyComputeBehavior::Allow);
+            styleSet->ResolveStyleFor(element, nullptr, LazyComputeBehavior::Assert);
         } else {
           styleContext =
             ResolveStyleContext(styleContext->GetParent(), aContent, &aState);
         }
 
         display = styleContext->StyleDisplay();
         aStyleContext = styleContext;
       }
--- a/layout/base/nsPresContext.cpp
+++ b/layout/base/nsPresContext.cpp
@@ -1480,19 +1480,18 @@ GetPropagatedScrollbarStylesForViewport(
 
   // docElement might be null if we're doing this after removing it.
   if (!docElement) {
     return nullptr;
   }
 
   // Check the style on the document root element
   StyleSetHandle styleSet = aPresContext->StyleSet();
-  RefPtr<nsStyleContext> rootStyle;
-  rootStyle = styleSet->ResolveStyleFor(docElement, nullptr,
-                                        LazyComputeBehavior::Allow);
+  RefPtr<nsStyleContext> rootStyle =
+    styleSet->ResolveStyleFor(docElement, nullptr, LazyComputeBehavior::Allow);
   if (CheckOverflow(rootStyle->StyleDisplay(), aStyles)) {
     // tell caller we stole the overflow style from the root element
     return docElement;
   }
 
   // Don't look in the BODY for non-HTML documents or HTML documents
   // with non-HTML roots
   // XXX this should be earlier; we shouldn't even look at the document root
@@ -1509,19 +1508,19 @@ GetPropagatedScrollbarStylesForViewport(
   nsCOMPtr<nsIContent> bodyElement = do_QueryInterface(body);
 
   if (!bodyElement ||
       !bodyElement->NodeInfo()->Equals(nsGkAtoms::body)) {
     // The body is not a <body> tag, it's a <frameset>.
     return nullptr;
   }
 
-  RefPtr<nsStyleContext> bodyStyle;
-  bodyStyle = styleSet->ResolveStyleFor(bodyElement->AsElement(), rootStyle,
-                                        LazyComputeBehavior::Allow);
+  RefPtr<nsStyleContext> bodyStyle =
+    styleSet->ResolveStyleFor(bodyElement->AsElement(), rootStyle,
+                              LazyComputeBehavior::Allow);
 
   if (CheckOverflow(bodyStyle->StyleDisplay(), aStyles)) {
     // tell caller we stole the overflow style from the body element
     return bodyElement;
   }
 
   return nullptr;
 }
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -426,16 +426,21 @@ pub trait DomTraversal<E: TElement> : Sy
         // children after shuffling the subtree. That explicit traversal may in
         // turn find other bound elements, which get handled in the same way.
         //
         // We explicitly avoid handling restyles here (explicitly removing or
         // changing bindings), since that adds complexity and is rarer. If it
         // happens, we may just end up doing wasted work, since Gecko
         // recursively drops Servo ElementData when the XBL insertion parent of
         // an Element is changed.
+        //
+        // Also, we avoid culling the traversal for the root element on
+        // initialization, because we load XBL bindings for it on the frame
+        // constructor synchronously, so they're already applied, and we may
+        // not have another chance to style the children.
         if cfg!(feature = "gecko") && context.thread_local.is_initial_style() &&
            parent_data.styles.primary().has_moz_binding() {
             if log.allow() {
                 debug!("Parent {:?} has XBL binding, deferring traversal",
                        parent);
             }
             return false;
         }