Fix OwnedMailbox use-after-free on destruction.

OwnedMailbox is ref counted and can outlive the GLES2Interface* it
holds. This is because the callback has a scoped_refptr<OwnedMailbox>
which prevents destroyed the OwnedMailbox.

The ownership model for OwnedMailbox should probably be reworked, but
the class will be replaced fairly soon anyways. Change callback to hold
a WeakPtr instead of scoped_refptr so that OwnedMailbox gets destroyed
at the correct time.

OwnedMailbox is also no longer used with GLHelper and the shared main
thread context, so it doesn't need to be a ContextFactoryObserver to
find out about losing the shared main thread context. Delete
ImageTransportFactoryTearDownBrowserTest.LoseOnTearDown which tested the
now deleted functionality.

Bug: 874616
Change-Id: Iab95a906c4006427e0a0046c56fe20f75d9788a6
Reviewed-on: https://chromium-review.googlesource.com/1178221
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584991}

content/browser/compositor/image_transport_factory_browsertest.cc

@@ -6,9 +6,7 @@
 
 #include "base/run_loop.h"
 #include "build/build_config.h"
-#include "components/viz/common/features.h"
 #include "components/viz/common/gpu/context_provider.h"
-#include "content/browser/compositor/owned_mailbox.h"
 #include "content/public/test/content_browser_test.h"
 #include "gpu/GLES2/gl2extchromium.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
@@ -63,43 +61,5 @@ IN_PROC_BROWSER_TEST_F(ImageTransportFactoryBrowserTest,
   factory->GetContextFactory()->RemoveObserver(&observer);
 }
 
-class ImageTransportFactoryTearDownBrowserTest : public ContentBrowserTest {
- public:
-  void TearDown() override {
-    // Mailbox is null if the test exited early.
-    if (mailbox_.get())
-      EXPECT_TRUE(mailbox_->mailbox().IsZero());
-    ContentBrowserTest::TearDown();
-  }
-
- protected:
-  scoped_refptr<OwnedMailbox> mailbox_;
-};
-
-// Checks that upon destruction of the ImageTransportFactory, the observer is
-// called and the created resources are reset.
-IN_PROC_BROWSER_TEST_F(ImageTransportFactoryTearDownBrowserTest,
-                       LoseOnTearDown) {
-  ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
-
-  // TODO(crbug.com/844469): Delete after OOP-D is launched because OwnedMailbox
-  // isn't used.
-  if (base::FeatureList::IsEnabled(features::kVizDisplayCompositor))
-    return;
-
-  // This test doesn't make sense in software compositing mode.
-  if (factory->IsGpuCompositingDisabled())
-    return;
-
-  gpu::gles2::GLES2Interface* const gl = factory->GetContextFactory()
-                                             ->SharedMainThreadContextProvider()
-                                             ->ContextGL();
-  ASSERT_TRUE(gl);
-  mailbox_ = base::MakeRefCounted<OwnedMailbox>(gl);
-  EXPECT_FALSE(mailbox_->mailbox().IsZero());
-
-  // See TearDown() for the test expectation that |mailbox_| has been reset.
-}
-
 }  // anonymous namespace
 }  // namespace content

content/browser/compositor/owned_mailbox.cc

@@ -4,13 +4,13 @@
 
 #include "content/browser/compositor/owned_mailbox.h"
 
