Adding a new message type to the Mojo channel.

mojo/edk/system/channel.h

 // Copyright 2016 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.
 
 #ifndef MOJO_EDK_SYSTEM_CHANNEL_H_
 #define MOJO_EDK_SYSTEM_CHANNEL_H_
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/process/process_handle.h"
 #include "base/task_runner.h"
 #include "mojo/edk/embedder/platform_handle_vector.h"
 #include "mojo/edk/embedder/scoped_platform_handle.h"
 
 namespace mojo {
 namespace edk {
 
 const size_t kChannelMessageAlignment = 8;

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: We could also now add a utility like:

constexpr bool IsAlignedForChannelMessage(size_t n) {
  return n % kChannelMessageAlignment == 0;
}

which can then be used to make the static_assert sites more readable.

[Jay Civelli, 2017/02/27 17:07:03]

Good idea, done.

 
 // Channel provides a thread-safe interface to read and write arbitrary
 // delimited messages over an underlying I/O channel, optionally transferring
 // one or more platform handles in the process.
-class Channel : public base::RefCountedThreadSafe<Channel> {
+class MOJO_SYSTEM_IMPL_EXPORT Channel
+    : public base::RefCountedThreadSafe<Channel> {
  public:
   struct Message;
 
   using MessagePtr = std::unique_ptr<Message>;
 
   // A message to be written to a channel.
   struct Message {

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

You also need an EXPORT macro on the nested types you reference in tests.

[Jay Civelli, 2017/02/27 17:07:03]

Yes, that's the only other one used in the test I think.

+    enum class MessageType : uint16_t {
+      // A normal message.

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: update comment?

[Jay Civelli, 2017/02/27 17:07:03]

Done.

+      NORMAL_LEGACY = 0,
+#if defined(OS_MACOSX)
+      // A control message containing handles to echo back.
+      HANDLES_SENT,
+      // A control message containing handles that can now be closed.
+      HANDLES_SENT_ACK,
+#endif
+      NORMAL_VERSIONED,

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: NORMAL is probably fine if we call the old one NORMAL_LEGACY
nit: add comment?

[Jay Civelli, 2017/02/27 17:07:03]

Done.

+    };
+
 #pragma pack(push, 1)
-    struct Header {
-      enum class MessageType : uint16_t {
-        // A normal message.
-        NORMAL = 0,
-#if defined(OS_MACOSX)
-        // A control message containing handles to echo back.
-        HANDLES_SENT,
-        // A control message containing handles that can now be closed.
-        HANDLES_SENT_ACK,
-#endif
-      };
-
+    // Old message wire format for ChromeOS and Android, used by NORMAL_LEGACY
+    // messages.
+    // TODO: Remove once Arc++ uses versioned messages.
+    struct LegacyHeader {
       // Message size in bytes, including the header.
       uint32_t num_bytes;
 
-#if defined(MOJO_EDK_LEGACY_PROTOCOL)
-      // Old message wire format for ChromeOS and Android.
       // Number of attached handles.
       uint16_t num_handles;
 
       MessageType message_type;
-#else
+    };
+
+    // Header used by NORMAL_VERSIONED messages.
+    // To preserve backward compatibility with LegacyHeader, the num_bytes and
+    // message_type field should be at the same offset than in LegacyHeader.

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: s/should be/must be/

Also please add a notice to increment kMessageCurrentVersion in channel.cc when
changing this structure, and note that such changes may only augment the data
structure (i.e. we can never delete or rearrange existing fields.) We may
someday wish to violate that policy but until we need to, this grants us a
reasonable level of flexibility while keeping backwards-compatibility relatively
simple.

[Jay Civelli, 2017/02/27 17:07:03]

Done.

+    struct VersionedHeader {
+      // Message size in bytes, including the header.
+      uint32_t num_bytes;
+
       // Total size of header, including extra header data (i.e. HANDLEs on
       // windows).
       uint16_t num_header_bytes;
 
+      MessageType message_type;
+
+      uint16_t version_number;
+
       // Number of attached handles. May be less than the reserved handle
       // storage size in this message on platforms that serialise handles as
       // data (i.e. HANDLEs on Windows, Mach ports on OSX).
       uint16_t num_handles;
 
-      MessageType message_type;
-
-      char padding[6];
-#endif  // defined(MOJO_EDK_LEGACY_PROTOCOL)
+      char padding[4];
     };
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
     struct MachPortsEntry {
       // Index of Mach port in the original vector of PlatformHandles.
       uint16_t index;
 
       // Mach port name.
       uint32_t mach_port;
       static_assert(sizeof(mach_port_t) <= sizeof(uint32_t),
@@ skipping 21 matching lines @@
     };
     static_assert(sizeof(HandleEntry) == 4,
                   "sizeof(HandleEntry) must be 4 bytes");
 #endif
 #pragma pack(pop)
 
     // Allocates and owns a buffer for message data with enough capacity for
     // |payload_size| bytes plus a header, plus |max_handles| platform handles.
     Message(size_t payload_size,
             size_t max_handles,
-            Header::MessageType message_type = Header::MessageType::NORMAL);
+            MessageType message_type = MessageType::NORMAL_LEGACY);

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: I'd prefer to use a delegating constructor rather than a default argument

[Jay Civelli, 2017/02/27 17:07:03]

Done.

 
     ~Message();
 
     // Constructs a Message from serialized message data.
     static MessagePtr Deserialize(const void* data, size_t data_num_bytes);
 
     const void* data() const { return data_; }
     size_t data_num_bytes() const { return size_; }
 
-#if defined(MOJO_EDK_LEGACY_PROTOCOL)
-    void* mutable_payload() { return static_cast<void*>(header_ + 1); }
-    const void* payload() const {
-      return static_cast<const void*>(header_ + 1);
-    }
+    const void* extra_header() const;
+    void* mutable_extra_header();
+    size_t extra_header_size() const;
+
+    void* mutable_payload();
+    const void* payload() const;
     size_t payload_size() const;
-#else
-    const void* extra_header() const { return data_ + sizeof(Header); }
-    void* mutable_extra_header() { return data_ + sizeof(Header); }
-    size_t extra_header_size() const {
-      return header_->num_header_bytes - sizeof(Header);
-    }
 
-    void* mutable_payload() { return data_ + header_->num_header_bytes; }
-    const void* payload() const { return data_ + header_->num_header_bytes; }
-    size_t payload_size() const;
-#endif  // defined(MOJO_EDK_LEGACY_PROTOCOL)
-
-    size_t num_handles() const { return header_->num_handles; }
-    bool has_handles() const { return header_->num_handles > 0; }
+    size_t num_handles() const;
+    bool has_handles() const;
 #if defined(OS_MACOSX) && !defined(OS_IOS)
     bool has_mach_ports() const;
 #endif
 
     // Note: SetHandles() and TakeHandles() invalidate any previous value of
     // handles().
     void SetHandles(ScopedPlatformHandleVectorPtr new_handles);
     ScopedPlatformHandleVectorPtr TakeHandles();
     // Version of TakeHandles that returns a vector of platform handles suitable
     // for transfer over an underlying OS mechanism. i.e. file descriptors over
     // a unix domain socket. Any handle that cannot be transferred this way,
     // such as Mach ports, will be removed.
     ScopedPlatformHandleVectorPtr TakeHandlesForTransport();
 
 #if defined(OS_WIN)
     // Prepares the handles in this message for use in a different process.
     // Upon calling this the handles should belong to |from_process|; after the
     // call they'll belong to |to_process|. The source handles are always
     // closed by this call. Returns false iff one or more handles failed
     // duplication.
     static bool RewriteHandles(base::ProcessHandle from_process,
                                base::ProcessHandle to_process,
                                PlatformHandleVector* handles);
 #endif
 
+    void SetVersionForTest(uint16_t version_number);
+
    private:
     size_t size_;
     size_t max_handles_;
     char* data_;
-    Header* header_;
+    LegacyHeader* legacy_header_ = nullptr;

[Ken Rockot(use gerrit already), 2017/02/23 22:07:05]

nit: use an anonymous union for legacy_header_ and versioned_header_ storage?

[Jay Civelli, 2017/02/27 17:07:03]

I use the fact that versioned_header_ is not null to figure out if I am in
legacy or not mode. I could not do it if this was unionized, right?

+    VersionedHeader* versioned_header_ = nullptr;
 
     ScopedPlatformHandleVectorPtr handle_vector_;
 
 #if defined(OS_WIN)
     // On Windows, handles are serialised into the extra header section.
     HandleEntry* handles_ = nullptr;
 #elif defined(OS_MACOSX) && !defined(OS_IOS)
     // On OSX, handles are serialised into the extra header section.
     MachPortsExtraHeader* mach_ports_header_ = nullptr;
 #endif
@@ skipping 84 matching lines @@
   // this is not necessarily an error condition. In such cases this returns
   // |true| but |*handles| will also be reset to null.
   virtual bool GetReadPlatformHandles(
       size_t num_handles,
       const void* extra_header,
       size_t extra_header_size,
       ScopedPlatformHandleVectorPtr* handles) = 0;
 
   // Handles a received control message. Returns |true| if the message is
   // accepted, or |false| otherwise.
-  virtual bool OnControlMessage(Message::Header::MessageType message_type,
+  virtual bool OnControlMessage(Message::MessageType message_type,
                                 const void* payload,
                                 size_t payload_size,
                                 ScopedPlatformHandleVectorPtr handles);
 
  private:
   friend class base::RefCountedThreadSafe<Channel>;
 
   class ReadBuffer;
 
   Delegate* delegate_;
   const std::unique_ptr<ReadBuffer> read_buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(Channel);
 };
 
 }  // namespace edk
 }  // namespace mojo
 
 #endif  // MOJO_EDK_SYSTEM_CHANNEL_H_