Bug 618265: Fix leaking gfxSharedImageSurfaces. r=joe sr=vlad a=b
authorChris Jones <jones.chris.g@gmail.com>
Tue, 04 Jan 2011 10:40:54 -0600
changeset 59847 1d1dfec6de0e519fd3be1df84856a62a14bdd68e
parent 59846 d4084c318c9ee870bf6eb5b08f3a9f635034c404
child 59848 d72a6aaf66a4bff4150ce34a0f6e1c390e6cddf6
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewersjoe, vlad, b
bugs618265
milestone2.0b9pre
Bug 618265: Fix leaking gfxSharedImageSurfaces. r=joe sr=vlad a=b
dom/plugins/PluginInstanceChild.cpp
dom/plugins/PluginInstanceParent.cpp
gfx/layers/ipc/ShadowLayers.cpp
gfx/layers/ipc/ShadowLayersParent.cpp
gfx/thebes/gfxASurface.h
gfx/thebes/gfxImageSurface.cpp
gfx/thebes/gfxImageSurface.h
gfx/thebes/gfxSharedImageSurface.cpp
gfx/thebes/gfxSharedImageSurface.h
--- a/dom/plugins/PluginInstanceChild.cpp
+++ b/dom/plugins/PluginInstanceChild.cpp
@@ -2287,19 +2287,19 @@ PluginInstanceChild::CreateOptSurface(vo
         mCurrentSurface = s;
         return true;
     }
 
     NS_RUNTIMEABORT("Shared-memory drawing not expected on Windows.");
 #endif
 
     // Make common shmem implementation working for any platform
