cc: Don't create cached skia objects in paint objects lazily.

For PaintShader and PaintImage, we lazily create the internally cached
SkShader and SkImage. This can cause this access to be racy when later
done on the raster worker threads during playback. Ensure that these
objects are created during Paint object construction.

R=enne@chromium.org, vmpstr@chromium.org

Bug: 771156
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I8538442e0459e1762a81d5fb150301621360d850
Reviewed-on: https://chromium-review.googlesource.com/701974Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506792}

cc/paint/paint_image.cc

@@ -50,26 +50,6 @@ PaintImage::ContentId PaintImage::GetNextContentId() {
 }
 
 const sk_sp<SkImage>& PaintImage::GetSkImage() const {
-  if (cached_sk_image_)
-    return cached_sk_image_;
-
-  if (sk_image_) {
-    cached_sk_image_ = sk_image_;
-  } else if (paint_record_) {
-    cached_sk_image_ = SkImage::MakeFromPicture(
-        ToSkPicture(paint_record_, gfx::RectToSkRect(paint_record_rect_)),
-        SkISize::Make(paint_record_rect_.width(), paint_record_rect_.height()),
-        nullptr, nullptr, SkImage::BitDepth::kU8, SkColorSpace::MakeSRGB());
-  } else if (paint_image_generator_) {
-    cached_sk_image_ =
-        SkImage::MakeFromGenerator(base::MakeUnique<SkiaPaintImageGenerator>(
-            paint_image_generator_, frame_index_));
-  }
-
-  if (!subset_rect_.IsEmpty() && cached_sk_image_) {
-    cached_sk_image_ =
-        cached_sk_image_->makeSubset(gfx::RectToSkIRect(subset_rect_));
-  }
   return cached_sk_image_;
 }
 
@@ -89,16 +69,36 @@ PaintImage PaintImage::MakeSubset(const gfx::Rect& subset) const {
   // Store the subset from the original image.
   result.subset_rect_.Offset(subset_rect_.x(), subset_rect_.y());
 
-  // Creating the |cached_sk_image_| is an optimization to allow re-use of the
-  // original decode for image subsets in skia, for cases that rely on skia's
-  // image decode cache.
-  // TODO(khushalsagar): Remove this when we no longer have such cases. See
-  // crbug.com/753639.
+  // Creating the |cached_sk_image_| using the SkImage from the original
+  // PaintImage is an optimization to allow re-use of the original decode for
+  // image subsets in skia, for cases that rely on skia's image decode cache.
   result.cached_sk_image_ =
       GetSkImage()->makeSubset(gfx::RectToSkIRect(subset));
   return result;
 }
 
+void PaintImage::CreateSkImage() {
+  DCHECK(!cached_sk_image_);
+
+  if (sk_image_) {
+    cached_sk_image_ = sk_image_;
+  } else if (paint_record_) {
+    cached_sk_image_ = SkImage::MakeFromPicture(
+        ToSkPicture(paint_record_, gfx::RectToSkRect(paint_record_rect_)),
+        SkISize::Make(paint_record_rect_.width(), paint_record_rect_.height()),
+        nullptr, nullptr, SkImage::BitDepth::kU8, SkColorSpace::MakeSRGB());
+  } else if (paint_image_generator_) {
+    cached_sk_image_ =
+        SkImage::MakeFromGenerator(base::MakeUnique<SkiaPaintImageGenerator>(
+            paint_image_generator_, frame_index_));
+  }
+
+  if (!subset_rect_.IsEmpty() && cached_sk_image_) {
+    cached_sk_image_ =
+        cached_sk_image_->makeSubset(gfx::RectToSkIRect(subset_rect_));
+  }
+}
+
 PaintImage PaintImage::MakeStatic() const {
   PaintImage result(*this);
   result.repetition_count_ = kAnimationNone;

cc/paint/paint_image.h

@@ -171,6 +171,8 @@ class CC_PAINT_EXPORT PaintImage {
                          sk_sp<SkColorSpace> color_space,
                          size_t frame_index) const;
 
+  void CreateSkImage();
+
   sk_sp<SkImage> sk_image_;
 
   sk_sp<PaintRecord> paint_record_;
@@ -200,7 +202,7 @@ class CC_PAINT_EXPORT PaintImage {
   // recording with a PaintImage storing the updated sequence id.
   AnimationSequenceId reset_animation_sequence_id_ = 0u;
 
-  mutable sk_sp<SkImage> cached_sk_image_;
+  sk_sp<SkImage> cached_sk_image_;
 };
 
 }  // namespace cc

cc/paint/paint_image_builder.cc

@@ -20,7 +20,7 @@ PaintImageBuilder::PaintImageBuilder(PaintImage image)
 }
 PaintImageBuilder::~PaintImageBuilder() = default;
 
