Fix race condition when clearing thumbnail

ConvertJPEGDataToImageSkiaAndNotifyObservers() copies
ThumbnailImage::data_ (which is a scoped_refptr) and posts a task
referring to it to another thread. ThumbnailImage::ClearData() cleared
the data behind the pointer before resetting the pointer. This led to
a race condition where the data was being cleared on the UI thread and
read on a background thread without synchronization.

This CL simply resets the pointer in ThumbnailImage::ClearData()
without modifying the wrapped data. As long as the data inside is
treated as immutable, this is safe: updating ThumbnailImage::data_
does not change data that outstanding tasks see.

Fixed: 1160146
Change-Id: I5ef9e1712cc5935f85d1b8d9226588cc43921b67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611174Reviewed-by: Dana Fried <dfried@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840776}

chrome/browser/ui/thumbnails/thumbnail_image.cc

@@ -82,12 +82,18 @@ void ThumbnailImage::AssignSkBitmap(SkBitmap bitmap) {
 }
 
 void ThumbnailImage::ClearData() {
-  // TODO(collinbaker): Update this to notify the observers that the data has
-  // changed. It may be necessary to re-engineer them to accept empty/invalid
-  // data. crbug.com/1152894
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // TODO(crbug.com/1163121): Update this to notify the observers that the data
+  // has changed. It may be necessary to re-engineer them to accept
+  // empty/invalid data. crbug.com/1152894
   if (!data_)
     return;
-  data_->data.clear();
+
+  // TODO(crbug.com/1163121): Cancel existing thumbnail request. If
+  // called after ConvertJPEGDataToImageSkiaAndNotifyObservers() but
+  // before observers get notified, the observers will receive the stale
+  // thumbnail.
   data_.reset();
 }
 
@@ -121,6 +127,8 @@ void ThumbnailImage::AssignJPEGData(base::TimeTicks assign_sk_bitmap_time,
 }
 
 bool ThumbnailImage::ConvertJPEGDataToImageSkiaAndNotifyObservers() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!data_) {
     if (async_operation_finished_callback_)
       async_operation_finished_callback_.Run();

chrome/browser/ui/thumbnails/thumbnail_image.h

@@ -132,6 +132,10 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
 
   Delegate* delegate_;
 
+  // This is a scoped_refptr to immutable data. Once set, the wrapped
+  // data must not be modified; it is referenced by other threads.
+  // |data_| itself can be changed as this does not affect references to
+  // the old data.
   CompressedThumbnailData data_;
 
   base::ObserverList<Observer> observers_;

chrome/browser/ui/thumbnails/thumbnail_image_unittest.cc

@@ -280,3 +280,17 @@ TEST_F(ThumbnailImageTest, RequestCompressedThumbnailData) {
   image->RequestCompressedThumbnailData();
   EXPECT_EQ(count_before_request + 1, observer.new_compressed_count());
 }
+
+TEST_F(ThumbnailImageTest, ClearThumbnailWhileNotifyingObservers) {
+  auto image = base::MakeRefCounted<ThumbnailImage>(this);
+  TestThumbnailImageObserver observer;
+  observer.scoped_observer()->Add(image.get());
+
+  SkBitmap bitmap = CreateBitmap(kTestBitmapWidth, kTestBitmapHeight);
+  image->AssignSkBitmap(bitmap);
+  observer.WaitForImage();
+
+  image->RequestThumbnailImage();
+  image->ClearData();
+  observer.WaitForImage();
+}