Bug 377015 - Simplify nsSVGClipPathFrame API and make clip reference loop detection more robust. r=jwatt,sr=tor

layout/svg/base/src/nsSVGClipPathFrame.cpp

@@ -101,10 +101,10 @@ nsSVGClipPathFrame::InitSVG()
   return NS_OK;
 }
 
-NS_IMETHODIMP
+nsresult
 nsSVGClipPathFrame::ClipPaint(nsSVGRenderState* aContext,
                               nsISVGChildFrame* aParent,
-                              nsCOMPtr<nsIDOMSVGMatrix> aMatrix)
+                              nsIDOMSVGMatrix *aMatrix)
 {
   // If the flag is set when we get here, it means this clipPath frame
   // has already been used painting the current clip, and the document
@@ -113,13 +113,12 @@ nsSVGClipPathFrame::ClipPaint(nsSVGRenderState* aContext,
     NS_WARNING("Clip loop detected!");
     return NS_OK;
   }
-  mInUse = PR_TRUE;
+  AutoClipPathReferencer clipRef(this);
 
   mClipParent = aParent,
   mClipParentMatrix = aMatrix;
 
-  PRBool isTrivial;
+  PRBool isTrivial = IsTrivial();
-  IsTrivial(&isTrivial);
 
   nsAutoSVGRenderMode mode(aContext,
                            isTrivial ? nsSVGRenderState::CLIP
@@ -140,26 +139,22 @@ nsSVGClipPathFrame::ClipPaint(nsSVGRenderState* aContext,
     aContext->GetGfxContext()->NewPath();
   }
 
-  mInUse = PR_FALSE;
-
   return NS_OK;
 }
 
-NS_IMETHODIMP
+PRBool
 nsSVGClipPathFrame::ClipHitTest(nsISVGChildFrame* aParent,
-                                nsCOMPtr<nsIDOMSVGMatrix> aMatrix,
+                                nsIDOMSVGMatrix *aMatrix,
-                                float aX, float aY, PRBool *aHit)
+                                float aX, float aY)
 {
-  *aHit = PR_FALSE;
-
   // If the flag is set when we get here, it means this clipPath frame
   // has already been used in hit testing against the current clip,
   // and the document has a clip reference loop.
   if (mInUse) {
     NS_WARNING("Clip loop detected!");
-    return NS_OK;
+    return PR_FALSE;
   }
-  mInUse = PR_TRUE;
+  AutoClipPathReferencer clipRef(this);
 
   nsRect dirty;
   mClipParent = aParent,
@@ -177,23 +172,16 @@ nsSVGClipPathFrame::ClipHitTest(nsISVGChildFrame* aParent,
 
       nsIFrame *temp = nsnull;
       nsresult rv = SVGFrame->GetFrameForPointSVG(aX, aY, &temp);
-      if (NS_SUCCEEDED(rv) && temp) {
+      if (NS_SUCCEEDED(rv) && temp)
-        *aHit = PR_TRUE;
+        return PR_TRUE;
-        mInUse = PR_FALSE;
-        return NS_OK;
-      }
     }
   }
-
+  return PR_FALSE;
-  mInUse = PR_FALSE;
-
-  return NS_OK;
 }
 
-NS_IMETHODIMP
+PRBool
-nsSVGClipPathFrame::IsTrivial(PRBool *aTrivial)
+nsSVGClipPathFrame::IsTrivial()
 {
-  *aTrivial = PR_TRUE;
   PRBool foundChild = PR_FALSE;
 
   for (nsIFrame* kid = mFrames.FirstChild(); kid;
@@ -204,15 +192,12 @@ nsSVGClipPathFrame::IsTrivial(PRBool *aTrivial)
     if (svgChild) {
       // We consider a non-trivial clipPath to be one containing
       // either more than one svg child and/or a svg container
-      if (foundChild || svgChild->IsDisplayContainer()) {
+      if (foundChild || svgChild->IsDisplayContainer())
-        *aTrivial = PR_FALSE;
+        return PR_FALSE;
-        return NS_OK;
-      }
       foundChild = PR_TRUE;
     }
   }
-
+  return PR_TRUE;
-  return NS_OK;
 }
 
 nsIAtom *

layout/svg/base/src/nsSVGClipPathFrame.h

@@ -48,22 +48,22 @@ class nsSVGClipPathFrame : public nsSVGClipPathFrameBase
 
   NS_IMETHOD InitSVG();
 
- public:
   nsSVGClipPathFrame(nsStyleContext* aContext) : nsSVGClipPathFrameBase(aContext) {}
 