-PaintImage PaintImageBuilder::TakePaintImage() const {
+PaintImage PaintImageBuilder::TakePaintImage() {
 #if DCHECK_IS_ON()
   DCHECK(id_set_);
   if (paint_image_.sk_image_) {
@@ -52,6 +52,7 @@ PaintImage PaintImageBuilder::TakePaintImage() const {
   }
 #endif
 
+  paint_image_.CreateSkImage();
   return std::move(paint_image_);
 }
 

cc/paint/paint_image_builder.h

@@ -79,7 +79,7 @@ class CC_PAINT_EXPORT PaintImageBuilder {
     return *this;
   }
 
-  PaintImage TakePaintImage() const;
+  PaintImage TakePaintImage();
 
  private:
   PaintImage paint_image_;

cc/paint/paint_shader.cc

@@ -16,6 +16,7 @@ sk_sp<PaintShader> PaintShader::MakeColor(SkColor color) {
   // Just one color. Store it in the fallback color. Easy.
   shader->fallback_color_ = color;
 
+  shader->CreateSkShader();
   return shader;
 }
 
@@ -36,6 +37,7 @@ sk_sp<PaintShader> PaintShader::MakeLinearGradient(const SkPoint points[],
   shader->SetMatrixAndTiling(local_matrix, mode, mode);
   shader->SetFlagsAndFallback(flags, fallback_color);
 
+  shader->CreateSkShader();
   return shader;
 }
 
@@ -56,6 +58,7 @@ sk_sp<PaintShader> PaintShader::MakeRadialGradient(const SkPoint& center,
   shader->SetMatrixAndTiling(local_matrix, mode, mode);
   shader->SetFlagsAndFallback(flags, fallback_color);
 
+  shader->CreateSkShader();
   return shader;
 }
 
@@ -81,6 +84,7 @@ sk_sp<PaintShader> PaintShader::MakeTwoPointConicalGradient(
   shader->SetMatrixAndTiling(local_matrix, mode, mode);
   shader->SetFlagsAndFallback(flags, fallback_color);
 
+  shader->CreateSkShader();
   return shader;
 }
 
@@ -104,6 +108,7 @@ sk_sp<PaintShader> PaintShader::MakeSweepGradient(SkScalar cx,
   shader->SetMatrixAndTiling(local_matrix, mode, mode);
   shader->SetFlagsAndFallback(flags, fallback_color);
 
+  shader->CreateSkShader();
   return shader;
 }
 
@@ -116,6 +121,7 @@ sk_sp<PaintShader> PaintShader::MakeImage(const PaintImage& image,
   shader->image_ = image;
   shader->SetMatrixAndTiling(local_matrix, tx, ty);
 
+  shader->CreateSkShader();
   return shader;
 }
 
@@ -133,6 +139,7 @@ sk_sp<PaintShader> PaintShader::MakePaintRecord(
   shader->scaling_behavior_ = scaling_behavior;
   shader->SetMatrixAndTiling(local_matrix, tx, ty);
 
+  shader->CreateSkShader();
   return shader;
 }
 
@@ -140,8 +147,11 @@ PaintShader::PaintShader(Type type) : shader_type_(type) {}
 PaintShader::~PaintShader() = default;
 
 sk_sp<SkShader> PaintShader::GetSkShader() const {
-  if (cached_shader_)
   return cached_shader_;
+}
+
+void PaintShader::CreateSkShader() {
+  DCHECK(!cached_shader_);
 
   switch (shader_type_) {
     case Type::kColor:
@@ -214,7 +224,6 @@ sk_sp<SkShader> PaintShader::GetSkShader() const {
   // one.
   if (!cached_shader_)
     cached_shader_ = SkShader::MakeColorShader(fallback_color_);
-  return cached_shader_;
 }
 
 void PaintShader::SetColorsAndPositions(const SkColor* colors,

cc/paint/paint_shader.h

@@ -131,6 +131,7 @@ class CC_PAINT_EXPORT PaintShader : public SkRefCnt {
   explicit PaintShader(Type type);
 
   sk_sp<SkShader> GetSkShader() const;
+  void CreateSkShader();
 
   void SetColorsAndPositions(const SkColor* colors,
                              const SkScalar* positions,
@@ -166,7 +167,7 @@ class CC_PAINT_EXPORT PaintShader : public SkRefCnt {
   std::vector<SkColor> colors_;
   std::vector<SkScalar> positions_;
 
-  mutable sk_sp<SkShader> cached_shader_;
+  sk_sp<SkShader> cached_shader_;
 
   DISALLOW_COPY_AND_ASSIGN(PaintShader);
 };