Test: deleting RTM before BVR does not leak resources.

android_webview/browser/browser_view_renderer_unittest.cc

Index: android_webview/browser/browser_view_renderer_unittest.cc
diff --git a/android_webview/browser/browser_view_renderer_unittest.cc b/android_webview/browser/browser_view_renderer_unittest.cc
index aa7d51872d2e7d638d079bcf07cd12779189bdfb..9917126ef08bd53f7085dfc9f7deffbe2511fe74 100644
--- a/android_webview/browser/browser_view_renderer_unittest.cc
+++ b/android_webview/browser/browser_view_renderer_unittest.cc
@@ -9,6 +9,7 @@
 #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"
@@ -20,9 +21,7 @@ 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);
@@ -49,9 +48,8 @@ class ClearViewTest : public RenderingTest {
     }
   }
 
-  void DidDrawOnRT(RenderThreadManager* functor) override {
-    EndTest();
-  }
+  void DidDrawOnRT() override { EndTest(); }
+
  private:
   int on_draw_count_;
 };
@@ -87,8 +85,7 @@ class TestAnimateInAndOutOfScreen : public RenderingTest {
     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(
@@ -109,9 +106,7 @@ class TestAnimateInAndOutOfScreen : public RenderingTest {
     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,
@@ -194,95 +189,143 @@ class CompositorNoFrameTest : public RenderingTest {
 
 RENDERING_TEST_F(CompositorNoFrameTest);
 
-class SwitchOutputSurfaceIdTest : public RenderingTest {
+class ResourceRenderingTest : public RenderingTest {
  public:
+  typedef std::map<uint32_t, std::map<cc::ResourceId, int>> ResourceCountMap;

[boliu, 2016/04/25 19:56:53]

add some comments, what's the key here again?

[Tobias Sargeant, 2016/04/26 15:29:51]

After adding the aliases, I don't think this is necessary.

+
+  virtual std::unique_ptr<content::SynchronousCompositor::Frame> GetFrame(
+      int frame_number) = 0;
+
+  void StartTest() override {
+    frame_number_ = 0;
+    next_frame_ = GetFrame(frame_number_++);

[boliu, 2016/04/25 19:56:54]

maybe factor these 4 lines in to a helper?

[Tobias Sargeant, 2016/04/26 15:29:51]

Done.

+    if (next_frame_) {
+      browser_view_renderer_->PostInvalidate();
+    }
+  }
+
+  void WillOnDraw() override {
+    compositor_->SetHardwareFrame(next_frame_->output_surface_id,

[boliu, 2016/04/25 19:56:53]

Hmm, might need to handle the next_frame_ being null case:

The UpdateParentDrawConstraints thing calls invalidate too, so might get a
trailing call here. Plus generally not safe to assume test is the only thing
that calls PostInvalidate, at least not the way the harness is currently
written. Hmm... maybe should fix that

+                                  std::move(next_frame_->frame));
+  }
+
+  void DidOnDraw(bool success) override {
+    EXPECT_TRUE(success);
+
+    next_frame_ = GetFrame(frame_number_++);
+    if (next_frame_) {
+      browser_view_renderer_->PostInvalidate();
+    } else {
+      ui_task_runner_->PostTask(FROM_HERE,
+                                base::Bind(&ResourceRenderingTest::CheckResults,
+                                           base::Unretained(this)));
+    }
+  }
+
+  ResourceCountMap GetReturnedResourceCounts() {
+    ResourceCountMap 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];
+      }
+    }
+    return counts;
+  }
+
+  virtual void CheckResults() { EndTest(); }

[boliu, 2016/04/25 19:56:53]

tihs should be = 0 too

[Tobias Sargeant, 2016/04/26 15:29:51]

Done.

+
+ 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;
-      }
+    static const int nFrames = int(sizeof(infos) / sizeof(infos[0]));

[boliu, 2016/04/25 19:56:53]

nFrames is java style

also base has arraysize:
https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ar...

Also also.. just use arraysize where needed, don't need to save as static

[Tobias Sargeant, 2016/04/26 15:29:51]

Done.

I figured there was a define for this somewhere, but I didn't know what it was
called.

+
+    if (frame_number >= nFrames) {
+      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);
 
-  void WillOnDraw() override {
-    if (!frames_.empty()) {
-      compositor_->SetHardwareFrame(frames_.front().output_surface_id,
-                                    std::move(frames_.front().frame));
+    if (last_output_surface_id_ != infos[frame_number].output_surface_id) {
+      expected_return_count_.clear();

[boliu, 2016/04/25 19:56:53]

and then you need to count the current frame

and update last_output_surface_id_

..old code was fine?

[Tobias Sargeant, 2016/04/26 15:29:51]

Oops, yeah. The old code was fine, but refactoring it to create frames as needed
made it look cleaner to me to update this incrementally rather than in
StartTest. Could still do it in StartTest, and just loop over infos there.

+    } else {
+      ++expected_return_count_[infos[frame_number].resource_id];
     }
+    return frame;
   }
 
-  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 StartTest() override {
+    last_output_surface_id_ = -1U;
+    ResourceRenderingTest::StartTest();
   }
 
-  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_]);

[Tobias Sargeant, 2016/04/25 17:00:20]

Is it ok to do this? I suspect that if it fails, the log message won't be as
informative as comparing element-wise.

[boliu, 2016/04/25 19:56:54]

should be fine, previous thing wasn't that useful either

[Tobias Sargeant, 2016/04/26 15:29:51]

Actually, it gives really good output:

C   63.323s Main  Value of: GetReturnedResourceCounts()[last_output_surface_id_]
C   63.323s Main    Actual: { (1, 1), (2, 1), (3, 1), (4, 1) }
C   63.323s Main  Expected: expected_return_count_
C   63.323s Main  Which is: { (1, 2), (2, 2), (3, 2), (4, 1) }

     EndTest();
   }
 
  private:
-  std::queue<content::SynchronousCompositor::Frame> frames_;
   uint32_t last_output_surface_id_;
   std::map<cc::ResourceId, int> expected_return_count_;

[boliu, 2016/04/25 19:56:53]

alias this

[Tobias Sargeant, 2016/04/26 15:29:51]

Done.

 };
 
 RENDERING_TEST_F(SwitchOutputSurfaceIdTest);
 
+class RenderThreadManagerDeletionTest : public ResourceRenderingTest {
+  std::unique_ptr<content::SynchronousCompositor::Frame> GetFrame(
+      int frame_number) override {
+    if (frame_number >= 2) {
+      return nullptr;
+    }
+
+    std::unique_ptr<content::SynchronousCompositor::Frame> frame(
+        new content::SynchronousCompositor::Frame);
+    frame->output_surface_id = 0U;
+    frame->frame = ConstructFrame((cc::ResourceId)frame_number);

[boliu, 2016/04/25 19:56:54]

static_cast

c-style casts are banned

[Tobias Sargeant, 2016/04/26 15:29:51]

Done.

+    return frame;
+  }
+
+  void CheckResults() override {
+    ResourceCountMap resource_counts;
+    resource_counts = GetReturnedResourceCounts();

[boliu, 2016/04/25 19:56:53]

I don't think you want to assume first frame has been returned by the time
CheckResults has run. I don't see any guarantee that's actually true, so it's
probably just an accident.

[Tobias Sargeant, 2016/04/26 15:29:51]

Won't the ReturnResourceFromParent call in OnDrawHardware for the second frame
return the first frame's resources?

[boliu, 2016/04/26 15:47:49]

Oh definitely not. In OnDrawHardware for from N+1, frame N is still be in use by
HR. HR only frees from N when frame N+1 is submitted to cc::surfaces.

+    EXPECT_EQ(resource_counts[0U].size(), 1U);
+    EXPECT_EQ(resource_counts[0U][0], 1);
+
+    render_thread_manager_.reset();
+    // Make sure resources for the last frame are returned.
+    resource_counts = GetReturnedResourceCounts();
+    EXPECT_EQ(resource_counts[0U].size(), 1U);
+    EXPECT_EQ(resource_counts[0U][1], 1);
+    EndTest();
+  }
+};
+
+RENDERING_TEST_F(RenderThreadManagerDeletionTest);
+
 }  // namespace android_webview