+ public:
   // nsSVGClipPathFrame methods:
-  NS_IMETHOD ClipPaint(nsSVGRenderState* aContext,
+  nsresult ClipPaint(nsSVGRenderState* aContext,
-                       nsISVGChildFrame* aParent,
+                     nsISVGChildFrame* aParent,
-                       nsCOMPtr<nsIDOMSVGMatrix> aMatrix);
+                     nsIDOMSVGMatrix *aMatrix);
 
-  NS_IMETHOD ClipHitTest(nsISVGChildFrame* aParent,
+  PRBool ClipHitTest(nsISVGChildFrame* aParent,
-                         nsCOMPtr<nsIDOMSVGMatrix> aMatrix,
+                     nsIDOMSVGMatrix *aMatrix,
-                         float aX, float aY, PRBool *aHit);
+                     float aX, float aY);
 
   // Check if this clipPath is made up of more than one geometry object.
   // If so, the clipping API in cairo isn't enough and we need to use
   // mask based clipping.
-  NS_IMETHOD IsTrivial(PRBool *aTrivial);
+  PRBool IsTrivial();
 
   /**
    * Get the "type" of the frame
@@ -80,6 +80,24 @@ class nsSVGClipPathFrame : public nsSVGClipPathFrameBase
 #endif
 
  private:
+  // A helper class to allow us to paint clip paths safely. The helper
+  // automatically sets and clears the mInUse flag on the clip path frame
+  // (to prevent nasty reference loops). It's easy to mess this up
+  // and break things, so this helper makes the code far more robust.
+  class AutoClipPathReferencer
+  {
+  public:
+    AutoClipPathReferencer(nsSVGClipPathFrame *aFrame) {
+      NS_ASSERTION(mFrame->mInUse == PR_FALSE, "reference loop!");
+      mFrame->mInUse = PR_TRUE;
+    }
+    ~AutoClipPathReferencer() {
+      mFrame->mInUse = PR_FALSE;
+    }
+  private:
+    nsSVGClipPathFrame *mFrame;
+  };
+
   nsISVGChildFrame *mClipParent;
   nsCOMPtr<nsIDOMSVGMatrix> mClipParentMatrix;
 

layout/svg/base/src/nsSVGGFrame.cpp

@@ -40,7 +40,6 @@
 #include "nsSVGGFrame.h"
 #include "nsIFrame.h"
 #include "nsSVGMatrix.h"
-#include "nsSVGClipPathFrame.h"
 #include "nsGkAtoms.h"
 #include "nsSVGUtils.h"
 #include "nsISVGValueUtils.h"

layout/svg/base/src/nsSVGUtils.cpp

@@ -811,13 +811,9 @@ AddEffectProperties(nsIFrame *aFrame)
                                                       aFrame->GetContent());
     if (clip) {
       aFrame->SetProperty(nsGkAtoms::clipPath, clip);
-
+      aFrame->AddStateBits(clip->IsTrivial() ?
-      PRBool trivialClip;
+                             NS_STATE_SVG_CLIPPED_TRIVIAL :
-      clip->IsTrivial(&trivialClip);
+                             NS_STATE_SVG_CLIPPED_COMPLEX);
-      if (trivialClip)
-        aFrame->AddStateBits(NS_STATE_SVG_CLIPPED_TRIVIAL);
-      else
-        aFrame->AddStateBits(NS_STATE_SVG_CLIPPED_COMPLEX);
     }
   }
 
@@ -1038,8 +1034,6 @@ nsSVGUtils::StyleEffects(nsIFrame *aFrame)
 PRBool
 nsSVGUtils::HitTestClip(nsIFrame *aFrame, float x, float y)
 {
-  PRBool clipHit = PR_TRUE;
-
   nsISVGChildFrame* SVGFrame;
   CallQueryInterface(aFrame, &SVGFrame);
 
@@ -1048,10 +1042,10 @@ nsSVGUtils::HitTestClip(nsIFrame *aFrame, float x, float y)
     clip = NS_STATIC_CAST(nsSVGClipPathFrame *,
                           aFrame->GetProperty(nsGkAtoms::clipPath));
     nsCOMPtr<nsIDOMSVGMatrix> matrix = GetCanvasTM(aFrame);
-    clip->ClipHitTest(SVGFrame, matrix, x, y, &clipHit);
+    return clip->ClipHitTest(SVGFrame, matrix, x, y);
   }
 
-  return clipHit;
+  return PR_TRUE;
 }
 
 void