Test: deleting RTM before BVR does not leak resources.

android_webview/browser/browser_view_renderer_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <map>
 #include <queue>
 #include <utility>
 
 #include "android_webview/browser/browser_view_renderer.h"
 #include "android_webview/browser/child_frame.h"
 #include "android_webview/browser/compositor_frame_consumer.h"
+#include "android_webview/browser/render_thread_manager.h"
 #include "android_webview/browser/test/rendering_test.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "cc/output/compositor_frame.h"
 #include "content/public/test/test_synchronous_compositor_android.h"
 
 namespace android_webview {
 
 class SmokeTest : public RenderingTest {
   void StartTest() override { browser_view_renderer_->PostInvalidate(); }
 
-  void DidDrawOnRT(RenderThreadManager* functor) override {
-    EndTest();
-  }
+  void DidDrawOnRT() override { EndTest(); }
 };
 
 RENDERING_TEST_F(SmokeTest);
 
 class ClearViewTest : public RenderingTest {
  public:
   ClearViewTest() : on_draw_count_(0) {}
 
   void StartTest() override {
     browser_view_renderer_->PostInvalidate();
     browser_view_renderer_->ClearView();
   }
 
   void DidOnDraw(bool success) override {
     on_draw_count_++;
     if (on_draw_count_ == 1) {
       // First OnDraw should be skipped due to ClearView.
       EXPECT_FALSE(success);
       browser_view_renderer_->DidUpdateContent();  // Unset ClearView.
       browser_view_renderer_->PostInvalidate();
     } else {
       // Following OnDraws should succeed.
       EXPECT_TRUE(success);
     }
   }
 
-  void DidDrawOnRT(RenderThreadManager* functor) override {
-    EndTest();
-  }
+  void DidDrawOnRT() override { EndTest(); }
+
  private:
   int on_draw_count_;
 };
 
 RENDERING_TEST_F(ClearViewTest);
 
 class TestAnimateInAndOutOfScreen : public RenderingTest {
  public:
   TestAnimateInAndOutOfScreen() : on_draw_count_(0), draw_gl_count_on_rt_(0) {}
 
@@ skipping 15 matching lines @@
     // draw constraints of BVR is updated to initial constraints.
     if (on_draw_count_ == 1 || on_draw_count_ == 2)
       browser_view_renderer_->PrepareToDraw(gfx::Vector2d(), gfx::Rect());
   }
 
   void DidOnDraw(bool success) override {
     EXPECT_TRUE(success);
     on_draw_count_++;
   }
 
-  bool WillDrawOnRT(RenderThreadManager* functor,
-                    AwDrawGLInfo* draw_info) override {
+  bool WillDrawOnRT(AwDrawGLInfo* draw_info) override {
     if (draw_gl_count_on_rt_ == 1) {
       draw_gl_count_on_rt_++;
       ui_task_runner_->PostTask(
           FROM_HERE,
           base::Bind(&RenderingTest::PostInvalidate, base::Unretained(this)));
       return false;
     }
 
     draw_info->width = window_->surface_size().width();
     draw_info->height = window_->surface_size().height();
     draw_info->is_layer = false;
 
     gfx::Transform transform;
     if (draw_gl_count_on_rt_ == 0)
       transform = new_constraints_.transform;
 
     transform.matrix().asColMajorf(draw_info->transform);
     return true;
   }
 
-  void DidDrawOnRT(RenderThreadManager* functor) override {
-    draw_gl_count_on_rt_++;
-  }
+  void DidDrawOnRT() override { draw_gl_count_on_rt_++; }
 
   bool DrawConstraintsEquals(
       const ParentCompositorDrawConstraints& constraints1,
       const ParentCompositorDrawConstraints& constraints2) {
     if (constraints1.is_layer != constraints2.is_layer ||
         constraints1.transform != constraints2.transform)
       return false;
 
     return !constraints1.is_layer ||
            constraints1.surface_rect_empty == constraints2.surface_rect_empty;
@@ skipping 62 matching lines @@
     }
     on_draw_count_++;
   }
 
  private:
   int on_draw_count_;
 };
 
 RENDERING_TEST_F(CompositorNoFrameTest);
 
