Bug 1530657. Remove webrender user data properties from frames first, then destory them. r=jrmuizel a=lizzard
authorTimothy Nikkel <tnikkel@gmail.com>
Thu, 28 Feb 2019 16:55:25 -0600
changeset 516283 e84952f939308cf1a262087b5d42c76600fd08c9
parent 516282 230fcfcad611c7bb7f186f2610cbe8d884b6c88d
child 516284 494caa8f588da55abb9bf5eb1bfb045f77669693
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel, lizzard
bugs1530657
milestone66.0
Bug 1530657. Remove webrender user data properties from frames first, then destory them. r=jrmuizel a=lizzard 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
@@ -904,16 +904,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