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..6405697d031127cad510dc506dbbb6131d806e31 100644
--- a/chrome/browser/image_decoder.cc
+++ b/chrome/browser/image_decoder.cc
@@ -36,13 +36,21 @@ ImageDecoder::~ImageDecoder() {
 
 ImageDecoder::ImageRequest::ImageRequest(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner)
-    : task_runner_(task_runner) {
+    : task_runner_(task_runner), weak_factory_(this) {
 }
 
 ImageDecoder::ImageRequest::~ImageRequest() {
+  // Destructor needs to run on the same thread as |OnImageDecoded| and
+  // |OnDecodeImageFailed|, otherwise cancellation is racy.
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
   ImageDecoder::Cancel(this);
 }
 
+base::WeakPtr<ImageDecoder::ImageRequest>
+ImageDecoder::ImageRequest::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 // static
 void ImageDecoder::Start(ImageRequest* image_request,
                          const std::string& image_data) {
@@ -54,13 +62,29 @@ 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());
+
+  int request_id;
+  {
+    base::AutoLock lock(map_lock_);
+    request_id = image_request_id_counter_++;
+    image_request_id_map_.insert(std::make_pair(
+        request_id, std::make_pair(image_request->task_runner(),
+                                   image_request->GetWeakPtr())));
+  }
   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 +96,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,36 +109,35 @@ 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) {
+  // Cancellation needs to be done on the same thread as |OnImageDecoded| and
+  // |OnDecodeImageFailed|, otherwise there's a race where these functions can
+  // run while the request is being cancelled.
+  // This is also necessary for WeakPtr<> correctness.
+  DCHECK(image_request->task_runner()->RunsTasksOnCurrentThread());

[Lei Zhang, 2015/04/07 22:29:21]

The header file makes it sound as though ImageDecoder::Cancel() can run on any
thread, but internally, it's being restricted here.

+
   base::AutoLock lock(map_lock_);
   for (auto it = image_request_id_map_.begin();
        it != image_request_id_map_.end();) {
-    if (it->second == image_request) {
+    if (it->second.second.get() == image_request) {
       image_request_id_map_.erase(it++);
     } else {
       ++it;
@@ -170,10 +193,10 @@ void ImageDecoder::OnDecodeImageSucceeded(
   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));
+    auto& task_runner = it->second.first;
+    base::WeakPtr<ImageRequest> image_request = it->second.second;
+    task_runner->PostTask(FROM_HERE, base::Bind(&ImageRequest::OnImageDecoded,
+                                                image_request, decoded_image));
 
     image_request_id_map_.erase(it);
   }
@@ -184,10 +207,11 @@ void ImageDecoder::OnDecodeImageFailed(int request_id) {
   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)));
+    auto& task_runner = it->second.first;
+    base::WeakPtr<ImageRequest> image_request = it->second.second;
+    task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&ImageRequest::OnDecodeImageFailed, image_request));
 
     image_request_id_map_.erase(it);
   }