Bug 1330874: Preserve neutral style changes for stylo. r=heycam

Since we use the presence of a change hint to determine whether we should
recreate a style context, we can't just strip them out on CalcStyleDifference.

MozReview-Commit-ID: GLhbTc2W3d7
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>

layout/base/ServoRestyleManager.cpp

@@ -30,6 +30,8 @@ ServoRestyleManager::PostRestyleEvent(Element* aElement,
                                       nsRestyleHint aRestyleHint,
                                       nsChangeHint aMinChangeHint)
 {
+  MOZ_ASSERT(!(aMinChangeHint & nsChangeHint_NeutralChange),
+             "Didn't expect explicit change hints to be neutral!");
   if (MOZ_UNLIKELY(IsDisconnected()) ||
       MOZ_UNLIKELY(PresContext()->PresShell()->IsDestroying())) {
     return;
@@ -147,8 +149,9 @@ ServoRestyleManager::RecreateStyleContexts(Element* aElement,
   // FIXME(bholley): Once we transfer ownership of the styles to the frame, we
   // can fast-reject without the FFI call by checking mServoData for null.
   nsChangeHint changeHint = Servo_TakeChangeHint(aElement);
-  if (changeHint) {
-      aChangeListToProcess.AppendChange(primaryFrame, aElement, changeHint);
+  if (changeHint & ~nsChangeHint_NeutralChange) {
+    aChangeListToProcess.AppendChange(
+      primaryFrame, aElement, changeHint & ~nsChangeHint_NeutralChange);
   }
 
   // If our change hint is reconstruct, we delegate to the frame constructor,

layout/base/nsStyleChangeList.cpp

@@ -18,6 +18,10 @@ nsStyleChangeList::AppendChange(nsIFrame* aFrame, nsIContent* aContent, nsChange
 {
   MOZ_ASSERT(aFrame || (aHint & nsChangeHint_ReconstructFrame),
              "must have frame");
+  MOZ_ASSERT(aHint, "No hint to process?");
+  MOZ_ASSERT(!(aHint & nsChangeHint_NeutralChange),
+             "Neutral changes do not need extra processing, "
+             "and should be stripped out");
   MOZ_ASSERT(aContent || !(aHint & nsChangeHint_ReconstructFrame),
              "must have content");
   // XXXbz we should make this take Element instead of nsIContent

layout/style/nsStyleContext.cpp

@@ -959,7 +959,8 @@ nsStyleContext::ApplyStyleFixups(bool aSkipParentDisplayBasedStyleFixup)
 #undef GET_UNIQUE_STYLE_DATA
 }
 
-template<class StyleContextLike>
+template<class StyleContextLike,
+         nsStyleContext::NeutralChangeHandling aNeutralChangeHandling>
 nsChangeHint
 nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
                                             nsChangeHint aParentHintsNotHandledForDescendants,
@@ -969,6 +970,10 @@ nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
   PROFILER_LABEL("nsStyleContext", "CalcStyleDifference",
     js::ProfileEntry::Category::CSS);
 
+  // See the comment in CalcStyleDifference(ServoComputedValues*, ...) to
+  // understand why we need to manually handle the neutral change in Servo.
+  MOZ_ASSERT_IF(mSource.IsServoComputedValues(),
+                aNeutralChangeHandling == NeutralChangeHandling::Retain);
   MOZ_ASSERT(NS_IsHintSubset(aParentHintsNotHandledForDescendants,
                              nsChangeHint_Hints_NotHandledForDescendants),
              "caller is passing inherited hints, but shouldn't be");
@@ -1214,7 +1219,9 @@ nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
 
   MOZ_ASSERT(NS_IsHintSubset(hint, nsChangeHint_AllHints),
              "Added a new hint without bumping AllHints?");
-  return hint & ~nsChangeHint_NeutralChange;
+  return aNeutralChangeHandling == NeutralChangeHandling::Strip
+           ? (hint & ~nsChangeHint_NeutralChange)
+           : hint;
 }
 
 nsChangeHint
@@ -1223,8 +1230,9 @@ nsStyleContext::CalcStyleDifference(nsStyleContext* aNewContext,
                                     uint32_t* aEqualStructs,
                                     uint32_t* aSamePointerStructs)
 {
-  return CalcStyleDifferenceInternal(aNewContext, aParentHintsNotHandledForDescendants,
-                                     aEqualStructs, aSamePointerStructs);
+  return CalcStyleDifferenceInternal<nsStyleContext, NeutralChangeHandling::Strip>
+    (aNewContext, aParentHintsNotHandledForDescendants, aEqualStructs,
+     aSamePointerStructs);
 }
 
 class MOZ_STACK_CLASS FakeStyleContext
@@ -1260,9 +1268,15 @@ nsStyleContext::CalcStyleDifference(const ServoComputedValues* aNewComputedValue
                                     uint32_t* aEqualStructs,
                                     uint32_t* aSamePointerStructs)
 {
+  // NB: Servo uses the presence of a change hint to determine whether it should
+  // generate a new nsStyleContext.
+  //
+  // Given that, we can't strip the neutral change hint here, since it may
+  // provoke errors like bug 1330874.
   FakeStyleContext newContext(aNewComputedValues);
-  return CalcStyleDifferenceInternal(&newContext, aParentHintsNotHandledForDescendants,
-                                     aEqualStructs, aSamePointerStructs);
+  return CalcStyleDifferenceInternal<FakeStyleContext, NeutralChangeHandling::Retain>(
+      &newContext, aParentHintsNotHandledForDescendants, aEqualStructs,
+      aSamePointerStructs);
 }
 
 #ifdef DEBUG

layout/style/nsStyleContext.h

@@ -399,7 +399,12 @@ public:
                                    uint32_t* aSamePointerStructs);
 
 private:
-  template<class StyleContextLike>
+  enum class NeutralChangeHandling {
+    Retain,
+    Strip,
+  };
+
+  template<class StyleContextLike, NeutralChangeHandling aNeutralChangeHandling>
   nsChangeHint CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
                                            nsChangeHint aParentHintsNotHandledForDescendants,
                                            uint32_t* aEqualStructs,