aura: Ensure OwnedMailbox not used with invalid or destroyed GLHelper.

There are 2 scenarios when the GLHelper becomes invalid:

1. On lost context. In this case, the OwnedMailbox is reset and the
pointer is destroyed. However, the copy request result code would
continue trying to use the OwnedMailbox regardless. Instead we can
use this as a hint that the mailbox is no longer valid and abort the
readback.

2. On shutdown. Once the RenderWidgetHostViewAura is shutdown, there
is no way to easily tell if the GLHelper attached to the OwnedMailbox
being stored in copy request callbacks is still alive. Since a readback
on the RenderWidgetHostViewAura can not complete once the RWHVA class
is destroyed anyways, we can destroy the contents of the OwnedMailboxes
in flight at that time. This way when the readback is later aborted,
the OwnedMailbox does not try to clean itself up with a potentially
destroyed GLHelper.

R=piman@chromium.org
BUG=270918

Review URL: https://codereview.chromium.org/120913004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243971 0039d316-1c4b-4281-b951-d872f2087c98

cc/test/test_context_support.cc

@@ -19,11 +19,19 @@ TestContextSupport::~TestContextSupport() {}
 void TestContextSupport::SignalSyncPoint(uint32 sync_point,
                                          const base::Closure& callback) {
   sync_point_callbacks_.push_back(callback);
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestContextSupport::CallAllSyncPointCallbacks,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void TestContextSupport::SignalQuery(uint32 query,
                                      const base::Closure& callback) {
   sync_point_callbacks_.push_back(callback);
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestContextSupport::CallAllSyncPointCallbacks,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void TestContextSupport::SetSurfaceVisible(bool visible) {
@@ -53,7 +61,6 @@ void TestContextSupport::Swap() {
   base::MessageLoop::current()->PostTask(
       FROM_HERE, base::Bind(&TestContextSupport::OnSwapBuffersComplete,
                             weak_ptr_factory_.GetWeakPtr()));
-  CallAllSyncPointCallbacks();
 }
 
 void TestContextSupport::PartialSwapBuffers(gfx::Rect sub_buffer) {
@@ -62,7 +69,6 @@ void TestContextSupport::PartialSwapBuffers(gfx::Rect sub_buffer) {
   base::MessageLoop::current()->PostTask(
       FROM_HERE, base::Bind(&TestContextSupport::OnSwapBuffersComplete,
                             weak_ptr_factory_.GetWeakPtr()));
-  CallAllSyncPointCallbacks();
 }
 
 void TestContextSupport::SetSwapBuffersCompleteCallback(

content/browser/aura/owned_mailbox.cc

@@ -18,11 +18,8 @@ OwnedMailbox::OwnedMailbox(GLHelper* gl_helper)
 }
 
 OwnedMailbox::~OwnedMailbox() {
-  ImageTransportFactory::GetInstance()->RemoveObserver(this);
-  if (gl_helper_) {
-    gl_helper_->WaitSyncPoint(sync_point_);
-    gl_helper_->DeleteTexture(texture_id_);
-  }
+  if (gl_helper_)
+    Destroy();
 }
 
 void OwnedMailbox::UpdateSyncPoint(uint32 sync_point) {
@@ -30,7 +27,8 @@ void OwnedMailbox::UpdateSyncPoint(uint32 sync_point) {
     sync_point_ = sync_point;
 }
 
-void OwnedMailbox::OnLostResources() {
+void OwnedMailbox::Destroy() {
+  ImageTransportFactory::GetInstance()->RemoveObserver(this);
   gl_helper_->WaitSyncPoint(sync_point_);
   gl_helper_->DeleteTexture(texture_id_);
   texture_id_ = 0;
@@ -39,4 +37,9 @@ void OwnedMailbox::OnLostResources() {
   gl_helper_ = NULL;
 }
 
+void OwnedMailbox::OnLostResources() {
+  if (gl_helper_)
+    Destroy();
+}
+
 }  // namespace content

content/browser/aura/owned_mailbox.h

@@ -23,10 +23,12 @@ class OwnedMailbox : public base::RefCounted<OwnedMailbox>,
   const gpu::Mailbox& mailbox() const { return mailbox_; }
 
   void UpdateSyncPoint(uint32 sync_point);
+  void Destroy();
 
  protected:
   virtual ~OwnedMailbox();
 
+  // ImageTransportFactoryObserver implementation.
   virtual void OnLostResources() OVERRIDE;
 
  private:

content/browser/renderer_host/render_widget_host_view_aura.cc

@@ -726,6 +726,11 @@ scoped_ptr<ResizeLock> RenderWidgetHostViewAura::CreateResizeLock(
       base::TimeDelta::FromMilliseconds(kResizeLockTimeoutMs)));
 }
 
+void RenderWidgetHostViewAura::RequestCopyOfOutput(
+    scoped_ptr<cc::CopyOutputRequest> request) {
+  window_->layer()->RequestCopyOfOutput(request.Pass());
+}
+
 gfx::NativeView RenderWidgetHostViewAura::GetNativeView() const {
   return window_;
 }
@@ -1065,7 +1070,7 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurface(
   gfx::Rect src_subrect_in_pixel =
       ConvertRectToPixel(current_device_scale_factor_, src_subrect);
   request->set_area(src_subrect_in_pixel);
-  window_->layer()->RequestCopyOfOutput(request.Pass());
+  RequestCopyOfOutput(request.Pass());
 }
 
 void RenderWidgetHostViewAura::CopyFromCompositingSurfaceToVideoFrame(
@@ -1087,6 +1092,8 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurfaceToVideoFrame(
                    ImageTransportFactory::GetInstance()->GetGLHelper()) {
       subscriber_texture = new OwnedMailbox(helper);
     }
+    if (subscriber_texture.get())
+      active_frame_subscriber_textures_.insert(subscriber_texture.get());
   }
 
   scoped_ptr<cc::CopyOutputRequest> request =
@@ -1100,11 +1107,11 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurfaceToVideoFrame(
   gfx::Rect src_subrect_in_pixel =
       ConvertRectToPixel(current_device_scale_factor_, src_subrect);
   request->set_area(src_subrect_in_pixel);
-  if (subscriber_texture) {
+  if (subscriber_texture.get()) {
     request->SetTextureMailbox(cc::TextureMailbox(
         subscriber_texture->mailbox(), subscriber_texture->sync_point()));
   }
-  window_->layer()->RequestCopyOfOutput(request.Pass());
+  RequestCopyOfOutput(request.Pass());
 }
 
 bool RenderWidgetHostViewAura::CanCopyToBitmap() const {
@@ -1906,6 +1913,26 @@ void RenderWidgetHostViewAura::PrepareBitmapCopyOutputResult(
   callback.Run(true, bitmap);
 }
 
+// static
+void RenderWidgetHostViewAura::ReturnSubscriberTexture(
+    base::WeakPtr<RenderWidgetHostViewAura> rwhva,
+    scoped_refptr<OwnedMailbox> subscriber_texture,
+    uint32 sync_point) {
+  if (!subscriber_texture.get())
+    return;
+  if (!rwhva)
+    return;
+  DCHECK_NE(
+      rwhva->active_frame_subscriber_textures_.count(subscriber_texture.get()),
+      0u);
+
+  subscriber_texture->UpdateSyncPoint(sync_point);
+
+  rwhva->active_frame_subscriber_textures_.erase(subscriber_texture.get());
+  if (rwhva->frame_subscriber_ && subscriber_texture->texture_id())
+    rwhva->idle_frame_subscriber_textures_.push_back(subscriber_texture);
+}
+
 void RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinishedForVideo(
     base::WeakPtr<RenderWidgetHostViewAura> rwhva,
     const base::Callback<void(bool)>& callback,
@@ -1917,17 +1944,12 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinishedForVideo(
   GLHelper* gl_helper = ImageTransportFactory::GetInstance()->GetGLHelper();
   uint32 sync_point = gl_helper ? gl_helper->InsertSyncPoint() : 0;
   if (release_callback) {
+    // A release callback means the texture came from the compositor, so there
+    // should be no |subscriber_texture|.
     DCHECK(!subscriber_texture);
     release_callback->Run(sync_point, false);
-  } else {
-    // If there's no release callback, then the texture is from
-    // idle_frame_subscriber_textures_ and we can put it back there.
-    DCHECK(subscriber_texture);
-    subscriber_texture->UpdateSyncPoint(sync_point);
-    if (rwhva && rwhva->frame_subscriber_ && subscriber_texture->texture_id())
-      rwhva->idle_frame_subscriber_textures_.push_back(subscriber_texture);
-    subscriber_texture = NULL;
   }
+  ReturnSubscriberTexture(rwhva, subscriber_texture, sync_point);
 }
 
 // static
@@ -1938,6 +1960,8 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurfaceHasResultForVideo(
     const base::Callback<void(bool)>& callback,
     scoped_ptr<cc::CopyOutputResult> result) {
   base::ScopedClosureRunner scoped_callback_runner(base::Bind(callback, false));
+  base::ScopedClosureRunner scoped_return_subscriber_texture(
+      base::Bind(&ReturnSubscriberTexture, rwhva, subscriber_texture, 0));
 
   if (!rwhva)
     return;
@@ -1962,8 +1986,6 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurfaceHasResultForVideo(
   if (region_in_frame.IsEmpty())
     return;
 
-  // We only handle texture readbacks for now. If the compositor is in software
-  // mode, we could produce a software-backed VideoFrame here as well.
   if (!result->HasTexture()) {
     DCHECK(result->HasBitmap());
     scoped_ptr<SkBitmap> bitmap = result->TakeBitmap();
@@ -1998,6 +2020,8 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurfaceHasResultForVideo(
   GLHelper* gl_helper = factory->GetGLHelper();
   if (!gl_helper)
     return;
+  if (subscriber_texture.get() && !subscriber_texture->texture_id())
+    return;
 
   cc::TextureMailbox texture_mailbox;
   scoped_ptr<cc::SingleReleaseCallback> release_callback;
@@ -2042,6 +2066,7 @@ void RenderWidgetHostViewAura::CopyFromCompositingSurfaceHasResultForVideo(
   }
 
   ignore_result(scoped_callback_runner.Release());
+  ignore_result(scoped_return_subscriber_texture.Release());
   base::Callback<void(bool result)> finished_callback = base::Bind(
       &RenderWidgetHostViewAura::CopyFromCompositingSurfaceFinishedForVideo,
       rwhva->AsWeakPtr(),
@@ -3225,6 +3250,7 @@ void RenderWidgetHostViewAura::OnLostResources() {
   UpdateExternalTexture();
 
   idle_frame_subscriber_textures_.clear();
+  yuv_readback_pipeline_.reset();
 
   // Make sure all ImageTransportClients are deleted now that the context those
   // are using is becoming invalid. This sends pending ACKs and needs to happen
@@ -3277,6 +3303,16 @@ RenderWidgetHostViewAura::~RenderWidgetHostViewAura() {
   if (resource_collection_.get())
     resource_collection_->SetClient(NULL);
 
+  // An OwnedMailbox should not refer to the GLHelper anymore once the RWHVA is
+  // destroyed, as it may then outlive the GLHelper.
+  for (std::set<OwnedMailbox*>::iterator it =
+           active_frame_subscriber_textures_.begin();
+       it != active_frame_subscriber_textures_.end();
+       ++it) {
+    (*it)->Destroy();
+  }
+  active_frame_subscriber_textures_.clear();
+
 #if defined(OS_WIN)
   if (::IsWindow(plugin_parent_window_))
     ::DestroyWindow(plugin_parent_window_);

content/browser/renderer_host/render_widget_host_view_aura.h

@@ -6,6 +6,7 @@
 #define CONTENT_BROWSER_RENDERER_HOST_RENDER_WIDGET_HOST_VIEW_AURA_H_
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -48,6 +49,7 @@ class ScopedTooltipDisabler;
 }
 
 namespace cc {
+class CopyOutputRequest;
 class CopyOutputResult;
 class DelegatedFrameData;
 }
@@ -371,6 +373,8 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
   virtual bool ShouldCreateResizeLock();
   virtual scoped_ptr<ResizeLock> CreateResizeLock(bool defer_compositor_lock);
 
+  virtual void RequestCopyOfOutput(scoped_ptr<cc::CopyOutputRequest> request);
+
   // Exposed for tests.
   aura::Window* window() { return window_; }
   gfx::Size current_frame_size() const { return current_frame_size_; }
@@ -404,6 +408,8 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraTest, SoftwareDPIChange);
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraTest,
                            UpdateCursorIfOverSelf);
+  FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewAuraCopyRequestTest,
+                           DestroyedAfterCopyRequest);
 
   class WindowObserver;
   friend class WindowObserver;
@@ -506,6 +512,10 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
       scoped_refptr<OwnedMailbox> subscriber_texture,
       scoped_ptr<cc::SingleReleaseCallback> release_callback,
       bool result);
+  static void ReturnSubscriberTexture(
+      base::WeakPtr<RenderWidgetHostViewAura> rwhva,
+      scoped_refptr<OwnedMailbox> subscriber_texture,
+      uint32 sync_point);
 
   ui::Compositor* GetCompositor() const;
 
@@ -765,6 +775,7 @@ class CONTENT_EXPORT RenderWidgetHostViewAura
   // Subscriber that listens to frame presentation events.
   scoped_ptr<RenderWidgetHostViewFrameSubscriber> frame_subscriber_;
   std::vector<scoped_refptr<OwnedMailbox> > idle_frame_subscriber_textures_;
+  std::set<OwnedMailbox*> active_frame_subscriber_textures_;
 
   // YUV readback pipeline.
   scoped_ptr<content::ReadbackYUVInterface>

content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

@@ -7,9 +7,11 @@
 #include "base/basictypes.h"
 #include "base/memory/shared_memory.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/output/compositor_frame_metadata.h"
+#include "cc/output/copy_output_request.h"
 #include "cc/output/gl_frame_data.h"
 #include "content/browser/aura/resize_lock.h"
 #include "content/browser/browser_thread_impl.h"
@@ -18,6 +20,7 @@
 #include "content/common/gpu/gpu_messages.h"
 #include "content/common/input_messages.h"
 #include "content/common/view_messages.h"
+#include "content/port/browser/render_widget_host_view_frame_subscriber.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_browser_context.h"
@@ -113,6 +116,34 @@ class TestWindowObserver : public aura::WindowObserver {
   DISALLOW_COPY_AND_ASSIGN(TestWindowObserver);
 };
 
+class FakeFrameSubscriber : public RenderWidgetHostViewFrameSubscriber {
+ public:
+  FakeFrameSubscriber(gfx::Size size, base::Callback<void(bool)> callback)
+      : size_(size), callback_(callback) {}
+
+  virtual bool ShouldCaptureFrame(base::TimeTicks present_time,
+                                  scoped_refptr<media::VideoFrame>* storage,
+                                  DeliverFrameCallback* callback) OVERRIDE {
+    *storage = media::VideoFrame::CreateFrame(media::VideoFrame::YV12,
+                                              size_,
+                                              gfx::Rect(size_),
+                                              size_,
+                                              base::TimeDelta());
+    *callback = base::Bind(&FakeFrameSubscriber::CallbackMethod, callback_);
+    return true;
+  }
+
+  static void CallbackMethod(base::Callback<void(bool)> callback,
+                             base::TimeTicks timestamp,
+                             bool success) {
+    callback.Run(success);
+  }
+
+ private:
+  gfx::Size size_;
+  base::Callback<void(bool)> callback_;
+};
+
 class FakeRenderWidgetHostViewAura : public RenderWidgetHostViewAura {
  public:
   FakeRenderWidgetHostViewAura(RenderWidgetHost* widget)
@@ -132,6 +163,11 @@ class FakeRenderWidgetHostViewAura : public RenderWidgetHostViewAura {
         new FakeResizeLock(desired_size, defer_compositor_lock));
   }
 
+  virtual void RequestCopyOfOutput(scoped_ptr<cc::CopyOutputRequest> request)
+      OVERRIDE {
+    last_copy_request_ = request.Pass();
+  }
+
   void RunOnCompositingDidCommit() {
     OnCompositingDidCommit(window()->GetDispatcher()->host()->compositor());
   }
@@ -146,6 +182,7 @@ class FakeRenderWidgetHostViewAura : public RenderWidgetHostViewAura {
 
   bool has_resize_lock_;
   gfx::Size last_frame_size_;
+  scoped_ptr<cc::CopyOutputRequest> last_copy_request_;
 };
 
 class RenderWidgetHostViewAuraTest : public testing::Test {
@@ -153,7 +190,7 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
   RenderWidgetHostViewAuraTest()
       : browser_thread_for_ui_(BrowserThread::UI, &message_loop_) {}
 
-  virtual void SetUp() {
+  void SetUpEnvironment() {
     ImageTransportFactory::InitializeForUnitTests(
         scoped_ptr<ui::ContextFactory>(new ui::TestContextFactory));
     aura_test_helper_.reset(new aura::test::AuraTestHelper(&message_loop_));
@@ -182,7 +219,7 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
     view_ = new FakeRenderWidgetHostViewAura(widget_host_);
   }
 
-  virtual void TearDown() {
+  void TearDownEnvironment() {
     sink_ = NULL;
     process_host_ = NULL;
     if (view_)
@@ -200,6 +237,10 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
     ImageTransportFactory::Terminate();
   }
 
+  virtual void SetUp() { SetUpEnvironment(); }
+
+  virtual void TearDown() { TearDownEnvironment(); }
+
  protected:
   base::MessageLoopForUI message_loop_;
   BrowserThreadImpl browser_thread_for_ui_;
@@ -224,6 +265,19 @@ class RenderWidgetHostViewAuraTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewAuraTest);
 };
 
+class RenderWidgetHostViewAuraShutdownTest
+    : public RenderWidgetHostViewAuraTest {
+ public:
+  RenderWidgetHostViewAuraShutdownTest() {}
+
+  virtual void TearDown() OVERRIDE {
+    // No TearDownEnvironment here, we do this explicitly during the test.
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewAuraShutdownTest);
+};
+
 // A layout manager that always resizes a child to the root window size.
 class FullscreenLayoutManager : public aura::LayoutManager {
  public:
@@ -1218,4 +1272,95 @@ TEST_F(RenderWidgetHostViewAuraTest, SoftwareDPIChange) {
   EXPECT_NE(frame_provider.get(), view_->frame_provider_.get());
 }
 
+class RenderWidgetHostViewAuraCopyRequestTest
+    : public RenderWidgetHostViewAuraShutdownTest {
+ public:
+  RenderWidgetHostViewAuraCopyRequestTest()
+      : callback_count_(0), result_(false) {}
+
+  void CallbackMethod(bool result) {
+    result_ = result;
+    callback_count_++;
+  }
+
+  int callback_count_;
+  bool result_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RenderWidgetHostViewAuraCopyRequestTest);
+};
+
+TEST_F(RenderWidgetHostViewAuraCopyRequestTest, DestroyedAfterCopyRequest) {
+  gfx::Rect view_rect(100, 100);
+  scoped_ptr<cc::CopyOutputRequest> request;
+
+  view_->InitAsChild(NULL);
+  aura::client::ParentWindowWithContext(
+      view_->GetNativeView(),
+      parent_view_->GetNativeView()->GetRootWindow(),
+      gfx::Rect());
+  view_->SetSize(view_rect.size());
+  view_->WasShown();
+
+  scoped_ptr<FakeFrameSubscriber> frame_subscriber(new FakeFrameSubscriber(
+      view_rect.size(),
+      base::Bind(&RenderWidgetHostViewAuraCopyRequestTest::CallbackMethod,
+                 base::Unretained(this))));
+
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_FALSE(view_->last_copy_request_);
+
+  view_->BeginFrameSubscription(
+      frame_subscriber.PassAs<RenderWidgetHostViewFrameSubscriber>());
+  view_->OnSwapCompositorFrame(
+      1, MakeDelegatedFrame(1.f, view_rect.size(), gfx::Rect(view_rect)));
+
+  EXPECT_EQ(0, callback_count_);
+  EXPECT_TRUE(view_->last_copy_request_);
+  EXPECT_TRUE(view_->last_copy_request_->has_texture_mailbox());
+  request = view_->last_copy_request_.Pass();
+
+  // There should be one subscriber texture in flight.
+  EXPECT_EQ(1u, view_->active_frame_subscriber_textures_.size());
+
+  // Send back the mailbox included in the request. There's no release callback
+  // since the mailbox came from the RWHVA originally.
+  request->SendTextureResult(view_rect.size(),
+                             request->texture_mailbox(),
+                             scoped_ptr<cc::SingleReleaseCallback>());
+
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();
+
+  // The callback should succeed.
+  EXPECT_EQ(0u, view_->active_frame_subscriber_textures_.size());
+  EXPECT_EQ(1, callback_count_);
+  EXPECT_TRUE(result_);
+
+  view_->OnSwapCompositorFrame(
+      1, MakeDelegatedFrame(1.f, view_rect.size(), gfx::Rect(view_rect)));
+
+  EXPECT_EQ(1, callback_count_);
+  request = view_->last_copy_request_.Pass();
+
+  // There should be one subscriber texture in flight again.
+  EXPECT_EQ(1u, view_->active_frame_subscriber_textures_.size());
+
+  // Destroy the RenderWidgetHostViewAura and ImageTransportFactory.
+  TearDownEnvironment();
+
+  // Send back the mailbox included in the request. There's no release callback
+  // since the mailbox came from the RWHVA originally.
+  request->SendTextureResult(view_rect.size(),
+                             request->texture_mailbox(),
+                             scoped_ptr<cc::SingleReleaseCallback>());
+
+  // Because the copy request callback may be holding state within it, that
+  // state must handle the RWHVA and ImageTransportFactory going away before the
+  // callback is called. This test passes if it does not crash as a result of
+  // these things being destroyed.
+  EXPECT_EQ(2, callback_count_);
+  EXPECT_FALSE(result_);
+}
+
 }  // namespace content