Backed out changeset 74eb0b08e42b (bug 1353187 patch 2) for test failures (assertions firing).
authorL. David Baron <dbaron@dbaron.org>
Mon, 03 Apr 2017 22:47:57 -0700
changeset 351041 a56372e9dc96187eedecebf29a7ddf8d1e180e0c
parent 351040 9dcefc9cd349f546492fc79caeaf5315d3912225
child 351042 ebc1182ae0a58e1e6d4641edd41913e2c879da64
push id88786
push userdbaron@mozilla.com
push dateTue, 04 Apr 2017 05:48:09 +0000
treeherdermozilla-inbound@a56372e9dc96 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1353187
milestone55.0a1
backs out74eb0b08e42bd5c0ddd9f1b140cb98880f5377e8
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset 74eb0b08e42b (bug 1353187 patch 2) for test failures (assertions firing).
layout/base/FramePropertyTable.cpp
layout/base/FramePropertyTable.h
layout/base/RestyleManager.cpp
layout/generic/nsFrameStateBits.h
--- a/layout/base/FramePropertyTable.cpp
+++ b/layout/base/FramePropertyTable.cpp
@@ -17,17 +17,16 @@ FramePropertyTable::SetInternal(
 {
   MOZ_ASSERT(NS_IsMainThread());
   NS_ASSERTION(aFrame, "Null frame?");
   NS_ASSERTION(aProperty, "Null property?");
 
   if (mLastFrame != aFrame || !mLastEntry) {
     mLastFrame = aFrame;
     mLastEntry = mEntries.PutEntry(aFrame);
-    aFrame->AddStateBits(NS_FRAME_HAS_PROPERTIES);
   }
   Entry* entry = mLastEntry;
 
   if (!entry->mProp.IsArray()) {
     if (!entry->mProp.mProperty) {
       // Empty entry, so we can just store our property in the empty slot
       entry->mProp.mProperty = aProperty;
       entry->mProp.mValue = aValue;
@@ -59,42 +58,35 @@ FramePropertyTable::SetInternal(
     return;
   }
 
   array->AppendElement(PropertyValue(aProperty, aValue));
 }
 
 void*
 FramePropertyTable::GetInternal(
-  const nsIFrame* aFrame, UntypedDescriptor aProperty, bool aSkipBitCheck,
-  bool* aFoundResult)
+  const nsIFrame* aFrame, UntypedDescriptor aProperty, bool* aFoundResult)
 {
   NS_ASSERTION(aFrame, "Null frame?");
   NS_ASSERTION(aProperty, "Null property?");
 
   if (aFoundResult) {
     *aFoundResult = false;
   }
 
-  if (!aSkipBitCheck && !(aFrame->GetStateBits() & NS_FRAME_HAS_PROPERTIES)) {
-    return nullptr;
-  }
-
   // We can end up here during parallel style traversal, in which case the main
   // thread is blocked. Reading from the cache is fine on any thread, but we
   // only want to write to it in the main-thread case.
   bool cacheHit = mLastFrame == aFrame;
   Entry* entry = cacheHit ? mLastEntry : mEntries.GetEntry(aFrame);
   if (!cacheHit && !ServoStyleSet::IsInServoTraversal()) {
     mLastFrame = aFrame;
     mLastEntry = entry;
   }
 
-  MOZ_ASSERT(entry || aSkipBitCheck,
-             "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists");
   if (!entry)
     return nullptr;
 
   if (entry->mProp.mProperty == aProperty) {
     if (aFoundResult) {
       *aFoundResult = true;
     }
     return entry->mProp.mValue;
@@ -124,38 +116,31 @@ FramePropertyTable::RemoveInternal(
   MOZ_ASSERT(NS_IsMainThread());
   NS_ASSERTION(aFrame, "Null frame?");
   NS_ASSERTION(aProperty, "Null property?");
 
   if (aFoundResult) {
     *aFoundResult = false;
   }
 
-  if (!(aFrame->GetStateBits() & NS_FRAME_HAS_PROPERTIES)) {
-    return nullptr;
-  }
-
   if (mLastFrame != aFrame) {
     mLastFrame = aFrame;
     mLastEntry = mEntries.GetEntry(aFrame);
   }
   Entry* entry = mLastEntry;
-  MOZ_ASSERT(entry,
-             "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists");
   if (!entry)
     return nullptr;
 
   if (entry->mProp.mProperty == aProperty) {
     // There's only one entry and it's the one we want
     void* value = entry->mProp.mValue;
 
     // Here it's ok to use RemoveEntry() -- which may resize mEntries --
     // because we null mLastEntry at the same time.
     mEntries.RemoveEntry(entry);
-    aFrame->RemoveStateBits(NS_FRAME_HAS_PROPERTIES);
     mLastEntry = nullptr;
     if (aFoundResult) {
       *aFoundResult = true;
     }
     return value;
   }
   if (!entry->mProp.IsArray()) {
     // There's just one property and it's not the one we want, bail
@@ -220,40 +205,32 @@ FramePropertyTable::DeleteAllForEntry(En
   array->~nsTArray<PropertyValue>();
 }
 
 void
 FramePropertyTable::DeleteAllFor(nsIFrame* aFrame)
 {
   NS_ASSERTION(aFrame, "Null frame?");
 
-  if (!(aFrame->GetStateBits() & NS_FRAME_HAS_PROPERTIES)) {
-    return;
-  }
-
   Entry* entry = mEntries.GetEntry(aFrame);
-  MOZ_ASSERT(entry,
-             "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists");
   if (!entry)
     return;
 
   if (mLastFrame == aFrame) {
     // Flush cache. We assume DeleteAllForEntry will be called before
     // a frame is destroyed.
     mLastFrame = nullptr;
     mLastEntry = nullptr;
   }
 
   DeleteAllForEntry(entry);
 
   // mLastEntry points into mEntries, so we use RawRemoveEntry() which will not
   // resize mEntries.
   mEntries.RawRemoveEntry(entry);
-
-  // Don't bother unsetting NS_FRAME_HAS_PROPERTIES, since aFrame is going away
 }
 
 void
 FramePropertyTable::DeleteAll()
 {
   mLastFrame = nullptr;
   mLastEntry = nullptr;
 
--- a/layout/base/FramePropertyTable.h
+++ b/layout/base/FramePropertyTable.h
@@ -183,35 +183,22 @@ public:
    * property is set. Legitimate non-assertion uses include:
    *
    *   - Checking if a frame property is set in cases where that's all we want
    *     to know (i.e., we don't intend to read the actual value or remove the
    *     property).
    *
    *   - Calling Has() before Set() in cases where we don't want to overwrite
    *     an existing value for the frame property.
-   *
-   * The HasSkippingBitCheck variant doesn't test NS_FRAME_HAS_PROPERTIES
-   * on aFrame, so it is safe to call after aFrame has been destroyed as
-   * long as, since that destruction happened, it isn't possible for a
-   * new frame to have been created and the same property added.
    */
   template<typename T>
   bool Has(const nsIFrame* aFrame, Descriptor<T> aProperty)
   {
     bool foundResult = false;
-    mozilla::Unused << GetInternal(aFrame, aProperty, false, &foundResult);
-    return foundResult;
-  }
-
-  template<typename T>
-  bool HasSkippingBitCheck(const nsIFrame* aFrame, Descriptor<T> aProperty)
-  {
-    bool foundResult = false;
-    mozilla::Unused << GetInternal(aFrame, aProperty, true, &foundResult);
+    mozilla::Unused << GetInternal(aFrame, aProperty, &foundResult);
     return foundResult;
   }
 
   /**
    * Get a property value for a frame. This requires one hashtable
    * lookup (using the frame as the key) and a linear search through
    * the properties of that frame. If the frame has no such property,
    * returns zero-filled result, which means null for pointers and
@@ -220,17 +207,17 @@ public:
    * the frame has a value for the property. This lets callers
    * disambiguate a null result, which can mean 'no such property' or
    * 'property value is null'.
    */
   template<typename T>
   PropertyType<T> Get(const nsIFrame* aFrame, Descriptor<T> aProperty,
                       bool* aFoundResult = nullptr)
   {
-    void* ptr = GetInternal(aFrame, aProperty, false, aFoundResult);
+    void* ptr = GetInternal(aFrame, aProperty, aFoundResult);
     return ReinterpretHelper<T>::FromPointer(ptr);
   }
   /**
    * Remove a property value for a frame. This requires one hashtable
    * lookup (using the frame as the key) and a linear search through
    * the properties of that frame. The old property value is returned
    * (and not destroyed). If the frame has no such property,
    * returns zero-filled result, which means null for pointers and
@@ -270,17 +257,17 @@ public:
 
   size_t SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
 
 protected:
   void SetInternal(nsIFrame* aFrame, UntypedDescriptor aProperty,
                    void* aValue);
 
   void* GetInternal(const nsIFrame* aFrame, UntypedDescriptor aProperty,
-                    bool aSkipBitCheck, bool* aFoundResult);
+                    bool* aFoundResult);
 
   void* RemoveInternal(nsIFrame* aFrame, UntypedDescriptor aProperty,
                        bool* aFoundResult);
 
   void DeleteInternal(nsIFrame* aFrame, UntypedDescriptor aProperty);
 
   template<typename T>
   struct ReinterpretHelper
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -1391,33 +1391,28 @@ RestyleManager::ProcessRestyledFrames(ns
     for (size_t j = lazyRangeStart; j < i; ++j) {
       MOZ_ASSERT_IF(aChangeList[j].mContent->GetPrimaryFrame(),
                     !aChangeList[j].mContent->HasFlag(NODE_NEEDS_FRAME));
     }
     if (i == aChangeList.Length()) {
       break;
     }
 
-    nsStyleChangeData& mutable_data = aChangeList[i];
-    const nsStyleChangeData& data = mutable_data;
+    const nsStyleChangeData& data = aChangeList[i];
     nsIFrame* frame = data.mFrame;
     nsIContent* content = data.mContent;
     nsChangeHint hint = data.mHint;
     bool didReflowThisFrame = false;
 
     NS_ASSERTION(!(hint & nsChangeHint_AllReflowHints) ||
                  (hint & nsChangeHint_NeedReflow),
                  "Reflow hint bits set without actually asking for a reflow");
 
     // skip any frame that has been destroyed due to a ripple effect
-    if (frame && !propTable->HasSkippingBitCheck(frame, ChangeListProperty())) {
-      // Null out the pointer since the frame was already destroyed.
-      // This is important so we don't try to delete its
-      // ChangeListProperty() below.
-      mutable_data.mFrame = nullptr;
+    if (frame && !propTable->Get(frame, ChangeListProperty())) {
       continue;
     }
 
     if (frame && frame->GetContent() != content) {
       // XXXbz this is due to image maps messing with the primary frame of
       // <area>s.  See bug 135040.  Remove this block once that's fixed.
       frame = nullptr;
       if (!(hint & nsChangeHint_ReconstructFrame)) {
@@ -1475,21 +1470,16 @@ RestyleManager::ProcessRestyledFrames(ns
         }
         // Don't remove NS_FRAME_MAY_BE_TRANSFORMED since it may still be
         // transformed by other means. It's OK to have the bit even if it's
         // not needed.
       }
     }
 
     if (hint & nsChangeHint_ReconstructFrame) {
-      // We're about to destroy data.mFrame, so null out the pointer.
-      // This is important so we don't try to delete its
-      // ChangeListProperty() below.
-      mutable_data.mFrame = nullptr;
-
       // If we ever start passing true here, be careful of restyles
       // that involve a reframe and animations.  In particular, if the
       // restyle we're processing here is an animation restyle, but
       // the style resolution we will do for the frame construction
       // happens async when we're not in an animation restyle already,
       // problems could arise.
       // We could also have problems with triggering of CSS transitions
       // on elements whose frames are reconstructed, since we depend on
--- a/layout/generic/nsFrameStateBits.h
+++ b/layout/generic/nsFrameStateBits.h
@@ -265,19 +265,16 @@ FRAME_STATE_BIT(Generic, 53, NS_FRAME_IS
 
 // Frame has a LayerActivityProperty property
 FRAME_STATE_BIT(Generic, 54, NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY)
 
 // Frame owns anonymous boxes whose style contexts it will need to update during
 // a stylo tree traversal.
 FRAME_STATE_BIT(Generic, 55, NS_FRAME_OWNS_ANON_BOXES)
 
-// Frame has properties in the nsIFrame::Properties() hash.
-FRAME_STATE_BIT(Generic, 56, NS_FRAME_HAS_PROPERTIES)
-
 // Set for all descendants of MathML sub/supscript elements (other than the
 // base frame) to indicate that the SSTY font feature should be used.
 FRAME_STATE_BIT(Generic, 58, NS_FRAME_MATHML_SCRIPT_DESCENDANT)
 
 // This state bit is set on frames within token MathML elements if the
 // token represents an <mi> tag whose inner HTML consists of a single
 // non-whitespace character to allow special rendering behaviour.
 FRAME_STATE_BIT(Generic, 59, NS_FRAME_IS_IN_SINGLE_CHAR_MI)