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:
+  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));
+    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.

 
-      // 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 >= 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);
+
+    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 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_]);

[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