Bug 1368589: handle the case where a RebuildAllStyleData event is posted and stylesheets are added at the same time. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 30 May 2017 02:46:33 +0200
changeset 586404 3c409b6e910fc5bbb12663b8e8a50b9e71f8eb5c
parent 586403 b0dadbd7f96d152db2d1981ae3f9bb0ade9bdfee
child 630960 77ebc4df2e9d9dd7c4b1896e81eb60dd5fc32541
push id61375
push userbmo:emilio+bugs@crisal.io
push dateTue, 30 May 2017 09:02:08 +0000
reviewersheycam
bugs1368589
milestone55.0a1
Bug 1368589: handle the case where a RebuildAllStyleData event is posted and stylesheets are added at the same time. r?heycam MozReview-Commit-ID: IWiTCCo55cg
image/test/reftest/jpeg/reftest.list
layout/style/ServoStyleSet.cpp
layout/style/ServoStyleSet.h
--- a/image/test/reftest/jpeg/reftest.list
+++ b/image/test/reftest/jpeg/reftest.list
@@ -46,9 +46,9 @@ random-if(Android) == jpg-srgb-icc.jpg j
 # \r\n
 # <contents of blue.jpg> (no newline)
 # --BOUNDARYOMG--\r\n
 # 
 # (The boundary is arbitrary, and just has to be defined as something that
 # won't be in the text of the contents themselves. --$(boundary)\r\n means
 # "Here is the beginning of a boundary," and --$(boundary)-- means "All done
 # sending you parts.")
-random-if(stylo||styloVsGecko) HTTP == webcam-simulacrum.mjpg blue.jpg # bug 1368589 for stylo
+HTTP == webcam-simulacrum.mjpg blue.jpg
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -1088,25 +1088,24 @@ ServoStyleSet::EnsureUniqueInnerOnCSSShe
   return res;
 }
 
 void
 ServoStyleSet::RebuildData()
 {
   ClearNonInheritingStyleContexts();
   Servo_StyleSet_RebuildData(mRawSet.get());
-  mStylistState = StylistState::NotDirty;
 }
 
 void
 ServoStyleSet::ClearDataAndMarkDeviceDirty()
 {
   ClearNonInheritingStyleContexts();
   Servo_StyleSet_Clear(mRawSet.get());
-  mStylistState = StylistState::FullyDirty;
+  mStylistState |= StylistState::FullyDirty;
 }
 
 already_AddRefed<ServoComputedValues>
 ServoStyleSet::ResolveServoStyle(Element* aElement)
 {
   UpdateStylistIfNeeded();
   return Servo_ResolveStyle(aElement, mRawSet.get(),
                             mAllowResolveStaleStyles).Consume();
@@ -1198,19 +1197,38 @@ ServoStyleSet::ResolveForDeclarations(
                                                aParentOrNull,
                                                aDeclarations).Consume();
 }
 
 void
 ServoStyleSet::UpdateStylist()
 {
   MOZ_ASSERT(StylistNeedsUpdate());
-  if (mStylistState == StylistState::FullyDirty) {
+  if (mStylistState & StylistState::FullyDirty) {
     RebuildData();
+
+    if (mStylistState & StylistState::StyleSheetsDirty) {
+      // Normally, whoever was in charge of posting a RebuildAllStyleDataEvent,
+      // would also be in charge of posting a restyle/change hint according to
+      // it.
+      //
+      // However, other stylesheets may have been added to the document in the
+      // same period, so when both bits are set, we need to do a full subtree
+      // update, because we can no longer reason about the state of the style
+      // data.
+      //
+      // We could not clear the invalidations when rebuilding the data and
+      // process them here... But it's not clear if that complexity is worth
+      // to handle this edge case more efficiently.
+      if (Element* root = mPresContext->Document()->GetDocumentElement()) {
+        Servo_NoteExplicitHints(root, eRestyle_Subtree, nsChangeHint(0));
+      }
+    }
   } else {
+    MOZ_ASSERT(mStylistState & StylistState::StyleSheetsDirty);
     Element* root = mPresContext->Document()->GetDocumentElement();
     Servo_StyleSet_FlushStyleSheets(mRawSet.get(), root);
   }
   mStylistState = StylistState::NotDirty;
 }
 
 void
 ServoStyleSet::PrependSheetOfType(SheetType aType,
--- a/layout/style/ServoStyleSet.h
+++ b/layout/style/ServoStyleSet.h
@@ -42,16 +42,36 @@ class nsStyleContext;
 class nsPresContext;
 struct nsTimingFunction;
 struct RawServoRuleNode;
 struct TreeMatchContext;
 
 namespace mozilla {
 
 /**
+ * A few flags used to track which kind of stylist state we may need to
+ * update.
+ */
+enum class StylistState : uint8_t {
+  /** The stylist is not dirty, we should do nothing */
+  NotDirty = 0,
+
+  /** The style sheets have changed, so we need to update the style data. */
+  StyleSheetsDirty = 1 << 0,
+
+  /**
+   * All style data is dirty and both style sheet data and default computed
+   * values need to be recomputed.
+   */
+  FullyDirty = 1 << 1,
+};
+
+MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(StylistState)
+
+/**
  * The set of style sheets that apply to a document, backed by a Servo
  * Stylist.  A ServoStyleSet contains ServoStyleSheets.
  */
 class ServoStyleSet
 {
   friend class ServoRestyleManager;
   typedef ServoElementSnapshotTable SnapshotTable;
 
@@ -467,39 +487,21 @@ private:
    */
   void PreTraverse(dom::Element* aRoot = nullptr,
                    EffectCompositor::AnimationRestyleType =
                      EffectCompositor::AnimationRestyleType::Throttled);
   // Subset of the pre-traverse steps that involve syncing up data
   void PreTraverseSync();
 
   /**
-   * A tri-state used to track which kind of stylist state we may need to
-   * update.
-   */
-  enum class StylistState : uint8_t {
-    /** The stylist is not dirty, we should do nothing */
-    NotDirty,
-    /** The style sheets have changed, so we need to update the style data. */
-    StyleSheetsDirty,
-    /**
-     * All style data is dirty and both style sheet data and default computed
-     * values need to be recomputed.
-     */
-    FullyDirty,
-  };
-
-  /**
    * Note that the stylist needs a style flush due to style sheet changes.
    */
   void SetStylistStyleSheetsDirty()
   {
-    if (mStylistState == StylistState::NotDirty) {
-      mStylistState = StylistState::StyleSheetsDirty;
-    }
+    mStylistState |= StylistState::StyleSheetsDirty;
   }
 
   bool StylistNeedsUpdate() const
   {
     return mStylistState != StylistState::NotDirty;
   }
 
   /**