exo: Make Texture::Buffer a ContextLostObserver.

Texture::Buffer is using the deprecated ContextFactoryObserver interface
to find out about main thread context loss. The ContextFactory adds
itself as a ContextLostObserver, then on context loss forwards that
message along to all the ContextFactoryObservers.

There are multiple message pipes between the browser and GPU process
that have indeterminate ordering for when they find out about connection
errors. This leads to the following ordering problem:

1. The GPU process crashes. The old shared main thread
   RasterContextProvider is lost and any message pipes between the
   browser and GPU process will encounter connection errors.
2. A new shared main thread RasterContextProvider is created.
3. Exo allocates a new Buffer::Texture using the new shared main thread
   RasterContextProvider.
4. The new Buffer::Texture adds itself as a ContextFactoryObserver.
5. All ContextFactoryObservers get notified about OnLostSharedContext()
   for the old shared main thread RasterContextProvider loss. For the
   new Buffer::Texture this is wrong, the RasterContextProvider is still
   valid.
6. The new Buffer::Texture destroys all of it's resources incorrectly.
   This triggers a flood of GL errors from the GPU process.

Make Buffer::Texture a ContextLossObserver instead. This way the
Buffer::Texture gets notified about context loss for only the
RasterContextProvider it's using.

Bug: 954151, 944597
Test: BufferTest.OnLostResources
Change-Id: Ic68dbaab8864b835b2bbb83b3b43f03495902aef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1597390
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657432}

components/exo/buffer.cc

@@ -20,6 +20,7 @@
 #include "base/trace_event/traced_value.h"
 #include "components/exo/frame_sink_resource_manager.h"
 #include "components/exo/wm_helper.h"
+#include "components/viz/common/gpu/context_lost_observer.h"
 #include "components/viz/common/gpu/context_provider.h"
 #include "components/viz/common/resources/resource_format.h"
 #include "components/viz/common/resources/resource_format_utils.h"
@@ -50,18 +51,20 @@ constexpr char kBufferInUse[] = "BufferInUse";
 // Buffer::Texture
 
 // Encapsulates the state and logic needed to bind a buffer to a SharedImage.
