Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 23 Jun 2017 19:31:35 +0200
changeset 600268 00dca8a43c42b939b619ba7f5721b7f214316522
parent 600267 f987df55a8c57b6a4d2718bcdaf59b3b7e587aa2
child 600269 ba2fbd7083c041c5eb1b29571684ae5c2b6381f2
push id65700
push userbmo:emilio+bugs@crisal.io
push dateSun, 25 Jun 2017 04:57:07 +0000
reviewersheycam
bugs1375674
milestone56.0a1
Bug 1375674: Track the owner that represents the changes handled, and fix a bunch of issues while at it. r?heycam In particular, this fixes: * Restyling of <area> elements that reuse the primary frame mapping for the image map. * Restyling of children of display: contents elements when the display: contents element generates a hint. * Restyling of out-of-flows (if my analysis in the bug is right). * Restyling of the ::backdrop pseudo-element. * Restyling of the Viewport frame. I only managed to do a reftest for the second one, but the rest of them are covered by the assertions added. MozReview-Commit-ID: E7QtiQ1vPqu
layout/base/ServoRestyleManager.cpp
layout/base/ServoRestyleManager.h
layout/generic/nsFrame.cpp
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -21,16 +21,88 @@
 #include "nsPrintfCString.h"
 #include "nsRefreshDriver.h"
 #include "nsStyleChangeList.h"
 
 using namespace mozilla::dom;
 
 namespace mozilla {
 
+#ifdef DEBUG
+static bool
+IsAnonBox(const nsIFrame& aFrame)
+{
+  return aFrame.StyleContext()->GetPseudo() &&
+         nsCSSAnonBoxes::IsAnonBox(aFrame.StyleContext()->GetPseudo());
+}
+
+const nsIFrame*
+FirstContinuationOrPartOfIBSplit(const nsIFrame* aFrame)
+{
+  if (!aFrame) {
+    return nullptr;
+  }
+
+  return nsLayoutUtils::FirstContinuationOrIBSplitSibling(aFrame);
+}
+
+static const nsIFrame*
+ExpectedOwnerForChild(const nsIFrame& aFrame)
+{
+  if (IsAnonBox(aFrame) || aFrame.IsBulletFrame()) {
+    return aFrame.GetParent()->IsViewportFrame() ? nullptr : aFrame.GetParent();
+  }
+
+  const nsIFrame* parent = FirstContinuationOrPartOfIBSplit(aFrame.GetParent());
+  if (aFrame.IsTableFrame()) {
+    MOZ_ASSERT(parent->IsTableWrapperFrame());
+    parent = FirstContinuationOrPartOfIBSplit(parent->GetParent());
+  }
+
+  while (parent && (IsAnonBox(*parent) || parent->IsLineFrame() ||
+                    parent->IsPopupSetFrame())) {
+    auto* pseudo = parent->StyleContext()->GetPseudo();
+    if (pseudo == nsCSSAnonBoxes::mozBlockInsideInlineWrapper) {
+      parent = parent->GetProperty(nsIFrame::IBSplitPrevSibling());
+      MOZ_ASSERT(parent);
+    } else if (pseudo == nsCSSAnonBoxes::tableWrapper) {
+      const nsIFrame* tableFrame = parent->PrincipalChildList().FirstChild();
+      MOZ_ASSERT(tableFrame->IsTableFrame());
+      parent = IsAnonBox(*tableFrame) ? parent->GetParent() : tableFrame;
+    } else {
+      parent = parent->GetParent();
+    }
+    parent = FirstContinuationOrPartOfIBSplit(parent);
+  }
+
+  return parent;
+}
+
+void
+ServoRestyleState::AssertOwner(const ServoRestyleState& aParent) const
+{
+  MOZ_ASSERT(mOwner);
+  MOZ_ASSERT(!mOwner->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW));
+  MOZ_ASSERT(ExpectedOwnerForChild(*mOwner) == aParent.mOwner);
+}
+
+nsChangeHint
+ServoRestyleState::ChangesHandledFor(const nsIFrame& aFrame) const
+{
+  if (!mOwner) {
+    MOZ_ASSERT(!mChangesHandled);
+    return mChangesHandled;
+  }
+
+  MOZ_ASSERT(mOwner == ExpectedOwnerForChild(aFrame),
+             "Missed some frame in the hierarchy?");
+  return mChangesHandled;
+}
+#endif
+
 ServoRestyleManager::ServoRestyleManager(nsPresContext* aPresContext)
   : RestyleManager(StyleBackendType::Servo, aPresContext)
   , mReentrantChanges(nullptr)
 {
 }
 
 void
 ServoRestyleManager::PostRestyleEvent(Element* aElement,
@@ -227,17 +299,17 @@ public:
     if (mShouldComputeHints) {
       mShouldComputeHints = false;
       uint32_t equalStructs, samePointerStructs;
       mComputedHint =
         oldContext->CalcStyleDifference(&aNewContext,
                                         &equalStructs,
                                         &samePointerStructs);
       mComputedHint = NS_RemoveSubsumedHints(
-        mComputedHint, mParentRestyleState.ChangesHandled());
+        mComputedHint, mParentRestyleState.ChangesHandledFor(*aTextFrame));
     }
 
     if (mComputedHint) {
       mParentRestyleState.ChangeList().AppendChange(
         aTextFrame, aContent, mComputedHint);
     }
   }
 
