Bug 772079 - Fix ThebesLayerInvalidRegion being destroyed too soon. r=roc
authorChris Lord <chrislord.net@gmail.com>
Sat, 14 Jul 2012 08:49:05 +0100
changeset 99280 a7f80f6408eda9a2cf2243bc5da427d95260bc8e
parent 99279 9f3534c54fa38db04f783b218c8f9d310a779900
child 99281 977ee9208065e945aeb2ad85bfeb2eeeed05b515
push id23116
push userryanvm@gmail.com
push dateSat, 14 Jul 2012 16:58:48 +0000
treeherdermozilla-central@9046ecf4db8f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs772079, 758620, 769541
milestone16.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 772079 - Fix ThebesLayerInvalidRegion being destroyed too soon. r=roc A comment in ApplyThebesLayerInvalidation says that it preserves the content of ThebesLayerInvalidRegion, in case there are multiple container layers for the same frame. SetHasContainerLayer, however, immediately clears said property. This was causing invalidations to be lost since Bug 758620 on fixed-position elements, as they were being separated out onto their own layers but were still merged in the root scroll layer. This is tracked in Bug 769541. This fixes the problem by storing the new invalid region in DisplayItemDataEntry and clearing/setting the ThebesLayerInvalidRegion property in the UpdateDisplayItemData callback from FrameLayerBuilder::WillEndTransaction.
layout/base/FrameLayerBuilder.cpp
layout/base/FrameLayerBuilder.h
layout/base/nsDisplayItemTypes.h
layout/base/nsDisplayList.h
--- a/layout/base/FrameLayerBuilder.cpp
+++ b/layout/base/FrameLayerBuilder.cpp
@@ -67,21 +67,16 @@ static inline MaskLayerImageCache* GetMa
 {
   if (!gMaskLayerImageCache) {
     gMaskLayerImageCache = new MaskLayerImageCache();
   }
 
   return gMaskLayerImageCache;
 }
 
-class RefCountedRegion : public RefCounted<RefCountedRegion> {
-public:
-  nsRegion mRegion;
-};
-
 static void DestroyRefCountedRegion(void* aPropertyValue)
 {
   static_cast<RefCountedRegion*>(aPropertyValue)->Release();
 }
 
 /**
  * This property represents a region that should be invalidated in every
  * ThebesLayer child whose parent ContainerLayer is associated with the
@@ -716,63 +711,59 @@ FrameLayerBuilder::WillEndTransaction(La
                "Some frame must have a layer!");
 }
 
 /**
  * If *aThebesLayerInvalidRegion is non-null, use it as this frame's
  * region property. Otherwise set it to the frame's region property.
  */
 static void
-SetHasContainerLayer(nsIFrame* aFrame, nsPoint aOffsetToRoot,
-                     RefCountedRegion** aThebesLayerInvalidRegion)
+SetHasContainerLayer(nsIFrame* aFrame, nsPoint aOffsetToRoot)
 {
   aFrame->AddStateBits(NS_FRAME_HAS_CONTAINER_LAYER);
   for (nsIFrame* f = aFrame;
        f && !(f->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT);
        f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
     f->AddStateBits(NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT);
   }
 
   FrameProperties props = aFrame->Properties();
   nsPoint* lastPaintOffset = static_cast<nsPoint*>
     (props.Get(ThebesLayerLastPaintOffsetProperty()));
   if (lastPaintOffset) {
     *lastPaintOffset = aOffsetToRoot;
   } else {
     props.Set(ThebesLayerLastPaintOffsetProperty(), new nsPoint(aOffsetToRoot));
   }
-
-  // Reset or create the invalid region now so we can start collecting
-  // new dirty areas.
-  if (*aThebesLayerInvalidRegion) {
-    (*aThebesLayerInvalidRegion)->AddRef();
-    props.Set(ThebesLayerInvalidRegionProperty(), *aThebesLayerInvalidRegion);
-  } else {
-    RefCountedRegion* invalidRegion = static_cast<RefCountedRegion*>
-      (props.Get(ThebesLayerInvalidRegionProperty()));
-    if (invalidRegion) {
-      invalidRegion->mRegion.SetEmpty();
-    } else {
-      invalidRegion = new RefCountedRegion();
-      invalidRegion->AddRef();
-      props.Set(ThebesLayerInvalidRegionProperty(), invalidRegion);
-    }
-    *aThebesLayerInvalidRegion = invalidRegion;
-  }
 }
 
 static void
 SetNoContainerLayer(nsIFrame* aFrame)
 {
   FrameProperties props = aFrame->Properties();
   props.Delete(ThebesLayerInvalidRegionProperty());
   props.Delete(ThebesLayerLastPaintOffsetProperty());
   aFrame->RemoveStateBits(NS_FRAME_HAS_CONTAINER_LAYER);
 }
 