-class Buffer::Texture : public ui::ContextFactoryObserver {
+class Buffer::Texture : public viz::ContextLostObserver {
  public:
-  Texture(ui::ContextFactory* context_factory, const gfx::Size& size);
-  Texture(ui::ContextFactory* context_factory,
+  Texture(scoped_refptr<viz::RasterContextProvider> context_provider,
+          const gfx::Size& size);
+  Texture(scoped_refptr<viz::RasterContextProvider> context_provider,
+          gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
           gfx::GpuMemoryBuffer* gpu_memory_buffer,
           unsigned texture_target,
           unsigned query_type,
           base::TimeDelta wait_for_release_time);
   ~Texture() override;
 
-  // Overridden from ui::ContextFactoryObserver:
-  void OnLostSharedContext() override;
+  // Overridden from viz::ContextLostObserver:
+  void OnContextLost() override;
 
   // Returns true if the RasterInterface context has been lost.
   bool IsLost();
@@ -101,7 +104,6 @@ class Buffer::Texture : public ui::ContextFactoryObserver {
 
   gfx::GpuMemoryBuffer* const gpu_memory_buffer_;
   const gfx::Size size_;
-  ui::ContextFactory* context_factory_;
   scoped_refptr<viz::RasterContextProvider> context_provider_;
   const unsigned texture_target_;
   const unsigned query_type_;
@@ -116,13 +118,12 @@ class Buffer::Texture : public ui::ContextFactoryObserver {
   DISALLOW_COPY_AND_ASSIGN(Texture);
 };
 
-Buffer::Texture::Texture(ui::ContextFactory* context_factory,
-                         const gfx::Size& size)
+Buffer::Texture::Texture(
+    scoped_refptr<viz::RasterContextProvider> context_provider,
+    const gfx::Size& size)
     : gpu_memory_buffer_(nullptr),
       size_(size),
-      context_factory_(context_factory),
-      context_provider_(
-          context_factory->SharedMainThreadRasterContextProvider()),
+      context_provider_(std::move(context_provider)),
       texture_target_(GL_TEXTURE_2D),
       query_type_(GL_COMMANDS_COMPLETED_CHROMIUM),
       weak_ptr_factory_(this) {
@@ -137,19 +138,19 @@ Buffer::Texture::Texture(ui::ContextFactory* context_factory,
   ri->WaitSyncTokenCHROMIUM(sii->GenUnverifiedSyncToken().GetConstData());
 
   // Provides a notification when |context_provider_| is lost.
-  context_factory_->AddObserver(this);
+  context_provider_->AddObserver(this);
 }
 
-Buffer::Texture::Texture(ui::ContextFactory* context_factory,
-                         gfx::GpuMemoryBuffer* gpu_memory_buffer,
-                         unsigned texture_target,
-                         unsigned query_type,
-                         base::TimeDelta wait_for_release_delay)
+Buffer::Texture::Texture(
+    scoped_refptr<viz::RasterContextProvider> context_provider,
+    gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
+    gfx::GpuMemoryBuffer* gpu_memory_buffer,
+    unsigned texture_target,
+    unsigned query_type,
+    base::TimeDelta wait_for_release_delay)
     : gpu_memory_buffer_(gpu_memory_buffer),
       size_(gpu_memory_buffer->GetSize()),
-      context_factory_(context_factory),
-      context_provider_(
-          context_factory->SharedMainThreadRasterContextProvider()),
+      context_provider_(std::move(context_provider)),
       texture_target_(texture_target),
       query_type_(query_type),
       wait_for_release_delay_(wait_for_release_delay),
@@ -160,31 +161,28 @@ Buffer::Texture::Texture(ui::ContextFactory* context_factory,
                          gpu::SHARED_IMAGE_USAGE_SCANOUT;
 
   mailbox_ = sii->CreateSharedImage(
-      gpu_memory_buffer_, context_factory_->GetGpuMemoryBufferManager(),
-      gfx::ColorSpace(), usage);
+      gpu_memory_buffer_, gpu_memory_buffer_manager, gfx::ColorSpace(), usage);
   DCHECK(!mailbox_.IsZero());
   gpu::raster::RasterInterface* ri = context_provider_->RasterInterface();
   ri->WaitSyncTokenCHROMIUM(sii->GenUnverifiedSyncToken().GetConstData());
   ri->GenQueriesEXT(1, &query_id_);
 
   // Provides a notification when |context_provider_| is lost.
-  context_factory_->AddObserver(this);
+  context_provider_->AddObserver(this);
 }
 
 Buffer::Texture::~Texture() {
   DestroyResources();
-  if (context_factory_)
-    context_factory_->RemoveObserver(this);
+  if (context_provider_)
+    context_provider_->RemoveObserver(this);
 }
 
-void Buffer::Texture::OnLostSharedContext() {
+void Buffer::Texture::OnContextLost() {
   DestroyResources();
-  context_factory_->RemoveObserver(this);
-  context_provider_ = nullptr;
-  context_factory_ = nullptr;
+  context_provider_->RemoveObserver(this);
+  context_provider_.reset();
 }
 
-
 bool Buffer::Texture::IsLost() {
   if (context_provider_) {
     gpu::raster::RasterInterface* ri = context_provider_->RasterInterface();
@@ -409,7 +407,8 @@ bool Buffer::ProduceTransferableResource(
   // |texture| using a call to CopyTexImage.
   if (!contents_texture_) {
     contents_texture_ = std::make_unique<Texture>(
-        context_factory, gpu_memory_buffer_.get(), texture_target_, query_type_,
+        context_provider, context_factory->GetGpuMemoryBufferManager(),
+        gpu_memory_buffer_.get(), texture_target_, query_type_,
         wait_for_release_delay_);
   }
   Texture* contents_texture = contents_texture_.get();
@@ -445,7 +444,7 @@ bool Buffer::ProduceTransferableResource(
 
   // Create a mailbox texture that we copy the buffer contents to.
   if (!texture_) {
-    texture_ = std::make_unique<Texture>(context_factory,
+    texture_ = std::make_unique<Texture>(context_provider,
                                          gpu_memory_buffer_->GetSize());
   }
   Texture* texture = texture_.get();

components/exo/buffer_unittest.cc

@@ -2,12 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "components/exo/buffer.h"
+
 #include <GLES2/gl2extchromium.h>
 
 #include "ash/shell.h"
 #include "base/bind.h"
 #include "base/run_loop.h"
-#include "components/exo/buffer.h"
 #include "components/exo/frame_sink_resource_manager.h"
 #include "components/exo/surface_tree_host.h"
 #include "components/exo/test/exo_test_base.h"
@@ -18,7 +19,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/env.h"
 #include "ui/compositor/compositor.h"
-#include "ui/compositor/test/in_process_context_factory.h"
+#include "ui/compositor/test/in_process_context_provider.h"
 #include "ui/gfx/gpu_memory_buffer.h"
 
 namespace exo {
@@ -155,8 +156,13 @@ TEST_F(BufferTest, OnLostResources) {
       frame_sink_holder->resource_manager(), false, &resource);
   ASSERT_TRUE(rv);
 
-  static_cast<ui::InProcessContextFactory*>(GetAuraEnv()->context_factory())
-      ->SendOnLostSharedContext();
+  viz::RasterContextProvider* context_provider =
+      GetAuraEnv()
+          ->context_factory()
+          ->SharedMainThreadRasterContextProvider()
+          .get();
+  static_cast<ui::InProcessContextProvider*>(context_provider)
+      ->SendOnContextLost();
 }
 
 TEST_F(BufferTest, SurfaceTreeHostDestruction) {

ui/compositor/test/in_process_context_provider.cc

@@ -182,11 +182,11 @@ base::Lock* InProcessContextProvider::GetLock() {
 }
 
 void InProcessContextProvider::AddObserver(viz::ContextLostObserver* obs) {
-  // Pixel tests do not test lost context.
+  observers_.AddObserver(obs);
 }
 
 void InProcessContextProvider::RemoveObserver(viz::ContextLostObserver* obs) {
-  // Pixel tests do not test lost context.
+  observers_.RemoveObserver(obs);
 }
 
 uint32_t InProcessContextProvider::GetCopyTextureInternalFormat() {
@@ -198,4 +198,9 @@ uint32_t InProcessContextProvider::GetCopyTextureInternalFormat() {
   return GL_RGB;
 }
 
+void InProcessContextProvider::SendOnContextLost() {
+  for (auto& observer : observers_)
+    observer.OnContextLost();
+}
+
 }  // namespace ui

ui/compositor/test/in_process_context_provider.h

@@ -11,8 +11,10 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
+#include "components/viz/common/gpu/context_lost_observer.h"
 #include "components/viz/common/gpu/context_provider.h"
 #include "components/viz/common/gpu/raster_context_provider.h"
 #include "gpu/command_buffer/common/context_creation_attribs.h"
@@ -70,6 +72,9 @@ class InProcessContextProvider
   // on the default framebuffer.
   uint32_t GetCopyTextureInternalFormat();
 
+  // Calls OnContextLost() on all observers. This doesn't modify the context.
+  void SendOnContextLost();
+
  private:
   friend class base::RefCountedThreadSafe<InProcessContextProvider>;
 
@@ -112,6 +117,8 @@ class InProcessContextProvider
 
   base::Lock context_lock_;
 
+  base::ObserverList<viz::ContextLostObserver>::Unchecked observers_;
+
   DISALLOW_COPY_AND_ASSIGN(InProcessContextProvider);
 };