cc: Fix potential texture lifetime issues in the GPU image cache.

This fixes 2 bugs which could result in using invalid textures from the
image cache, if an image is originally uploaded at original size and
mips are generated with a subsequent copy later.

1) When we generate mips the original texture is deleted, while if the
DrawImage is ref-ed and there is an external ref on the SkImage backed
by the original texture it could potentially be used after its deleted.

2) If skia fails to mip the texture for any reason, the "mipped" image
returned is backed by the original texture which we would subsequently
delete.

R=ccameron@chromium.org, ericrk@chromium.org
TBR=piman@chromium.org

Bug: 870317
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iec925f35c7880db89f9b68a9cee5e0b76b8ce4d9
Reviewed-on: https://chromium-review.googlesource.com/1171960Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582484}

cc/tiles/gpu_image_decode_cache.cc

@@ -234,21 +234,6 @@ bool DrawAndScaleImage(const DrawImage& draw_image, SkPixmap* target_pixmap) {
   return scaled_pixmap.readPixels(pixmap);
 }
 
-// Returns the GL texture ID backing the given SkImage.
-GrGLuint GlIdFromSkImage(SkImage* image) {
-  DCHECK(image->isTextureBacked());
-  GrBackendTexture backend_texture =
-      image->getBackendTexture(true /* flushPendingGrContextIO */);
-  if (!backend_texture.isValid())
-    return 0;
-
-  GrGLTextureInfo info;
-  if (!backend_texture.getGLTextureInfo(&info))
-    return 0;
-
-  return info.fID;
-}
-
 // Takes ownership of the backing texture of an SkImage. This allows us to
 // delete this texture under Skia (via discardable).
 sk_sp<SkImage> TakeOwnershipOfSkImageBacking(GrContext* context,
@@ -287,7 +272,8 @@ void DeleteSkImageAndPreventCaching(viz::RasterContextProvider* context,
   if (image_owned) {
     // Delete |original_image_owned| as Skia will not clean it up. We are
     // holding the context lock here, so we can delete immediately.
-    uint32_t texture_id = GlIdFromSkImage(image_owned.get());
+    uint32_t texture_id =
+        GpuImageDecodeCache::GlIdFromSkImage(image_owned.get());
     context->ContextGL()->DeleteTextures(1, &texture_id);
   }
 }
@@ -669,6 +655,21 @@ void GpuImageDecodeCache::ImageData::ValidateBudgeted() const {
   DCHECK_GT(upload.ref_count, 0u);
 }
 
+// static
+GrGLuint GpuImageDecodeCache::GlIdFromSkImage(const SkImage* image) {
+  DCHECK(image->isTextureBacked());
+  GrBackendTexture backend_texture =
+      image->getBackendTexture(true /* flushPendingGrContextIO */);
+  if (!backend_texture.isValid())
+    return 0;
+
+  GrGLTextureInfo info;
+  if (!backend_texture.getGLTextureInfo(&info))
+    return 0;
+
+  return info.fID;
+}
+
 GpuImageDecodeCache::GpuImageDecodeCache(viz::RasterContextProvider* context,
                                          bool use_transfer_cache,
                                          SkColorType color_type,
@@ -1732,6 +1733,12 @@ void GpuImageDecodeCache::UnlockImage(ImageData* image_data) {
     ids_pending_unlock_.push_back(*image_data->upload.transfer_cache_id());
   }
   image_data->upload.OnUnlock();
+
+  // If we were holding onto an unmipped image for defering deletion, do it now
+  // it is guarenteed to have no-refs.
+  auto unmipped_image = image_data->upload.take_unmipped_image();
+  if (unmipped_image)
+    images_pending_deletion_.push_back(std::move(unmipped_image));
 }
 
 // We always run pending operations in the following order:
@@ -2020,8 +2027,10 @@ void GpuImageDecodeCache::UpdateMipsIfNeeded(const DrawImage& draw_image,
   if (!image_with_mips)
     return;
 
-  // We *must* get a new SkImage, or we will have lifetime issues.
-  DCHECK_NE(image_with_mips.get(), previous_image.get());
+  // No need to do anything if mipping this image results in the same texture.
+  // Deleting it below will result in lifetime issues.
+  if (GlIdFromSkImage(image_with_mips.get()) == image_data->upload.gl_id())
+    return;
 
   // Skia owns our new image, take ownership.
   sk_sp<SkImage> image_with_mips_owned = TakeOwnershipOfSkImageBacking(
@@ -2031,8 +2040,11 @@ void GpuImageDecodeCache::UpdateMipsIfNeeded(const DrawImage& draw_image,
   if (!image_with_mips_owned)
     return;
 
-  // Delete the previous image and set the new one to the cache.
-  images_pending_deletion_.push_back(image_data->upload.image());
+  // The previous image might be in the in-use cache, potentially held
+  // externally. We must defer deleting it until the entry is unlocked.
+  image_data->upload.set_unmipped_image(image_data->upload.image());
+
+  // Set the new image on the cache.
   image_data->upload.Reset();
   image_data->upload.SetImage(std::move(image_with_mips_owned));
   context_->ContextGL()->InitializeDiscardableTextureCHROMIUM(

cc/tiles/gpu_image_decode_cache.h

@@ -110,6 +110,9 @@ class CC_EXPORT GpuImageDecodeCache
                                int max_texture_size);
   ~GpuImageDecodeCache() override;
 
+  // Returns the GL texture ID backing the given SkImage.
+  static GrGLuint GlIdFromSkImage(const SkImage* image);
+
   // ImageDecodeCache overrides.
 
   // Finds the existing uploaded image for the provided DrawImage. Creates an
@@ -272,6 +275,14 @@ class CC_EXPORT GpuImageDecodeCache
       return transfer_cache_id_;
     }
 
+    void set_unmipped_image(sk_sp<SkImage> image) {
+      unmipped_image_ = std::move(image);
+    }
+    sk_sp<SkImage> take_unmipped_image() {
+      DCHECK(!is_locked_);
+      return std::move(unmipped_image_);
+    }
+
    private:
     // Used for internal DCHECKs only.
     enum class Mode {
@@ -291,6 +302,9 @@ class CC_EXPORT GpuImageDecodeCache
 
     // Used if |mode_| == kTransferCache.
     base::Optional<uint32_t> transfer_cache_id_;
+
+    // The original un-mipped image, retained until it can be safely deleted.
+    sk_sp<SkImage> unmipped_image_;
   };
 
   struct ImageData : public base::RefCountedThreadSafe<ImageData> {

cc/tiles/gpu_image_decode_cache_unittest.cc

@@ -19,6 +19,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkImageGenerator.h"
 #include "third_party/skia/include/core/SkRefCnt.h"
+#include "third_party/skia/include/gpu/GrBackendSurface.h"
 #include "third_party/skia/include/gpu/GrContext.h"
 
 namespace cc {
@@ -61,6 +62,16 @@ class FakeDiscardableManager {
     cached_textures_limit_ = limit;
   }
 
+  void ExpectLocked(GLuint texture_id) {
+    EXPECT_TRUE(textures_.end() != textures_.find(texture_id));
+
+    // Any value > kHandleLockedStart represents a locked texture. As we
+    // increment this value with each lock, we need the entire range and can't
+    // add additional values > kHandleLockedStart in the future.
+    EXPECT_GE(textures_[texture_id], kHandleLockedStart);
+    EXPECT_LE(textures_[texture_id], kHandleLockedEnd);
+  }
+
  private:
   void EnforceLimit() {
     for (auto it = textures_.begin(); it != textures_.end(); ++it) {
@@ -75,16 +86,6 @@ class FakeDiscardableManager {
     }
   }
 
-  void ExpectLocked(GLuint texture_id) {
-    EXPECT_TRUE(textures_.end() != textures_.find(texture_id));
-
-    // Any value > kHandleLockedStart represents a locked texture. As we
-    // increment this value with each lock, we need the entire range and can't
-    // add additional values > kHandleLockedStart in the future.
-    EXPECT_GE(textures_[texture_id], kHandleLockedStart);
-    EXPECT_LE(textures_[texture_id], kHandleLockedEnd);
-  }
-
   const int32_t kHandleDeleted = 0;
   const int32_t kHandleUnlocked = 1;
   const int32_t kHandleLockedStart = 2;
@@ -2617,6 +2618,103 @@ TEST_P(GpuImageDecodeCacheTest, MipsAddedSubsequentDraw) {
   }
 }
 
+TEST_P(GpuImageDecodeCacheTest, MipsAddedWhileOriginalInUse) {
+#if defined(OS_WIN)
+  // TODO(ericrk): Mips are temporarily disabled to investigate a memory
+  // regression on Windows. https://crbug.com/867468
+  return;
+#endif  // defined(OS_WIN)
+
+  auto cache = CreateCache();
+  bool is_decomposable = true;
+  auto filter_quality = kMedium_SkFilterQuality;
+
+  PaintImage image = CreateDiscardablePaintImage(gfx::Size(100, 100));
+
+  struct Decode {
+    DrawImage image;
+    DecodedDrawImage decoded_image;
+  };
+  std::vector<Decode> images_to_unlock;
+
+  // Create an image with no scaling. It will not have mips.
+  {
+    DrawImage draw_image(
+        image, SkIRect::MakeWH(image.width(), image.height()), filter_quality,
+        CreateMatrix(SkSize::Make(1.0f, 1.0f), is_decomposable),
+        PaintImage::kDefaultFrameIndex, DefaultColorSpace());
+    ImageDecodeCache::TaskResult result = cache->GetTaskForImageAndRef(
+        draw_image, ImageDecodeCache::TracingInfo());
+    EXPECT_TRUE(result.need_unref);
+    EXPECT_TRUE(result.task);
+
+    TestTileTaskRunner::ProcessTask(result.task->dependencies()[0].get());
+    TestTileTaskRunner::ProcessTask(result.task.get());
+
+    // Must hold context lock before calling GetDecodedImageForDraw /
+    // DrawWithImageFinished.
+    viz::ContextProvider::ScopedContextLock context_lock(context_provider());
+    DecodedDrawImage decoded_draw_image =
+        EnsureImageBacked(cache->GetDecodedImageForDraw(draw_image));
+    EXPECT_TRUE(decoded_draw_image.image());
+    EXPECT_TRUE(decoded_draw_image.image()->isTextureBacked());
+
+    // No mips should be generated
+    sk_sp<SkImage> image_with_mips =
+        decoded_draw_image.image()->makeTextureImage(
+            context_provider()->GrContext(), nullptr, GrMipMapped::kYes);
+    EXPECT_NE(image_with_mips, decoded_draw_image.image());
+
+    images_to_unlock.push_back({draw_image, decoded_draw_image});
+  }
+
+  // Second decode with mips.
+  {
+    DrawImage draw_image(
+        image, SkIRect::MakeWH(image.width(), image.height()), filter_quality,
+        CreateMatrix(SkSize::Make(0.6f, 0.6f), is_decomposable),
+        PaintImage::kDefaultFrameIndex, DefaultColorSpace());
+    ImageDecodeCache::TaskResult result = cache->GetTaskForImageAndRef(
+        draw_image, ImageDecodeCache::TracingInfo());
+    EXPECT_TRUE(result.need_unref);
+    EXPECT_FALSE(result.task);
+
+    // Must hold context lock before calling GetDecodedImageForDraw /
+    // DrawWithImageFinished.
+    viz::ContextProvider::ScopedContextLock context_lock(context_provider());
+    DecodedDrawImage decoded_draw_image =
+        EnsureImageBacked(cache->GetDecodedImageForDraw(draw_image));
+
+    EXPECT_TRUE(decoded_draw_image.image());
+    EXPECT_TRUE(decoded_draw_image.image()->isTextureBacked());
+
+    // Mips should be generated
+    sk_sp<SkImage> image_with_mips =
+        decoded_draw_image.image()->makeTextureImage(
+            context_provider()->GrContext(), nullptr, GrMipMapped::kYes);
+    EXPECT_EQ(image_with_mips, decoded_draw_image.image());
+
+    images_to_unlock.push_back({draw_image, decoded_draw_image});
+  }
+
+  // Reduce cache usage to make sure anything marked for deletion is actually
+  // deleted.
+  cache->ReduceCacheUsage();
+
+  {
+    // All images which are currently ref-ed must have locked textures.
+    viz::ContextProvider::ScopedContextLock context_lock(context_provider());
+    for (const auto& decode : images_to_unlock) {
+      if (!use_transfer_cache_) {
+        discardable_manager_.ExpectLocked(GpuImageDecodeCache::GlIdFromSkImage(
+            decode.decoded_image.image().get()));
+      }
+      cache->DrawWithImageFinished(decode.image, decode.decoded_image);
+      cache->UnrefImage(decode.image);
+    }
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(
     GpuImageDecodeCacheTests,
     GpuImageDecodeCacheTest,

gpu/command_buffer/service/gles2_cmd_decoder.cc

@@ -24,6 +24,7 @@
 #include "base/containers/queue.h"
 #include "base/containers/span.h"
 #include "base/debug/alias.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/ranges.h"
@@ -19838,6 +19839,8 @@ error::Error GLES2DecoderImpl::HandleLockDiscardableTextureCHROMIUM(
   GLuint texture_id = c.texture_id;
   if (!GetContextGroup()->discardable_manager()->LockTexture(
           texture_id, group_->texture_manager())) {
+    // Temporarily log a crash dump for debugging crbug.com/870317.
+    base::debug::DumpWithoutCrashing();
     LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, "glLockDiscardableTextureCHROMIUM",
                        "Texture ID not initialized");
   }