Fix two race conditions in ImageDecoder.

chrome/browser/image_decoder.cc

Index: chrome/browser/image_decoder.cc
diff --git a/chrome/browser/image_decoder.cc b/chrome/browser/image_decoder.cc
index 9e21662088f5ab09a5cd61b71057da235bb6fe24..dd6ac13a1ec415d62c9207000a795a83bb54a674 100644
--- a/chrome/browser/image_decoder.cc
+++ b/chrome/browser/image_decoder.cc
@@ -24,6 +24,84 @@ const int kBatchModeTimeoutSeconds = 5;
 
 }  // namespace
 
+class ImageDecoder::Job : public base::RefCountedThreadSafe<ImageDecoder::Job> {
+ public:
+  Job(ImageDecoder* decoder, ImageDecoder::ImageRequest* request);
+
+  void Cancel();
+  void NotifyDecodeSucceeded(const SkBitmap& decoded_image);
+  void NotifyDecodeFailed();
+
+  ImageDecoder::ImageRequest* image_request() const { return request_; }
+
+ private:
+  friend class base::RefCountedThreadSafe<ImageDecoder::Job>;
+  ~Job() = default;
+
+  // Runs on the requests's task runner.
+  void OnSuccess(const SkBitmap& decoded_image);
+  void OnFailed();
+
+  void RemoveTask();
+
+  // Not owned, and must remain valid for the life of this object.
+  ImageDecoder* decoder_;

[Lei Zhang, 2015/04/08 01:30:10]

Isn't there just a single ImageDecoder that will outlive everything else since
it's leaky?

[Anand Mistry (off Chromium), 2015/04/08 03:13:46]

Good point. Removed and use lazy instance.

+
+  base::Lock lock_;
+  bool is_cancelled_;

[Lei Zhang, 2015/04/08 01:30:10]

Just null out |request_| instead?

[Anand Mistry (off Chromium), 2015/04/08 03:13:46]

Can't. Doing so would require always accessing |request_| under |lock_|, which
may cause a deadlock between ImageDecoder::CancelImpl and OnSuccess/OnFailed.

+  // Only valid while |is_cancelled_| == false.
+  ImageDecoder::ImageRequest* request_;
+};
+
+ImageDecoder::Job::Job(ImageDecoder* decoder,
+                       ImageDecoder::ImageRequest* request)
+    : decoder_(decoder), is_cancelled_(false), request_(request) {
+}
+
+void ImageDecoder::Job::Cancel() {
+  base::AutoLock lock(lock_);
+  is_cancelled_ = true;
+}
+
+void ImageDecoder::Job::NotifyDecodeSucceeded(const SkBitmap& decoded_image) {
+  base::AutoLock lock(lock_);
+  if (is_cancelled_)
+    return;
+
+  request_->task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&ImageDecoder::Job::OnSuccess, this, decoded_image));
+}
+
+void ImageDecoder::Job::NotifyDecodeFailed() {
+  base::AutoLock lock(lock_);
+  if (is_cancelled_)
+    return;
+
+  request_->task_runner()->PostTask(
+      FROM_HERE, base::Bind(&ImageDecoder::Job::OnFailed, this));
+}
+
+void ImageDecoder::Job::OnSuccess(const SkBitmap& decoded_image) {
+  base::AutoLock lock(lock_);
+  if (is_cancelled_)
+    return;
+
+  DCHECK(request_->task_runner()->RunsTasksOnCurrentThread());
+  decoder_->RemoveJob(this);

[Lei Zhang, 2015/04/08 01:30:10]

This and the RemoveJob() call in ImageDecoder::Job::OnFailed() are the only two
places that can remove a job from |g_decoder|'s |image_request_id_map_|.

If a given job has been cancelled, RemoveJob() never gets to run. Wouldn't the
job remain in the RequestMap forever and effectively be leaked?

[Anand Mistry (off Chromium), 2015/04/08 03:13:45]

ImageDecoder::CancelImpl also does. However, I can see the confusion, so I've
moved cancel's removal from the map into Job::Cancel for consistency. It's less
efficient, but clearer.

+  request_->OnImageDecoded(decoded_image);
+}
+
+void ImageDecoder::Job::OnFailed() {
+  base::AutoLock lock(lock_);
+  if (is_cancelled_)
+    return;
+
+  DCHECK(request_->task_runner()->RunsTasksOnCurrentThread());
+  decoder_->RemoveJob(this);
+  request_->OnDecodeImageFailed();
+}
+
 ImageDecoder::ImageDecoder()
     : image_request_id_counter_(0), last_request_(base::TimeTicks::Now()) {
   // A single ImageDecoder instance should live for the life of the program.
@@ -54,13 +132,28 @@ void ImageDecoder::StartWithOptions(ImageRequest* image_request,
                                     const std::string& image_data,
                                     ImageCodec image_codec,
                                     bool shrink_to_fit) {
+  g_decoder.Pointer()->StartWithOptionsImpl(image_request, image_data,
+                                            image_codec, shrink_to_fit);
+}
+
+void ImageDecoder::StartWithOptionsImpl(ImageRequest* image_request,
+                                        const std::string& image_data,
+                                        ImageCodec image_codec,
+                                        bool shrink_to_fit) {
   DCHECK(image_request);
   DCHECK(image_request->task_runner());
+
+  scoped_refptr<Job> job(new Job(this, image_request));
+  int request_id;
+  {
+    base::AutoLock lock(map_lock_);
+    request_id = image_request_id_counter_++;
+    image_request_id_map_.insert(std::make_pair(request_id, job));
+  }
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(
-          &ImageDecoder::DecodeImageInSandbox,
-          g_decoder.Pointer(), image_request,
+          &ImageDecoder::DecodeImageInSandbox, this, request_id,
           std::vector<unsigned char>(image_data.begin(), image_data.end()),
           image_codec, shrink_to_fit));
 }
