Bug 860524. Remove hacky (and buggy) pseudo-destruction of DisplayItemClips created by GetCurrentClip; ensure that they're all destroyed properly when the arena goes away, by tracking them explicitly in nsDisplayListBuilder. r=mattwoodrow
authorRobert O'Callahan <robert@ocallahan.org>
Mon, 15 Apr 2013 17:11:10 +1200
changeset 140662 d13fc07e67afaf4320078f5954de3a3ea3c30596
parent 140661 fad77e38f79e2db7a8e9406acc98bfa88ecce0ec
child 140663 0b2c39985587efd2ea3f772c277942e3a4693aee
push id2579
push userakeybl@mozilla.com
push dateMon, 24 Jun 2013 18:52:47 +0000
treeherdermozilla-beta@b69b7de8a05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow
bugs860524
milestone23.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 860524. Remove hacky (and buggy) pseudo-destruction of DisplayItemClips created by GetCurrentClip; ensure that they're all destroyed properly when the arena goes away, by tracking them explicitly in nsDisplayListBuilder. r=mattwoodrow
layout/base/DisplayItemClip.h
layout/base/DisplayListClipState.cpp
layout/base/nsDisplayList.cpp
layout/base/nsDisplayList.h
layout/reftests/border-radius/reftest.list
layout/reftests/border-radius/transforms-1-ref.html
layout/reftests/border-radius/transforms-1.html
--- a/layout/base/DisplayItemClip.h
+++ b/layout/base/DisplayItemClip.h
@@ -48,25 +48,17 @@ public:
       return true;
     }
     bool operator!=(const RoundedRect& aOther) const {
       return !(*this == aOther);
     }
   };
 
   // Constructs a DisplayItemClip that does no clipping at all.
-  DisplayItemClip() : mHaveClipRect(false), mHasBeenDestroyed(false) {}
-  ~DisplayItemClip() { mHasBeenDestroyed = true; }
-
-  void MaybeDestroy() const
-  {
-    if (!mHasBeenDestroyed) {
-      this->~DisplayItemClip();
-    }
-  }
+  DisplayItemClip() : mHaveClipRect(false) {}
 
   void SetTo(const nsRect& aRect);
   void SetTo(const nsRect& aRect, const nscoord* aRadii);
   void IntersectWith(const DisplayItemClip& aOther);
 
   // Apply this |DisplayItemClip| to the given gfxContext.  Any saving of state
   // or clearing of other clips must be done by the caller.
   // See aBegin/aEnd note on ApplyRoundedRectsTo.
@@ -171,16 +163,13 @@ public:
   static void Shutdown();
 
 private:
   nsRect mClipRect;
   nsTArray<RoundedRect> mRoundedClipRects;
   // If mHaveClipRect is false then this object represents no clipping at all
   // and mRoundedClipRects must be empty.
   bool mHaveClipRect;
-  // Set to true when the destructor has run. This is a bit of a hack
-  // to ensure that we can easily share arena-allocated DisplayItemClips.
-  bool mHasBeenDestroyed;
 };
 
 }
 
 #endif /* DISPLAYITEMCLIP_H_ */
--- a/layout/base/DisplayListClipState.cpp
+++ b/layout/base/DisplayListClipState.cpp
@@ -13,27 +13,26 @@ const DisplayItemClip*
 DisplayListClipState::GetCurrentCombinedClip(nsDisplayListBuilder* aBuilder)
 {
   if (mCurrentCombinedClip) {
     return mCurrentCombinedClip;
   }
   if (!mClipContentDescendants && !mClipContainingBlockDescendants) {
     return nullptr;
   }
-  void* mem = aBuilder->Allocate(sizeof(DisplayItemClip));
   if (mClipContentDescendants) {
     DisplayItemClip* newClip =
-      new (mem) DisplayItemClip(*mClipContentDescendants);
+      aBuilder->AllocateDisplayItemClip(*mClipContentDescendants);
     if (mClipContainingBlockDescendants) {
       newClip->IntersectWith(*mClipContainingBlockDescendants);
     }
     mCurrentCombinedClip = newClip;
   } else {
     mCurrentCombinedClip =
-      new (mem) DisplayItemClip(*mClipContainingBlockDescendants);
+      aBuilder->AllocateDisplayItemClip(*mClipContainingBlockDescendants);
   }
   return mCurrentCombinedClip;
 }
 
 void
 DisplayListClipState::ClipContainingBlockDescendants(const nsRect& aRect,
                                                      const nscoord* aRadii,
                                                      DisplayItemClip& aClipOnStack)