-#include "content/browser/compositor/image_transport_factory.h"
+#include "base/bind.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 
 namespace content {
 
 OwnedMailbox::OwnedMailbox(gpu::gles2::GLES2Interface* gl)
-    : gl_(gl), texture_id_(0) {
+    : gl_(gl), texture_id_(0), weak_ptr_factory_(this) {
   DCHECK(gl_);
 
   // Create the texture.
@@ -28,36 +28,23 @@ OwnedMailbox::OwnedMailbox(gpu::gles2::GLES2Interface* gl)
   gl_->ProduceTextureDirectCHROMIUM(texture_id_, mailbox_holder_.mailbox.name);
   gl_->GenSyncTokenCHROMIUM(mailbox_holder_.sync_token.GetData());
   mailbox_holder_.texture_target = GL_TEXTURE_2D;
-
-  ImageTransportFactory::GetInstance()->GetContextFactory()->AddObserver(this);
 }
 
 OwnedMailbox::~OwnedMailbox() {
-  Destroy();
-}
-
-void OwnedMailbox::UpdateSyncToken(const gpu::SyncToken& sync_token) {
-  if (sync_token.HasData())
-    mailbox_holder_.sync_token = sync_token;
-}
-
-void OwnedMailbox::Destroy() {
-  if (texture_id_ == 0)
-    return;
-
-  ImageTransportFactory::GetInstance()->GetContextFactory()->RemoveObserver(
-      this);
-
   gl_->WaitSyncTokenCHROMIUM(mailbox_holder_.sync_token.GetConstData());
   gl_->DeleteTextures(1, &texture_id_);
-  texture_id_ = 0;
-  mailbox_holder_ = gpu::MailboxHolder();
 }
 
-void OwnedMailbox::OnLostSharedContext() {
-  Destroy();
+std::unique_ptr<viz::SingleReleaseCallback>
+OwnedMailbox::GetSingleReleaseCallback() {
+  return viz::SingleReleaseCallback::Create(base::BindOnce(
+      &OwnedMailbox::UpdateSyncToken, weak_ptr_factory_.GetWeakPtr()));
 }
 
-void OwnedMailbox::OnLostVizProcess() {}
+void OwnedMailbox::UpdateSyncToken(const gpu::SyncToken& sync_token,
+                                   bool is_lost) {
+  if (sync_token.HasData())
+    mailbox_holder_.sync_token = sync_token;
+}
 
 }  // namespace content

content/browser/compositor/owned_mailbox.h

@@ -7,8 +7,11 @@
 
 #include <stdint.h>
 
+#include <memory>
+
 #include "base/memory/ref_counted.h"
-#include "content/browser/compositor/image_transport_factory.h"
+#include "base/memory/weak_ptr.h"
+#include "components/viz/common/resources/single_release_callback.h"
 #include "content/common/content_export.h"
 #include "gpu/command_buffer/common/mailbox_holder.h"
 #include "ui/compositor/compositor.h"
@@ -23,8 +26,7 @@ namespace content {
 
 // This class holds a texture id and gpu::Mailbox, and deletes the texture
 // id when the object itself is destroyed.
-class CONTENT_EXPORT OwnedMailbox : public base::RefCounted<OwnedMailbox>,
-                                    public ui::ContextFactoryObserver {
+class CONTENT_EXPORT OwnedMailbox : public base::RefCounted<OwnedMailbox> {
  public:
   explicit OwnedMailbox(gpu::gles2::GLES2Interface* gl);
 
@@ -34,22 +36,21 @@ class CONTENT_EXPORT OwnedMailbox : public base::RefCounted<OwnedMailbox>,
     return mailbox_holder_.sync_token;
   }
   uint32_t texture_id() const { return texture_id_; }
-  void UpdateSyncToken(const gpu::SyncToken& sync_token);
-  void Destroy();
-
- protected:
-  ~OwnedMailbox() override;
 
-  // ImageTransportFactoryObserver implementation.
-  void OnLostSharedContext() override;
-  void OnLostVizProcess() override;
+  // Returns a SingleReleaseCallback for TextureLayer.
+  std::unique_ptr<viz::SingleReleaseCallback> GetSingleReleaseCallback();
 
  private:
   friend class base::RefCounted<OwnedMailbox>;
 
+  ~OwnedMailbox();
+
+  void UpdateSyncToken(const gpu::SyncToken& sync_token, bool is_lost);
+
   gpu::gles2::GLES2Interface* const gl_;
   uint32_t texture_id_;
   gpu::MailboxHolder mailbox_holder_;
+  base::WeakPtrFactory<OwnedMailbox> weak_ptr_factory_;
 };
 
 }  // namespace content

content/browser/compositor/reflector_impl.cc

@@ -4,7 +4,6 @@
 
 #include "content/browser/compositor/reflector_impl.h"
 
-#include "base/bind.h"
 #include "base/location.h"
 #include "components/viz/common/resources/transferable_resource.h"
 #include "content/browser/compositor/browser_compositor_output_surface.h"
@@ -30,8 +29,7 @@ ReflectorImpl::ReflectorImpl(ui::Compositor* mirrored_compositor,
     AddMirroringLayer(mirroring_layer);
 }
 
-ReflectorImpl::~ReflectorImpl() {
-}
+ReflectorImpl::~ReflectorImpl() = default;
 
 void ReflectorImpl::Shutdown() {
   if (output_surface_)
@@ -43,8 +41,8 @@ void ReflectorImpl::Shutdown() {
 void ReflectorImpl::DetachFromOutputSurface() {
   DCHECK(output_surface_);
   output_surface_->SetReflector(nullptr);
-  DCHECK(mailbox_.get());
-  mailbox_ = nullptr;
+  DCHECK(mailbox_);
+  mailbox_.reset();
   output_surface_ = nullptr;
   for (const auto& layer_data : mirroring_layers_)
     layer_data->layer->SetShowSolidColorContent();
@@ -140,12 +138,6 @@ void ReflectorImpl::OnSourcePostSubBuffer(const gfx::Rect& swap_rect,
     UpdateTexture(layer_data.get(), surface_size, mirroring_rect);
 }
 
-static void ReleaseMailbox(scoped_refptr<OwnedMailbox> mailbox,
-                           const gpu::SyncToken& sync_token,
-                           bool is_lost) {
-  mailbox->UpdateSyncToken(sync_token);
-}
-
 std::vector<std::unique_ptr<ReflectorImpl::LayerData>>::iterator
 ReflectorImpl::FindLayerData(ui::Layer* layer) {
   return std::find_if(mirroring_layers_.begin(), mirroring_layers_.end(),
@@ -162,9 +154,7 @@ void ReflectorImpl::UpdateTexture(ReflectorImpl::LayerData* layer_data,
         viz::TransferableResource::MakeGL(mailbox_->holder().mailbox, GL_LINEAR,
                                           mailbox_->holder().texture_target,
                                           mailbox_->holder().sync_token),
-        viz::SingleReleaseCallback::Create(
-            base::BindOnce(ReleaseMailbox, mailbox_)),
-        source_size);
+        mailbox_->GetSingleReleaseCallback(), source_size);
     layer_data->needs_set_mailbox = false;
   } else {
     layer_data->layer->SetTextureSize(source_size);