Fix two race conditions in ImageDecoder.

chrome/browser/image_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/image_decoder.h"
 
 #include "base/bind.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/common/chrome_utility_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/utility_process_host.h"
 
 using content::BrowserThread;
 using content::UtilityProcessHost;
 
 namespace {
 
 // static, Leaky to allow access from any thread.
 base::LazyInstance<ImageDecoder>::Leaky g_decoder = LAZY_INSTANCE_INITIALIZER;
 
 // How long to wait after the last request has been received before ending
 // batch mode.
 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.
   // Explicitly add a reference so the object isn't deleted.
   AddRef();
 }
 
 ImageDecoder::~ImageDecoder() {
 }
 
@@ skipping 10 matching lines @@
 void ImageDecoder::Start(ImageRequest* image_request,
                          const std::string& image_data) {
   StartWithOptions(image_request, image_data, DEFAULT_CODEC, false);
 }
 
 // static
 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));
 }
 
 // static
 void ImageDecoder::Cancel(ImageRequest* image_request) {
   DCHECK(image_request);
   g_decoder.Pointer()->CancelImpl(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) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (!utility_process_host_) {
     StartBatchMode();
   }
   if (!utility_process_host_) {
     // Utility process failed to start; notify delegate and return.
     // 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() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   utility_process_host_ =
       UtilityProcessHost::Create(this, base::MessageLoopProxy::current().get())
           ->AsWeakPtr();
   if (!utility_process_host_->StartBatchMode()) {
      utility_process_host_.reset();
      return;
@@ skipping 27 matching lines @@
                         OnDecodeImageFailed)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 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;
+    }
   }
 }