+/* static */ void
+FrameLayerBuilder::SetAndClearInvalidRegion(DisplayItemDataEntry* aEntry)
+{
+  if (aEntry->mInvalidRegion) {
+    nsIFrame* f = aEntry->GetKey();
+    FrameProperties props = f->Properties();
+
+    RefCountedRegion* invalidRegion;
+    aEntry->mInvalidRegion.forget(&invalidRegion);
+
+    invalidRegion->mRegion.SetEmpty();
+    props.Set(ThebesLayerInvalidRegionProperty(), invalidRegion);
+  }
+}
+
 /* static */ PLDHashOperator
 FrameLayerBuilder::UpdateDisplayItemDataForFrame(DisplayItemDataEntry* aEntry,
                                                  void* aUserArg)
 {
   FrameLayerBuilder* builder = static_cast<FrameLayerBuilder*>(aUserArg);
   nsIFrame* f = aEntry->GetKey();
   FrameProperties props = f->Properties();
   DisplayItemDataEntry* newDisplayItems =
@@ -785,30 +776,39 @@ FrameLayerBuilder::UpdateDisplayItemData
     SetNoContainerLayer(f);
     return PL_DHASH_REMOVE;
   }
 
   if (!newDisplayItems->HasNonEmptyContainerLayer()) {
     SetNoContainerLayer(f);
   }
 
-  // Steal the list of display item layers
+  // Steal the list of display item layers and invalid region
   aEntry->mData.SwapElements(newDisplayItems->mData);
