Fix race conditions in ImageDecoder.

chrome/browser/image_decoder.h

Index: chrome/browser/image_decoder.h
diff --git a/chrome/browser/image_decoder.h b/chrome/browser/image_decoder.h
index 438622c442d48a9b2bc5b125b11a8d299faccbd1..392005366b37324cc3e265aff13ec977c1b043d7 100644
--- a/chrome/browser/image_decoder.h
+++ b/chrome/browser/image_decoder.h
@@ -9,11 +9,11 @@
 #include <string>
 #include <vector>
 
-#include "base/compiler_specific.h"
 #include "base/lazy_instance.h"
 #include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
 #include "base/synchronization/lock.h"
-#include "base/threading/sequenced_worker_pool.h"
 #include "base/timer/timer.h"
 #include "content/public/browser/utility_process_host.h"
 #include "content/public/browser/utility_process_host_client.h"
@@ -32,6 +32,8 @@ class SkBitmap;
 // to protect the data that is accessed from multiple threads.
 class ImageDecoder : public content::UtilityProcessHostClient {
  public:
+  // ImageRequest objects needs to be created and destroyed on the same
+  // SequencedTaskRunner.
   class ImageRequest {
    public:
     // Called when image is decoded.
@@ -46,14 +48,20 @@ class ImageDecoder : public content::UtilityProcessHostClient {
     }
 
    protected:
+    // Creates an ImageRequest that runs on the thread creating it.
+    ImageRequest();

[Theresa, 2015/04/20 22:35:26]

I like this introduction, definitely makes things easier for the clients.

+    // Explicitly pass in |task_runner| if the current thread is part of a
+    // thread pool.
     explicit ImageRequest(
         const scoped_refptr<base::SequencedTaskRunner>& task_runner);
     virtual ~ImageRequest();
 
    private:
-    // The thread to post OnImageDecoded or OnDecodeImageFailed once the
+    // The thread to post OnImageDecoded() or OnDecodeImageFailed() once the
     // the image has been decoded.
     const scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+    base::SequenceChecker sequence_checker_;
   };
 
   enum ImageCodec {
@@ -61,7 +69,7 @@ class ImageDecoder : public content::UtilityProcessHostClient {
     ROBUST_JPEG_CODEC,  // Restrict decoding to robust jpeg codec.
   };
 
-  // Calls StartWithOptions with ImageCodec::DEFAULT_CODEC and
+  // Calls StartWithOptions() with ImageCodec::DEFAULT_CODEC and
   // shrink_to_fit = false.
   static void Start(ImageRequest* image_request,
                     const std::string& image_data);
@@ -73,13 +81,15 @@ class ImageDecoder : public content::UtilityProcessHostClient {
                                ImageCodec image_codec,
                                bool shrink_to_fit);
 
-  // Removes all instances of image_request from |image_request_id_map_|,
+  // Removes all instances of |image_request| from |image_request_id_map_|,
   // ensuring callbacks are not made to the image_request after it is destroyed.
   static void Cancel(ImageRequest* image_request);
 
  private:
   friend struct base::DefaultLazyInstanceTraits<ImageDecoder>;
 
+  using RequestMap = std::map<int, ImageRequest*>;
+
   ImageDecoder();
   // It's a reference counted object, so destructor is private.
   ~ImageDecoder() override;
@@ -87,22 +97,24 @@ class ImageDecoder : public content::UtilityProcessHostClient {
   // Sends a request to the sandboxed process to decode the image. Starts
   // batch mode if necessary. If the utility process fails to start,
   // an OnDecodeImageFailed task is posted to image_request's |task_runner_|.
-  void DecodeImageInSandbox(ImageRequest* image_request,
+  void DecodeImageInSandbox(int image_request_counter,

[Theresa, 2015/04/20 22:35:26]

int request_id?

[Lei Zhang, 2015/04/21 00:44:00]

Done.

                             const std::vector<unsigned char>& image_data,
                             ImageCodec image_codec,
                             bool shrink_to_fit);
 
+  void StartWithOptionsImpl(ImageRequest* image_request,
+                            const std::string& image_data,
+                            ImageCodec image_codec,
+                            bool shrink_to_fit);
   void CancelImpl(ImageRequest* image_request);
 
-  using RequestMap = std::map<int, ImageRequest*>;
-
   // Starts UtilityProcessHost in batch mode and starts |batch_mode_timer_|.
   // If the utility process fails to start, the method resets
   // |utility_process_host_| and returns.
   void StartBatchMode();
 
   // Stops batch mode if no requests have come in since
-  // kBatchModeTimeoutSeconds.
+  // |kBatchModeTimeoutSeconds|.
   void StopBatchMode();
 
   // Overidden from UtilityProcessHostClient.
@@ -112,7 +124,12 @@ class ImageDecoder : public content::UtilityProcessHostClient {
   void OnDecodeImageSucceeded(const SkBitmap& decoded_image, int request_id);
   void OnDecodeImageFailed(int request_id);
 
-  // id to use for the next Start request that comes in.
+  // For the ImageRequest identified by |request_id|, call its OnImageDecoded()
+  // or OnDecodeImageFailed() method on its task runner thread.
+  void RunOnImageDecoded(const SkBitmap& decoded_image, int request_id);
+  void RunOnDecodeImageFailed(int request_id);
+
+  // id to use for the next Start() request that comes in.
   int image_request_id_counter_;
 
   // Map of request id's to ImageRequests.
@@ -124,10 +141,10 @@ class ImageDecoder : public content::UtilityProcessHostClient {
   // The UtilityProcessHost requests are sent to.
   base::WeakPtr<content::UtilityProcessHost> utility_process_host_;
 
-  // Calls StopBatchMode after kBatchModeTimeoutSeconds have elapsed.
+  // Calls StopBatchMode() after |kBatchModeTimeoutSeconds| have elapsed.
   base::RepeatingTimer<ImageDecoder> batch_mode_timer_;
 
-  // The time Start was last called.
+  // The time Start() was last called.
   base::TimeTicks last_request_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageDecoder);