gpu: Fix oop raster font cache memory leak.

The code for tracking locked font handles on the service currently
assumes that only one DoRasterCHROMIUM command is interleaved between a
Begin/EndRaster sequence, which is an incorrect assumption. As a result
since we clear the vector of locked handles in each DoRasterCHROMIUM,
this leaks ref-counts if multiple DoRasterCHROMIUM commands are present
between a single Begin/EndRaster sequence.

Fix this by appending ref-counts on the service instead of over-writing
the current list.

R=piman@chromium.org

Bug: 829622
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: I15025153fe887e712e0879cbeb32944c3e258e92
Reviewed-on: https://chromium-review.googlesource.com/1077819
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563092}

cc/paint/oop_pixeltest.cc

@@ -8,6 +8,7 @@
 #include "base/command_line.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "cc/base/completion_event.h"
 #include "cc/base/region.h"
 #include "cc/layers/recording_source.h"
 #include "cc/paint/display_item_list.h"
@@ -28,6 +29,7 @@
 #include "gpu/ipc/gl_in_process_context.h"
 #include "gpu/skia_bindings/grcontext_for_gles2_interface.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkGraphics.h"
 #include "third_party/skia/include/core/SkSurface.h"
 #include "third_party/skia/include/gpu/GrBackendSurface.h"
 #include "third_party/skia/include/gpu/GrContext.h"