@@ -304,29 +376,42 @@ UpdateFramePseudoElementStyles(nsIFrame*
 
 bool
 ServoRestyleManager::ProcessPostTraversal(Element* aElement,
                                           nsStyleContext* aParentContext,
                                           ServoRestyleState& aRestyleState)
 {
   nsIFrame* styleFrame = nsLayoutUtils::GetStyleFrame(aElement);
 
+  // NOTE(emilio): This is needed because for table frames the bit is set on the
+  // table wrapper (which is the primary frame), not on the table itself.
+  const bool isOutOfFlow =
+    aElement->GetPrimaryFrame() &&
+    aElement->GetPrimaryFrame()->HasAnyStateBits(NS_FRAME_OUT_OF_FLOW);
+
   // Grab the change hint from Servo.
   nsChangeHint changeHint = Servo_TakeChangeHint(aElement);
-  changeHint =
-    NS_RemoveSubsumedHints(changeHint, aRestyleState.ChangesHandled());
+
+  // We should really fix the weird primary frame mapping for image maps
+  // (bug 135040)...
+  if (styleFrame && styleFrame->GetContent() != aElement) {
+    MOZ_ASSERT(styleFrame->IsImageFrame());
+    styleFrame = nullptr;
+  }
 
   // Handle lazy frame construction by posting a reconstruct for any lazily-
   // constructed roots.
   if (aElement->HasFlag(NODE_NEEDS_FRAME)) {
     changeHint |= nsChangeHint_ReconstructFrame;
-    // The only time the primary frame is non-null is when image maps do hacky
-    // SetPrimaryFrame calls.
-    MOZ_ASSERT(!styleFrame || styleFrame->IsImageFrame());
-    styleFrame = nullptr;
+    MOZ_ASSERT(!styleFrame);
+  }
+
+  if (styleFrame && !isOutOfFlow) {
+    changeHint = NS_RemoveSubsumedHints(
+      changeHint, aRestyleState.ChangesHandledFor(*styleFrame));
   }
 
   // Although we shouldn't generate non-ReconstructFrame hints for elements with
   // no frames, we can still get them here if they were explicitly posted by
   // PostRestyleEvent, such as a RepaintFrame hint when a :link changes to be
   // :visited.  Skip processing these hints if there is no frame.
   if ((styleFrame || (changeHint & nsChangeHint_ReconstructFrame)) && changeHint) {
     aRestyleState.ChangeList().AppendChange(styleFrame, aElement, changeHint);
@@ -374,17 +459,29 @@ ServoRestyleManager::ProcessPostTraversa
   // expensive checks in the common case.
   //
   // Also, we're going to need to check for pseudos of display: contents
   // elements, though that is buggy right now even in non-stylo mode, see
   // bug 1251799.
   const bool recreateContext = oldStyleContext &&
     oldStyleContext->ComputedValues() != computedValues;
 
-  ServoRestyleState childrenRestyleState(aRestyleState, changeHint);
+  Maybe<ServoRestyleState> thisFrameRestyleState;
+  if (styleFrame) {
+    auto type = isOutOfFlow
+      ? ServoRestyleState::Type::OutOfFlow
+      : ServoRestyleState::Type::InFlow;
+
+    thisFrameRestyleState.emplace(*styleFrame, aRestyleState, changeHint, type);
+  }
+
+  // We can't really assume as used changes from display: contents elements (or
+  // other elements without frames).
+  ServoRestyleState& childrenRestyleState =
+    thisFrameRestyleState ? *thisFrameRestyleState : aRestyleState;
 
   RefPtr<nsStyleContext> newContext = nullptr;
   if (recreateContext) {
     MOZ_ASSERT(styleFrame || displayContentsNode);
 
     auto pseudo = aElement->GetPseudoElementType();
     nsIAtom* pseudoTag = pseudo == CSSPseudoElementType::NotPseudo
       ? nullptr : nsCSSPseudoElements::GetPseudoAtom(pseudo);
@@ -415,17 +512,18 @@ ServoRestyleManager::ProcessPostTraversa
       UpdateFramePseudoElementStyles(styleFrame, childrenRestyleState);
     }
 
     if (!aElement->GetParent()) {
       // This is the root.  Update styles on the viewport as needed.
       ViewportFrame* viewport =
         do_QueryFrame(mPresContext->PresShell()->GetRootFrame());
       if (viewport) {
-        viewport->UpdateStyle(childrenRestyleState);
+        // NB: The root restyle state, not the one for our children!
+        viewport->UpdateStyle(aRestyleState);
       }
     }
 
     // Some changes to animations don't affect the computed style and yet still
     // require the layer to be updated. For example, pausing an animation via
     // the Web Animations API won't affect an element's style but still
     // requires to update the animation on the layer.
     //
--- a/layout/base/ServoRestyleManager.h
+++ b/layout/base/ServoRestyleManager.h
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_ServoRestyleManager_h
 #define mozilla_ServoRestyleManager_h
 
 #include "mozilla/EventStates.h"
 #include "mozilla/RestyleManager.h"
+#include "mozilla/Maybe.h"
 #include "mozilla/ServoElementSnapshot.h"
 #include "mozilla/ServoElementSnapshotTable.h"
 #include "nsChangeHint.h"
 #include "nsPresContext.h"
 
 namespace mozilla {
 namespace dom {
 class Element;
@@ -24,42 +25,84 @@ class nsIAtom;
 class nsIContent;
 class nsIFrame;
 class nsStyleChangeList;
 
 namespace mozilla {
 
 /**
  * A stack class used to pass some common restyle state in a slightly more
- * comfortable way than a bunch of individual arguments.
+ * comfortable way than a bunch of individual arguments, and that also checks
+ * that the change hint used for optimization is correctly used in debug mode.
  */
 class ServoRestyleState
 {
 public:
   ServoRestyleState(ServoStyleSet& aStyleSet, nsStyleChangeList& aChangeList)
     : mStyleSet(aStyleSet)
     , mChangeList(aChangeList)
     , mChangesHandled(nsChangeHint(0))
   {}
 
-  ServoRestyleState(ServoRestyleState& aParentState,
-                    nsChangeHint aHintForThisFrame)
+  // We shouldn't assume that changes handled from our parent are handled for
+  // our children too if we're out of flow since they aren't necessarily
+  // parented in DOM order, and thus a change handled by a DOM ancestor doesn't
+  // necessarily mean that it's handled for an ancestor frame.
+  enum class Type
+  {
+    InFlow,
+    OutOfFlow,
+  };
+
+  ServoRestyleState(const nsIFrame& aOwner,
+                    ServoRestyleState& aParentState,
+                    nsChangeHint aHintForThisFrame,
+                    Type aType)
     : mStyleSet(aParentState.mStyleSet)
     , mChangeList(aParentState.mChangeList)
-    , mChangesHandled(aParentState.mChangesHandled | aHintForThisFrame)
-  {}
+    , mChangesHandled(
+        aType == Type::InFlow
+          ? aParentState.mChangesHandled | aHintForThisFrame
+          : aHintForThisFrame)
+#ifdef DEBUG
+    , mOwner(&aOwner)
+#endif
+  {
+    if (aType == Type::InFlow) {
+      AssertOwner(aParentState);
+    }
+  }
 
-  nsChangeHint ChangesHandled() const { return mChangesHandled; }
   nsStyleChangeList& ChangeList() { return mChangeList; }
   ServoStyleSet& StyleSet() { return mStyleSet; }
 
+#ifdef DEBUG
+  void AssertOwner(const ServoRestyleState& aParentState) const;
+  nsChangeHint ChangesHandledFor(const nsIFrame&) const;
+#else
+  void AssertOwner(const ServoRestyleState&) const {}
+  nsChangeHint ChangesHandledFor(const nsIFrame&) const
+  {
+    return mChangesHandled;
+  }
+#endif
+
 private:
   ServoStyleSet& mStyleSet;
   nsStyleChangeList& mChangeList;
   const nsChangeHint mChangesHandled;
+
+  // We track the "owner" frame of this restyle state, that is, the frame that
+  // generated the last change that is stored in mChangesHandled, in order to
+  // verify that we only use mChangesHandled for actual descendants of that
+  // frame (given DOM order isn't always frame order, and that there are a few
+  // special cases for stuff like wrapper frames, ::backdrop, and so on).
+#ifdef DEBUG
+  const nsIFrame* mOwner { nullptr };
+#endif
 };
 
 /**
  * Restyle manager for a Servo-backed style system.
  */
 class ServoRestyleManager : public RestyleManager
 {
   friend class ServoStyleSet;
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -10224,17 +10224,18 @@ nsIFrame::UpdateStyleOfChildAnonBox(nsIF
     aRestyleState.StyleSet().ResolveInheritingAnonymousBoxStyle(pseudo,
                                                                 StyleContext());
 
   nsChangeHint childHint =
     UpdateStyleOfOwnedChildFrame(aChildFrame, newContext, aRestyleState);
 
   // Now that we've updated the style on aChildFrame, check whether it itself
   // has anon boxes to deal with.
-  ServoRestyleState childrenState(aRestyleState, childHint);
+  ServoRestyleState childrenState(
+      *aChildFrame, aRestyleState, childHint, ServoRestyleState::Type::InFlow);
   aChildFrame->UpdateStyleOfOwnedAnonBoxes(childrenState);
 
   // Assuming anon boxes don't have ::backdrop associated with them... if that
   // ever changes, we'd need to handle that here, like we do in
   // ServoRestyleManager::ProcessPostTraversal
 
   // We do need to handle block pseudo-elements here, though.  Especially list
   // bullets.
@@ -10259,17 +10260,18 @@ nsIFrame::UpdateStyleOfOwnedChildFrame(n
   // 2) Content can change stylesheets that change the styles of pseudos, and
   //    extensions can add/remove stylesheets that change the styles of
   //    anonymous boxes directly.
   uint32_t equalStructs, samePointerStructs; // Not used, actually.
   nsChangeHint childHint = aChildFrame->StyleContext()->CalcStyleDifference(
     aNewStyleContext,
     &equalStructs,
     &samePointerStructs);
-  childHint = NS_RemoveSubsumedHints(childHint, aRestyleState.ChangesHandled());
+  childHint = NS_RemoveSubsumedHints(
+    childHint, aRestyleState.ChangesHandledFor(*aChildFrame));
   if (childHint) {
     if (childHint & nsChangeHint_ReconstructFrame) {
       // If we generate a reconstruct here, remove any non-reconstruct hints we
       // may have already generated for this content.
       aRestyleState.ChangeList().PopChangesForContent(
         aChildFrame->GetContent());
     }
     aRestyleState.ChangeList().AppendChange(