Revert "JPEGCodec{RobustSlow}::Decode: refactor to use unique_ptr."

This reverts commit 607ea4c4.

Reason for revert: breaks gfx_unittests on Linux Chromium OS ASan LSan bot. Findit analysis:
https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/29208


Original change's description:
> JPEGCodec{RobustSlow}::Decode: refactor to use unique_ptr.
> 
> This CL removes the class DecompressDestroyer in favour of
> a unique_ptr<> with specific Deleter. Less lines of code.
> 
> Functionality covered by gfx_unittest, also under asan bot.
> 
> Bug: 868400
> Change-Id: I7ac2d254d6f5771a81620b14db7e610ff3b7e4b9
> Reviewed-on: https://chromium-review.googlesource.com/1245467
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#594638}

TBR=dcheng@chromium.org,mcasas@chromium.org

Change-Id: Ia8291bed9f539217969c7978c7dbc120c4fead43
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 868400
Reviewed-on: https://chromium-review.googlesource.com/1249204Reviewed-by: Marina Ciocea <marinaciocea@chromium.org>
Commit-Queue: Marina Ciocea <marinaciocea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594679}

ui/gfx/codec/chromeos/jpeg_codec_robust_slow.cc

@@ -139,9 +139,26 @@ void RGBtoBGRA(const unsigned char* bgra, int pixel_width, unsigned char* rgb) {
 }
 #endif  // !defined(JCS_EXTENSIONS)
 
-// jpeg_decompress_struct Deleter.
-struct JpegDecompressStructDeleter {
-  void operator()(jpeg_decompress_struct* ptr) { jpeg_destroy_decompress(ptr); }
+// This class destroys the given jpeg_decompress object when it goes out of
+// scope. It simplifies the error handling in Decode (and even applies to the
+// success case).
+class IjgDecompressDestroyer {
+ public:
+  IjgDecompressDestroyer() : cinfo_(NULL) {}
+  ~IjgDecompressDestroyer() { DestroyManagedObject(); }
+  void SetManagedObject(jpeg_decompress_struct* ci) {
+    DestroyManagedObject();
+    cinfo_ = ci;
+  }
+  void DestroyManagedObject() {
+    if (cinfo_) {
+      jpeg_destroy_decompress(cinfo_);
+      cinfo_ = NULL;
+    }
+  }
+
+ private:
+  jpeg_decompress_struct* cinfo_;
 };
 
 }  // namespace