@@ -99,6 +101,7 @@ class OopPixelTest : public testing::Test {
     bool preclear = false;
     SkColor preclear_color;
     ImageDecodeCache* image_cache = nullptr;
+    std::vector<scoped_refptr<DisplayItemList>> additional_lists;
   };
 
   SkBitmap Raster(scoped_refptr<DisplayItemList> display_item_list,
@@ -151,6 +154,12 @@ class OopPixelTest : public testing::Test {
         display_item_list.get(), &image_provider, options.content_size,
         options.full_raster_rect, options.playback_rect, options.post_translate,
         options.post_scale, options.requires_clear);
+    for (const auto& list : options.additional_lists) {
+      raster_implementation->RasterCHROMIUM(
+          list.get(), &image_provider, options.content_size,
+          options.full_raster_rect, options.playback_rect,
+          options.post_translate, options.post_scale, options.requires_clear);
+    }
     raster_implementation->EndRasterCHROMIUM();
 
     // Produce a mailbox and insert an ordering barrier (assumes the raster
@@ -1200,10 +1209,12 @@ TEST_F(OopPixelTest, DrawRectColorSpace) {
   ExpectEquals(actual, expected);
 }
 
-scoped_refptr<PaintTextBlob> buildTextBlob() {
+scoped_refptr<PaintTextBlob> BuildTextBlob(
+    PaintTypeface typeface = PaintTypeface()) {
   SkFontStyle style;
-  PaintTypeface typeface =
-      PaintTypeface::FromFamilyNameAndFontStyle("monospace", style);
+  if (!typeface) {
+    typeface = PaintTypeface::FromFamilyNameAndFontStyle("monospace", style);
+  }
 
   PaintFont font;
   font.SetTypeface(typeface);
@@ -1235,7 +1246,7 @@ TEST_F(OopPixelTest, DrawTextBlob) {
   PaintFlags flags;
   flags.setStyle(PaintFlags::kFill_Style);
   flags.setColor(SK_ColorGREEN);
-  display_item_list->push<DrawTextBlobOp>(buildTextBlob(), 0u, 0u, flags);
+  display_item_list->push<DrawTextBlobOp>(BuildTextBlob(), 0u, 0u, flags);
   display_item_list->EndPaintOfUnpaired(options.full_raster_rect);
   display_item_list->Finalize();
 
@@ -1256,7 +1267,7 @@ TEST_F(OopPixelTest, DrawRecordShaderWithTextScaled) {
   PaintFlags flags;
   flags.setStyle(PaintFlags::kFill_Style);
   flags.setColor(SK_ColorGREEN);
-  paint_record->push<DrawTextBlobOp>(buildTextBlob(), 0u, 0u, flags);
+  paint_record->push<DrawTextBlobOp>(BuildTextBlob(), 0u, 0u, flags);
   auto paint_record_shader = PaintShader::MakePaintRecord(
       paint_record, SkRect::MakeWH(25, 25), SkShader::kRepeat_TileMode,
       SkShader::kRepeat_TileMode, nullptr);
@@ -1287,7 +1298,7 @@ TEST_F(OopPixelTest, DrawRecordFilterWithTextScaled) {
   PaintFlags flags;
   flags.setStyle(PaintFlags::kFill_Style);
   flags.setColor(SK_ColorGREEN);
-  paint_record->push<DrawTextBlobOp>(buildTextBlob(), 0u, 0u, flags);
+  paint_record->push<DrawTextBlobOp>(BuildTextBlob(), 0u, 0u, flags);
   auto paint_record_filter =
       sk_make_sp<RecordPaintFilter>(paint_record, SkRect::MakeWH(100, 100));
 
@@ -1305,6 +1316,57 @@ TEST_F(OopPixelTest, DrawRecordFilterWithTextScaled) {
   ExpectEquals(actual, expected);
 }
 
+void ClearFontCache(CompletionEvent* event) {
+  SkGraphics::PurgeFontCache();
+  event->Signal();
+}
+
+TEST_F(OopPixelTest, DrawTextMultipleRasterCHROMIUM) {
+  RasterOptions options;
+  options.resource_size = gfx::Size(100, 100);
+  options.content_size = options.resource_size;
+  options.full_raster_rect = gfx::Rect(options.content_size);
+  options.playback_rect = options.full_raster_rect;
+  options.color_space = gfx::ColorSpace::CreateSRGB();
+
+  auto sk_typeface_1 = SkTypeface::MakeFromName("monospace", SkFontStyle());
+  auto sk_typeface_2 = SkTypeface::MakeFromName("roboto", SkFontStyle());
+
+  auto display_item_list = base::MakeRefCounted<DisplayItemList>();
+  display_item_list->StartPaint();
+  PaintFlags flags;
+  flags.setStyle(PaintFlags::kFill_Style);
+  flags.setColor(SK_ColorGREEN);
+  display_item_list->push<DrawTextBlobOp>(
+      BuildTextBlob(PaintTypeface::FromSkTypeface(sk_typeface_1)), 0u, 0u,
+      flags);
+  display_item_list->EndPaintOfUnpaired(options.full_raster_rect);
+  display_item_list->Finalize();
+
+  // Create another list with a different typeface.
+  auto display_item_list_2 = base::MakeRefCounted<DisplayItemList>();
+  display_item_list_2->StartPaint();
+  display_item_list_2->push<DrawTextBlobOp>(
+      BuildTextBlob(PaintTypeface::FromSkTypeface(sk_typeface_2)), 0u, 0u,
+      flags);
+  display_item_list_2->EndPaintOfUnpaired(options.full_raster_rect);
+  display_item_list_2->Finalize();
+
+  // Raster both these lists with 2 RasterCHROMIUM commands between a single
+  // Begin/EndRaster sequence.
+  options.additional_lists = {display_item_list_2};
+  Raster(display_item_list, options);
+
+  // Clear skia's font cache. No entries should remain since the service
+  // should unpin everything.
+  EXPECT_GT(SkGraphics::GetFontCacheUsed(), 0u);
+  CompletionEvent event;
+  raster_context_provider_->ExecuteOnGpuThread(
+      base::BindOnce(&ClearFontCache, &event));
+  event.Wait();
+  EXPECT_EQ(SkGraphics::GetFontCacheUsed(), 0u);
+}
+
 INSTANTIATE_TEST_CASE_P(P, OopImagePixelTest, ::testing::Bool());
 
 }  // namespace

cc/test/test_in_process_context_provider.cc

@@ -172,4 +172,11 @@ const gpu::GpuFeatureInfo& TestInProcessContextProvider::GetGpuFeatureInfo()
   return gpu_feature_info_;
 }
 
+void TestInProcessContextProvider::ExecuteOnGpuThread(base::OnceClosure task) {
+  DCHECK(raster_context_);
+  raster_context_->GetCommandBufferForTest()
+      ->service_for_testing()
+      ->ScheduleTask(std::move(task));
+}
+
 }  // namespace cc

cc/test/test_in_process_context_provider.h

@@ -54,6 +54,8 @@ class TestInProcessContextProvider
   void AddObserver(viz::ContextLostObserver* obs) override {}
   void RemoveObserver(viz::ContextLostObserver* obs) override {}
 
+  void ExecuteOnGpuThread(base::OnceClosure task);
+
  protected:
   friend class base::RefCountedThreadSafe<TestInProcessContextProvider>;
   ~TestInProcessContextProvider() override;

gpu/command_buffer/service/raster_decoder.cc

@@ -3058,12 +3058,15 @@ void RasterDecoderImpl::DoRasterCHROMIUM(GLuint raster_shm_id,
       return;
     }
 
+    std::vector<SkDiscardableHandleId> new_locked_handles;
     if (!font_manager_.Deserialize(font_buffer_memory, font_shm_size,
-                                   &locked_handles_)) {
+                                   &new_locked_handles)) {
       LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, "glRasterCHROMIUM",
                          "Invalid font buffer.");
       return;
     }
+    locked_handles_.insert(locked_handles_.end(), new_locked_handles.begin(),
+                           new_locked_handles.end());
   }
 
   char* paint_buffer_memory = GetSharedMemoryAs<char*>(

gpu/command_buffer/service/service_font_manager.cc

@@ -86,16 +86,22 @@ class ServiceFontManager::SkiaDiscardableManager
 
 ServiceFontManager::ServiceFontManager(Client* client)
     : client_(client), weak_factory_(this) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   strike_client_ = std::make_unique<SkStrikeClient>(
       sk_make_sp<SkiaDiscardableManager>(weak_factory_.GetWeakPtr()));
 }
 
