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"
@@ skipping 18 matching lines @@
   // 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() {
 }
 
 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) {
   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());
+
+  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));
 }
 
 // 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) {
+  // 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;
     }
   }
 }
 
 void ImageDecoder::StartBatchMode() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   utility_process_host_ =
@@ skipping 35 matching lines @@
   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));
+    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);
   }
 }
 
 void ImageDecoder::OnDecodeImageFailed(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::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);
   }
 }