@@ -72,7 +165,7 @@ void ImageDecoder::Cancel(ImageRequest* image_request) {
 }
 
 void ImageDecoder::DecodeImageInSandbox(
-    ImageRequest* image_request,
+    int request_id,
     const std::vector<unsigned char>& image_data,
     ImageCodec image_codec,
     bool shrink_to_fit) {
@@ -85,41 +178,45 @@ void ImageDecoder::DecodeImageInSandbox(
     // Without this check, we were seeing crashes on startup. Further
     // investigation is needed to determine why the utility process
     // is failing to start. See crbug.com/472272
-    image_request->task_runner()->PostTask(
-        FROM_HERE, base::Bind(&ImageRequest::OnDecodeImageFailed,
-                              base::Unretained(image_request)));
+    OnDecodeImageFailed(request_id);
     return;
   }
 
   last_request_ = base::TimeTicks::Now();
-  base::AutoLock lock(map_lock_);
-  image_request_id_map_.insert(
-      std::make_pair(image_request_id_counter_, image_request));
 
   switch (image_codec) {
     case ROBUST_JPEG_CODEC:
-      utility_process_host_->Send(new ChromeUtilityMsg_RobustJPEGDecodeImage(
-          image_data, image_request_id_counter_));
+      utility_process_host_->Send(
+          new ChromeUtilityMsg_RobustJPEGDecodeImage(image_data, request_id));
       break;
     case DEFAULT_CODEC:
       utility_process_host_->Send(new ChromeUtilityMsg_DecodeImage(
-          image_data, shrink_to_fit, image_request_id_counter_));
+          image_data, shrink_to_fit, request_id));
       break;
   }
-
-  ++image_request_id_counter_;
 }
 
 void ImageDecoder::CancelImpl(ImageRequest* image_request) {
-  base::AutoLock lock(map_lock_);
-  for (auto it = image_request_id_map_.begin();
-       it != image_request_id_map_.end();) {
-    if (it->second == image_request) {
-      image_request_id_map_.erase(it++);
-    } else {
-      ++it;
+  scoped_refptr<Job> job;
+
+  {
+    base::AutoLock lock(map_lock_);
+    for (auto it = image_request_id_map_.begin();
+         it != image_request_id_map_.end();) {
+      if (it->second->image_request() == image_request) {
+        // There should only be one.

[Lei Zhang, 2015/04/08 01:30:10]

If a caller does Start(), Start(), Cancel(), wouldn't there be 2?

[Anand Mistry (off Chromium), 2015/04/08 03:13:45]

I wrote that off since it seems like a "Bad Idea", because there's no way of
associating a decoded image with the original compressed one. But I guess it's
currently supported, so might as well continue doing so.

Fixed to support this case.

+        DCHECK(!job);
+        job = it->second;
+        image_request_id_map_.erase(it++);
+      } else {
+        ++it;
+      }
     }
   }
+
+  // Will block if |OnImageDecoded| or |OnDecodeImageFailed| is running.
+  if (job)
+    job->Cancel();
 }
 
 void ImageDecoder::StartBatchMode() {
@@ -167,28 +264,46 @@ void ImageDecoder::OnDecodeImageSucceeded(
     const SkBitmap& decoded_image,
     int request_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  base::AutoLock lock(map_lock_);
-  auto it = image_request_id_map_.find(request_id);
-  if (it != image_request_id_map_.end()) {
-    ImageRequest* image_request = it->second;
-    image_request->task_runner()->PostTask(
-        FROM_HERE, base::Bind(&ImageRequest::OnImageDecoded,
-                              base::Unretained(image_request), decoded_image));
-
-    image_request_id_map_.erase(it);
+
+  scoped_refptr<Job> job;
+  {
+    base::AutoLock lock(map_lock_);
+    auto it = image_request_id_map_.find(request_id);
+    if (it != image_request_id_map_.end()) {
+      job = it->second;
+      // NOTE: The job isn't removed from the map because it can still be
+      // cancelled.
+    }
   }
+  // Call Job's functions outside |map_lock_| to avoid a potential deadlock with
+  // |Job.lock_|.
+  if (job)
+    job->NotifyDecodeSucceeded(decoded_image);
 }
 
 void ImageDecoder::OnDecodeImageFailed(int request_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  scoped_refptr<Job> job;
+  {
+    base::AutoLock lock(map_lock_);
+    auto it = image_request_id_map_.find(request_id);
+    if (it != image_request_id_map_.end()) {
+      job = it->second;
+    }
+  }
+  if (job)
+    job->NotifyDecodeFailed();
+}
+
+void ImageDecoder::RemoveJob(const scoped_refptr<Job>& job) {
   base::AutoLock lock(map_lock_);
-  auto it = image_request_id_map_.find(request_id);
-  if (it != image_request_id_map_.end()) {
-    ImageRequest* image_request = it->second;
-    image_request->task_runner()->PostTask(
-        FROM_HERE, base::Bind(&ImageRequest::OnDecodeImageFailed,
-                              base::Unretained(image_request)));
-
-    image_request_id_map_.erase(it);
+  for (auto it = image_request_id_map_.begin();
+       it != image_request_id_map_.end();) {
+    if (it->second == job) {
+      image_request_id_map_.erase(it++);

[Lei Zhang, 2015/04/08 01:30:10]

Why keep going? Isn't there only a single instance of |job| in
|image_request_id_map_| ?

[Anand Mistry (off Chromium), 2015/04/08 03:13:45]

Done.

+    } else {
+      ++it;
+    }
   }
 }