CopyOutputResult must have a working release callback when received over mojo

cc/ipc/copy_output_result_struct_traits.cc

Index: cc/ipc/copy_output_result_struct_traits.cc
diff --git a/cc/ipc/copy_output_result_struct_traits.cc b/cc/ipc/copy_output_result_struct_traits.cc
index 0398fe115cd52463b56fbf2a1a1540d3d96b2935..4548de3a43181412a9996d02d3e747bea4640c2c 100644
--- a/cc/ipc/copy_output_result_struct_traits.cc
+++ b/cc/ipc/copy_output_result_struct_traits.cc
@@ -4,9 +4,78 @@
 
 #include "cc/ipc/copy_output_result_struct_traits.h"
 
+#include "mojo/public/cpp/bindings/strong_binding.h"
+
+namespace {
+
+// This class retains the release_callback_ of the CopyOutputResult that is
+// being sent over mojo. A TextureMailboxReleaserPtr that talks to this impl
+// object will be sent over mojo instead of the release_callback_ (which is not
+// serializable). Once the client calls Release, the release_callback_ will be
+// called. An object of this class will remain alive until the MessagePipe
+// attached to it goes away (i.e. StrongBinding is used).
+class TextureMailboxReleaserImpl : public cc::mojom::TextureMailboxReleaser {
+ public:
+  TextureMailboxReleaserImpl(
+      std::unique_ptr<cc::SingleReleaseCallback> release_callback)
+      : release_callback_(std::move(release_callback)) {
+    DCHECK(release_callback_);
+  }
+
+  ~TextureMailboxReleaserImpl() override {
+    // If the client fails to call Release, we should do it ourselves because

[danakj, 2017/02/13 20:50:16]

This should be a DCHECK instead I think, the same as SingleReleaseCallback is.
Or just do nothing and let the SingleReleaseCallback DCHECK.

[Saman Sami, 2017/02/13 21:03:01]

I'm not sure we want the display compositor to crash when some other process
fails to call the SingleReleaseCallback?

[danakj, 2017/02/13 22:43:17]

Oh this is on the other process, okay. Ya I agree. I thought this is on the
client, my bad.

+    // release_callback_ will fail if it's never called.
+    if (release_callback_)
+      release_callback_->Run(gpu::SyncToken(), true);
+  }
+
+  // mojom::TextureMailboxReleaser implementation:
+  void Release(const gpu::SyncToken& sync_token, bool is_lost) override {
+    DCHECK(release_callback_)

[danakj, 2017/02/13 20:50:16]

The SingleReleaseCallback already does this why double it?

[Saman Sami, 2017/02/13 21:03:01]

SingleReleaseCallback doesn't do anything because we reset it below. I think
this DCHECK is more detailed than what unique_ptr provides. We reset the release
callback because we want to know whether it's called or not because if it's not
we call it in the destructor.

[danakj, 2017/02/13 22:43:17]

Oh I was thinking if the reset() isnt there, but ya this is on the service side
so seems good to deal with. But this would DCHECK now if the client misbehaved.
We should just do nothing in this case.

[Saman Sami, 2017/02/13 23:23:43]

That's right. I'll get rid of the DCHECK.

+        << "TextureMailboxReleaser::Release cannot be called twice";
+    release_callback_->Run(sync_token, is_lost);
+    release_callback_.reset();
+  }
+
+ private:
+  std::unique_ptr<cc::SingleReleaseCallback> release_callback_;
+};
+
+void Release(cc::mojom::TextureMailboxReleaserPtr ptr,
+             const gpu::SyncToken& sync_token,
+             bool is_lost) {
+  ptr->Release(sync_token, is_lost);
+}
+
+}  // namespace
+
 namespace mojo {
 
 // static
+const SkBitmap& StructTraits<cc::mojom::CopyOutputResultDataView,
+                             std::unique_ptr<cc::CopyOutputResult>>::
+    bitmap(const std::unique_ptr<cc::CopyOutputResult>& result) {
+  static SkBitmap* null_bitmap = new SkBitmap();
+  if (!result->bitmap_)
+    return *null_bitmap;
+  return *result->bitmap_;
+}
+
+// static
+cc::mojom::TextureMailboxReleaserPtr
+StructTraits<cc::mojom::CopyOutputResultDataView,
+             std::unique_ptr<cc::CopyOutputResult>>::
+    releaser(const std::unique_ptr<cc::CopyOutputResult>& result) {
+  if (!result->release_callback_)
+    return {};
+  cc::mojom::TextureMailboxReleaserPtr releaser;
+  auto impl = base::MakeUnique<TextureMailboxReleaserImpl>(
+      std::move(result->release_callback_));
+  MakeStrongBinding(std::move(impl), MakeRequest(&releaser));
+  return releaser;
+}
+
+// static
 bool StructTraits<cc::mojom::CopyOutputResultDataView,
                   std::unique_ptr<cc::CopyOutputResult>>::
     Read(cc::mojom::CopyOutputResultDataView data,
@@ -23,19 +92,22 @@ bool StructTraits<cc::mojom::CopyOutputResultDataView,
   if (!data.ReadTextureMailbox(&result->texture_mailbox_))
     return false;
 
+  auto releaser = data.TakeReleaser<cc::mojom::TextureMailboxReleaserPtr>();
+  if (releaser) {
+    // CopyOutputResult does not have a TextureMailboxReleaserPtr member.
+    // We use base::Bind to turn TextureMailboxReleaser::Release into a
+    // ReleaseCallback.
+    result->release_callback_ = cc::SingleReleaseCallback::Create(
+        base::Bind(Release, base::Passed(&releaser)));
+  }
+
+  // Check that release callback and texture mailbox are set together.
+  if (!!result->release_callback_ != result->texture_mailbox_.IsValid())

[danakj, 2017/02/13 20:50:16]

you could just if (!releaser) return false above right? That's the same thing?

[Saman Sami, 2017/02/13 21:03:01]

What if the result is bitmap?

[danakj, 2017/02/13 22:43:17]

Oh ya ok. This is a nit, but I think it'd maybe be nicer to use the bool values
you have here already to check this rather than going thru result.

We should also reject both bitmap & texture.

[Saman Sami, 2017/02/13 23:23:43]

Added the check for both bitmap and texture.

+    return false;
+
   *out_p = std::move(result);
 
   return true;
 }
 
-// static
-const SkBitmap& StructTraits<cc::mojom::CopyOutputResultDataView,
-                             std::unique_ptr<cc::CopyOutputResult>>::
-    bitmap(const std::unique_ptr<cc::CopyOutputResult>& result) {
-  static SkBitmap* null_bitmap = new SkBitmap();
-  if (!result->bitmap_)
-    return *null_bitmap;
-  return *result->bitmap_;
-}
-
 }  // namespace mojo