Reland r206537 - cc: Don't return mailboxes if the TextureLayer is removed from the tree.

cc/layers/texture_layer.cc

Index: cc/layers/texture_layer.cc
diff --git a/cc/layers/texture_layer.cc b/cc/layers/texture_layer.cc
index d09963139101e955de76cb914513e31d51777b6f..37169dc72c8f25e4b820c517cc532709f3749720 100644
--- a/cc/layers/texture_layer.cc
+++ b/cc/layers/texture_layer.cc
@@ -4,6 +4,9 @@
 
 #include "cc/layers/texture_layer.h"
 
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/message_loop/message_loop_proxy.h"
 #include "cc/base/thread.h"
 #include "cc/layers/texture_layer_client.h"
 #include "cc/layers/texture_layer_impl.h"
@@ -12,28 +15,6 @@
 
 namespace cc {
 
-namespace {
-
-void RunCallback(
-    const TextureMailbox::ReleaseCallback& callback,
-    unsigned sync_point,
-    bool lost_resource) {
-  callback.Run(sync_point, lost_resource);
-}
-
-void PostCallbackToThread(
-    Thread* thread,
-    const TextureMailbox::ReleaseCallback& callback,
-    unsigned sync_point,
-    bool lost_resource) {
-  if (!callback.is_null()) {
-    thread->PostTask(base::Bind(&RunCallback, callback,
-                                sync_point, lost_resource));
-  }
-}
-
-}  // namespace
-
 scoped_refptr<TextureLayer> TextureLayer::Create(TextureLayerClient* client) {
   return scoped_refptr<TextureLayer>(new TextureLayer(client, false));
 }
@@ -55,7 +36,7 @@ TextureLayer::TextureLayer(TextureLayerClient* client, bool uses_mailbox)
       context_lost_(false),
       content_committed_(false),
       texture_id_(0),
-      own_mailbox_(false) {
+      needs_set_mailbox_(false) {
   vertex_opacity_[0] = 1.0f;
   vertex_opacity_[1] = 1.0f;
   vertex_opacity_[2] = 1.0f;
@@ -69,8 +50,6 @@ TextureLayer::~TextureLayer() {
     if (rate_limit_context_ && client_)
       layer_tree_host()->StopRateLimiter(client_->Context3d());
   }
-  if (own_mailbox_)
-    texture_mailbox_.RunReleaseCallback(texture_mailbox_.sync_point(), false);
 }
 
 void TextureLayer::ClearClient() {
@@ -147,13 +126,10 @@ void TextureLayer::SetTextureId(unsigned id) {
 
 void TextureLayer::SetTextureMailbox(const TextureMailbox& mailbox) {
   DCHECK(uses_mailbox_);
-  if (own_mailbox_)
-    DCHECK(!mailbox.IsValid() || !mailbox.Equals(texture_mailbox_));
-  // If we never commited the mailbox, we need to release it here
-  if (own_mailbox_)
-    texture_mailbox_.RunReleaseCallback(texture_mailbox_.sync_point(), false);
-  texture_mailbox_ = mailbox;
-  own_mailbox_ = true;
+  DCHECK(!mailbox.IsValid() || !holder_ || !mailbox.Equals(holder_->mailbox()));
+  // If we never commited the mailbox, we need to release it here.
+  holder_ = mailbox.IsValid() ? new MailboxHolder(mailbox) : NULL;
+  needs_set_mailbox_ = true;
   SetNeedsCommit();
 }
 
@@ -174,11 +150,16 @@ void TextureLayer::SetNeedsDisplayRect(const gfx::RectF& dirty_rect) {
 void TextureLayer::SetLayerTreeHost(LayerTreeHost* host) {
   if (texture_id_ && layer_tree_host() && host != layer_tree_host())
     layer_tree_host()->AcquireLayerTextures();
+  // If we're removed from the tree, the TextureLayerImpl will be destroyed, and
+  // we will need to set the mailbox again on a new TextureLayerImpl the next
+  // time we push.
+  if (!host && uses_mailbox_ && holder_)
+    needs_set_mailbox_ = true;
   Layer::SetLayerTreeHost(host);
 }
 
 bool TextureLayer::DrawsContent() const {
-  return (client_ || texture_id_ || texture_mailbox_.IsValid()) &&
+  return (client_ || texture_id_ || holder_.get()) &&
          !context_lost_ && Layer::DrawsContent();
 }
 
@@ -213,13 +194,15 @@ void TextureLayer::PushPropertiesTo(LayerImpl* layer) {
   texture_layer->set_uv_bottom_right(uv_bottom_right_);
   texture_layer->set_vertex_opacity(vertex_opacity_);
   texture_layer->set_premultiplied_alpha(premultiplied_alpha_);
-  if (uses_mailbox_ && own_mailbox_) {
-    Thread* main_thread = layer_tree_host()->proxy()->MainThread();
-    TextureMailbox::ReleaseCallback callback = base::Bind(
-        &PostCallbackToThread, main_thread, texture_mailbox_.callback());
-    texture_layer->SetTextureMailbox(
-        texture_mailbox_.CopyWithNewCallback(callback));
-    own_mailbox_ = false;
+  if (uses_mailbox_ && needs_set_mailbox_) {
+    TextureMailbox texture_mailbox;
+    if (holder_) {
+      TextureMailbox::ReleaseCallback callback =
+          holder_->GetCallbackForImplThread();
+      texture_mailbox = holder_->mailbox().CopyWithNewCallback(callback);
+    }
+    texture_layer->SetTextureMailbox(texture_mailbox);
+    needs_set_mailbox_ = false;
   } else {
     texture_layer->set_texture_id(texture_id_);
   }
@@ -237,4 +220,47 @@ bool TextureLayer::CanClipSelf() const {
   return true;
 }
 
+TextureLayer::MailboxHolder::MailboxHolder(const TextureMailbox& mailbox)
+    : message_loop_(base::MessageLoopProxy::current()),
+      mailbox_(mailbox),
+      sync_point_(mailbox.sync_point()),
+      is_lost_(false) {
+}
+
+TextureLayer::MailboxHolder::~MailboxHolder() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  if (mailbox_.callback().is_null())
+    return;
+  mailbox_.RunReleaseCallback(sync_point_, is_lost_);
+}
+
+void TextureLayer::MailboxHolder::Return(unsigned sync_point, bool is_lost) {
+  sync_point_ = sync_point;
+  is_lost_ = is_lost;
+}
+
+TextureMailbox::ReleaseCallback
+TextureLayer::MailboxHolder::GetCallbackForImplThread() {
+  // Note: we refcount manually here and pass Unretained, because otherwise the
+  // callback would keep a reference on the MailboxHolder, which may be released
+  // when the callback is destroyed on the impl thread.
+  AddRef();
+  return base::Bind(&MailboxHolder::ReturnAndReleaseOnImplThread,
+                    Unretained(this));
+}
+
+void TextureLayer::MailboxHolder::ReturnAndReleaseOnMainThread(
+    unsigned sync_point, bool is_lost) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  Return(sync_point, is_lost);
+  Release();
+}
+
+void TextureLayer::MailboxHolder::ReturnAndReleaseOnImplThread(
+    unsigned sync_point, bool is_lost) {
+  message_loop_->PostTask(FROM_HERE, base::Bind(

[danakj, 2013/06/18 00:53:24]

I guess the downside here is we can't DCHECK that we called this function, we
would just leak the MailboxHolder if we failed to call it.

Maybe we could get around that by keeping track in here if the impl thread has a
ref on the object, and if it does when we get to the ~ then we post task in the
~ to the main thread, otherwise run the callback immediately.
GetCallbackForImplThread() would just have to set a bool ref_on_impl_thread_ =
true;

Is that idea crazy or helpful, I'm not sure?

[piman, 2013/06/18 02:10:58]

Following our discussion, see the new version. Not entirely sure whether the
MainThreadReference is overkill or not...

+      &MailboxHolder::ReturnAndReleaseOnMainThread,
+      Unretained(this), sync_point, is_lost));
+}
+
 }  // namespace cc