+  aEntry->mInvalidRegion.swap(newDisplayItems->mInvalidRegion);
+  // Clear and reset the invalid region now so we can start collecting new
+  // dirty areas.
+  SetAndClearInvalidRegion(aEntry);
   // Don't need to process this frame again
   builder->mNewDisplayItemData.RawRemoveEntry(newDisplayItems);
   return PL_DHASH_NEXT;
 }
 
 /* static */ PLDHashOperator
 FrameLayerBuilder::StoreNewDisplayItemData(DisplayItemDataEntry* aEntry,
                                            void* aUserArg)
 {
   LayerManagerData* data = static_cast<LayerManagerData*>(aUserArg);
   nsIFrame* f = aEntry->GetKey();
   FrameProperties props = f->Properties();
+
+  // Clear and reset the invalid region now so we can start collecting new
+  // dirty areas.
+  SetAndClearInvalidRegion(aEntry);
+
   // Remember that this frame has display items in retained layers
   NS_ASSERTION(!data->mFramesWithLayers.GetEntry(f),
                "We shouldn't get here if we're already in mFramesWithLayers");
   DisplayItemDataEntry *newEntry = data->mFramesWithLayers.PutEntry(f);
   NS_ASSERTION(!props.Get(LayerManagerDataProperty()),
                "mFramesWithLayers out of sync");
 
   newEntry->mData.SwapElements(aEntry->mData);
@@ -2132,42 +2132,54 @@ FrameLayerBuilder::BuildContainerLayerFo
 
   ContainerParameters scaleParameters =
     ChooseScaleAndSetTransform(this, aContainerFrame, aTransform, aParameters,
                                containerLayer);
   ContainerState state(aBuilder, aManager, aContainerFrame, containerLayer,
                        scaleParameters);
 
   if (aManager == mRetainingManager) {
+    FrameProperties props = aContainerFrame->Properties();
+    RefCountedRegion* thebesLayerInvalidRegion = nsnull;
     DisplayItemDataEntry* entry = mNewDisplayItemData.PutEntry(aContainerFrame);
     if (entry) {
       entry->mData.AppendElement(
           DisplayItemData(containerLayer, containerDisplayItemKey, LAYER_ACTIVE));
+      thebesLayerInvalidRegion = static_cast<RefCountedRegion*>
+        (props.Get(ThebesLayerInvalidRegionProperty()));
+      if (!thebesLayerInvalidRegion) {
+        thebesLayerInvalidRegion = new RefCountedRegion();
+      }
+      entry->mInvalidRegion = thebesLayerInvalidRegion;
     }
     nsPoint currentOffset;
     ApplyThebesLayerInvalidation(aBuilder, aContainerFrame, aContainerItem, state,
                                  &currentOffset);
-    RefCountedRegion* thebesLayerInvalidRegion = nsnull;
-    SetHasContainerLayer(aContainerFrame, currentOffset, &thebesLayerInvalidRegion);
+    SetHasContainerLayer(aContainerFrame, currentOffset);
 
     nsAutoTArray<nsIFrame*,4> mergedFrames;
     if (aContainerItem) {
       aContainerItem->GetMergedFrames(&mergedFrames);
     }
     for (PRUint32 i = 0; i < mergedFrames.Length(); ++i) {
       nsIFrame* mergedFrame = mergedFrames[i];
       DisplayItemDataEntry* entry = mNewDisplayItemData.PutEntry(mergedFrame);
       if (entry) {
         // Ensure that UpdateDisplayItemDataForFrame recognizes that we
         // still have a container layer associated with this frame.
         entry->mIsSharingContainerLayer = true;
+
+        // Store the invalid region property in case this frame is represented
+        // by multiple container layers. This is cleared and set when iterating
+        // over the DisplayItemDataEntry's in WillEndTransaction.
+        entry->mInvalidRegion = thebesLayerInvalidRegion;
       }
       ApplyThebesLayerInvalidation(aBuilder, mergedFrame, nsnull, state,
                                    &currentOffset);
-      SetHasContainerLayer(mergedFrame, currentOffset, &thebesLayerInvalidRegion);
+      SetHasContainerLayer(mergedFrame, currentOffset);
     }
   }
 
   Clip clip;
   state.ProcessDisplayItems(aChildren, clip);
 
   // Set CONTENT_COMPONENT_ALPHA if any of our children have it.
   // This is suboptimal ... a child could have text that's over transparent
--- a/layout/base/FrameLayerBuilder.h
+++ b/layout/base/FrameLayerBuilder.h
@@ -27,16 +27,21 @@ enum LayerState {
   LAYER_ACTIVE,
   // Force an active layer even if it causes incorrect rendering, e.g.
   // when the layer has rounded rect clips.
   LAYER_ACTIVE_FORCE,
   // Special layer that is metadata only.
   LAYER_ACTIVE_EMPTY
 };
 
+class RefCountedRegion : public RefCounted<RefCountedRegion> {
+public:
+  nsRegion mRegion;
+};
+
 /**
  * The FrameLayerBuilder belongs to an nsDisplayListBuilder and is
  * responsible for converting display lists into layer trees.
  * 
  * The most important API in this class is BuildContainerLayerFor. This
  * method takes a display list as input and constructs a ContainerLayer
  * with child layers that render the contents of the display list. It
  * also updates userdata for the retained layer manager, and
@@ -439,23 +444,25 @@ protected:
    */
   class DisplayItemDataEntry : public nsPtrHashKey<nsIFrame> {
   public:
     DisplayItemDataEntry(const nsIFrame *key) : nsPtrHashKey<nsIFrame>(key), mIsSharingContainerLayer(false) {}
     DisplayItemDataEntry(DisplayItemDataEntry &toCopy) :
       nsPtrHashKey<nsIFrame>(toCopy.mKey), mIsSharingContainerLayer(toCopy.mIsSharingContainerLayer)
     {
       // This isn't actually a copy-constructor; notice that it steals toCopy's
-      // array.  Be careful.
+      // array and invalid region.  Be careful.
       mData.SwapElements(toCopy.mData);
+      mInvalidRegion.swap(toCopy.mInvalidRegion);
     }
 
     bool HasNonEmptyContainerLayer();
 
     nsAutoTArray<DisplayItemData, 1> mData;
+    nsRefPtr<RefCountedRegion> mInvalidRegion;
     bool mIsSharingContainerLayer;
 
     enum { ALLOW_MEMMOVE = false };
   };
 
   // LayerManagerData needs to see DisplayItemDataEntry.
   friend class LayerManagerData;
 
@@ -542,16 +549,17 @@ public:
   ThebesLayerItemsEntry* GetThebesLayerItemsEntry(ThebesLayer* aLayer)
   {
     return mThebesLayerItems.GetEntry(aLayer);
   }
 
 protected:
   void RemoveThebesItemsForLayerSubtree(Layer* aLayer);
 
+  static void SetAndClearInvalidRegion(DisplayItemDataEntry* aEntry);
   static PLDHashOperator UpdateDisplayItemDataForFrame(DisplayItemDataEntry* aEntry,
                                                        void* aUserArg);
   static PLDHashOperator StoreNewDisplayItemData(DisplayItemDataEntry* aEntry,
                                                  void* aUserArg);
 
   /**
    * Returns true if the DOM has been modified since we started painting,
    * in which case we should bail out and not paint anymore. This should
--- a/layout/base/nsDisplayItemTypes.h
+++ b/layout/base/nsDisplayItemTypes.h
@@ -29,16 +29,17 @@ enum Type {
   TYPE_CHECKED_CHECKBOX,
   TYPE_CHECKED_RADIOBUTTON,
   TYPE_CLIP,
   TYPE_CLIP_ROUNDED_RECT,
   TYPE_COLUMN_RULE,
   TYPE_COMBOBOX_FOCUS,
   TYPE_EVENT_RECEIVER,
   TYPE_FIELDSET_BORDER_BACKGROUND,
+  TYPE_FIXED_POSITION,
   TYPE_FORCEPAINTONSCROLL,
   TYPE_FRAMESET_BORDER,
   TYPE_FRAMESET_BLANK,
   TYPE_HEADER_FOOTER,
   TYPE_IMAGE,
   TYPE_LIST_FOCUS,
   TYPE_OPACITY,
   TYPE_OPTION_EVENT_GRABBER,
--- a/layout/base/nsDisplayList.h
+++ b/layout/base/nsDisplayList.h
@@ -1920,17 +1920,17 @@ public:
                          nsDisplayList* aList);
 #ifdef NS_BUILD_REFCNT_LOGGING
   virtual ~nsDisplayFixedPosition();
 #endif
 
   virtual already_AddRefed<Layer> BuildLayer(nsDisplayListBuilder* aBuilder,
                                              LayerManager* aManager,
                                              const ContainerParameters& aContainerParameters);
-  NS_DISPLAY_DECL_NAME("FixedPosition", TYPE_OWN_LAYER)
+  NS_DISPLAY_DECL_NAME("FixedPosition", TYPE_FIXED_POSITION)
 };
 
 /**
  * This potentially creates a layer for the given list of items, whose
  * visibility is determined by the displayport for the given frame instead of
  * what is passed in to ComputeVisibility.
  *
  * Here in content, we can use this to render more content than is actually