Fix holding space image lifetime issues

HoldingSpaceImage's image source has a pointer to the HoldingSpaceImage,
so it can notify it that new image representation has been loaded.
Image source is owned by the image itself, and may outlive the
associated HoldingSpaceItem, which owns HoldingSpaceImage. If the
thumbnail generation for the item finishes after the item has been
destroyed, the holding space image source may attempt to access the
HoldingSpaceImage object after destruction.
To fix this, pass a weak pointer to HoldingSpaceImage source, so it can
verify the HoldingSpaceImage is still around before notifying it that
the image has changed.

HoldingSpcareTrayIconPreview contents image had a similar issue where it
was getting the item image from the item ptr. This was done in
GetImageForScale, which may get called as long as the associated image
skia is around, which doesn't guarantee that the item has not already
been deleted - instead, have the contents image use the image skia
directly.

BUG=1156776

Change-Id: I6e6c6d14ed5a25739e8b4366225f29dfa8706c8e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587655Reviewed-by: David Black <dmblack@google.com>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836402}

ash/public/cpp/holding_space/holding_space_image.cc

@@ -17,7 +17,7 @@ namespace ash {
 
 class HoldingSpaceImage::ImageSkiaSource : public gfx::ImageSkiaSource {
  public:
-  ImageSkiaSource(HoldingSpaceImage* owner,
+  ImageSkiaSource(const base::WeakPtr<HoldingSpaceImage>& owner,
                   const gfx::ImageSkia& placeholder,
                   AsyncBitmapResolver async_bitmap_resolver)
       : owner_(owner),
@@ -48,11 +48,12 @@ class HoldingSpaceImage::ImageSkiaSource : public gfx::ImageSkiaSource {
   void CacheImageForScale(float scale, const SkBitmap* bitmap) {
     if (bitmap) {
       cache_[scale].AddRepresentation(gfx::ImageSkiaRep(*bitmap, scale));
-      owner_->NotifyUpdated(scale);
+      if (owner_)
+        owner_->NotifyUpdated(scale);
     }
   }
 
-  HoldingSpaceImage* const owner_;
+  const base::WeakPtr<HoldingSpaceImage> owner_;
   const gfx::ImageSkia placeholder_;
   AsyncBitmapResolver async_bitmap_resolver_;
   std::map<float, gfx::ImageSkia> cache_;
@@ -62,12 +63,14 @@ class HoldingSpaceImage::ImageSkiaSource : public gfx::ImageSkiaSource {
 
 // HoldingSpaceImage -----------------------------------------------------------
 
-HoldingSpaceImage::HoldingSpaceImage(const gfx::ImageSkia& placeholder,
-                                     AsyncBitmapResolver async_bitmap_resolver)
-    : image_skia_(std::make_unique<ImageSkiaSource>(/*owner=*/this,
-                                                    placeholder,
-                                                    async_bitmap_resolver),
-                  placeholder.size()) {}
+HoldingSpaceImage::HoldingSpaceImage(
+    const gfx::ImageSkia& placeholder,
+    AsyncBitmapResolver async_bitmap_resolver) {
+  image_skia_ = gfx::ImageSkia(
+      std::make_unique<ImageSkiaSource>(/*owner=*/weak_factory_.GetWeakPtr(),
+                                        placeholder, async_bitmap_resolver),
+      placeholder.size());
+}
 
 HoldingSpaceImage::~HoldingSpaceImage() = default;
 

ash/public/cpp/holding_space/holding_space_image.h

@@ -51,6 +51,8 @@ class ASH_PUBLIC_EXPORT HoldingSpaceImage {
 
   // Mutable to allow const access from `AddImageSkiaChangedCallback()`.
   mutable CallbackList callback_list_;
+
+  base::WeakPtrFactory<HoldingSpaceImage> weak_factory_{this};
 };
 
 }  // namespace ash

ash/system/holding_space/holding_space_tray_icon_preview.cc

@@ -67,7 +67,8 @@ void SetUpAnimation(ui::ScopedLayerAnimationSettings* animation_settings) {
 
 class ContentsImageSource : public gfx::ImageSkiaSource {
  public:
-  explicit ContentsImageSource(const HoldingSpaceItem* item) : item_(item) {}
+  explicit ContentsImageSource(gfx::ImageSkia item_image)
+      : item_image_(item_image) {}
   ContentsImageSource(const ContentsImageSource&) = delete;
   ContentsImageSource& operator=(const ContentsImageSource&) = delete;
   ~ContentsImageSource() override = default;
@@ -75,7 +76,7 @@ class ContentsImageSource : public gfx::ImageSkiaSource {
  private:
   // gfx::ImageSkiaSource:
   gfx::ImageSkiaRep GetImageForScale(float scale) override {
-    gfx::ImageSkia image = item_->image().image_skia();
+    gfx::ImageSkia image = item_image_;
 
     // Crop to square (if necessary).
     gfx::Size square_size = image.size();
@@ -105,7 +106,7 @@ class ContentsImageSource : public gfx::ImageSkiaSource {
     return gfx::ImageSkiaRep(canvas.GetBitmap(), scale);
   }
 
-  const HoldingSpaceItem* item_;
+  const gfx::ImageSkia item_image_;
 };
 
 // ContentsImage ---------------------------------------------------------------
@@ -114,8 +115,9 @@ class ContentsImage : public gfx::ImageSkia {
  public:
   ContentsImage(const HoldingSpaceItem* item,
                 base::RepeatingClosure image_invalidated_closure)
-      : gfx::ImageSkia(std::make_unique<ContentsImageSource>(item),
-                       GetPreviewSize()),
+      : gfx::ImageSkia(
+            std::make_unique<ContentsImageSource>(item->image().image_skia()),
+            GetPreviewSize()),
         image_invalidated_closure_(image_invalidated_closure) {
     image_subscription_ = item->image().AddImageSkiaChangedCallback(
         base::BindRepeating(&ContentsImage::OnHoldingSpaceItemImageChanged,