@@ -152,26 +169,28 @@ bool JPEGCodecRobustSlow::Decode(const unsigned char* input,
                                  std::vector<unsigned char>* output,
                                  int* w,
                                  int* h) {
-  std::unique_ptr<jpeg_decompress_struct, JpegDecompressStructDeleter> cinfo(
-      new jpeg_decompress_struct);
+  jpeg_decompress_struct cinfo;
+  IjgDecompressDestroyer destroyer;
+  destroyer.SetManagedObject(&cinfo);
   output->clear();
 
   // We set up the normal JPEG error routines, then override error_exit.
   // This must be done before the call to create_decompress.
   IjgCoderErrorMgr errmgr;
-  cinfo->err = jpeg_std_error(&errmgr.pub);
+  cinfo.err = jpeg_std_error(&errmgr.pub);
   errmgr.pub.error_exit = IjgErrorExit;
   // Establish the setjmp return context for IjgErrorExit to use.
   if (setjmp(errmgr.setjmp_buffer)) {
     // If we get here, the JPEG code has signaled an error.
-    // Release |cinfo| by hand to avoid use-after-free of |errmgr|.
-    cinfo.reset();
+    // See note in JPEGCodec::Encode() for why we need to destroy the cinfo
+    // manually here.
+    destroyer.DestroyManagedObject();
     return false;
   }
 
   // The destroyer will destroy() cinfo on exit.  We don't want to set the
   // destroyer's object until cinfo is initialized.
-  jpeg_create_decompress(cinfo.get());
+  jpeg_create_decompress(&cinfo);
 
   // set up the source manager
   jpeg_source_mgr srcmgr;
@@ -180,17 +199,17 @@ bool JPEGCodecRobustSlow::Decode(const unsigned char* input,
   srcmgr.skip_input_data = IjgSkipInputData;
   srcmgr.resync_to_restart = jpeg_resync_to_restart;  // use default routine
   srcmgr.term_source = IjgTermSource;
-  cinfo->src = &srcmgr;
+  cinfo.src = &srcmgr;
 
   IjgJpegDecoderState state(input, input_size);
-  cinfo->client_data = &state;
+  cinfo.client_data = &state;
 
   // fill the file metadata into our buffer
-  if (jpeg_read_header(cinfo.get(), true) != JPEG_HEADER_OK)
+  if (jpeg_read_header(&cinfo, true) != JPEG_HEADER_OK)
     return false;
 
   // we want to always get RGB data out
-  switch (cinfo->jpeg_color_space) {
+  switch (cinfo.jpeg_color_space) {
     case JCS_GRAYSCALE:
     case JCS_RGB:
     case JCS_YCbCr:
@@ -200,22 +219,24 @@ bool JPEGCodecRobustSlow::Decode(const unsigned char* input,
       // used by Chromium (i.e. RGB, RGBA, and BGRA) and we just map the input
       // parameters to a colorspace.
       if (format == FORMAT_RGB) {
-        cinfo->out_color_space = JCS_RGB;
-        cinfo->output_components = 3;
+        cinfo.out_color_space = JCS_RGB;
+        cinfo.output_components = 3;
       } else if (format == FORMAT_RGBA ||
                  (format == FORMAT_SkBitmap && SK_R32_SHIFT == 0)) {
-        cinfo->out_color_space = JCS_EXT_RGBX;
-        cinfo->output_components = 4;
+        cinfo.out_color_space = JCS_EXT_RGBX;
+        cinfo.output_components = 4;
       } else if (format == FORMAT_BGRA ||
                  (format == FORMAT_SkBitmap && SK_B32_SHIFT == 0)) {
-        cinfo->out_color_space = JCS_EXT_BGRX;
-        cinfo->output_components = 4;
+        cinfo.out_color_space = JCS_EXT_BGRX;
+        cinfo.output_components = 4;
       } else {
+        // We can exit this function without calling jpeg_destroy_decompress()
+        // because IjgDecompressDestroyer automaticaly calls it.
         NOTREACHED() << "Invalid pixel format";
         return false;
       }
 #else
-      cinfo->out_color_space = JCS_RGB;
+      cinfo.out_color_space = JCS_RGB;
 #endif
       break;
     case JCS_CMYK:
@@ -227,39 +248,39 @@ bool JPEGCodecRobustSlow::Decode(const unsigned char* input,
       return false;
   }
 #ifndef JCS_EXTENSIONS
-  cinfo->output_components = 3;
+  cinfo.output_components = 3;
 #endif
 
-  jpeg_calc_output_dimensions(cinfo.get());
-  *w = cinfo->output_width;
-  *h = cinfo->output_height;
+  jpeg_calc_output_dimensions(&cinfo);
+  *w = cinfo.output_width;
+  *h = cinfo.output_height;
 
-  jpeg_start_decompress(cinfo.get());
+  jpeg_start_decompress(&cinfo);
 
   // FIXME(brettw) we may want to allow the capability for callers to request
   // how to align row lengths as we do for the compressor.
-  int row_read_stride = cinfo->output_width * cinfo->output_components;
+  int row_read_stride = cinfo.output_width * cinfo.output_components;
 
 #ifdef JCS_EXTENSIONS
   // Create memory for a decoded image and write decoded lines to the memory
   // without conversions same as JPEGCodec::Encode().
   int row_write_stride = row_read_stride;
-  output->resize(row_write_stride * cinfo->output_height);
+  output->resize(row_write_stride * cinfo.output_height);
 
-  for (int row = 0; row < static_cast<int>(cinfo->output_height); row++) {
+  for (int row = 0; row < static_cast<int>(cinfo.output_height); row++) {
     unsigned char* rowptr = &(*output)[row * row_write_stride];
-    if (!jpeg_read_scanlines(cinfo.get(), &rowptr, 1))
+    if (!jpeg_read_scanlines(&cinfo, &rowptr, 1))
       return false;
   }
 #else
   if (format == FORMAT_RGB) {
     // easy case, row needs no conversion
     int row_write_stride = row_read_stride;
-    output->resize(row_write_stride * cinfo->output_height);
+    output->resize(row_write_stride * cinfo.output_height);
 
-    for (int row = 0; row < static_cast<int>(cinfo->output_height); row++) {
+    for (int row = 0; row < static_cast<int>(cinfo.output_height); row++) {
       unsigned char* rowptr = &(*output)[row * row_write_stride];
-      if (!jpeg_read_scanlines(cinfo.get(), &rowptr, 1))
+      if (!jpeg_read_scanlines(&cinfo, &rowptr, 1))
         return false;
     }
   } else {
@@ -270,32 +291,33 @@ bool JPEGCodecRobustSlow::Decode(const unsigned char* input,
     void (*converter)(const unsigned char* rgb, int w, unsigned char* out);
     if (format == FORMAT_RGBA ||
         (format == FORMAT_SkBitmap && SK_R32_SHIFT == 0)) {
-      row_write_stride = cinfo->output_width * 4;
+      row_write_stride = cinfo.output_width * 4;
       converter = AddAlpha;
     } else if (format == FORMAT_BGRA ||
                (format == FORMAT_SkBitmap && SK_B32_SHIFT == 0)) {
-      row_write_stride = cinfo->output_width * 4;
+      row_write_stride = cinfo.output_width * 4;
       converter = RGBtoBGRA;
     } else {
       NOTREACHED() << "Invalid pixel format";
-      jpeg_destroy_decompress(cinfo.get());
+      jpeg_destroy_decompress(&cinfo);
       return false;
     }
 
-    output->resize(row_write_stride * cinfo->output_height);
+    output->resize(row_write_stride * cinfo.output_height);
 
     std::unique_ptr<unsigned char[]> row_data(
         new unsigned char[row_read_stride]);
     unsigned char* rowptr = row_data.get();
-    for (int row = 0; row < static_cast<int>(cinfo->output_height); row++) {
-      if (!jpeg_read_scanlines(cinfo.get(), &rowptr, 1))
+    for (int row = 0; row < static_cast<int>(cinfo.output_height); row++) {
+      if (!jpeg_read_scanlines(&cinfo, &rowptr, 1))
         return false;
       converter(rowptr, *w, &(*output)[row * row_write_stride]);
     }
   }
 #endif
 
-  jpeg_finish_decompress(cinfo.get());
+  jpeg_finish_decompress(&cinfo);
+  jpeg_destroy_decompress(&cinfo);
   return true;
 }
 

ui/gfx/codec/jpeg_codec.cc

@@ -150,14 +150,31 @@ void SkipInputData(j_decompress_ptr cinfo, long num_bytes) {
 //  "Terminate source --- called by jpeg_finish_decompress() after all data has
 //   been read to clean up JPEG source manager. NOT called by jpeg_abort() or
 //   jpeg_destroy()."
-void TermSource(j_decompress_ptr cinfo) {}
+void TermSource(j_decompress_ptr cinfo) {
+}
 
-// jpeg_decompress_struct Deleter.
-struct JpegDecompressStructDeleter {
-  void operator()(jpeg_decompress_struct* ptr) {
-    jpeg_destroy_decompress(ptr);
-    delete ptr;
+// This class destroys the given jpeg_decompress object when it goes out of
+// scope. It simplifies the error handling in Decode (and even applies to the
+// success case).
+class DecompressDestroyer {
+ public:
+  DecompressDestroyer() : cinfo_(NULL) {
+  }
+  ~DecompressDestroyer() {
+    DestroyManagedObject();
+  }
+  void SetManagedObject(jpeg_decompress_struct* ci) {
+    DestroyManagedObject();
+    cinfo_ = ci;
+  }
+  void DestroyManagedObject() {
+    if (cinfo_) {
+      jpeg_destroy_decompress(cinfo_);
+      cinfo_ = NULL;
+    }
   }
+ private:
+  jpeg_decompress_struct* cinfo_;
 };
 
 }  // namespace
@@ -165,26 +182,28 @@ struct JpegDecompressStructDeleter {
 bool JPEGCodec::Decode(const unsigned char* input, size_t input_size,
                        ColorFormat format, std::vector<unsigned char>* output,
                        int* w, int* h) {
-  std::unique_ptr<jpeg_decompress_struct, JpegDecompressStructDeleter> cinfo(
-      new jpeg_decompress_struct);
+  jpeg_decompress_struct cinfo;
+  DecompressDestroyer destroyer;
+  destroyer.SetManagedObject(&cinfo);
   output->clear();
 
   // We set up the normal JPEG error routines, then override error_exit.
-  // This must be done before the call to jpeg_create_decompress.
+  // This must be done before the call to create_decompress.
   CoderErrorMgr errmgr;
-  cinfo->err = jpeg_std_error(&errmgr.pub);
+  cinfo.err = jpeg_std_error(&errmgr.pub);
   errmgr.pub.error_exit = ErrorExit;
   // Establish the setjmp return context for ErrorExit to use.
   if (setjmp(errmgr.setjmp_buffer)) {
     // If we get here, the JPEG code has signaled an error.
-    // Release |cinfo| by hand to avoid use-after-free of |errmgr|.
-    cinfo.reset();
+    // See note in JPEGCodec::Encode() for why we need to destroy the cinfo
+    // manually here.
+    destroyer.DestroyManagedObject();
     return false;
   }
 
   // The destroyer will destroy() cinfo on exit.  We don't want to set the
   // destroyer's object until cinfo is initialized.
-  jpeg_create_decompress(cinfo.get());
+  jpeg_create_decompress(&cinfo);
 
   // set up the source manager
   jpeg_source_mgr srcmgr;
@@ -193,17 +212,17 @@ bool JPEGCodec::Decode(const unsigned char* input, size_t input_size,
   srcmgr.skip_input_data = SkipInputData;
   srcmgr.resync_to_restart = jpeg_resync_to_restart;  // use default routine
   srcmgr.term_source = TermSource;
-  cinfo->src = &srcmgr;
+  cinfo.src = &srcmgr;
 
   JpegDecoderState state(input, input_size);
-  cinfo->client_data = &state;
+  cinfo.client_data = &state;
 
   // fill the file metadata into our buffer
-  if (jpeg_read_header(cinfo.get(), true) != JPEG_HEADER_OK)
+  if (jpeg_read_header(&cinfo, true) != JPEG_HEADER_OK)
     return false;
 
   // we want to always get RGB data out
-  switch (cinfo->jpeg_color_space) {
+  switch (cinfo.jpeg_color_space) {
     case JCS_GRAYSCALE:
     case JCS_RGB:
     case JCS_YCbCr:
@@ -213,13 +232,15 @@ bool JPEGCodec::Decode(const unsigned char* input, size_t input_size,
       // parameters to a colorspace.
       if (format == FORMAT_RGBA ||
           (format == FORMAT_SkBitmap && SK_R32_SHIFT == 0)) {
-        cinfo->out_color_space = JCS_EXT_RGBX;
-        cinfo->output_components = 4;
+        cinfo.out_color_space = JCS_EXT_RGBX;
+        cinfo.output_components = 4;
       } else if (format == FORMAT_BGRA ||
                  (format == FORMAT_SkBitmap && SK_B32_SHIFT == 0)) {
-        cinfo->out_color_space = JCS_EXT_BGRX;
-        cinfo->output_components = 4;
+        cinfo.out_color_space = JCS_EXT_BGRX;
+        cinfo.output_components = 4;
       } else {
+        // We can exit this function without calling jpeg_destroy_decompress()
+        // because DecompressDestroyer automaticaly calls it.
         NOTREACHED() << "Invalid pixel format";
         return false;
       }
@@ -233,28 +254,29 @@ bool JPEGCodec::Decode(const unsigned char* input, size_t input_size,
       return false;
   }
 
-  jpeg_calc_output_dimensions(cinfo.get());
-  *w = cinfo->output_width;
-  *h = cinfo->output_height;
+  jpeg_calc_output_dimensions(&cinfo);
+  *w = cinfo.output_width;
+  *h = cinfo.output_height;
 
-  jpeg_start_decompress(cinfo.get());
+  jpeg_start_decompress(&cinfo);
 
   // FIXME(brettw) we may want to allow the capability for callers to request
   // how to align row lengths as we do for the compressor.
-  int row_read_stride = cinfo->output_width * cinfo->output_components;
+  int row_read_stride = cinfo.output_width * cinfo.output_components;
 
   // Create memory for a decoded image and write decoded lines to the memory
   // without conversions same as JPEGCodec::Encode().
   int row_write_stride = row_read_stride;
-  output->resize(row_write_stride * cinfo->output_height);
+  output->resize(row_write_stride * cinfo.output_height);
 
-  for (int row = 0; row < static_cast<int>(cinfo->output_height); row++) {
+  for (int row = 0; row < static_cast<int>(cinfo.output_height); row++) {
     unsigned char* rowptr = &(*output)[row * row_write_stride];
-    if (!jpeg_read_scanlines(cinfo.get(), &rowptr, 1))
+    if (!jpeg_read_scanlines(&cinfo, &rowptr, 1))
       return false;
   }
 
-  jpeg_finish_decompress(cinfo.get());
+  jpeg_finish_decompress(&cinfo);
+  jpeg_destroy_decompress(&cinfo);
   return true;
 }