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

--HG--
extra : rebase_source : f5d572ed37255b9036ce2678645788768e0a52ae

layout/base/DisplayItemClip.h

@@ -53,15 +53,7 @@ public:
   };
 
   // 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);
@@ -176,9 +168,6 @@ private:
   // 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;
 };
 
 }

layout/base/DisplayListClipState.cpp

@@ -18,17 +18,16 @@ DisplayListClipState::GetCurrentCombinedClip(nsDisplayListBuilder* aBuilder)
   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;
 }

layout/base/nsDisplayList.cpp

@@ -707,6 +707,10 @@ nsDisplayListBuilder::~nsDisplayListBuilder() {
 
   nsCSSRendering::EndFrameTreesLocked();
 
+  for (uint32_t i = 0; i < mDisplayItemClipsToDestroy.Length(); ++i) {
+    mDisplayItemClipsToDestroy[i]->DisplayItemClip::~DisplayItemClip();
+  }
+
   PL_FinishArenaPool(&mPool);
   MOZ_COUNT_DTOR(nsDisplayListBuilder);
 }
@@ -868,6 +872,15 @@ nsDisplayListBuilder::Allocate(size_t aSize) {
   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());

layout/base/nsDisplayList.h

@@ -478,7 +478,13 @@ public:
    * 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
@@ -646,6 +652,7 @@ private:
   nsRegion                       mExcludedGlassRegion;
   // The display item for the Windows window glass background, if any
   nsDisplayItem*                 mGlassDisplayItem;
+  nsTArray<DisplayItemClip*>     mDisplayItemClipsToDestroy;
   Mode                           mMode;
   bool                           mBuildCaret;
   bool                           mIgnoreSuppression;
@@ -746,12 +753,7 @@ public:
 #endif
   {
   }
-  virtual ~nsDisplayItem()
-  {
-    if (mClip) {
-      mClip->MaybeDestroy();
-    }
-  }
+  virtual ~nsDisplayItem() {}
   
   void* operator new(size_t aSize,
                      nsDisplayListBuilder* aBuilder) CPP_THROW_NEW {
@@ -1193,17 +1195,11 @@ public:
   }
   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:

layout/reftests/border-radius/reftest.list

@@ -76,6 +76,8 @@ skip-if(B2G) random-if(winWidget) HTTP(..) == corner-joins-2.xhtml corner-joins-
 
 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

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>

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>