CopyOutputResult must have a working release callback when received over mojo

cc/ipc/copy_output_result_struct_traits.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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
+    // 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 {
+    if (!release_callback_)
+      return;
+    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,
          std::unique_ptr<cc::CopyOutputResult>* out_p) {
   auto result = cc::CopyOutputResult::CreateEmptyResult();

[danakj, 2017/02/14 17:49:11]

Thanks this LG. What I think gets very confusing is that this is a friend of
CopyOutputResult, so it's building the private members, instead of using the
constructors. If it did the latter, then it would be much more clear which
fields can go together and which can not. And we'd never end up with a
CopyOutputResult with HasBitmap() and HasTexture() true because there'd be no
way to construct such a thing.

Why did you choose to make this a friend instead of collecting the various
fields locally here and then invoking the appropriate constructor?

[Saman Sami, 2017/02/14 22:17:04]

It has to be a friend so we can return the private members in the getters. Using
the constructors might help a little bit but not too much. Remember that the
object that we receive can be totally invalid and we still need the checks
below. But I'll give it a shot to see how it can improve my code.

 
   if (!data.ReadSize(&result->size_))
     return false;
 
   result->bitmap_ = base::MakeUnique<SkBitmap>();
   if (!data.ReadBitmap(result->bitmap_.get()))
     return false;
 
   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())
+    return false;
+
+  // We can't have both a bitmap and a texture at the same time.
+  if (result->HasBitmap() && result->HasTexture())
+    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