Submit empty compositor frames for non-visible notifications.

Expanding/contracting a notification changes its size, which generates a new
LocalSurfaceId. Creating a new LocalSurfaceId makes viz create a new
viz::Surface upon submitting the compositor frame. Viz's SurfaceManager tracks
the lifetime and performs garbage collection for Surfaces, which reference
TransferableResources, which correspond to e.g. the notification buffers.
A viz::Surface can be destroyed and deleted. Destroyed means it is marked for
deletion. Deletion will happen at some point in the future during
GarbageCollectSurfaces.

Normally, a notification would be displayed+embedded in the message center so it
will have real references to the SurfaceIds. This means that the old viz Surface
can be garbage collected immediately if the notification produces more buffers.
But, if the notification is producing buffers while the message centre is
closed, it's not embedded in anything. In this case, Viz creates temporary
references. If you expand and then contract, two Viz Surfaces are created (one
for each LocalSurfaceId) but neither can be deleted - both only have temporary
references. These Surfaces hold onto the associated TransferableResources, the
buffers, and cause buffer starvation on the Android side. Android HWUI starts
waiting on a fence for a buffer but the ANR timeout occurs before the Viz
temporary reference timeout. This causes the SystemUI ANR.

Because the triggers for this bug - "notifications not being visible on the
Chrome side" + "receiving input on a notification on the Android side" are
completely asynchronous we can't realistically prevent these triggers from
occurring. Instead, this change explicitly tells Viz it can release the
resources by pushing empty compositor frames when the notification becomes
invisible.

Test: Android applications still can produce buffers
Test: Notifications appear correctly when closing/opening the message center.
Test: Following the steps in b/130856284, the ANR no longer reproduces.
Bug: b/130856284
Change-Id: I42529267f75d7c30a8bcb03586e6dd08f658c69b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1645432Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Eliot Courtney <edcourtney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669068}

components/exo/BUILD.gn

