Bug 1530657. Remove webrender user data properties from frames first, then destory them. r=jrmuizel
authorTimothy Nikkel <tnikkel@gmail.com>
Thu, 28 Feb 2019 16:55:25 -0600
changeset 461876 a8fafeb134457ed0fd12e2d38dba3fc00eaf6cae
parent 461875 9ce3a685bbcd0901128dab0c7261ae00dd209381
child 461877 75dd13876d89896a73a456d92fb0f3de4e3c72dc
push id35631
push userrgurzau@mozilla.com
push dateFri, 01 Mar 2019 13:06:03 +0000
treeherdermozilla-central@d4e19870e27f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1530657
milestone67.0a1
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
Bug 1530657. Remove webrender user data properties from frames first, then destory them. r=jrmuizel If we destroy at the same time as removal, then the destruction can destroy other objects which in turn might remove frame properties and mess up the frame property table. This should cover all ways the webrender user data can get deleted from a frame property. The nsIFrame::RemoveDisplayItemDataForDeletion bit doesn't seem necessary to fix the intermittent but I think it is still necessary.
layout/generic/nsFrame.cpp
layout/painting/FrameLayerBuilder.cpp
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -900,16 +900,32 @@ bool nsIFrame::HasDisplayItem(nsDisplayI
   DisplayItemArray* items = GetProperty(DisplayItems());
   if (!items) {
     return false;
   }
   return items->Contains(aItem);
 }
 
 void nsIFrame::RemoveDisplayItemDataForDeletion() {
+  // Destroying a WebRenderUserDataTable can cause destruction of other objects
+  // which can remove frame properties in their destructor. If we delete a frame
+  // property it runs the destructor of the stored object in the middle of
+  // updating the frame property table, so if the destruction of that object
+  // causes another update to the frame property table it would leave the frame
+  // property table in an inconsistent state. So we remove it from the table and
+  // then destroy it. (bug 1530657)
+  WebRenderUserDataTable* userDataTable =
+      RemoveProperty(WebRenderUserDataProperty::Key());
+  if (userDataTable) {
+    for (auto iter = userDataTable->Iter(); !iter.Done(); iter.Next()) {
+      iter.UserData()->RemoveFromTable();
+    }
+    delete userDataTable;
+  }
+
   FrameLayerBuilder::RemoveFrameFromLayerManager(this, DisplayItemData());
   DisplayItemData().Clear();
 
   DisplayItemArray* items = RemoveProperty(DisplayItems());
   if (items) {
     for (nsDisplayItem* i : *items) {
       if (i->GetDependentFrame() == this && !i->HasDeletedFrame()) {
         i->Frame()->MarkNeedsDisplayItemRebuild();
--- a/layout/painting/FrameLayerBuilder.cpp
+++ b/layout/painting/FrameLayerBuilder.cpp
@@ -469,17 +469,33 @@ class LayerManagerData : public LayerUse
   std::vector<RefPtr<DisplayItemData>> mDisplayItems;
   bool mInvalidateAllLayers;
 };
 
 /* static */ void FrameLayerBuilder::DestroyDisplayItemDataFor(
     nsIFrame* aFrame) {
   RemoveFrameFromLayerManager(aFrame, aFrame->DisplayItemData());
   aFrame->DisplayItemData().Clear();
-  aFrame->DeleteProperty(WebRenderUserDataProperty::Key());
+
+  // Destroying a WebRenderUserDataTable can cause destruction of other objects
+  // which can remove frame properties in their destructor. If we delete a frame
+  // property it runs the destructor of the stored object in the middle of
+  // updating the frame property table, so if the destruction of that object
+  // causes another update to the frame property table it would leave the frame
+  // property table in an inconsistent state. So we remove it from the table and
+  // then destroy it. (bug 1530657)
+  WebRenderUserDataTable* userDataTable =
+      aFrame->RemoveProperty(WebRenderUserDataProperty::Key());
+  if (userDataTable) {
+    for (auto iter = userDataTable->Iter(); !iter.Done(); iter.Next()) {
+      iter.UserData()->RemoveFromTable();
+    }
+    delete userDataTable;
+  }
+
 }
 
 /**
  * We keep a stack of these to represent the PaintedLayers that are
  * currently available to have display items added to.
  * We use a stack here because as much as possible we want to
  * assign display items to existing PaintedLayers, and to the lowest
  * PaintedLayer in z-order. This reduces the number of layers and