-    mCurrentSurface = new gfxSharedImageSurface();
-    return static_cast<gfxSharedImageSurface*>(mCurrentSurface.get())->
-        InitUnsafe(this, gfxIntSize(mWindow.width, mWindow.height), format);
+    mCurrentSurface =
+        gfxSharedImageSurface::CreateUnsafe(this, gfxIntSize(mWindow.width, mWindow.height), format);
+    return !!mCurrentSurface;
 }
 
 bool
 PluginInstanceChild::MaybeCreatePlatformHelperSurface(void)
 {
     if (!mCurrentSurface) {
         NS_ERROR("Cannot create helper surface without mCurrentSurface");
         return false;
--- a/dom/plugins/PluginInstanceParent.cpp
+++ b/dom/plugins/PluginInstanceParent.cpp
@@ -492,17 +492,17 @@ PluginInstanceParent::RecvShow(const NPR
                                SurfaceDescriptor* prevSurface)
 {
     nsRefPtr<gfxASurface> surface;
     if (newSurface.type() == SurfaceDescriptor::TShmem) {
         if (!newSurface.get_Shmem().IsReadable()) {
             NS_WARNING("back surface not readable");
             return false;
         }
-        surface = new gfxSharedImageSurface(newSurface.get_Shmem());
+        surface = gfxSharedImageSurface::Open(newSurface.get_Shmem());
     }
 #ifdef MOZ_X11
     else if (newSurface.type() == SurfaceDescriptor::TSurfaceDescriptorX11) {
         SurfaceDescriptorX11 xdesc = newSurface.get_SurfaceDescriptorX11();
         XRenderPictFormat pf;
         pf.id = xdesc.xrenderPictID();
         XRenderPictFormat *incFormat =
             XRenderFindFormat(DefaultXDisplay(), PictFormatID, &pf, 0);
--- a/gfx/layers/ipc/ShadowLayers.cpp
+++ b/gfx/layers/ipc/ShadowLayers.cpp
@@ -446,18 +446,19 @@ ShadowLayerForwarder::AllocBuffer(const 
                                   gfxASurface::gfxContentType aContent,
                                   gfxSharedImageSurface** aBuffer)
 {
   NS_ABORT_IF_FALSE(HasShadowManager(), "no manager to forward to");
 
   gfxASurface::gfxImageFormat format = OptimalFormatFor(aContent);
   SharedMemory::SharedMemoryType shmemType = OptimalShmemType();
 
-  nsRefPtr<gfxSharedImageSurface> back = new gfxSharedImageSurface();
-  if (!back->InitUnsafe(mShadowManager, aSize, format, shmemType))
+  nsRefPtr<gfxSharedImageSurface> back =
+    gfxSharedImageSurface::CreateUnsafe(mShadowManager, aSize, format, shmemType);
+  if (!back)
     return PR_FALSE;
 
   *aBuffer = nsnull;
   back.swap(*aBuffer);
   return PR_TRUE;
 }
 
 PRBool
@@ -515,17 +516,17 @@ ShadowLayerForwarder::OpenDescriptor(con
 {
   nsRefPtr<gfxASurface> surf = PlatformOpenDescriptor(aSurface);
   if (surf) {
     return surf.forget();
   }
 
   switch (aSurface.type()) {
   case SurfaceDescriptor::TShmem: {
-    surf = new gfxSharedImageSurface(aSurface.get_Shmem());
+    surf = gfxSharedImageSurface::Open(aSurface.get_Shmem());
     return surf.forget();
   }
   default:
     NS_RUNTIMEABORT("unexpected SurfaceDescriptor type!");
     return nsnull;
   }
 }
 
--- a/gfx/layers/ipc/ShadowLayersParent.cpp
+++ b/gfx/layers/ipc/ShadowLayersParent.cpp
@@ -202,33 +202,35 @@ ShadowLayersParent::RecvUpdate(const Inf
     }
     case Edit::TOpCreateCanvasBuffer: {
       MOZ_LAYERS_LOG(("[ParentSide] CreateCanvasBuffer"));
 
       const OpCreateCanvasBuffer& ocb = edit.get_OpCreateCanvasBuffer();
       ShadowCanvasLayer* canvas = static_cast<ShadowCanvasLayer*>(
         AsShadowLayer(ocb)->AsLayer());
       nsRefPtr<gfxSharedImageSurface> front =
-        new gfxSharedImageSurface(ocb.initialFront());
+        gfxSharedImageSurface::Open(ocb.initialFront());
       CanvasLayer::Data data;
       data.mSurface = front;
       data.mSize = ocb.size();
 
       canvas->Initialize(data);
 
       break;
     }
     case Edit::TOpCreateImageBuffer: {
       MOZ_LAYERS_LOG(("[ParentSide] CreateImageBuffer"));
 
       const OpCreateImageBuffer ocb = edit.get_OpCreateImageBuffer();
       ShadowImageLayer* image = static_cast<ShadowImageLayer*>(
         AsShadowLayer(ocb)->AsLayer());
 
-      image->Init(new gfxSharedImageSurface(ocb.initialFront()), ocb.size());
+      nsRefPtr<gfxSharedImageSurface> surf =
+        gfxSharedImageSurface::Open(ocb.initialFront());
+      image->Init(surf, ocb.size());
 
       break;
     }
     case Edit::TOpDestroyThebesFrontBuffer: {
       MOZ_LAYERS_LOG(("[ParentSide] DestroyThebesFrontBuffer"));
 
       const OpDestroyThebesFrontBuffer& odfb =
         edit.get_OpDestroyThebesFrontBuffer();
@@ -391,35 +393,37 @@ ShadowLayersParent::RecvUpdate(const Inf
     case Edit::TOpPaintCanvas: {
       MOZ_LAYERS_LOG(("[ParentSide] Paint CanvasLayer"));
 
       const OpPaintCanvas& op = edit.get_OpPaintCanvas();
       ShadowLayerParent* shadow = AsShadowLayer(op);
       ShadowCanvasLayer* canvas =
         static_cast<ShadowCanvasLayer*>(shadow->AsLayer());
 
-      nsRefPtr<gfxSharedImageSurface> newBack =
-        canvas->Swap(new gfxSharedImageSurface(op.newFrontBuffer()));
+      nsRefPtr<gfxSharedImageSurface> newFront =
+        gfxSharedImageSurface::Open(op.newFrontBuffer());
+      nsRefPtr<gfxSharedImageSurface> newBack = canvas->Swap(newFront);
       canvas->Updated(op.updated());
 
       replyv.push_back(OpBufferSwap(shadow, NULL,
                                     newBack->GetShmem()));
 
       break;
     }
     case Edit::TOpPaintImage: {
       MOZ_LAYERS_LOG(("[ParentSide] Paint ImageLayer"));
 
       const OpPaintImage& op = edit.get_OpPaintImage();
       ShadowLayerParent* shadow = AsShadowLayer(op);
       ShadowImageLayer* image =
         static_cast<ShadowImageLayer*>(shadow->AsLayer());
 
-      nsRefPtr<gfxSharedImageSurface> newBack =
-        image->Swap(new gfxSharedImageSurface(op.newFrontBuffer()));
+      nsRefPtr<gfxSharedImageSurface> newFront =
+        gfxSharedImageSurface::Open(op.newFrontBuffer());
+      nsRefPtr<gfxSharedImageSurface> newBack = image->Swap(newFront);
 
       replyv.push_back(OpBufferSwap(shadow, NULL,
                                     newBack->GetShmem()));
 
       break;
     }
 
     default:
--- a/gfx/thebes/gfxASurface.h
+++ b/gfx/thebes/gfxASurface.h
@@ -233,16 +233,19 @@ protected:
                     mSurfaceValid(PR_FALSE), mAllowUseAsSource(PR_TRUE)
     {
         MOZ_COUNT_CTOR(gfxASurface);
     }
 
     static gfxASurface* GetSurfaceWrapper(cairo_surface_t *csurf);
     static void SetSurfaceWrapper(cairo_surface_t *csurf, gfxASurface *asurf);
 
+    // NB: Init() *must* be called from within subclass's
+    // constructors.  It's unsafe to call it after the ctor finishes;
+    // leaks and use-after-frees are possible.
     void Init(cairo_surface_t *surface, PRBool existingSurface = PR_FALSE);
 
     virtual ~gfxASurface()
     {
         RecordMemoryFreed();
 
         MOZ_COUNT_DTOR(gfxASurface);
     }
--- a/gfx/thebes/gfxImageSurface.cpp
+++ b/gfx/thebes/gfxImageSurface.cpp
@@ -144,34 +144,34 @@ gfxImageSurface::gfxImageSurface(cairo_s
 }
 
 gfxImageSurface::~gfxImageSurface()
 {
     if (mOwnsData)
         free(mData);
 }
 
-long
-gfxImageSurface::ComputeStride() const
+/*static*/ long
+gfxImageSurface::ComputeStride(const gfxIntSize& aSize, gfxImageFormat aFormat)
 {
     long stride;
 
-    if (mFormat == ImageFormatARGB32)
-        stride = mSize.width * 4;
-    else if (mFormat == ImageFormatRGB24)
-        stride = mSize.width * 4;
-    else if (mFormat == ImageFormatRGB16_565)
-        stride = mSize.width * 2;
-    else if (mFormat == ImageFormatA8)
-        stride = mSize.width;
-    else if (mFormat == ImageFormatA1) {
-        stride = (mSize.width + 7) / 8;
+    if (aFormat == ImageFormatARGB32)
+        stride = aSize.width * 4;
+    else if (aFormat == ImageFormatRGB24)
+        stride = aSize.width * 4;
+    else if (aFormat == ImageFormatRGB16_565)
+        stride = aSize.width * 2;
+    else if (aFormat == ImageFormatA8)
+        stride = aSize.width;
+    else if (aFormat == ImageFormatA1) {
+        stride = (aSize.width + 7) / 8;
     } else {
         NS_WARNING("Unknown format specified to gfxImageSurface!");
-        stride = mSize.width * 4;
+        stride = aSize.width * 4;
     }
 
     stride = ((stride + 3) / 4) * 4;
 
     return stride;
 }
 
 PRBool
--- a/gfx/thebes/gfxImageSurface.h
+++ b/gfx/thebes/gfxImageSurface.h
@@ -110,17 +110,19 @@ public:
 
     virtual PRBool SupportsSelfCopy() { return PR_FALSE; }
 
 protected:
     gfxImageSurface();
     void InitWithData(unsigned char *aData, const gfxIntSize& aSize,
                       long aStride, gfxImageFormat aFormat);
     void InitFromSurface(cairo_surface_t *csurf);
-    long ComputeStride() const;
+    long ComputeStride() const { return ComputeStride(mSize, mFormat); }
+
+    static long ComputeStride(const gfxIntSize&, gfxImageFormat);
 
     gfxIntSize mSize;
     PRBool mOwnsData;
     unsigned char *mData;
     gfxImageFormat mFormat;
     long mStride;
 };
 
--- a/gfx/thebes/gfxSharedImageSurface.cpp
+++ b/gfx/thebes/gfxSharedImageSurface.cpp
@@ -37,91 +37,87 @@
  * ***** END LICENSE BLOCK ***** */
 
 #include "base/basictypes.h"
 #include "gfxSharedImageSurface.h"
 #include "cairo.h"
 
 #define MOZ_ALIGN_WORD(x) (((x) + 3) & ~3)
 
-using mozilla::ipc::SharedMemory;
+using namespace mozilla::ipc;
 
 static const cairo_user_data_key_t SHM_KEY = {0};
 
-typedef struct _SharedImageInfo
-{
+struct SharedImageInfo {
     PRInt32 width;
     PRInt32 height;
     PRInt32 format;
-} SharedImageInfo;
+};
 
 static SharedImageInfo*
-GetShmInfoPtr(const mozilla::ipc::Shmem &aShmem)
+GetShmInfoPtr(const Shmem& aShmem)
 {
     return reinterpret_cast<SharedImageInfo*>
         (aShmem.get<char>() + aShmem.Size<char>() - sizeof(SharedImageInfo));
 }
 
-size_t
-gfxSharedImageSurface::GetAlignedSize()
+gfxSharedImageSurface::~gfxSharedImageSurface()
 {
-   return MOZ_ALIGN_WORD(sizeof(SharedImageInfo) + mSize.height * mStride);
 }
 
-bool
-gfxSharedImageSurface::InitSurface(PRBool aUpdateShmemInfo)
+/*static*/ PRBool
+gfxSharedImageSurface::IsSharedImage(gfxASurface* aSurface)
 {
-    if (!CheckSurfaceSize(mSize))
-        return false;
+    return (aSurface
+            && aSurface->GetType() == gfxASurface::SurfaceTypeImage
+            && aSurface->GetData(&SHM_KEY));
+}
 
+gfxSharedImageSurface::gfxSharedImageSurface(const gfxIntSize& aSize,
+                                             gfxImageFormat aFormat,
+                                             const Shmem& aShmem)
+{
+    mSize = aSize;
+    mFormat = aFormat;
+    mStride = ComputeStride(aSize, aFormat);
+    mShmem = aShmem;
     cairo_surface_t *surface =
         cairo_image_surface_create_for_data(mShmem.get<unsigned char>(),
                                             (cairo_format_t)mFormat,
                                             mSize.width,
                                             mSize.height,
                                             mStride);
-
-    if (!surface)
-        return false;
-
-    cairo_surface_set_user_data(surface,
-                                &SHM_KEY,
-                                this, NULL);
-
-    if (aUpdateShmemInfo) {
-        SharedImageInfo *shmInfo = GetShmInfoPtr(mShmem);
-        shmInfo->width = mSize.width;
-        shmInfo->height = mSize.height;
-        shmInfo->format = mFormat;
+    if (surface) {
+        cairo_surface_set_user_data(surface, &SHM_KEY, this, NULL);
     }
-
-    InitFromSurface(surface);
-    return true;
+    Init(surface);
 }
 
-gfxSharedImageSurface::~gfxSharedImageSurface()
+void
+gfxSharedImageSurface::WriteShmemInfo()
 {
+    SharedImageInfo* shmInfo = GetShmInfoPtr(mShmem);
+    shmInfo->width = mSize.width;
+    shmInfo->height = mSize.height;
+    shmInfo->format = mFormat;
 }
 
-gfxSharedImageSurface::gfxSharedImageSurface()
+/*static*/ size_t
+gfxSharedImageSurface::GetAlignedSize(const gfxIntSize& aSize, long aStride)
 {
+   return MOZ_ALIGN_WORD(sizeof(SharedImageInfo) + aSize.height * aStride);
 }
 
-gfxSharedImageSurface::gfxSharedImageSurface(const mozilla::ipc::Shmem &aShmem)
+/*static*/ already_AddRefed<gfxSharedImageSurface>
+gfxSharedImageSurface::Open(const Shmem& aShmem)
 {
-    mShmem = aShmem;
-    SharedImageInfo *shmInfo = GetShmInfoPtr(aShmem);
-    mSize.width = shmInfo->width;
-    mSize.height = shmInfo->height;
-    mFormat = (gfxImageFormat)shmInfo->format;
-    mStride = ComputeStride();
+    SharedImageInfo* shmInfo = GetShmInfoPtr(aShmem);
+    gfxIntSize size(shmInfo->width, shmInfo->height);
+    if (!CheckSurfaceSize(size))
+        return nsnull;
 
-    if (!InitSurface(PR_FALSE))
-        NS_RUNTIMEABORT("Shared memory is bad");
+    nsRefPtr<gfxSharedImageSurface> s =
+        new gfxSharedImageSurface(size,
+                                  (gfxImageFormat)shmInfo->format,
+                                  aShmem);
+    // We didn't create this Shmem and so don't free it on errors
+    return (s->CairoStatus() != 0) ? nsnull : s.forget();
 }
-
-PRBool
-gfxSharedImageSurface::IsSharedImage(gfxASurface *aSurface)
-{
-    return (aSurface
-            && aSurface->GetType() == gfxASurface::SurfaceTypeImage
-            && aSurface->GetData(&SHM_KEY));
-}
--- a/gfx/thebes/gfxSharedImageSurface.h
+++ b/gfx/thebes/gfxSharedImageSurface.h
@@ -45,82 +45,98 @@
 #include "gfxASurface.h"
 #include "gfxImageSurface.h"
 
 class THEBES_API gfxSharedImageSurface : public gfxImageSurface {
     typedef mozilla::ipc::SharedMemory SharedMemory;
     typedef mozilla::ipc::Shmem Shmem;
 
 public:
-    /**
-     * Init must be called after ctor
-     */
-    gfxSharedImageSurface();
+    virtual ~gfxSharedImageSurface();
 
     /**
-     * Create shared image from external Shmem
-     * Shmem must be initialized by this class
-     */
-    gfxSharedImageSurface(const Shmem &aShmem);
-
-    ~gfxSharedImageSurface();
-
-    /**
-     * Initialize shared image surface
-     * @param aAllocator The pointer to protocol class which has AllocShmem method
-     * @param aSize The size of the buffer
-     * @param aFormat Format of the data
-     * @see gfxImageFormat
+     * Return a new gfxSharedImageSurface around a shmem segment newly
+     * allocated by this function.  |aAllocator| is the object used to
+     * allocate the new shmem segment.  Null is returned if creating
+     * the surface failed.
+     *
+     * NB: the *caller* is responsible for freeing the Shmem allocated
+     * by this function.
      */
     template<class ShmemAllocator>
-    bool Init(ShmemAllocator *aAllocator,
-              const gfxIntSize& aSize,
-              gfxImageFormat aFormat,
-              SharedMemory::SharedMemoryType aShmType = SharedMemory::TYPE_BASIC)
+    static already_AddRefed<gfxSharedImageSurface>
+    Create(ShmemAllocator* aAllocator,
+           const gfxIntSize& aSize,
+           gfxImageFormat aFormat,
+           SharedMemory::SharedMemoryType aShmType = SharedMemory::TYPE_BASIC)
     {
-        return Init<ShmemAllocator, false>(aAllocator, aSize, aFormat, aShmType);
+        return Create<ShmemAllocator, false>(aAllocator, aSize, aFormat, aShmType);
     }
 
+    /**
+     * Return a new gfxSharedImageSurface that wraps a shmem segment
+     * already created by the Create() above.  Bad things will happen
+     * if an attempt is made to wrap any other shmem segment.  Null is
+     * returned if creating the surface failed.
+     */
+    static already_AddRefed<gfxSharedImageSurface>
+    Open(const Shmem& aShmem);
+
     template<class ShmemAllocator>
-    bool InitUnsafe(ShmemAllocator *aAllocator,
-                    const gfxIntSize& aSize,
-                    gfxImageFormat aFormat,
-                    SharedMemory::SharedMemoryType aShmType = SharedMemory::TYPE_BASIC)
+    static already_AddRefed<gfxSharedImageSurface>
+    CreateUnsafe(ShmemAllocator* aAllocator,
+                 const gfxIntSize& aSize,
+                 gfxImageFormat aFormat,
+                 SharedMemory::SharedMemoryType aShmType = SharedMemory::TYPE_BASIC)
     {
-        return Init<ShmemAllocator, true>(aAllocator, aSize, aFormat, aShmType);
+        return Create<ShmemAllocator, true>(aAllocator, aSize, aFormat, aShmType);
     }
 
-    /* Gives Shmem data, which can be passed to IPDL interfaces */
     Shmem& GetShmem() { return mShmem; }
 
-    // This can be used for recognizing normal gfxImageSurface as SharedImage
     static PRBool IsSharedImage(gfxASurface *aSurface);
 
 private:
+    gfxSharedImageSurface(const gfxIntSize&, gfxImageFormat, const Shmem&);
+
+    void WriteShmemInfo();
+
+    static size_t GetAlignedSize(const gfxIntSize&, long aStride);
+
     template<class ShmemAllocator, bool Unsafe>
-    bool Init(ShmemAllocator *aAllocator,
-              const gfxIntSize& aSize,
-              gfxImageFormat aFormat,
-              SharedMemory::SharedMemoryType aShmType)
+    static already_AddRefed<gfxSharedImageSurface>
+    Create(ShmemAllocator* aAllocator,
+           const gfxIntSize& aSize,
+           gfxImageFormat aFormat,
+           SharedMemory::SharedMemoryType aShmType)
     {
-        mSize = aSize;
-        mFormat = aFormat;
-        mStride = ComputeStride();
+        if (!CheckSurfaceSize(aSize))
+            return nsnull;
+
+        Shmem shmem;
+        long stride = ComputeStride(aSize, aFormat);
+        size_t size = GetAlignedSize(aSize, stride);
         if (!Unsafe) {
-            if (!aAllocator->AllocShmem(GetAlignedSize(),
-                                        aShmType, &mShmem))
-                return false;
+            if (!aAllocator->AllocShmem(size, aShmType, &shmem))
+                return nsnull;
         } else {
-            if (!aAllocator->AllocUnsafeShmem(GetAlignedSize(),
-                                              aShmType, &mShmem))
-                return false;
+            if (!aAllocator->AllocUnsafeShmem(size, aShmType, &shmem))
+                return nsnull;
         }
 
-        return InitSurface(PR_TRUE);
+        nsRefPtr<gfxSharedImageSurface> s =
+            new gfxSharedImageSurface(aSize, aFormat, shmem);
+        if (s->CairoStatus() != 0) {
+            aAllocator->DeallocShmem(shmem);
+            return nsnull;
+        }
+        s->WriteShmemInfo();
+        return s.forget();
     }
 
-    size_t GetAlignedSize();
-    bool InitSurface(PRBool aUpdateShmemInfo);
+    Shmem mShmem;
 
-    Shmem mShmem;
+    // Calling these is very bad, disallow it
+    gfxSharedImageSurface(const gfxSharedImageSurface&);
+    gfxSharedImageSurface& operator=(const gfxSharedImageSurface&);
 };
 
 #endif /* GFX_SHARED_IMAGESURFACE_H */