Fix memory leak problem in PdfPsMetafile....

printing/pdf_ps_metafile_linux.cc

Index: printing/pdf_ps_metafile_linux.cc
===================================================================
--- printing/pdf_ps_metafile_linux.cc	(revision 24519)
+++ printing/pdf_ps_metafile_linux.cc	(working copy)
@@ -6,19 +6,78 @@
 
 #include <stdio.h>
 
+#include <cairo.h>
 #include <cairo-pdf.h>
 #include <cairo-ps.h>
 
 #include "base/file_util.h"
 #include "base/logging.h"
 
+namespace {
+
+// Tests if |surface| is valid.
+bool IsSurfaceValid(cairo_surface_t* surface) {
+  return cairo_surface_status(surface) == CAIRO_STATUS_SUCCESS;
+}
+
+// Tests if |context| is valid.
+bool IsContextValid(cairo_t* context) {
+  return cairo_status(context) == CAIRO_STATUS_SUCCESS;
+}
+
+// Destroys and resets |surface|.
+void CleanUpSurface(cairo_surface_t** surface) {
+  if (*surface) {
+    cairo_surface_destroy(*surface);
+    *surface = NULL;
+  }
+}
+
+// Destroys and resets |context|.
+void CleanUpContext(cairo_t** context) {
+  if (*context) {
+    cairo_destroy(*context);
+    *context = NULL;
+  }
+}
+
+// Callback function for Cairo to write PDF/PS stream.
+// |dst_buffer| is actually a pointer of type `std::string*`.
+cairo_status_t WriteCairoStream(void* dst_buffer,
+                                const unsigned char* src_data,
+                                unsigned int src_data_length) {
+  DCHECK(dst_buffer);
+  DCHECK(src_data);
+  DCHECK(src_data_length > 0);
+
+  std::string* buffer = reinterpret_cast<std::string*>(dst_buffer);
+  buffer->append(reinterpret_cast<const char*>(src_data), src_data_length);
+
+  return CAIRO_STATUS_SUCCESS;
+}
+
+}  // namespace
+
 namespace printing {
 
 PdfPsMetafile::PdfPsMetafile(const FileFormat& format)
     : format_(format),
       surface_(NULL), context_(NULL),
       page_surface_(NULL), page_context_(NULL) {
+}
 
+PdfPsMetafile::~PdfPsMetafile() {
+  // Releases resources if we forgot to do so.
+  CleanUp();
+}
+
+bool PdfPsMetafile::Init() {
+  // We need to check at least these two members to ensure Init() has not been
+  // called before. Passing these two checks also implies that surface_,
+  // page_surface_, and page_context_ are NULL, and current_page_ is empty.
+  DCHECK(!context_);
+  DCHECK(all_pages_.empty());
+
   // Create an 1 by 1 Cairo surface for entire PDF/PS file.
   // The size for each page will be overwritten later in StartPage().
   switch (format_) {
@@ -36,51 +95,53 @@
 
     default:
       NOTREACHED();
-      return;
+      return false;
   }
 
   // Cairo always returns a valid pointer.
   // Hence, we have to check if it points to a "nil" object.
   if (!IsSurfaceValid(surface_)) {
     DLOG(ERROR) << "Cannot create Cairo surface for PdfPsMetafile!";
-    cairo_surface_destroy(surface_);
-    return;
+    CleanUpSurface(&surface_);
+    return false;
   }
 
   // Create a context.
   context_ = cairo_create(surface_);
   if (!IsContextValid(context_)) {
     DLOG(ERROR) << "Cannot create Cairo context for PdfPsMetafile!";
-    cairo_destroy(context_);
-    cairo_surface_destroy(surface_);
-    return;
+    CleanUpContext(&context_);
+    CleanUpSurface(&surface_);
+    return false;
   }
 
-  // Since |context_| will keep a reference of |surface_|, we can decreace
-  // surface's reference count by one safely.
-  cairo_surface_destroy(surface_);
+  return true;
 }
 
-PdfPsMetafile::PdfPsMetafile(const FileFormat& format,
-                             const void* src_buffer,
-                             size_t src_buffer_size)
-    : format_(format),
-      surface_(NULL), context_(NULL),
-      page_surface_(NULL), page_context_(NULL) {
+bool PdfPsMetafile::Init(const void* src_buffer, size_t src_buffer_size) {
+  // We need to check at least these two members to ensure Init() has not been
+  // called before. Passing these two checks also implies that surface_,
+  // page_surface_, and page_context_ are NULL, and current_page_ is empty.
+  DCHECK(!context_);
+  DCHECK(all_pages_.empty());
 
+  if (src_buffer == NULL || src_buffer_size == 0) {
+    return false;
+  }
+
   all_pages_ = std::string(reinterpret_cast<const char*>(src_buffer),
                            src_buffer_size);
-}
 
-PdfPsMetafile::~PdfPsMetafile() {
-  // Releases resources if we forgot to do so.
-  Close();
+  return true;
 }
 
-bool PdfPsMetafile::StartPage(double width_in_points, double height_in_points) {
+cairo_t* PdfPsMetafile::StartPage(double width_in_points,
+                                  double height_in_points) {
   DCHECK(IsSurfaceValid(surface_));
   DCHECK(IsContextValid(context_));
-  DCHECK(!page_surface_ && !page_context_);
+  // Passing this check implies page_surface_ is NULL, and current_page_ is
+  // empty.
+  DCHECK(!page_context_);
   DCHECK_GT(width_in_points, 0.);
   DCHECK_GT(height_in_points, 0.);
 
@@ -106,38 +167,35 @@
 
     default:
       NOTREACHED();
-      return false;
+      CleanUp();
+      return NULL;
   }
 
   // Cairo always returns a valid pointer.
   // Hence, we have to check if it points to a "nil" object.
   if (!IsSurfaceValid(page_surface_)) {
     DLOG(ERROR) << "Cannot create Cairo surface for PdfPsMetafile!";
-    cairo_surface_destroy(page_surface_);
-    return false;
+    CleanUp();
+    return NULL;
   }
 
   // Create a context.
   page_context_ = cairo_create(page_surface_);
   if (!IsContextValid(page_context_)) {
     DLOG(ERROR) << "Cannot create Cairo context for PdfPsMetafile!";
-    cairo_destroy(page_context_);
-    cairo_surface_destroy(page_surface_);
-    return false;
+    CleanUp();
+    return NULL;
   }
 
-  // Since |page_context_| will keep a reference of |page_surface_|, we can
-  // decreace surface's reference count by one safely.
-  cairo_surface_destroy(page_surface_);
-
-  return true;
+  return page_context_;
 }
 
-void PdfPsMetafile::FinishPage(float shrink) {
+bool PdfPsMetafile::FinishPage(float shrink) {
   DCHECK(IsSurfaceValid(surface_));
   DCHECK(IsContextValid(context_));
   DCHECK(IsSurfaceValid(page_surface_));
   DCHECK(IsContextValid(page_context_));
+  DCHECK(shrink > 0);
 
   // Flush all rendering for current page.
   cairo_surface_flush(page_surface_);
@@ -168,9 +226,17 @@
 
     default:
       NOTREACHED();
-      return;
+      CleanUp();
+      return false;
   }
 
+  // Check if our surface is still valid after resizing.
+  if (!IsSurfaceValid(surface_)) {
+    DLOG(ERROR) << "Cannot resize Cairo surface for PdfPsMetafile!";
+    CleanUp();
+    return false;
+  }
+
   // Save context's states.
   cairo_save(context_);
   // Copy current page onto the surface of final result.
@@ -214,44 +280,68 @@
   cairo_surface_flush(surface_);
 
   // Destroy resoreces for current page.
-  cairo_destroy(page_context_);
-  page_context_ = NULL;
-  page_surface_ = NULL;
+  CleanUpContext(&page_context_);
+  CleanUpSurface(&page_surface_);

[myhuang, 2009/08/26 20:50:36]

I forgot to do line#284 here in rev#24474

   current_page_.clear();
 
   // Restore context's states.
   cairo_restore(context_);
+
+  return true;
 }
 
 void PdfPsMetafile::Close() {
-  if (surface_ != NULL && IsSurfaceValid(surface_)) {
-    cairo_surface_finish(surface_);
-    surface_ = NULL;
-  }
-  if (context_ != NULL && IsContextValid(context_)) {
-    cairo_destroy(context_);
-    context_ = NULL;
-  }
+  DCHECK(IsSurfaceValid(surface_));
+  DCHECK(IsContextValid(context_));
+  // Passing this check implies page_surface_ is NULL, and current_page_ is
+  // empty.
+  DCHECK(!page_context_);
+
+  cairo_surface_finish(surface_);
+  DCHECK(!all_pages_.empty());  // Make sure we did get something.
+
+  CleanUpContext(&context_);
+  CleanUpSurface(&surface_);
 }
 
 unsigned int PdfPsMetafile::GetDataSize() const {
-  DCHECK(!surface_ && !context_);
+  // We need to check at least these two members to ensure that either Init()
+  // has been called to initialize |all_pages_|, or metafile has been closed.
+  // Passing these two checks also implies that surface_, page_surface_, and
+  // page_context_ are NULL, and current_page_ is empty.
+  DCHECK(!context_);
+  DCHECK(!all_pages_.empty());
 
   return all_pages_.size();
 }
 
-void PdfPsMetafile::GetData(void* dst_buffer, size_t dst_buffer_size) const {
-  DCHECK(!surface_ && !context_);
+bool PdfPsMetafile::GetData(void* dst_buffer, size_t dst_buffer_size) const {
   DCHECK(dst_buffer);
+  DCHECK(dst_buffer_size > 0);
+  // We need to check at least these two members to ensure that either Init()
+  // has been called to initialize |all_pages_|, or metafile has been closed.
+  // Passing these two checks also implies that surface_, page_surface_, and
+  // page_context_ are NULL, and current_page_ is empty.
+  DCHECK(!context_);
+  DCHECK(!all_pages_.empty());
 
   size_t data_size = GetDataSize();
-  if (data_size < dst_buffer_size)
-    dst_buffer_size = data_size;
+  if (dst_buffer_size > data_size) {
+    return false;
+  }
+
   memcpy(dst_buffer, all_pages_.data(), dst_buffer_size);
+
+  return true;
 }
 
 bool PdfPsMetafile::SaveTo(const FilePath& filename) const {
-  DCHECK(!surface_ && !context_);
+  // We need to check at least these two members to ensure that either Init()
+  // has been called to initialize |all_pages_|, or metafile has been closed.
+  // Passing these two checks also implies that surface_, page_surface_, and
+  // page_context_ are NULL, and current_page_ is empty.
+  DCHECK(!context_);
+  DCHECK(!all_pages_.empty());
 
   const unsigned int data_size = GetDataSize();
   const unsigned int bytes_written =
@@ -264,25 +354,14 @@
   return true;
 }
 
-cairo_status_t PdfPsMetafile::WriteCairoStream(void* dst_buffer,
-                                               const unsigned char* src_data,
-                                               unsigned int src_data_length) {
-  DCHECK(dst_buffer);
-  DCHECK(src_data);
-
-  std::string* buffer = reinterpret_cast<std::string* >(dst_buffer);
-  buffer->append(reinterpret_cast<const char*>(src_data), src_data_length);
-
-  return CAIRO_STATUS_SUCCESS;
+void PdfPsMetafile::CleanUp() {
+  CleanUpContext(&context_);
+  CleanUpSurface(&surface_);
+  CleanUpContext(&page_context_);
+  CleanUpSurface(&page_surface_);
+  current_page_.clear();
+  all_pages_.clear();
 }
 
-bool PdfPsMetafile::IsSurfaceValid(cairo_surface_t* surface) const {
-  return cairo_surface_status(surface) == CAIRO_STATUS_SUCCESS;
-}
-
-bool PdfPsMetafile::IsContextValid(cairo_t* context) const {
-  return cairo_status(context) == CAIRO_STATUS_SUCCESS;
-}
-
 }  // namespace printing