--- a/layout/base/nsDisplayList.cpp
+++ b/layout/base/nsDisplayList.cpp
@@ -702,16 +702,20 @@ nsDisplayListBuilder::~nsDisplayListBuil
   NS_ASSERTION(mFramesMarkedForDisplay.Length() == 0,
                "All frames should have been unmarked");
   NS_ASSERTION(mPresShellStates.Length() == 0,
                "All presshells should have been exited");
   NS_ASSERTION(!mCurrentTableItem, "No table item should be active");
 
   nsCSSRendering::EndFrameTreesLocked();
 
+  for (uint32_t i = 0; i < mDisplayItemClipsToDestroy.Length(); ++i) {
+    mDisplayItemClipsToDestroy[i]->DisplayItemClip::~DisplayItemClip();
+  }
+
   PL_FinishArenaPool(&mPool);
   MOZ_COUNT_DTOR(nsDisplayListBuilder);
 }
 
 uint32_t
 nsDisplayListBuilder::GetBackgroundPaintFlags() {
   uint32_t flags = 0;
   if (mSyncDecodeImages) {
@@ -863,16 +867,25 @@ nsDisplayListBuilder::Allocate(size_t aS
   void *tmp;
   PL_ARENA_ALLOCATE(tmp, &mPool, aSize);
   if (!tmp) {
     NS_RUNTIMEABORT("out of memory");
   }
   return tmp;
 }
 
+DisplayItemClip*
+nsDisplayListBuilder::AllocateDisplayItemClip(const DisplayItemClip& aOriginal)
+{
+  void* p = Allocate(sizeof(DisplayItemClip));
+  DisplayItemClip* c = new (p) DisplayItemClip(aOriginal);
+  mDisplayItemClipsToDestroy.AppendElement(c);
+  return c;
+}
+
 void nsDisplayListSet::MoveTo(const nsDisplayListSet& aDestination) const
 {
   aDestination.BorderBackground()->AppendToTop(BorderBackground());
   aDestination.BlockBorderBackgrounds()->AppendToTop(BlockBorderBackgrounds());
   aDestination.Floats()->AppendToTop(Floats());
   aDestination.Content()->AppendToTop(Content());
   aDestination.PositionedDescendants()->AppendToTop(PositionedDescendants());
   aDestination.Outlines()->AppendToTop(Outlines());
--- a/layout/base/nsDisplayList.h
+++ b/layout/base/nsDisplayList.h
@@ -473,17 +473,23 @@ public:
   }
 
   /**
    * Allocate memory in our arena. It will only be freed when this display list
    * builder is destroyed. This memory holds nsDisplayItems. nsDisplayItem
    * destructors are called as soon as the item is no longer used.
    */
   void* Allocate(size_t aSize);
-  
+
+  /**
+   * Allocate a new DisplayListClip in the arena. Will be cleaned up
+   * automatically when the arena goes away.
+   */
+  DisplayItemClip* AllocateDisplayItemClip(const DisplayItemClip& aOriginal);
+
   /**
    * A helper class to temporarily set the value of
    * mIsAtRootOfPseudoStackingContext and mIsInFixedPosition, and temporarily
    * update mCachedOffsetFrame/mCachedOffset from a frame to its child.
    * Also saves and restores mClipState.
    */
   class AutoBuildingDisplayList;
   friend class AutoBuildingDisplayList;
@@ -641,16 +647,17 @@ private:
   // mCachedOffsetFrame to mReferenceFrame.
   const nsIFrame*                mCachedOffsetFrame;
   const nsIFrame*                mCachedReferenceFrame;
   nsPoint                        mCachedOffset;
   nsRect                         mDisplayPort;
   nsRegion                       mExcludedGlassRegion;
   // The display item for the Windows window glass background, if any
   nsDisplayItem*                 mGlassDisplayItem;
+  nsTArray<DisplayItemClip*>     mDisplayItemClipsToDestroy;
   Mode                           mMode;
   bool                           mBuildCaret;
   bool                           mIgnoreSuppression;
   bool                           mHadToIgnoreSuppression;
   bool                           mIsAtRootOfPseudoStackingContext;
   bool                           mIncludeAllOutOfFlows;
   bool                           mSelectedFramesOnly;
   bool                           mAccurateVisibleRegions;
@@ -741,22 +748,17 @@ public:
     : mFrame(aFrame)
     , mClip(nullptr)
     , mReferenceFrame(nullptr)
 #ifdef MOZ_DUMP_PAINTING
     , mPainted(false)
 #endif
   {
   }
-  virtual ~nsDisplayItem()
-  {
-    if (mClip) {
-      mClip->MaybeDestroy();
-    }
-  }
+  virtual ~nsDisplayItem() {}
   
   void* operator new(size_t aSize,
                      nsDisplayListBuilder* aBuilder) CPP_THROW_NEW {
     return aBuilder->Allocate(aSize);
   }
 
 // Contains all the type integers for each display list item type
 #include "nsDisplayItemTypes.h"
@@ -1188,27 +1190,21 @@ public:
   virtual bool SupportsOptimizingToImage() { return false; }
 
   const DisplayItemClip& GetClip()
   {
     return mClip ? *mClip : DisplayItemClip::NoClip();
   }
   void SetClip(nsDisplayListBuilder* aBuilder, const DisplayItemClip& aClip)
   {
-    if (mClip) {
-      mClip->MaybeDestroy();
-    }
     if (!aClip.HasClip()) {
       mClip = nullptr;
       return;
     }
-    void* mem = aBuilder->Allocate(sizeof(DisplayItemClip));
-    DisplayItemClip* clip = new (mem) DisplayItemClip();
-    *clip = aClip;
-    mClip = clip;
+    mClip = aBuilder->AllocateDisplayItemClip(aClip);
   }
 
 protected:
   friend class nsDisplayList;
 
   nsDisplayItem() { mAbove = nullptr; }
 
   nsIFrame* mFrame;
--- a/layout/reftests/border-radius/reftest.list
+++ b/layout/reftests/border-radius/reftest.list
@@ -71,16 +71,18 @@ skip-if(B2G) fails-if(Android) == scroll
 skip-if(B2G) fails-if(Android) == scrollbar-clamping-2.html scrollbar-clamping-2-ref.html
 
 # Test for bad corner joins.
 fuzzy-if(true,1,1) == corner-joins-1.xhtml corner-joins-1-ref.xhtml
 skip-if(B2G) random-if(winWidget) HTTP(..) == corner-joins-2.xhtml corner-joins-2-ref.xhtml
 
 skip-if(B2G) fuzzy-if(/^Windows\x20NT\x206\.2/.test(http.oscpu),1,20) fuzzy-if(Android&&browserIsRemote,7,146) fuzzy-if(Android&&!browserIsRemote,166,248) == scroll-1.html scroll-1-ref.html # see bug 732535
 
+== transforms-1.html transforms-1-ref.html
+
 == zero-radius-clip-1.html zero-radius-clip-ref.html
 
 == iframe-1.html iframe-1-ref.html
 
 # Test for antialiasing gaps between background and border
 skip-if(B2G) fails-if(winWidget) == curved-border-background-nogap.html curved-border-background-nogap-ref.html
 
 == color-layer-1a.html color-layer-1-ref.html
new file mode 100644
--- /dev/null
+++ b/layout/reftests/border-radius/transforms-1-ref.html
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+<body style="background:white">
+<style>
+div {
+  position: absolute;
+  width: 100px;
+  height: 100px;
+  border-radius: 30px;
+  overflow: hidden;
+}
+
+span {
+  position: absolute;
+  top: 0;
+  left: 0;
+  width: 100%;
+  height: 100%;
+  background: black;
+}
+</style>
+<div><span></span></div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/border-radius/transforms-1.html
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<html>
+<body style="background:white">
+<style>
+div {
+  position: absolute;
+  width: 100px;
+  height: 100px;
+  border-radius: 30px;
+  overflow: hidden;
+}
+
+span {
+  position: absolute;
+  top: 0;
+  left: 0;
+  width: 100%;
+  height: 100%;
+  background: black;
+  transform: scale(2);
+}
+</style>
+<div><span></span></div>
+</body>
+</html>