-class SwitchOutputSurfaceIdTest : public RenderingTest {
+class ResourceRenderingTest : public RenderingTest {
  public:
+  using ResourceCountMap = std::map<cc::ResourceId, int>;
+  using OutputSurfaceResourceCountMap = std::map<uint32_t, ResourceCountMap>;
+
+  virtual std::unique_ptr<content::SynchronousCompositor::Frame> GetFrame(
+      int frame_number) = 0;
+
+  void StartTest() override {
+    frame_number_ = 0;
+    AdvanceFrame();
+  }
+
+  void WillOnDraw() override {
+    if (next_frame_) {
+      compositor_->SetHardwareFrame(next_frame_->output_surface_id,
+                                    std::move(next_frame_->frame));
+    }
+  }
+
+  void DidOnDraw(bool success) override {
+    EXPECT_EQ(next_frame_ != nullptr, success);
+    if (!AdvanceFrame()) {
+      ui_task_runner_->PostTask(FROM_HERE,
+                                base::Bind(&ResourceRenderingTest::CheckResults,
+                                           base::Unretained(this)));
+    }
+  }
+
+  OutputSurfaceResourceCountMap GetReturnedResourceCounts() {
+    OutputSurfaceResourceCountMap counts;
+    content::TestSynchronousCompositor::FrameAckArray returned_resources_array;
+    compositor_->SwapReturnedResources(&returned_resources_array);
+    for (const auto& resources : returned_resources_array) {
+      for (const auto& returned_resource : resources.resources) {
+        counts[resources.output_surface_id][returned_resource.id] +=
+            returned_resource.count;
+      }
+    }
+    return counts;
+  }
+
+  virtual void CheckResults() = 0;
+
+ private:
+  std::unique_ptr<content::SynchronousCompositor::Frame> next_frame_;
+  int frame_number_;
+};
+
+class SwitchOutputSurfaceIdTest : public ResourceRenderingTest {
   struct FrameInfo {
     uint32_t output_surface_id;
     cc::ResourceId resource_id;  // Each frame contains a single resource.
   };
 
-  void StartTest() override {
-    last_output_surface_id_ = 0;
-    FrameInfo infos[] = {
+  std::unique_ptr<content::SynchronousCompositor::Frame> GetFrame(
+      int frame_number) override {
+    static const FrameInfo infos[] = {
         // First output surface.
         {0u, 1u}, {0u, 1u}, {0u, 2u}, {0u, 2u}, {0u, 3u}, {0u, 3u}, {0u, 4u},
         // Second output surface.
         {1u, 1u}, {1u, 1u}, {1u, 2u}, {1u, 2u}, {1u, 3u}, {1u, 3u}, {1u, 4u},
     };
-    for (const auto& info : infos) {
-      content::SynchronousCompositor::Frame frame;
-      frame.output_surface_id = info.output_surface_id;
-      frame.frame = ConstructEmptyFrame();
-      cc::TransferableResource resource;
-      resource.id = info.resource_id;
-      frame.frame->delegated_frame_data->resource_list.push_back(resource);
-      frames_.push(std::move(frame));
-
-      // Keep a id -> count map for the last ouptut_surface_id.
-      if (last_output_surface_id_ != info.output_surface_id) {
-        expected_return_count_.clear();
-        last_output_surface_id_ = info.output_surface_id;
-      }
-      if (expected_return_count_.count(info.resource_id)) {
-        expected_return_count_[info.resource_id]++;
-      } else {
-        expected_return_count_[info.resource_id] = 1;
-      }
+    if (frame_number >= static_cast<int>(arraysize(infos))) {
+      return nullptr;
     }
 
-    browser_view_renderer_->PostInvalidate();
+    std::unique_ptr<content::SynchronousCompositor::Frame> frame(
+        new content::SynchronousCompositor::Frame);
+    frame->output_surface_id = infos[frame_number].output_surface_id;
+    frame->frame = ConstructFrame(infos[frame_number].resource_id);
+
+    if (last_output_surface_id_ != infos[frame_number].output_surface_id) {
+      expected_return_count_.clear();
+      last_output_surface_id_ = infos[frame_number].output_surface_id;
+    }
+    ++expected_return_count_[infos[frame_number].resource_id];
+    return frame;
   }
 
-  void WillOnDraw() override {
-    if (!frames_.empty()) {
-      compositor_->SetHardwareFrame(frames_.front().output_surface_id,
-                                    std::move(frames_.front().frame));
-    }
+  void StartTest() override {
+    last_output_surface_id_ = -1U;
+    ResourceRenderingTest::StartTest();
   }
 
-  void DidOnDraw(bool success) override {
-    EXPECT_TRUE(success);
-    if (frames_.empty()) {
-      ui_task_runner_->PostTask(
-          FROM_HERE, base::Bind(&SwitchOutputSurfaceIdTest::CheckResults,
-                                base::Unretained(this)));
-    } else {
-      frames_.pop();
-      browser_view_renderer_->PostInvalidate();
-    }
-  }
-
-  void CheckResults() {
+  void CheckResults() override {
+    GetCompositorFrameConsumer()->DeleteHardwareRendererOnUI();
     window_->Detach();
     window_.reset();
 
     // Make sure resources for the last output surface are returned.
-    content::TestSynchronousCompositor::FrameAckArray returned_resources_array;
-    compositor_->SwapReturnedResources(&returned_resources_array);
-    for (const auto& resources : returned_resources_array) {
-      if (resources.output_surface_id != last_output_surface_id_)
-        continue;
-      for (const auto& returned_resource : resources.resources) {
-        EXPECT_TRUE(!!expected_return_count_.count(returned_resource.id));
-        EXPECT_GE(expected_return_count_[returned_resource.id],
-                  returned_resource.count);
-        expected_return_count_[returned_resource.id] -=
-            returned_resource.count;
-        if (!expected_return_count_[returned_resource.id])
-          expected_return_count_.erase(returned_resource.id);
-      }
-    }
-    EXPECT_TRUE(expected_return_count_.empty());
-
+    EXPECT_EQ(expected_return_count_,
+              GetReturnedResourceCounts()[last_output_surface_id_]);
     EndTest();
   }
 
  private:
-  std::queue<content::SynchronousCompositor::Frame> frames_;
+  bool AdvanceFrame() {

[boliu, 2016/04/26 18:04:52]

Wrong class? Does this compile?

[Tobias Sargeant, 2016/04/26 22:48:06]

Yep, stupid mistake. Sorry for wasting your time.

+    next_frame_ = GetFrame(frame_number_++);
+    if (next_frame_) {
+      browser_view_renderer_->PostInvalidate();
+      return true;
+    }
+    return false;
+  }
+
   uint32_t last_output_surface_id_;
-  std::map<cc::ResourceId, int> expected_return_count_;
+  ResourceCountMap expected_return_count_;
 };
 
 RENDERING_TEST_F(SwitchOutputSurfaceIdTest);
 
+class RenderThreadManagerDeletionTest : public ResourceRenderingTest {
+  std::unique_ptr<content::SynchronousCompositor::Frame> GetFrame(
+      int frame_number) override {
+    if (frame_number > 0) {
+      return nullptr;
+    }
+
+    std::unique_ptr<content::SynchronousCompositor::Frame> frame(
+        new content::SynchronousCompositor::Frame);
+    frame->output_surface_id = 0U;
+    frame->frame = ConstructFrame(static_cast<cc::ResourceId>(frame_number));
+    return frame;
+  }
+
+  void CheckResults() override {
+    OutputSurfaceResourceCountMap resource_counts;
+    render_thread_manager_.reset();
+    // Make sure resources for the last frame are returned.
+    resource_counts = GetReturnedResourceCounts();
+    OutputSurfaceResourceCountMap expected;
+    expected[0U][static_cast<cc::ResourceId>(0)] = 1;

[boliu, 2016/04/26 18:04:52]

this is too far away from what it should match, ideally the expected would also
be constructed from GetFrame,

also improve readability: like untin32_t output_surface_id = 0u, instead of
random constants that make no sense on first glance

[Tobias Sargeant, 2016/04/26 22:48:06]

Done.

+    EXPECT_EQ(expected, resource_counts);
+    EndTest();
+  }
+};
+
+RENDERING_TEST_F(RenderThreadManagerDeletionTest);
+
 }  // namespace android_webview