Bug 970747 - 1/6 - Make ContainerLayer::InsertAfter always perform checks and return bool - r=mattwoodrow
authorBenoit Jacob <bjacob@mozilla.com>
Fri, 21 Feb 2014 16:50:25 -0500
changeset 170321 b0f5a1cf6f8a745d9e273592415c0ba574c1ff7a
parent 170320 e508a0767ec0bfb6f8cb7165d70400f7df54aec9
child 170322 f1a9d225b8ee26eb97ac29fca2d7213d64bc68d2
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersmattwoodrow
bugs970747
milestone30.0a1
Bug 970747 - 1/6 - Make ContainerLayer::InsertAfter always perform checks and return bool - r=mattwoodrow
gfx/layers/Layers.cpp
gfx/layers/Layers.h
gfx/layers/basic/BasicContainerLayer.h
gfx/layers/basic/BasicLayers.h
gfx/layers/client/ClientContainerLayer.h
gfx/layers/client/ClientLayerManager.h
gfx/tests/gtest/TestAsyncPanZoomController.cpp
--- a/gfx/layers/Layers.cpp
+++ b/gfx/layers/Layers.cpp
@@ -716,54 +716,63 @@ ContainerLayer::ContainerLayer(LayerMana
     mSupportsComponentAlphaChildren(false),
     mMayHaveReadbackChild(false)
 {
   mContentFlags = 0; // Clear NO_TEXT, NO_TEXT_OVER_TRANSPARENT
 }
 
 ContainerLayer::~ContainerLayer() {}
 
-void
+bool
 ContainerLayer::InsertAfter(Layer* aChild, Layer* aAfter)
 {
-  NS_ASSERTION(aChild->Manager() == Manager(),
-               "Child has wrong manager");
-  NS_ASSERTION(!aChild->GetParent(),
-               "aChild already in the tree");
-  NS_ASSERTION(!aChild->GetNextSibling() && !aChild->GetPrevSibling(),
-               "aChild already has siblings?");
-  NS_ASSERTION(!aAfter ||
-               (aAfter->Manager() == Manager() &&
-                aAfter->GetParent() == this),
-               "aAfter is not our child");
+  if(aChild->Manager() != Manager()) {
+    NS_ERROR("Child has wrong manager");
+    return false;
+  }
+  if(aChild->GetParent()) {
+    NS_ERROR("aChild already in the tree");
+    return false;
+  }
+  if (aChild->GetNextSibling() || aChild->GetPrevSibling()) {
+    NS_ERROR("aChild already has siblings?");
+    return false;
+  }
+  if (aAfter && (aAfter->Manager() != Manager() ||
+                 aAfter->GetParent() != this))
+  {
+    NS_ERROR("aAfter is not our child");
+    return false;
+  }
 
   aChild->SetParent(this);
   if (aAfter == mLastChild) {
     mLastChild = aChild;
   }
   if (!aAfter) {
     aChild->SetNextSibling(mFirstChild);
     if (mFirstChild) {
       mFirstChild->SetPrevSibling(aChild);
     }
     mFirstChild = aChild;
     NS_ADDREF(aChild);
     DidInsertChild(aChild);
-    return;
+    return true;
   }
 
   Layer* next = aAfter->GetNextSibling();
   aChild->SetNextSibling(next);
   aChild->SetPrevSibling(aAfter);
   if (next) {
     next->SetPrevSibling(aChild);
   }
   aAfter->SetNextSibling(aChild);
   NS_ADDREF(aChild);
   DidInsertChild(aChild);
+  return true;
 }
 
 void
 ContainerLayer::RemoveChild(Layer *aChild)
 {
   NS_ASSERTION(aChild->Manager() == Manager(),
                "Child has wrong manager");
   NS_ASSERTION(aChild->GetParent() == this,
--- a/gfx/layers/Layers.h
+++ b/gfx/layers/Layers.h
@@ -1556,17 +1556,17 @@ public:
 
   /**
    * CONSTRUCTION PHASE ONLY
    * Insert aChild into the child list of this container. aChild must
    * not be currently in any child list or the root for the layer manager.
    * If aAfter is non-null, it must be a child of this container and
    * we insert after that layer. If it's null we insert at the start.
    */
-  virtual void InsertAfter(Layer* aChild, Layer* aAfter);
+  virtual bool InsertAfter(Layer* aChild, Layer* aAfter);
   /**
    * CONSTRUCTION PHASE ONLY
    * Remove aChild from the child list of this container. aChild must
    * be a child of this container.
    */
   virtual void RemoveChild(Layer* aChild);
   /**
    * CONSTRUCTION PHASE ONLY
@@ -1957,18 +1957,18 @@ private:
  *
  * Clients will usually want to Connect/Clear() on each transaction to
  * avoid difficulties managing memory across multiple layer subtrees.
  */
 class RefLayer : public ContainerLayer {
   friend class LayerManager;
 
 private:
-  virtual void InsertAfter(Layer* aChild, Layer* aAfter)
-  { MOZ_CRASH(); }
+  virtual bool InsertAfter(Layer* aChild, Layer* aAfter) MOZ_OVERRIDE
+  { MOZ_CRASH(); return false; }
 
   virtual void RemoveChild(Layer* aChild)
   { MOZ_CRASH(); }
 
   virtual void RepositionChild(Layer* aChild, Layer* aAfter)
   { MOZ_CRASH(); }
 
   using ContainerLayer::SetFrameMetrics;
--- a/gfx/layers/basic/BasicContainerLayer.h
+++ b/gfx/layers/basic/BasicContainerLayer.h
@@ -29,21 +29,23 @@ public:
   virtual ~BasicContainerLayer();
 
   virtual void SetVisibleRegion(const nsIntRegion& aRegion)
   {
     NS_ASSERTION(BasicManager()->InConstruction(),
                  "Can only set properties in construction phase");
     ContainerLayer::SetVisibleRegion(aRegion);
   }
-  virtual void InsertAfter(Layer* aChild, Layer* aAfter)
+  virtual bool InsertAfter(Layer* aChild, Layer* aAfter)
   {
-    NS_ASSERTION(BasicManager()->InConstruction(),
-                 "Can only set properties in construction phase");
-    ContainerLayer::InsertAfter(aChild, aAfter);
+    if (!BasicManager()->InConstruction()) {
+      NS_ERROR("Can only set properties in construction phase");
+      return false;
+    }
+    return ContainerLayer::InsertAfter(aChild, aAfter);
   }
 
   virtual void RemoveChild(Layer* aChild)
   { 
     NS_ASSERTION(BasicManager()->InConstruction(),
                  "Can only set properties in construction phase");
     ContainerLayer::RemoveChild(aChild);
   }
--- a/gfx/layers/basic/BasicLayers.h
+++ b/gfx/layers/basic/BasicLayers.h
@@ -109,18 +109,18 @@ public:
   virtual already_AddRefed<CanvasLayer> CreateCanvasLayer();
   virtual already_AddRefed<ColorLayer> CreateColorLayer();
   virtual already_AddRefed<ReadbackLayer> CreateReadbackLayer();
   virtual ImageFactory *GetImageFactory();
 
   virtual LayersBackend GetBackendType() { return LayersBackend::LAYERS_BASIC; }
   virtual void GetBackendName(nsAString& name) { name.AssignLiteral("Basic"); }
 
+  bool InConstruction() { return mPhase == PHASE_CONSTRUCTION; }
 #ifdef DEBUG
-  bool InConstruction() { return mPhase == PHASE_CONSTRUCTION; }
   bool InDrawing() { return mPhase == PHASE_DRAWING; }
   bool InForward() { return mPhase == PHASE_FORWARD; }
 #endif
   bool InTransaction() { return mPhase != PHASE_NONE; }
 
   gfxContext* GetTarget() { return mTarget; }
   void SetTarget(gfxContext* aTarget) { mUsingDefaultTarget = false; mTarget = aTarget; }
   bool IsRetained() { return mWidget != nullptr; }
--- a/gfx/layers/client/ClientContainerLayer.h
+++ b/gfx/layers/client/ClientContainerLayer.h
@@ -85,24 +85,31 @@ public:
   }
 
   virtual void SetVisibleRegion(const nsIntRegion& aRegion)
   {
     NS_ASSERTION(ClientManager()->InConstruction(),
                  "Can only set properties in construction phase");
     ContainerLayer::SetVisibleRegion(aRegion);
   }
-  virtual void InsertAfter(Layer* aChild, Layer* aAfter) MOZ_OVERRIDE
+  virtual bool InsertAfter(Layer* aChild, Layer* aAfter) MOZ_OVERRIDE
   {
-    NS_ASSERTION(ClientManager()->InConstruction(),
-                 "Can only set properties in construction phase");
+    if(!ClientManager()->InConstruction()) {
+      NS_ERROR("Can only set properties in construction phase");
+      return false;
+    }
+
+    if (!ContainerLayer::InsertAfter(aChild, aAfter)) {
+      return false;
+    }
+
     ClientManager()->AsShadowForwarder()->InsertAfter(ClientManager()->Hold(this),
                                                       ClientManager()->Hold(aChild),
                                                       aAfter ? ClientManager()->Hold(aAfter) : nullptr);
-    ContainerLayer::InsertAfter(aChild, aAfter);
+    return true;
   }
 
   virtual void RemoveChild(Layer* aChild) MOZ_OVERRIDE
   { 
     NS_ASSERTION(ClientManager()->InConstruction(),
                  "Can only set properties in construction phase");
     ClientManager()->AsShadowForwarder()->RemoveChild(ClientManager()->Hold(this),
                                                       ClientManager()->Hold(aChild));
--- a/gfx/layers/client/ClientLayerManager.h
+++ b/gfx/layers/client/ClientLayerManager.h
@@ -134,18 +134,18 @@ public:
    * This is only called if gfxPlatform::UseProgressiveTilePainting() returns
    * true.
    */
   bool ProgressiveUpdateCallback(bool aHasPendingNewThebesContent,
                                  ScreenRect& aCompositionBounds,
                                  CSSToScreenScale& aZoom,
                                  bool aDrawingCritical);
 
+  bool InConstruction() { return mPhase == PHASE_CONSTRUCTION; }
 #ifdef DEBUG
-  bool InConstruction() { return mPhase == PHASE_CONSTRUCTION; }
   bool InDrawing() { return mPhase == PHASE_DRAWING; }
   bool InForward() { return mPhase == PHASE_FORWARD; }
 #endif
   bool InTransaction() { return mPhase != PHASE_NONE; }
 
   void SetNeedsComposite(bool aNeedsComposite)
   {
     mNeedsComposite = aNeedsComposite;
--- a/gfx/tests/gtest/TestAsyncPanZoomController.cpp
+++ b/gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ -81,17 +81,17 @@ private:
 
 
 class TestAPZCContainerLayer : public ContainerLayer {
   public:
     TestAPZCContainerLayer()
       : ContainerLayer(nullptr, nullptr)
     {}
   void RemoveChild(Layer* aChild) {}
-  void InsertAfter(Layer* aChild, Layer* aAfter) {}
+  bool InsertAfter(Layer* aChild, Layer* aAfter) { return true; }
   void ComputeEffectiveTransforms(const Matrix4x4& aTransformToSurface) {}
   void RepositionChild(Layer* aChild, Layer* aAfter) {}
 };
 
 class TestAsyncPanZoomController : public AsyncPanZoomController {
 public:
   TestAsyncPanZoomController(uint64_t aLayersId, MockContentController* aMcc,
                              APZCTreeManager* aTreeManager = nullptr,