IPC: Add MojoHandleAttachment

ipc/mojo/ipc_channel_mojo.cc

Index: ipc/mojo/ipc_channel_mojo.cc
diff --git a/ipc/mojo/ipc_channel_mojo.cc b/ipc/mojo/ipc_channel_mojo.cc
index d2d7fe5b7bd84089bd821bd3b030ac282c88f6a1..49274836ccef9735b90af115b9133454bb91aa56 100644
--- a/ipc/mojo/ipc_channel_mojo.cc
+++ b/ipc/mojo/ipc_channel_mojo.cc
@@ -13,6 +13,7 @@
 #include "ipc/ipc_message_macros.h"
 #include "ipc/mojo/client_channel.mojom.h"
 #include "ipc/mojo/ipc_mojo_bootstrap.h"
+#include "ipc/mojo/ipc_mojo_handle_attachment.h"
 #include "third_party/mojo/src/mojo/edk/embedder/embedder.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/error_handler.h"
 
@@ -341,29 +342,7 @@ int ChannelMojo::GetClientFileDescriptor() const {
 base::ScopedFD ChannelMojo::TakeClientFileDescriptor() {
   return bootstrap_->TakeClientFileDescriptor();
 }
-
-// static
-MojoResult ChannelMojo::WriteToMessageAttachmentSet(
-    const std::vector<MojoHandle>& handle_buffer,
-    Message* message) {
-  for (size_t i = 0; i < handle_buffer.size(); ++i) {
-    mojo::embedder::ScopedPlatformHandle platform_handle;
-    MojoResult unwrap_result = mojo::embedder::PassWrappedPlatformHandle(
-        handle_buffer[i], &platform_handle);
-    if (unwrap_result != MOJO_RESULT_OK) {
-      DLOG(WARNING) << "Pipe failed to covert handles. Closing: "
-                    << unwrap_result;
-      return unwrap_result;
-    }
-
-    bool ok = message->attachment_set()->AddAttachment(
-        new internal::PlatformFileAttachment(
-            base::ScopedFD(platform_handle.release().fd)));
-    DCHECK(ok);
-  }
-
-  return MOJO_RESULT_OK;
-}
+#endif  // defined(OS_POSIX) && !defined(OS_NACL)
 
 // static
 MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
@@ -373,38 +352,67 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
   // IPC::MessageAttachmentSet has intricate lifecycle semantics
   // of FDs, so just to dup()-and-own them is the safest option.
   if (message->HasAttachments()) {
-    MessageAttachmentSet* fdset = message->attachment_set();
-    std::vector<base::PlatformFile> fds_to_send(fdset->size());
-    fdset->PeekDescriptors(&fds_to_send[0]);
-    for (size_t i = 0; i < fds_to_send.size(); ++i) {
-      int fd_to_send = dup(fds_to_send[i]);
-      if (-1 == fd_to_send) {
-        DPLOG(WARNING) << "Failed to dup FD to transmit.";
-        fdset->CommitAll();
-        return MOJO_RESULT_UNKNOWN;
-      }
-
-      MojoHandle wrapped_handle;
-      MojoResult wrap_result = CreatePlatformHandleWrapper(
-          mojo::embedder::ScopedPlatformHandle(
-              mojo::embedder::PlatformHandle(fd_to_send)),
-          &wrapped_handle);
-      if (MOJO_RESULT_OK != wrap_result) {
-        DLOG(WARNING) << "Pipe failed to wrap handles. Closing: "
-                      << wrap_result;
-        fdset->CommitAll();
-        return wrap_result;
+    MessageAttachmentSet* set = message->attachment_set();
+    for (size_t i = 0; i < set->size(); ++i) {
+      scoped_refptr<MessageAttachment> attachment = set->GetAttachmentAt(i);
+      switch (attachment->GetType()) {
+        case MessageAttachment::TYPE_PLATFORM_FILE:
+#if defined(OS_POSIX) && !defined(OS_NACL)
+        {
+          base::PlatformFile file =
+              dup(static_cast<IPC::internal::PlatformFileAttachment*>(
+                      attachment.get())->file());
+          if (file == -1) {
+            DPLOG(WARNING) << "Failed to dup FD to transmit.";
+            set->CommitAll();
+            return MOJO_RESULT_UNKNOWN;
+          }
+
+          MojoHandle wrapped_handle;
+          MojoResult wrap_result = CreatePlatformHandleWrapper(
+              mojo::embedder::ScopedPlatformHandle(
+                  mojo::embedder::PlatformHandle(file)),
+              &wrapped_handle);
+          if (MOJO_RESULT_OK != wrap_result) {
+            DLOG(WARNING) << "Pipe failed to wrap handles. Closing: "
+                          << wrap_result;
+            set->CommitAll();
+            return wrap_result;
+          }
+
+          handles->push_back(wrapped_handle);
+        }
+#else
+          NOTREACHED();

[agl, 2015/02/04 00:17:47]

I think the indentation of this line is wrong.

[Hajime Morrita, 2015/02/04 00:44:51]

Done.

+#endif  //  defined(OS_POSIX) && !defined(OS_NACL)
+        break;
+        case MessageAttachment::TYPE_MOJO_HANDLE: {

[agl, 2015/02/04 00:17:46]

blank line before this.

[Hajime Morrita, 2015/02/04 00:44:51]

Done.

[agl, 2015/02/04 00:49:06]

Not clear from codereview that this happened.

[Hajime Morrita, 2015/02/05 00:42:06]

Hmm. I guess The tool shows you an older revision? 

+          mojo::ScopedHandle handle =
+              static_cast<IPC::internal::MojoHandleAttachment*>(
+                  attachment.get())->TakeHandle();
+          handles->push_back(handle.release().value());
+        } break;
       }
-
-      handles->push_back(wrapped_handle);
     }
 
-    fdset->CommitAll();
+    set->CommitAll();
   }
 
   return MOJO_RESULT_OK;
 }
 
-#endif  // defined(OS_POSIX) && !defined(OS_NACL)
+// static
+MojoResult ChannelMojo::WriteToMessageAttachmentSet(
+    const std::vector<MojoHandle>& handle_buffer,
+    Message* message) {
+  for (size_t i = 0; i < handle_buffer.size(); ++i) {
+    bool ok = message->attachment_set()->AddAttachment(
+        new IPC::internal::MojoHandleAttachment(
+            mojo::MakeScopedHandle(mojo::Handle(handle_buffer[i]))));
+    DCHECK(ok);

[agl, 2015/02/04 00:17:46]

if (!ok) {
  return <some error value>
}

?

[Hajime Morrita, 2015/02/04 00:44:51]

There is no upper bound of mojo handles.
Might be good to have the error check just in case though.
Done.

+  }
+
+  return MOJO_RESULT_OK;
+}
 
 }  // namespace IPC