[mojo-bindings] Catch bad handle attachments

It is possible for a received message to fail at handle extraction,
rendering the message essentially unusable. Today we silently ignore
this failure, producing an uninitialized Message object which then
continues along down the stack to message dispatch logic. This can
theoretically lead to bindings attempting to access an invalid
serialized message buffer.

This CL guards against that case by treating such a message as a bad
IPC and immediately closing the message pipe which sent it, rather
than propagating it on to dispatch.

TBR=sadrul@chromium.org

Bug: 862593
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I8359e85992e2e00a23996befc880d13897567863
Reviewed-on: https://chromium-review.googlesource.com/1222088
Commit-Queue: Ken Rockot <rockot@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590889}

mojo/public/cpp/bindings/lib/connector.cc

@@ -430,11 +430,29 @@ bool Connector::ReadSingleMessage(MojoResult* read_result) {
   // during message dispatch.
   base::WeakPtr<Connector> weak_self = weak_self_;
 
-  Message message;
-  const MojoResult rv = ReadMessage(message_pipe_.get(), &message);
+  ScopedMessageHandle message_handle;
+  const MojoResult rv = ReadMessageNew(message_pipe_.get(), &message_handle,
+                                       MOJO_READ_MESSAGE_FLAG_NONE);
   *read_result = rv;
 
   if (rv == MOJO_RESULT_OK) {
+    Message message = Message::CreateFromMessageHandle(&message_handle);
+    if (message.IsNull()) {
+      // Even if the read was successful, the Message may still be null if there
+      // was a problem extracting handles from it. We treat this essentially as
+      // a bad IPC because we don't really have a better option.
+      //
+      // We include |heap_profiler_tag_| in the error message since it usually
+      // (via this Connector's owner) provides useful information about which
+      // binding interface is using this Connector.
+      NotifyBadMessage(message_handle.get(),
+                       std::string(heap_profiler_tag_) +
+                           "One or more handle attachments were invalid.");
+      HandleError(false /* force_pipe_reset */,
+                  false /* force_async_handler */);
+      return false;
+    }
+
     base::Optional<ActiveDispatchTracker> dispatch_tracker;
     if (!is_dispatching_ && nesting_observer_) {
       is_dispatching_ = true;

mojo/public/cpp/bindings/lib/message.cc

@@ -157,7 +157,7 @@ void DestroyUnserializedContext(uintptr_t context) {
   delete reinterpret_cast<internal::UnserializedMessageContext*>(context);
 }
 
-ScopedMessageHandle CreateUnserializedMessageObject(
+Message CreateUnserializedMessage(
     std::unique_ptr<internal::UnserializedMessageContext> context) {
   ScopedMessageHandle handle;
   MojoResult rv = mojo::CreateMessage(&handle);
@@ -168,7 +168,8 @@ ScopedMessageHandle CreateUnserializedMessageObject(
       handle->value(), reinterpret_cast<uintptr_t>(context.release()),
       &SerializeUnserializedContext, &DestroyUnserializedContext, nullptr);
   DCHECK_EQ(MOJO_RESULT_OK, rv);
-  return handle;
+
+  return Message::CreateFromMessageHandle(&handle);
 }
 
 }  // namespace
@@ -192,7 +193,7 @@ Message::Message(Message&& other)
 }
 
 Message::Message(std::unique_ptr<internal::UnserializedMessageContext> context)
-    : Message(CreateUnserializedMessageObject(std::move(context))) {}
+    : Message(CreateUnserializedMessage(std::move(context))) {}
 
 Message::Message(uint32_t name,
                  uint32_t flags,
@@ -206,53 +207,53 @@ Message::Message(uint32_t name,
   serialized_ = true;
 }
 
-Message::Message(ScopedMessageHandle handle) {
+// static
+Message Message::CreateFromMessageHandle(ScopedMessageHandle* message_handle) {
+  DCHECK(message_handle);
+  const MessageHandle& handle = message_handle->get();
   DCHECK(handle.is_valid());
 
   uintptr_t context_value = 0;
   MojoResult get_context_result =
-      MojoGetMessageContext(handle->value(), nullptr, &context_value);
+      MojoGetMessageContext(handle.value(), nullptr, &context_value);
   if (get_context_result == MOJO_RESULT_NOT_FOUND) {
     // It's a serialized message. Extract handles if possible.
     uint32_t num_bytes;
     void* buffer;
     uint32_t num_handles = 0;
-    MojoResult rv = MojoGetMessageData(handle->value(), nullptr, &buffer,
+    std::vector<ScopedHandle> handles;
+    MojoResult rv = MojoGetMessageData(handle.value(), nullptr, &buffer,
                                        &num_bytes, nullptr, &num_handles);
     if (rv == MOJO_RESULT_RESOURCE_EXHAUSTED) {
-      handles_.resize(num_handles);
-      rv = MojoGetMessageData(handle->value(), nullptr, &buffer, &num_bytes,
-                              reinterpret_cast<MojoHandle*>(handles_.data()),
+      handles.resize(num_handles);
+      rv = MojoGetMessageData(handle.value(), nullptr, &buffer, &num_bytes,
+                              reinterpret_cast<MojoHandle*>(handles.data()),
                               &num_handles);
-    } else {
-      // No handles, so it's safe to retransmit this message if the caller
-      // really wants to.
-      transferable_ = true;
     }
 
     if (rv != MOJO_RESULT_OK) {
-      // Failed to deserialize handles. Leave the Message uninitialized.
-      return;
+      // Failed to deserialize handles. Return a null message and leave the
+      // |*message_handle| intact.
+      return Message();
     }
 
-    payload_buffer_ = internal::Buffer(buffer, num_bytes, num_bytes);
-    serialized_ = true;
-  } else {
-    DCHECK_EQ(MOJO_RESULT_OK, get_context_result);
-    auto* context =
-        reinterpret_cast<internal::UnserializedMessageContext*>(context_value);
-    // Dummy data address so common header accessors still behave properly. The
-    // choice is V1 reflects unserialized message capabilities: we may or may
-    // not need to support request IDs (which require at least V1), but we never
-    // (for now, anyway) need to support associated interface handles (V2).
-    payload_buffer_ =
-        internal::Buffer(context->header(), sizeof(internal::MessageHeaderV1),
-                         sizeof(internal::MessageHeaderV1));
-    transferable_ = true;
-    serialized_ = false;
+    return Message(std::move(*message_handle), std::move(handles),
+                   internal::Buffer(buffer, num_bytes, num_bytes),
+                   true /* serialized */);
   }
 
-  handle_ = std::move(handle);
+  DCHECK_EQ(MOJO_RESULT_OK, get_context_result);
+  auto* context =
+      reinterpret_cast<internal::UnserializedMessageContext*>(context_value);
+  // Dummy data address so common header accessors still behave properly. The
+  // choice is V1 reflects unserialized message capabilities: we may or may
+  // not need to support request IDs (which require at least V1), but we never
+  // (for now, anyway) need to support associated interface handles (V2).
+  internal::Buffer payload_buffer(context->header(),
+                                  sizeof(internal::MessageHeaderV1),
+                                  sizeof(internal::MessageHeaderV1));
+  return Message(std::move(*message_handle), {}, std::move(payload_buffer),
+                 false /* serialized */);
 }
 
 Message::~Message() = default;
@@ -430,7 +431,8 @@ void Message::SerializeIfNecessary() {
     return;
 
   // Reconstruct this Message instance from the serialized message's handle.
-  *this = Message(std::move(handle_));
+  ScopedMessageHandle handle = std::move(handle_);
+  *this = CreateFromMessageHandle(&handle);
 }
 
 std::unique_ptr<internal::UnserializedMessageContext>
@@ -455,6 +457,16 @@ Message::TakeUnserializedContext(
   return base::WrapUnique(context);
 }
 
+Message::Message(ScopedMessageHandle message_handle,
+                 std::vector<ScopedHandle> attached_handles,
+                 internal::Buffer payload_buffer,
+                 bool serialized)
+    : handle_(std::move(message_handle)),
+      payload_buffer_(std::move(payload_buffer)),
+      handles_(std::move(attached_handles)),
+      transferable_(!serialized || handles_.empty()),
+      serialized_(serialized) {}
+
 bool MessageReceiver::PrefersSerializedMessages() {
   return false;
 }
@@ -491,17 +503,6 @@ ReportBadMessageCallback SyncMessageResponseContext::GetBadMessageCallback() {
   return base::BindOnce(&DoNotifyBadMessage, std::move(response_));
 }
 
-MojoResult ReadMessage(MessagePipeHandle handle, Message* message) {
-  ScopedMessageHandle message_handle;
-  MojoResult rv =
-      ReadMessageNew(handle, &message_handle, MOJO_READ_MESSAGE_FLAG_NONE);
-  if (rv != MOJO_RESULT_OK)
-    return rv;
-
-  *message = Message(std::move(message_handle));
-  return MOJO_RESULT_OK;
-}
-
 void ReportBadMessage(const std::string& error) {
   internal::MessageDispatchContext* context =
       internal::MessageDispatchContext::current();

mojo/public/cpp/bindings/message.h

@@ -74,7 +74,11 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE) Message {
   // If the message had any handles attached, they will be extracted and
   // retrievable via |handles()|. Such messages may NOT be sent back over
   // another message pipe, but are otherwise safe to inspect and pass around.
-  Message(ScopedMessageHandle handle);
+  //
+  // If handles are attached and their extraction fails for any reason,
+  // |*handle| remains unchanged and the returned Message will be null (i.e.
+  // calling IsNull() on it will return |true|).
+  static Message CreateFromMessageHandle(ScopedMessageHandle* message_handle);
 
   ~Message();
 
@@ -90,6 +94,11 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE) Message {
   // Indicates whether this Message is uninitialized.
   bool IsNull() const { return !handle_.is_valid(); }
 
+  // Indicates whether this Message is in valid state. A Message may be in an
+  // invalid state iff it failed partial deserialization during construction
+  // over a ScopedMessageHandle.
+  bool IsValid() const;
+
   // Indicates whether this Message is serialized.
   bool is_serialized() const { return serialized_; }
 
@@ -222,6 +231,14 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE) Message {
 #endif
 
  private:
+  // Internal constructor used by |CreateFromMessageHandle()| when either there
+  // are no attached handles or all attached handles are successfully extracted
+  // from the message object.
+  Message(ScopedMessageHandle message_handle,
+          std::vector<ScopedHandle> attached_handles,
+          internal::Buffer payload_buffer,
+          bool serialized);
+
   ScopedMessageHandle handle_;
 
   // A Buffer which may be used to allocate blocks of data within the message
@@ -358,16 +375,6 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE) SyncMessageResponseContext {
   DISALLOW_COPY_AND_ASSIGN(SyncMessageResponseContext);
 };
 
-// Read a single message from the pipe. The caller should have created the
-// Message, but not called Initialize(). Returns MOJO_RESULT_SHOULD_WAIT if
-// the caller should wait on the handle to become readable. Returns
-// MOJO_RESULT_OK if the message was read successfully and should be
-// dispatched, otherwise returns an error code if something went wrong.
-//
-// NOTE: The message hasn't been validated and may be malformed!
-COMPONENT_EXPORT(MOJO_CPP_BINDINGS_BASE)
-MojoResult ReadMessage(MessagePipeHandle handle, Message* message);
-
 // Reports the currently dispatching Message as bad. Note that this is only
 // legal to call from directly within the stack frame of a message dispatch. If
 // you need to do asynchronous work before you can determine the legitimacy of

mojo/public/cpp/bindings/tests/validation_unittest.cc

@@ -51,7 +51,7 @@ Message CreateRawMessage(size_t size) {
                              nullptr, 0, &options, &buffer, &buffer_size);
   DCHECK_EQ(MOJO_RESULT_OK, rv);
 
-  return Message(std::move(handle));
+  return Message::CreateFromMessageHandle(&handle);
 }
 
 template <typename T>

mojo/public/cpp/test_support/test_utils.h

@@ -21,7 +21,9 @@ bool SerializeAndDeserialize(UserType* input, UserType* output) {
   // This accurately simulates full serialization to ensure that all attached
   // handles are serialized as well. Necessary for DeserializeFromMessage to
   // work properly.
-  message = mojo::Message(message.TakeMojoMessage());
+  mojo::ScopedMessageHandle handle = message.TakeMojoMessage();
+  message = mojo::Message::CreateFromMessageHandle(&handle);
+  DCHECK(!message.IsNull());
 
   return MojomType::DeserializeFromMessage(std::move(message), output);
 }

services/viz/public/cpp/compositing/BUILD.gn

@@ -15,6 +15,7 @@ source_set("tests") {
     "//components/viz/test:test_support",
     "//gpu/ipc/common:struct_traits",
     "//media/capture/mojom:video_capture",
+    "//mojo/public/cpp/test_support:test_utils",
     "//services/service_manager/public/cpp",
     "//services/service_manager/public/cpp:service_test_support",
     "//services/viz/public/interfaces",

services/viz/public/cpp/compositing/struct_traits_unittest.cc

@@ -25,6 +25,7 @@
 #include "ipc/ipc_message_utils.h"
 #include "mojo/public/cpp/base/time_mojom_traits.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
+#include "mojo/public/cpp/test_support/test_utils.h"
 #include "services/viz/public/cpp/compositing/begin_frame_args_struct_traits.h"
 #include "services/viz/public/cpp/compositing/compositor_frame_metadata_struct_traits.h"
 #include "services/viz/public/cpp/compositing/compositor_frame_struct_traits.h"
@@ -67,24 +68,6 @@ namespace {
 
 using StructTraitsTest = testing::Test;
 
-// Test StructTrait serialization and deserialization for copyable type. |input|
-// will be serialized and then deserialized into |output|.
-template <class MojomType, class Type>
-void SerializeAndDeserialize(const Type& input, Type* output) {
-  MojomType::DeserializeFromMessage(
-      mojo::Message(MojomType::SerializeAsMessage(&input).TakeMojoMessage()),
-      output);
-}
-
-// Test StructTrait serialization and deserialization for move only type.
-// |input| will be serialized and then deserialized into |output|.
-template <class MojomType, class Type>
-void SerializeAndDeserialize(Type&& input, Type* output) {
-  MojomType::DeserializeFromMessage(
-      mojo::Message(MojomType::SerializeAsMessage(&input).TakeMojoMessage()),
-      output);
-}
-
 }  // namespace
 
 TEST_F(StructTraitsTest, BeginFrameArgs) {
@@ -107,7 +90,7 @@ TEST_F(StructTraitsTest, BeginFrameArgs) {
   input.animate_only = animate_only;
 
   BeginFrameArgs output;
-  SerializeAndDeserialize<mojom::BeginFrameArgs>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::BeginFrameArgs>(&input, &output);
 
   EXPECT_EQ(source_id, output.source_id);
   EXPECT_EQ(sequence_number, output.sequence_number);
@@ -129,7 +112,7 @@ TEST_F(StructTraitsTest, BeginFrameAck) {
   input.has_damage = has_damage;
 
   BeginFrameAck output;
-  SerializeAndDeserialize<mojom::BeginFrameAck>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::BeginFrameAck>(&input, &output);
 
   EXPECT_EQ(source_id, output.source_id);
   EXPECT_EQ(sequence_number, output.sequence_number);
@@ -185,7 +168,7 @@ TEST_F(StructTraitsTest, FilterOperationBlur) {
   cc::FilterOperation input = cc::FilterOperation::CreateBlurFilter(20);
 
   cc::FilterOperation output;
-  SerializeAndDeserialize<mojom::FilterOperation>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::FilterOperation>(&input, &output);
   ExpectEqual(input, output);
 }
 
@@ -194,7 +177,7 @@ TEST_F(StructTraitsTest, FilterOperationDropShadow) {
       gfx::Point(4, 4), 4.0f, SkColorSetARGB(255, 40, 0, 0));
 
   cc::FilterOperation output;
-  SerializeAndDeserialize<mojom::FilterOperation>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::FilterOperation>(&input, &output);
   ExpectEqual(input, output);
 }
 
@@ -207,7 +190,7 @@ TEST_F(StructTraitsTest, FilterOperationReferenceFilter) {
           nullptr));
 
   cc::FilterOperation output;
-  SerializeAndDeserialize<mojom::FilterOperation>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::FilterOperation>(&input, &output);
   ExpectEqual(input, output);
 }
 
@@ -218,7 +201,7 @@ TEST_F(StructTraitsTest, FilterOperations) {
   input.Append(cc::FilterOperation::CreateZoomFilter(2.0f, 1));
 
   cc::FilterOperations output;
-  SerializeAndDeserialize<mojom::FilterOperations>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::FilterOperations>(&input, &output);
 
   EXPECT_EQ(input.size(), output.size());
   for (size_t i = 0; i < input.size(); ++i) {
@@ -231,7 +214,7 @@ TEST_F(StructTraitsTest, LocalSurfaceId) {
       42, base::UnguessableToken::Deserialize(0x12345678, 0x9abcdef0));
 
   LocalSurfaceId output;
-  SerializeAndDeserialize<mojom::LocalSurfaceId>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::LocalSurfaceId>(&input, &output);
 
   EXPECT_EQ(input, output);
 }
@@ -266,7 +249,8 @@ TEST_F(StructTraitsTest, CopyOutputRequest_BitmapRequest) {
   input->set_source(source);
   EXPECT_TRUE(input->is_scaled());
   std::unique_ptr<CopyOutputRequest> output;
-  SerializeAndDeserialize<mojom::CopyOutputRequest>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::CopyOutputRequest>(&input,
+                                                                &output);
 
   EXPECT_EQ(result_format, output->result_format());
   EXPECT_TRUE(output->is_scaled());
@@ -333,7 +317,8 @@ TEST_F(StructTraitsTest, CopyOutputRequest_TextureRequest) {
           run_loop_for_result.QuitClosure(), result_rect)));
   EXPECT_FALSE(input->is_scaled());
   std::unique_ptr<CopyOutputRequest> output;
-  SerializeAndDeserialize<mojom::CopyOutputRequest>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::CopyOutputRequest>(&input,
+                                                                &output);
 
   EXPECT_EQ(output->result_format(), result_format);
   EXPECT_FALSE(output->is_scaled());
@@ -392,7 +377,7 @@ TEST_F(StructTraitsTest, ResourceSettings) {
   input.use_gpu_memory_buffer_resources = kArbitraryBool;
 
   ResourceSettings output;
-  SerializeAndDeserialize<mojom::ResourceSettings>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::ResourceSettings>(&input, &output);
 
   EXPECT_EQ(input.use_gpu_memory_buffer_resources,
             output.use_gpu_memory_buffer_resources);
@@ -411,7 +396,7 @@ TEST_F(StructTraitsTest, Selection) {
   input.start = start;
   input.end = end;
   Selection<gfx::SelectionBound> output;
-  SerializeAndDeserialize<mojom::Selection>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::Selection>(&input, &output);
   EXPECT_EQ(start, output.start);
   EXPECT_EQ(end, output.end);
 }
@@ -433,7 +418,8 @@ TEST_F(StructTraitsTest, SharedQuadState) {
                    clip_rect, is_clipped, are_contents_opaque, opacity,
                    blend_mode, sorting_context_id);
   SharedQuadState output_sqs;
-  SerializeAndDeserialize<mojom::SharedQuadState>(input_sqs, &output_sqs);
+  mojo::test::SerializeAndDeserialize<mojom::SharedQuadState>(&input_sqs,
+                                                              &output_sqs);
   EXPECT_EQ(quad_to_target_transform, output_sqs.quad_to_target_transform);
   EXPECT_EQ(layer_rect, output_sqs.quad_layer_rect);
   EXPECT_EQ(visible_layer_rect, output_sqs.visible_quad_layer_rect);
@@ -515,7 +501,7 @@ TEST_F(StructTraitsTest, CompositorFrame) {
   input.metadata.begin_frame_ack = begin_frame_ack;
 
   CompositorFrame output;
-  SerializeAndDeserialize<mojom::CompositorFrame>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::CompositorFrame>(&input, &output);
 
   EXPECT_EQ(device_scale_factor, output.metadata.device_scale_factor);
   EXPECT_EQ(root_scroll_offset, output.metadata.root_scroll_offset);
@@ -573,9 +559,9 @@ TEST_F(StructTraitsTest, SurfaceInfo) {
   constexpr float device_scale_factor = 1.234f;
   constexpr gfx::Size size(987, 123);
 
-  const SurfaceInfo input(surface_id, device_scale_factor, size);
+  SurfaceInfo input(surface_id, device_scale_factor, size);
   SurfaceInfo output;
-  SerializeAndDeserialize<mojom::SurfaceInfo>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::SurfaceInfo>(&input, &output);
 
   EXPECT_EQ(input.id(), output.id());
   EXPECT_EQ(input.size_in_pixels(), output.size_in_pixels());
@@ -601,7 +587,7 @@ TEST_F(StructTraitsTest, ReturnedResource) {
   input.lost = lost;
 
   ReturnedResource output;
-  SerializeAndDeserialize<mojom::ReturnedResource>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::ReturnedResource>(&input, &output);
 
   EXPECT_EQ(id, output.id);
   EXPECT_EQ(sync_token, output.sync_token);
@@ -683,7 +669,8 @@ TEST_F(StructTraitsTest, CompositorFrameMetadata) {
 #endif  // defined(OS_ANDROID)
 
   CompositorFrameMetadata output;
-  SerializeAndDeserialize<mojom::CompositorFrameMetadata>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::CompositorFrameMetadata>(&input,
+                                                                      &output);
   EXPECT_EQ(device_scale_factor, output.device_scale_factor);
   EXPECT_EQ(root_scroll_offset, output.root_scroll_offset);
   EXPECT_EQ(page_scale_factor, output.page_scale_factor);
@@ -792,7 +779,7 @@ TEST_F(StructTraitsTest, RenderPass) {
       SK_ColorYELLOW, false);
 
   std::unique_ptr<RenderPass> output;
-  SerializeAndDeserialize<mojom::RenderPass>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::RenderPass>(&input, &output);
 
   EXPECT_EQ(input->quad_list.size(), output->quad_list.size());
   EXPECT_EQ(input->shared_quad_state_list.size(),
@@ -887,7 +874,7 @@ TEST_F(StructTraitsTest, RenderPassWithEmptySharedQuadStateList) {
   // Unlike the previous test, don't add any quads to the list; we need to
   // verify that the serialization code can deal with that.
   std::unique_ptr<RenderPass> output;
-  SerializeAndDeserialize<mojom::RenderPass>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::RenderPass>(&input, &output);
 
   EXPECT_EQ(input->quad_list.size(), output->quad_list.size());
   EXPECT_EQ(input->shared_quad_state_list.size(),
@@ -981,7 +968,7 @@ TEST_F(StructTraitsTest, QuadListBasic) {
                                  resource_id6, resource_size_in_pixels, matrix);
 
   std::unique_ptr<RenderPass> output;
-  SerializeAndDeserialize<mojom::RenderPass>(render_pass->DeepCopy(), &output);
+  mojo::test::SerializeAndDeserialize<mojom::RenderPass>(&render_pass, &output);
 
   ASSERT_EQ(render_pass->quad_list.size(), output->quad_list.size());
 
@@ -1062,7 +1049,7 @@ TEST_F(StructTraitsTest, SurfaceId) {
                                          base::UnguessableToken::Create());
   SurfaceId input(frame_sink_id, local_surface_id);
   SurfaceId output;
-  SerializeAndDeserialize<mojom::SurfaceId>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::SurfaceId>(&input, &output);
   EXPECT_EQ(frame_sink_id, output.frame_sink_id());
   EXPECT_EQ(local_surface_id, output.local_surface_id());
 }
@@ -1100,7 +1087,8 @@ TEST_F(StructTraitsTest, TransferableResource) {
   input.is_overlay_candidate = is_overlay_candidate;
 
   TransferableResource output;
-  SerializeAndDeserialize<mojom::TransferableResource>(input, &output);
+  mojo::test::SerializeAndDeserialize<mojom::TransferableResource>(&input,
+                                                                   &output);
 
   EXPECT_EQ(id, output.id);
   EXPECT_EQ(format, output.format);
@@ -1148,7 +1136,7 @@ TEST_F(StructTraitsTest, YUVDrawQuad) {
                bits_per_channel, require_overlay, is_protected_video);
 
   std::unique_ptr<RenderPass> output;
-  SerializeAndDeserialize<mojom::RenderPass>(render_pass->DeepCopy(), &output);
+  mojo::test::SerializeAndDeserialize<mojom::RenderPass>(&render_pass, &output);
 
   ASSERT_EQ(render_pass->quad_list.size(), output->quad_list.size());
 
@@ -1177,7 +1165,7 @@ TEST_F(StructTraitsTest, CopyOutputResult_Empty) {
   auto input = std::make_unique<CopyOutputResult>(
       CopyOutputResult::Format::RGBA_BITMAP, gfx::Rect());
   std::unique_ptr<CopyOutputResult> output;
-  SerializeAndDeserialize<mojom::CopyOutputResult>(std::move(input), &output);
+  mojo::test::SerializeAndDeserialize<mojom::CopyOutputResult>(&input, &output);
 
   EXPECT_TRUE(output->IsEmpty());
   EXPECT_EQ(output->format(), CopyOutputResult::Format::RGBA_BITMAP);
@@ -1197,7 +1185,7 @@ TEST_F(StructTraitsTest, CopyOutputResult_Bitmap) {
       std::make_unique<CopyOutputSkBitmapResult>(result_rect, bitmap);
 
   std::unique_ptr<CopyOutputResult> output;
-  SerializeAndDeserialize<mojom::CopyOutputResult>(std::move(input), &output);
+  mojo::test::SerializeAndDeserialize<mojom::CopyOutputResult>(&input, &output);
 
   EXPECT_FALSE(output->IsEmpty());
   EXPECT_EQ(output->format(), CopyOutputResult::Format::RGBA_BITMAP);
@@ -1250,7 +1238,7 @@ TEST_F(StructTraitsTest, CopyOutputResult_Texture) {
                                                 std::move(callback));
 
   std::unique_ptr<CopyOutputResult> output;
-  SerializeAndDeserialize<mojom::CopyOutputResult>(std::move(input), &output);
+  mojo::test::SerializeAndDeserialize<mojom::CopyOutputResult>(&input, &output);
 
   EXPECT_FALSE(output->IsEmpty());
   EXPECT_EQ(output->format(), CopyOutputResult::Format::RGBA_TEXTURE);

ui/gfx/BUILD.gn

@@ -774,6 +774,7 @@ test("gfx_unittests") {
       "//cc/paint",
       "//mojo/core/embedder",
       "//mojo/public/cpp/bindings",
+      "//mojo/public/cpp/test_support:test_utils",
       "//ui/gfx/geometry/mojo:unit_test",
       "//ui/gfx/image/mojo:unit_test",
       "//ui/gfx/mojo:test_interfaces",

ui/gfx/mojo/struct_traits_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/message_loop/message_loop.h"
 #include "build/build_config.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
+#include "mojo/public/cpp/test_support/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/mojo/accelerated_widget_struct_traits.h"
 #include "ui/gfx/mojo/buffer_types_struct_traits.h"
@@ -29,15 +30,6 @@ gfx::AcceleratedWidget CastToAcceleratedWidget(int i) {
 #endif
 }
 
-// Test StructTrait serialization and deserialization for copyable type. |input|
-// will be serialized and then deserialized into |output|.
-template <class MojomType, class Type>
-void SerializeAndDeserialize(const Type& input, Type* output) {
-  MojomType::DeserializeFromMessage(
-      mojo::Message(MojomType::SerializeAsMessage(&input).TakeMojoMessage()),
-      output);
-}
-
 class StructTraitsTest : public testing::Test, public mojom::TraitsTestService {
  public:
   StructTraitsTest() {}
@@ -148,7 +140,8 @@ TEST_F(StructTraitsTest, Transform) {
 TEST_F(StructTraitsTest, MAYBE_AcceleratedWidget) {
   gfx::AcceleratedWidget input(CastToAcceleratedWidget(1001));
   gfx::AcceleratedWidget output;
-  SerializeAndDeserialize<gfx::mojom::AcceleratedWidget>(input, &output);
+  mojo::test::SerializeAndDeserialize<gfx::mojom::AcceleratedWidget>(&input,
+                                                                     &output);
   EXPECT_EQ(input, output);
 }
 
@@ -236,7 +229,8 @@ TEST_F(StructTraitsTest, PresentationFeedback) {
       PresentationFeedback::kVSync | PresentationFeedback::kZeroCopy;
   PresentationFeedback input{timestamp, interval, flags};
   PresentationFeedback output;
-  SerializeAndDeserialize<gfx::mojom::PresentationFeedback>(input, &output);
+  mojo::test::SerializeAndDeserialize<gfx::mojom::PresentationFeedback>(
+      &input, &output);
   EXPECT_EQ(timestamp, output.timestamp);
   EXPECT_EQ(interval, output.interval);
   EXPECT_EQ(flags, output.flags);