Bug 1353187 - Guard access to the frame property table with a frame state bit. r=dholbert

This protects all accesses to the frame property table with a bit stored
on the frame.  This means we avoid hashtable operations when asking
about frame properties on frames that have no properties.

The changes to RestyleManager, and the new HasSkippingBitCheck API, are
needed because RestyleManager depended on being able to ask for
properties on a deleted frame (knowing that the property in question
could not have been set on any new frames since the deleted frame was
destroyed), in order to use the destruction of the properties that
happens at frame destruction as a mechanism for learning that the frame
was destroyed.  The changes there preserve the use of that mechanism,
although it becomes a bit uglier.  The ugliness is well-deserved.

MozReview-Commit-ID: BScmDUlWq65

--HG--
extra : transplant_source : %C8%C0%CD%DC%12g%5B%8ER%3A%FF%A7a%F8%91%D4%2C%9BF%2B

layout/base/FramePropertyTable.cpp

@@ -22,6 +22,7 @@ FramePropertyTable::SetInternal(
   if (mLastFrame != aFrame || !mLastEntry) {
     mLastFrame = aFrame;
     mLastEntry = mEntries.PutEntry(aFrame);
+    aFrame->AddStateBits(NS_FRAME_HAS_PROPERTIES);
   }
   Entry* entry = mLastEntry;
 
@@ -63,7 +64,8 @@ FramePropertyTable::SetInternal(
 
 void*
 FramePropertyTable::GetInternal(
-  const nsIFrame* aFrame, UntypedDescriptor aProperty, bool* aFoundResult)
+  const nsIFrame* aFrame, UntypedDescriptor aProperty, bool aSkipBitCheck,
+  bool* aFoundResult)
 {
   NS_ASSERTION(aFrame, "Null frame?");
   NS_ASSERTION(aProperty, "Null property?");
@@ -72,6 +74,10 @@ FramePropertyTable::GetInternal(
     *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.
@@ -82,6 +88,8 @@ FramePropertyTable::GetInternal(
     mLastEntry = entry;
   }
 
+  MOZ_ASSERT(entry || aSkipBitCheck,
+             "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists");
   if (!entry)
     return nullptr;
 
@@ -111,7 +119,8 @@ FramePropertyTable::GetInternal(
 
 void*
 FramePropertyTable::RemoveInternal(
-  nsIFrame* aFrame, UntypedDescriptor aProperty, bool* aFoundResult)
+  nsIFrame* aFrame, UntypedDescriptor aProperty, bool aSkipBitCheck,
+  bool* aFoundResult)
 {
   MOZ_ASSERT(NS_IsMainThread());
   NS_ASSERTION(aFrame, "Null frame?");
@@ -121,11 +130,17 @@ FramePropertyTable::RemoveInternal(
     *aFoundResult = false;
   }
 
+  if (!aSkipBitCheck && !(aFrame->GetStateBits() & NS_FRAME_HAS_PROPERTIES)) {
+    return nullptr;
+  }
+
   if (mLastFrame != aFrame) {
     mLastFrame = aFrame;
     mLastEntry = mEntries.GetEntry(aFrame);
   }
   Entry* entry = mLastEntry;
+  MOZ_ASSERT(entry || aSkipBitCheck,
+             "NS_FRAME_HAS_PROPERTIES bit should match whether entry exists");
   if (!entry)
     return nullptr;
 
@@ -136,6 +151,7 @@ FramePropertyTable::RemoveInternal(
     // 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;
@@ -176,14 +192,14 @@ FramePropertyTable::RemoveInternal(
 
 void
 FramePropertyTable::DeleteInternal(
-  nsIFrame* aFrame, UntypedDescriptor aProperty)
+  nsIFrame* aFrame, UntypedDescriptor aProperty, bool aSkipBitCheck)
 {
   MOZ_ASSERT(NS_IsMainThread());
   NS_ASSERTION(aFrame, "Null frame?");
   NS_ASSERTION(aProperty, "Null property?");
 
   bool found;
-  void* v = RemoveInternal(aFrame, aProperty, &found);
+  void* v = RemoveInternal(aFrame, aProperty, aSkipBitCheck, &found);
   if (found) {
     PropertyValue pv(aProperty, v);
     pv.DestroyValueFor(aFrame);
@@ -210,7 +226,13 @@ 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;
 
@@ -226,6 +248,8 @@ FramePropertyTable::DeleteAllFor(nsIFrame* aFrame)
   // 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

layout/base/FramePropertyTable.h

@@ -188,12 +188,25 @@ public:
    *
    *   - 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, &foundResult);
+    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);
     return foundResult;
   }
 
@@ -212,7 +225,7 @@ public:
   PropertyType<T> Get(const nsIFrame* aFrame, Descriptor<T> aProperty,
                       bool* aFoundResult = nullptr)
   {
-    void* ptr = GetInternal(aFrame, aProperty, aFoundResult);
+    void* ptr = GetInternal(aFrame, aProperty, false, aFoundResult);
     return ReinterpretHelper<T>::FromPointer(ptr);
   }
   /**
@@ -231,7 +244,7 @@ public:
   PropertyType<T> Remove(nsIFrame* aFrame, Descriptor<T> aProperty,
                          bool* aFoundResult = nullptr)
   {
-    void* ptr = RemoveInternal(aFrame, aProperty, aFoundResult);
+    void* ptr = RemoveInternal(aFrame, aProperty, false, aFoundResult);
     return ReinterpretHelper<T>::FromPointer(ptr);
   }
   /**
@@ -239,11 +252,23 @@ public:
    * 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, nothing happens.
+   *
+   * The DeleteSkippingBitCheck 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>
   void Delete(nsIFrame* aFrame, Descriptor<T> aProperty)
   {
-    DeleteInternal(aFrame, aProperty);
+    DeleteInternal(aFrame, aProperty, false);
+  }
+
+  template<typename T>
+  void DeleteSkippingBitCheck(nsIFrame* aFrame, Descriptor<T> aProperty)
+  {
+    DeleteInternal(aFrame, aProperty, true);
   }
   /**
    * Remove and destroy all property values for a frame. This requires one
@@ -262,12 +287,13 @@ protected:
                    void* aValue);
 
   void* GetInternal(const nsIFrame* aFrame, UntypedDescriptor aProperty,
-                    bool* aFoundResult);
+                    bool aSkipBitCheck, bool* aFoundResult);
 
   void* RemoveInternal(nsIFrame* aFrame, UntypedDescriptor aProperty,
-                       bool* aFoundResult);
+                       bool aSkipBitCheck, bool* aFoundResult);
 
-  void DeleteInternal(nsIFrame* aFrame, UntypedDescriptor aProperty);
+  void DeleteInternal(nsIFrame* aFrame, UntypedDescriptor aProperty,
+                      bool aSkipBitCheck);
 
   template<typename T>
   struct ReinterpretHelper

layout/base/RestyleManager.cpp

@@ -1396,7 +1396,8 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
       break;
     }
 
-    const nsStyleChangeData& data = aChangeList[i];
+    nsStyleChangeData& mutable_data = aChangeList[i];
+    const nsStyleChangeData& data = mutable_data;
     nsIFrame* frame = data.mFrame;
     nsIContent* content = data.mContent;
     nsChangeHint hint = data.mHint;
@@ -1407,7 +1408,11 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
                  "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->Get(frame, ChangeListProperty())) {
+    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;
       continue;
     }
 
@@ -1475,6 +1480,11 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
     }
 
     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
@@ -1664,7 +1674,7 @@ RestyleManager::ProcessRestyledFrames(nsStyleChangeList& aChangeList)
   // is not in fact in a consistent state.
   for (const nsStyleChangeData& data : aChangeList) {
     if (data.mFrame) {
-      propTable->Delete(data.mFrame, ChangeListProperty());
+      propTable->DeleteSkippingBitCheck(data.mFrame, ChangeListProperty());
     }
 
 #ifdef DEBUG

layout/generic/nsFrame.cpp

@@ -9982,8 +9982,9 @@ nsFrame::BoxReflow(nsBoxLayoutState&        aState,
       parentReflowInput(aPresContext, parentFrame, aRenderingContext,
                         LogicalSize(parentWM, parentSize),
                         ReflowInput::DUMMY_PARENT_REFLOW_STATE);
-    parentFrame->RemoveStateBits(~nsFrameState(0));
-    parentFrame->AddStateBits(savedState);
+    const nsFrameState bitsToLeaveUntouched = NS_FRAME_HAS_PROPERTIES;
+    parentFrame->RemoveStateBits(~bitsToLeaveUntouched);
+    parentFrame->AddStateBits(savedState & ~bitsToLeaveUntouched);
 
     // This may not do very much useful, but it's probably worth trying.
     if (parentSize.width != NS_INTRINSICSIZE)

layout/generic/nsFrameStateBits.h

@@ -270,6 +270,9 @@ FRAME_STATE_BIT(Generic, 54, NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY)
 // 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)