@@ -63,6 +63,7 @@ source_set("exo") {
   deps = [
     "//base",
     "//cc",
+    "//components/viz/host",
     "//components/viz/service",
     "//device/gamepad",
     "//device/gamepad/public/cpp:shared_with_blink",

components/exo/DEPS

@@ -3,6 +3,7 @@ include_rules = [
   "+cc",
   "+chromeos/audio/chromeos_sounds.h",
   "+components/viz/common",
+  "+components/viz/host",
   "+components/viz/service",
   "+device/gamepad",
   "+gpu",

components/exo/notification_surface.cc

@@ -8,7 +8,9 @@
 #include "components/exo/notification_surface_manager.h"
 #include "components/exo/shell_surface_util.h"
 #include "components/exo/surface.h"
+#include "components/viz/host/host_frame_sink_manager.h"
 #include "ui/aura/client/aura_constants.h"
+#include "ui/aura/env.h"
 #include "ui/aura/window.h"
 #include "ui/gfx/geometry/rect.h"
 
@@ -25,9 +27,12 @@ NotificationSurface::NotificationSurface(NotificationSurfaceManager* manager,
   host_window()->SetProperty(aura::client::kAppType,
                              static_cast<int>(ash::AppType::ARC_APP));
   host_window()->Show();
+  host_window()->AddObserver(this);
 }
 
 NotificationSurface::~NotificationSurface() {
+  if (host_window())
+    host_window()->RemoveObserver(this);
   if (added_to_manager_)
     manager_->RemoveSurface(this);
   if (root_surface())
@@ -51,7 +56,13 @@ void NotificationSurface::OnSurfaceCommit() {
     manager_->AddSurface(this);
   }
 
-  SubmitCompositorFrame();
+  // Only submit a compositor frame if the notification is being shown.
+  // Submitting compositor frames while invisible causes Viz to hold on to
+  // references to each notification buffer while it waits for an embedding (
+  // or a timeout occurs). This can cause buffer starvation on the Android side,
+  // leading to ANRs.
+  if (is_embedded_)
+    SubmitCompositorFrame();
 }
 
 void NotificationSurface::OnSurfaceDestroying(Surface* surface) {
@@ -60,4 +71,49 @@ void NotificationSurface::OnSurfaceDestroying(Surface* surface) {
   SetRootSurface(nullptr);
 }
 
+void NotificationSurface::OnWindowDestroying(aura::Window* window) {
+  window->RemoveObserver(this);
+}
+
+void NotificationSurface::OnWindowAddedToRootWindow(aura::Window* window) {
+  SubmitCompositorFrame();
+  is_embedded_ = true;
+}
+
+void NotificationSurface::OnWindowRemovingFromRootWindow(
+    aura::Window* window,
+    aura::Window* new_root) {
+  if (!new_root) {
+    // Submit an empty compositor frame if the notification becomes invisible to
+    // notify Viz that it can release its reference to the existing notification
+    // buffer. We can't just evict the surface, because LayerTreeFrameSinkHolder
+    // needs to hold on to resources that were used in the previous frame, if it
+    // was associated with a different local surface id.
+    SubmitEmptyCompositorFrame();
+
+    // Evict the current surface. We can't only submit
+    // an empty compositor frame, because then when re-opening the message
+    // center, Viz may think it can use the empty compositor frame to display.
+    // This will force viz to wait the given deadline for the next notification
+    // compositor frame when showing the message center. This is to prevent
+    // flashes when opening the message center.
+    aura::Env::GetInstance()
+        ->context_factory_private()
+        ->GetHostFrameSinkManager()
+        ->EvictSurfaces({host_window()->GetSurfaceId()});
+
+    // Allocate a new local surface id for the next compositor frame.
+    host_window()->AllocateLocalSurfaceId();
+
+    is_embedded_ = false;
+
+    // Set the deadline to default so Viz waits a while for the notification
+    // compositor frame to arrive before showing the message center.
+    host_window()->layer()->SetShowSurface(
+        host_window()->GetSurfaceId(), host_window()->bounds().size(),
+        SK_ColorWHITE, cc::DeadlinePolicy::UseDefaultDeadline(),
+        /*stretch_content_to_fill_bounds=*/false);
+  }
+}
+
 }  // namespace exo

components/exo/notification_surface.h

@@ -18,7 +18,9 @@ class NotificationSurfaceManager;
 class Surface;
 
 // Handles notification surface role of a given surface.
-class NotificationSurface : public SurfaceTreeHost, public SurfaceObserver {
+class NotificationSurface : public SurfaceTreeHost,
+                            public SurfaceObserver,
+                            public aura::WindowObserver {
  public:
   NotificationSurface(NotificationSurfaceManager* manager,
                       Surface* surface,
@@ -38,12 +40,22 @@ class NotificationSurface : public SurfaceTreeHost, public SurfaceObserver {
   // Overridden from SurfaceObserver:
   void OnSurfaceDestroying(Surface* surface) override;
 
+  // Overridden from WindowObserver:
+  void OnWindowDestroying(aura::Window* window) override;
+  void OnWindowAddedToRootWindow(aura::Window* window) override;
+  void OnWindowRemovingFromRootWindow(aura::Window* window,
+                                      aura::Window* new_root) override;
+
  private:
   NotificationSurfaceManager* const manager_;  // Not owned.
   const std::string notification_key_;
 
   bool added_to_manager_ = false;
 
+  // True if the notification is visible by e.g. being embedded in the message
+  // center.
+  bool is_embedded_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(NotificationSurface);
 };
 

components/exo/surface_tree_host.cc

@@ -12,6 +12,9 @@
 #include "components/exo/surface.h"
 #include "components/exo/wm_helper.h"
 #include "components/viz/common/quads/compositor_frame.h"
+#include "components/viz/common/quads/render_pass.h"
+#include "components/viz/common/quads/shared_quad_state.h"
+#include "components/viz/common/quads/solid_color_draw_quad.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "third_party/skia/include/core/SkPath.h"
 #include "ui/aura/env.h"
@@ -207,21 +210,10 @@ void SurfaceTreeHost::OnLostSharedContext() {
 // SurfaceTreeHost, protected:
 
 void SurfaceTreeHost::SubmitCompositorFrame() {
-  DCHECK(root_surface_);
-
-  if (layer_tree_frame_sink_holder_->is_lost()) {
-    // We can immediately delete the old LayerTreeFrameSinkHolder because all of
-    // it's resources are lost anyways.
-    layer_tree_frame_sink_holder_ = std::make_unique<LayerTreeFrameSinkHolder>(
-        this, host_window_->CreateLayerTreeFrameSink());
-  }
+  viz::CompositorFrame frame = PrepareToSubmitCompositorFrame();
 
-  viz::CompositorFrame frame;
-  frame.metadata.begin_frame_ack =
-      viz::BeginFrameAck::CreateManualAckWithDamage();
   root_surface_->AppendSurfaceHierarchyCallbacks(&frame_callbacks_,
                                                  &presentation_callbacks_);
-  frame.metadata.frame_token = ++next_token_;
   if (!presentation_callbacks_.empty()) {
     DCHECK_EQ(active_presentation_callbacks_.count(*next_token_), 0u);
     active_presentation_callbacks_[*next_token_] =
@@ -229,30 +221,9 @@ void SurfaceTreeHost::SubmitCompositorFrame() {
   } else {
     active_presentation_callbacks_[*next_token_] = PresentationCallbacks();
   }
-  frame.render_pass_list.push_back(viz::RenderPass::Create());
-  const std::unique_ptr<viz::RenderPass>& render_pass =
-      frame.render_pass_list.back();
 
-  const int kRenderPassId = 1;
-  // Compute a temporaly stable (across frames) size for the render pass output
-  // rectangle that is consistent with the window size. It is used to set the
-  // size of the output surface. Note that computing the actual coverage while
-  // building up the render pass can lead to the size being one pixel too large,
-  // especially if the device scale factor has a floating point representation
-  // that requires many bits of precision in the mantissa, due to the coverage
-  // computing an "enclosing" pixel rectangle. This isn't a problem for the
-  // dirty rectangle, so it is updated as part of filling in the render pass.
-  const float device_scale_factor =
-      host_window()->layer()->device_scale_factor();
-  const gfx::Size output_surface_size_in_pixels = gfx::ConvertSizeToPixel(
-      device_scale_factor, host_window_->bounds().size());
-  render_pass->SetNew(kRenderPassId, gfx::Rect(output_surface_size_in_pixels),
-                      gfx::Rect(), gfx::Transform());
-  frame.metadata.device_scale_factor = device_scale_factor;
-  frame.metadata.local_surface_id_allocation_time =
-      host_window()->GetLocalSurfaceIdAllocation().allocation_time();
   root_surface_->AppendSurfaceHierarchyContentsToFrame(
-      root_surface_origin_, device_scale_factor,
+      root_surface_origin_, host_window()->layer()->device_scale_factor(),
       layer_tree_frame_sink_holder_->resource_manager(), &frame);
 
   std::vector<GLbyte*> sync_tokens;
@@ -267,6 +238,28 @@ void SurfaceTreeHost::SubmitCompositorFrame() {
   layer_tree_frame_sink_holder_->SubmitCompositorFrame(std::move(frame));
 }
 
+void SurfaceTreeHost::SubmitEmptyCompositorFrame() {
+  viz::CompositorFrame frame = PrepareToSubmitCompositorFrame();
+
+  const std::unique_ptr<viz::RenderPass>& render_pass =
+      frame.render_pass_list.back();
+  const gfx::Rect quad_rect = gfx::Rect(0, 0, 1, 1);
+  viz::SharedQuadState* quad_state =
+      render_pass->CreateAndAppendSharedQuadState();
+  quad_state->SetAll(
+      gfx::Transform(), /*quad_layer_rect=*/quad_rect,
+      /*visible_quad_layer_rect=*/quad_rect,
+      /*rounded_corner_bounds=*/gfx::RRectF(), /*clip_rect=*/gfx::Rect(),
+      /*is_clipped=*/false, /*are_contents_opaque=*/true, /*opacity=*/1.f,
+      /*blend_mode=*/SkBlendMode::kSrcOver, /*sorting_context_id=*/0);
+
+  viz::SolidColorDrawQuad* solid_quad =
+      render_pass->CreateAndAppendDrawQuad<viz::SolidColorDrawQuad>();
+  solid_quad->SetNew(quad_state, quad_rect, quad_rect, SK_ColorBLACK,
+                     /*force_anti_aliasing_off=*/false);
+  layer_tree_frame_sink_holder_->SubmitCompositorFrame(std::move(frame));
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // SurfaceTreeHost, private:
 
@@ -289,4 +282,48 @@ void SurfaceTreeHost::UpdateHostWindowBounds() {
       root_surface_origin_, root_surface_->window()->bounds().size()));
 }
 
+viz::CompositorFrame SurfaceTreeHost::PrepareToSubmitCompositorFrame() {
+  DCHECK(root_surface_);
+
+  if (layer_tree_frame_sink_holder_->is_lost()) {
+    // We can immediately delete the old LayerTreeFrameSinkHolder because all of
+    // it's resources are lost anyways.
+    layer_tree_frame_sink_holder_ = std::make_unique<LayerTreeFrameSinkHolder>(
+        this, host_window_->CreateLayerTreeFrameSink());
+  }
+
+  viz::CompositorFrame frame;
+  frame.metadata.begin_frame_ack =
+      viz::BeginFrameAck::CreateManualAckWithDamage();
+  frame.metadata.frame_token = ++next_token_;
+  frame.render_pass_list.push_back(viz::RenderPass::Create());
+  const std::unique_ptr<viz::RenderPass>& render_pass =
+      frame.render_pass_list.back();
+
+  const int kRenderPassId = 1;
+  // Compute a temporally stable (across frames) size for the render pass output
+  // rectangle that is consistent with the window size. It is used to set the
+  // size of the output surface. Note that computing the actual coverage while
+  // building up the render pass can lead to the size being one pixel too large,
+  // especially if the device scale factor has a floating point representation
+  // that requires many bits of precision in the mantissa, due to the coverage
+  // computing an "enclosing" pixel rectangle. This isn't a problem for the
+  // dirty rectangle, so it is updated as part of filling in the render pass.
+  // Additionally, we must use this size even if we are submitting an empty
+  // compositor frame, otherwise we may set the Surface created by Viz to be the
+  // wrong size. Then, trying to submit a regular compositor frame will fail
+  // because  the size is different.
+  const float device_scale_factor =
+      host_window()->layer()->device_scale_factor();
+  const gfx::Size output_surface_size_in_pixels = gfx::ConvertSizeToPixel(
+      device_scale_factor, host_window_->bounds().size());
+  render_pass->SetNew(kRenderPassId, gfx::Rect(output_surface_size_in_pixels),
+                      gfx::Rect(), gfx::Transform());
+  frame.metadata.device_scale_factor = device_scale_factor;
+  frame.metadata.local_surface_id_allocation_time =
+      host_window()->GetLocalSurfaceIdAllocation().allocation_time();
+
+  return frame;
+}
+
 }  // namespace exo

components/exo/surface_tree_host.h

@@ -85,8 +85,14 @@ class SurfaceTreeHost : public SurfaceDelegate,
   // Call this to submit a compositor frame.
   void SubmitCompositorFrame();
 
+  // Call this to submit an empty compositor frame. This may be useful if
+  // the surface tree is becoming invisible but the resources (e.g. buffers)
+  // need to be released back to the client.
+  void SubmitEmptyCompositorFrame();
+
  private:
   void UpdateHostWindowBounds();
+  viz::CompositorFrame PrepareToSubmitCompositorFrame();
 
   Surface* root_surface_ = nullptr;