-ServiceFontManager::~ServiceFontManager() = default;
+ServiceFontManager::~ServiceFontManager() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+}
 
 bool ServiceFontManager::Deserialize(
     const volatile char* memory,
     size_t memory_size,
     std::vector<SkDiscardableHandleId>* locked_handles) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(locked_handles->empty());
+
   // All new handles.
   Deserializer deserializer(memory, memory_size);
   size_t new_handles_created;
@@ -143,6 +149,8 @@ bool ServiceFontManager::Deserialize(
 
 bool ServiceFontManager::AddHandle(SkDiscardableHandleId handle_id,
                                    ServiceDiscardableHandle handle) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   if (discardable_handle_map_.find(handle_id) != discardable_handle_map_.end())
     return false;
   discardable_handle_map_[handle_id] = std::move(handle);
@@ -151,6 +159,8 @@ bool ServiceFontManager::AddHandle(SkDiscardableHandleId handle_id,
 
 bool ServiceFontManager::Unlock(
     const std::vector<SkDiscardableHandleId>& handles) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   for (auto handle_id : handles) {
     auto it = discardable_handle_map_.find(handle_id);
     if (it == discardable_handle_map_.end())
@@ -161,6 +171,8 @@ bool ServiceFontManager::Unlock(
 }
 
 bool ServiceFontManager::DeleteHandle(SkDiscardableHandleId handle_id) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
   auto it = discardable_handle_map_.find(handle_id);
   if (it == discardable_handle_map_.end()) {
     LOG(ERROR) << "Tried to delete invalid SkDiscardableHandleId: "

gpu/command_buffer/service/service_font_manager.h

@@ -7,6 +7,7 @@
 
 #include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "gpu/command_buffer/common/discardable_handle.h"
 #include "gpu/gpu_gles2_export.h"
 #include "third_party/skia/src/core/SkRemoteGlyphCache.h"
@@ -42,6 +43,7 @@ class GPU_GLES2_EXPORT ServiceFontManager {
   std::unique_ptr<SkStrikeClient> strike_client_;
   base::flat_map<SkDiscardableHandleId, ServiceDiscardableHandle>
       discardable_handle_map_;
+  THREAD_CHECKER(thread_checker_);
   base::WeakPtrFactory<ServiceFontManager> weak_factory_;
 };
 

gpu/ipc/in_process_command_buffer.h

@@ -261,6 +261,8 @@ class GL_IN_PROCESS_CONTEXT_EXPORT InProcessCommandBuffer
     gles2::FramebufferCompletenessCache framebuffer_completeness_cache_;
   };
 
+  Service* service_for_testing() const { return service_.get(); }
+
  private:
   struct InitializeOnGpuThreadParams {
     bool is_offscreen;

gpu/ipc/raster_in_process_context.cc

@@ -125,4 +125,9 @@ ServiceTransferCache* RasterInProcessContext::GetTransferCacheForTest() const {
   return command_buffer_->GetTransferCacheForTest();
 }
 
+InProcessCommandBuffer* RasterInProcessContext::GetCommandBufferForTest()
+    const {
+  return command_buffer_.get();
+}
+
 }  // namespace gpu

gpu/ipc/raster_in_process_context.h

@@ -55,6 +55,7 @@ class RasterInProcessContext {
 
   // Test only functions.
   ServiceTransferCache* GetTransferCacheForTest() const;
+  InProcessCommandBuffer* GetCommandBufferForTest() const;
 
  private:
   std::unique_ptr<CommandBufferHelper> helper_;