commit aa112d47f50d2e437dde2e3f9aa5b9b712851117
author Joel Klinghed <the_jk@opera.com> Tue May 07 22:42:55 2019
committer Commit Bot <commit-bot@chromium.org> Tue May 07 22:42:55 2019
tree 941ab28139a7d653da91fd8d23d496ff1a92d884
parent e8532f51dec283fc5afc6bd96335d3c7c305235f

Avoid never-ending loop in FrameEvictionManager::CullUnlockedFrames

If enable surface synchronization is disabled and the frame is
visible then DelegatedFrameHostAndroid::EvictDelegatedFrame()
will only reset content_layer_ to null but not evict the
surface as that causes https://crbug.com/933374.

Trouble is that if the frame is then hidden without any new
content_layer being created the next call to
DelegatedFrameHostAndroid::EvictDelegatedFrame() will do nothing
as content_layer_ is null but the frame is still in the list of
unlocked frames in FrameEvicitionManager causing the next
call to FrameEvictionManager::CullUnlockedFrames that tries to evict
the frame to do nothing and loop forever (or hit a DCHECK if those
are enabled).

Fix this by not doing early exit if content_layer_ is null as this
is not directly connected to if there is a frame to evict.

Bug: 959914
Change-Id: I3f83036d9a5ae2c734dfca720115d1faf4278c87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596571
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Joel Klinghed <the_jk@opera.com>
Cr-Commit-Position: refs/heads/master@{#657483}

ui/android/delegated_frame_host_android.cc

diff --git a/ui/android/delegated_frame_host_android.cc b/ui/android/delegated_frame_host_android.cc
index 5798985..32c726f 100644
--- a/ui/android/delegated_frame_host_android.cc
+++ b/ui/android/delegated_frame_host_android.cc
@@ -188,13 +188,13 @@
 }
 
 void DelegatedFrameHostAndroid::EvictDelegatedFrame() {
-  if (!content_layer_)
-    return;
-  content_layer_->SetSurfaceId(viz::SurfaceId(),
-                               cc::DeadlinePolicy::UseDefaultDeadline());
-  if (!enable_surface_synchronization_) {
-    content_layer_->RemoveFromParent();
-    content_layer_ = nullptr;
+  if (content_layer_) {
+    content_layer_->SetSurfaceId(viz::SurfaceId(),
+                                 cc::DeadlinePolicy::UseDefaultDeadline());
+    if (!enable_surface_synchronization_) {
+      content_layer_->RemoveFromParent();
+      content_layer_ = nullptr;
+    }
   }
   if (!HasSavedFrame() || frame_evictor_->visible())
     return;

ui/android/delegated_frame_host_android_unittest.cc

diff --git a/ui/android/delegated_frame_host_android_unittest.cc b/ui/android/delegated_frame_host_android_unittest.cc
index 4ac6e360..a9985d4 100644
--- a/ui/android/delegated_frame_host_android_unittest.cc
+++ b/ui/android/delegated_frame_host_android_unittest.cc
@@ -415,5 +415,24 @@
   EXPECT_FALSE(request->has_result_selection());
 }
 
+TEST_F(DelegatedFrameHostAndroidLegacyNonVizTest, EvictWhileVisible) {
+  // Create a frame and mark it visible.
+  gfx::Size size(10, 10);
+  SetUpValidFrame(size);
+  auto id = allocator_.GetCurrentLocalSurfaceIdAllocation().local_surface_id();
+  frame_host_->WasShown(id, size);
+  EXPECT_TRUE(frame_host_->HasSavedFrame());
+
+  // Evict, which should do nothing as the frame is still visible.
+  frame_host_->EvictDelegatedFrame();
+  EXPECT_TRUE(frame_host_->HasSavedFrame());
+
+  // Hide frame and evict again, now it should release the frame.
+  EXPECT_CALL(client_, WasEvicted());
+  frame_host_->WasHidden();
+  frame_host_->EvictDelegatedFrame();
+  EXPECT_FALSE(frame_host_->HasSavedFrame());
+}
+
 }  // namespace
 }  // namespace ui