Mojo: Small fixes/cleanup to MessageInTransit, before I factor out the secondary buffer.

mojo/system/message_in_transit.cc

 // Copyright 2013 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 "mojo/system/message_in_transit.h"
 
 #include <string.h>
 
 #include <new>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
-#include "base/memory/aligned_memory.h"
 #include "mojo/system/constants.h"
 
 namespace mojo {
 namespace system {
 
 STATIC_CONST_MEMBER_DEFINITION const MessageInTransit::Type
     MessageInTransit::kTypeMessagePipeEndpoint;
 STATIC_CONST_MEMBER_DEFINITION const MessageInTransit::Type
     MessageInTransit::kTypeMessagePipe;
 STATIC_CONST_MEMBER_DEFINITION const MessageInTransit::Type
@@ skipping 75 matching lines @@
 
   return true;
 }
 
 MessageInTransit::MessageInTransit(Type type,
                                    Subtype subtype,
                                    uint32_t num_bytes,
                                    uint32_t num_handles,
                                    const void* bytes)
     : main_buffer_size_(RoundUpMessageAlignment(sizeof(Header) + num_bytes)),
-      main_buffer_(base::AlignedAlloc(main_buffer_size_, kMessageAlignment)),
-      secondary_buffer_size_(0),
-      secondary_buffer_(NULL) {
+      main_buffer_(static_cast<char*>(base::AlignedAlloc(main_buffer_size_,
+                                                         kMessageAlignment))),
+      secondary_buffer_size_(0) {
   DCHECK_LE(num_bytes, kMaxMessageNumBytes);
   DCHECK_LE(num_handles, kMaxMessageNumHandles);
 
   // |total_size| is updated below, from the other values.
   header()->type = type;
   header()->subtype = subtype;
   header()->source_id = kInvalidEndpointId;
   header()->destination_id = kInvalidEndpointId;
   header()->num_bytes = num_bytes;
   header()->num_handles = num_handles;
   // Note: If dispatchers are subsequently attached (in particular, if
   // |num_handles| is nonzero), then |total_size| will have to be adjusted.
   UpdateTotalSize();
 
   if (bytes) {
     memcpy(MessageInTransit::bytes(), bytes, num_bytes);
     memset(static_cast<char*>(MessageInTransit::bytes()) + num_bytes, 0,
            main_buffer_size_ - sizeof(Header) - num_bytes);
   } else {
     memset(MessageInTransit::bytes(), 0, main_buffer_size_ - sizeof(Header));
   }
 }
 
 // TODO(vtl): Do I really want/need to copy the secondary buffer here, or should
 // I just create (deserialize) the dispatchers right away?
 MessageInTransit::MessageInTransit(const View& message_view)
     : main_buffer_size_(message_view.main_buffer_size()),
-      main_buffer_(base::AlignedAlloc(main_buffer_size_, kMessageAlignment)),
+      main_buffer_(static_cast<char*>(base::AlignedAlloc(main_buffer_size_,
+                                                         kMessageAlignment))),
       secondary_buffer_size_(message_view.secondary_buffer_size()),
       secondary_buffer_(secondary_buffer_size_ ?
-                            base::AlignedAlloc(secondary_buffer_size_,
-                                               kMessageAlignment) : NULL) {
+                            static_cast<char*>(
+                                base::AlignedAlloc(secondary_buffer_size_,
+                                                   kMessageAlignment)) : NULL) {
   DCHECK_GE(main_buffer_size_, sizeof(Header));
   DCHECK_EQ(main_buffer_size_ % kMessageAlignment, 0u);
 
-  memcpy(main_buffer_, message_view.main_buffer(), main_buffer_size_);
-  memcpy(secondary_buffer_, message_view.secondary_buffer(),
+  memcpy(main_buffer_.get(), message_view.main_buffer(), main_buffer_size_);
+  memcpy(secondary_buffer_.get(), message_view.secondary_buffer(),
          secondary_buffer_size_);
 
   DCHECK_EQ(main_buffer_size_,
             RoundUpMessageAlignment(sizeof(Header) + num_bytes()));
 }
 
 MessageInTransit::~MessageInTransit() {
-  base::AlignedFree(main_buffer_);
-  base::AlignedFree(secondary_buffer_);  // Okay if null.
-
   if (dispatchers_) {
     for (size_t i = 0; i < dispatchers_->size(); i++) {
       if (!(*dispatchers_)[i])
         continue;
 
       DCHECK((*dispatchers_)[i]->HasOneRef());
       (*dispatchers_)[i]->Close();
     }
   }
 
   if (platform_handles_) {
     for (size_t i = 0; i < platform_handles_->size(); i++)
       (*platform_handles_)[i].CloseIfNecessary();
   }
 
 #ifndef NDEBUG
-  main_buffer_size_ = 0;
-  main_buffer_ = NULL;
   secondary_buffer_size_ = 0;
-  secondary_buffer_ = NULL;
   dispatchers_.reset();
   platform_handles_.reset();
 #endif
 }
 
 // static
 bool MessageInTransit::GetNextMessageSize(const void* buffer,
                                           size_t buffer_size,
                                           size_t* next_message_size) {
   DCHECK(next_message_size);
@@ skipping 20 matching lines @@
   dispatchers_ = dispatchers.Pass();
 #ifndef NDEBUG
   for (size_t i = 0; i < dispatchers_->size(); i++)
     DCHECK(!(*dispatchers_)[i] || (*dispatchers_)[i]->HasOneRef());
 #endif
 }
 
 void MessageInTransit::SerializeAndCloseDispatchers(Channel* channel) {
   DCHECK(channel);
   DCHECK(!secondary_buffer_);
-  CHECK_EQ(num_handles(),
-           dispatchers_ ? dispatchers_->size() : static_cast<size_t>(0));
 
-  if (!num_handles())
+  const size_t num_handles = dispatchers_ ? dispatchers_->size() : 0;
+  if (!num_handles)
     return;
 
   // The offset to the start of the (Mojo) handle table.
   // TODO(vtl): Add a header to the secondary buffer.
   const size_t handle_table_start_offset = 0;
   // The offset to the start of the serialized dispatcher data.
   const size_t serialized_dispatcher_start_offset =
-      handle_table_start_offset + num_handles() * sizeof(HandleTableEntry);
-  // The size of the secondary buffer we'll add to this as we go along).
-  size_t size = serialized_dispatcher_start_offset;
+      handle_table_start_offset + num_handles * sizeof(HandleTableEntry);
+  // The estimated size of the secondary buffer. We compute this estimate below.
+  // It must be at least as big as the (eventual) actual size.
+  size_t estimated_size = serialized_dispatcher_start_offset;
   size_t num_platform_handles = 0;
 #if DCHECK_IS_ON
-  std::vector<size_t> all_max_sizes(num_handles());
-  std::vector<size_t> all_max_platform_handles(num_handles());
+  std::vector<size_t> all_max_sizes(num_handles);
+  std::vector<size_t> all_max_platform_handles(num_handles);
 #endif
-  for (size_t i = 0; i < num_handles(); i++) {
+  for (size_t i = 0; i < num_handles; i++) {
     if (Dispatcher* dispatcher = (*dispatchers_)[i].get()) {
       size_t max_size = 0;
       size_t max_platform_handles = 0;
       Dispatcher::MessageInTransitAccess::StartSerialize(
               dispatcher, channel, &max_size, &max_platform_handles);
 
       DCHECK_LE(max_size, kMaxSerializedDispatcherSize);
-      size += RoundUpMessageAlignment(max_size);
-      DCHECK_LE(size, kMaxSecondaryBufferSize);
+      estimated_size += RoundUpMessageAlignment(max_size);
+      DCHECK_LE(estimated_size, kMaxSecondaryBufferSize);
 
       DCHECK_LE(max_platform_handles,
                 kMaxSerializedDispatcherPlatformHandles);
       num_platform_handles += max_platform_handles;
       DCHECK_LE(num_platform_handles, kMaxPlatformHandles);
 
 #if DCHECK_IS_ON
       all_max_sizes[i] = max_size;
       all_max_platform_handles[i] = max_platform_handles;
 #endif
     }
   }
 
-  secondary_buffer_ = base::AlignedAlloc(size, kMessageAlignment);
-  secondary_buffer_size_ = static_cast<uint32_t>(size);
+  secondary_buffer_.reset(static_cast<char*>(
+      base::AlignedAlloc(estimated_size, kMessageAlignment)));
   // Entirely clear out the secondary buffer, since then we won't have to worry
   // about clearing padding or unused space (e.g., if a dispatcher fails to
   // serialize).
-  memset(secondary_buffer_, 0, size);
+  memset(secondary_buffer_.get(), 0, estimated_size);
 
   if (num_platform_handles > 0) {
     DCHECK(!platform_handles_);
     platform_handles_.reset(new std::vector<embedder::PlatformHandle>());
   }
 
   HandleTableEntry* handle_table = reinterpret_cast<HandleTableEntry*>(
-      static_cast<char*>(secondary_buffer_) + handle_table_start_offset);
+      secondary_buffer_.get() + handle_table_start_offset);
   size_t current_offset = serialized_dispatcher_start_offset;
-  for (size_t i = 0; i < num_handles(); i++) {
+  for (size_t i = 0; i < num_handles; i++) {
     Dispatcher* dispatcher = (*dispatchers_)[i].get();
     if (!dispatcher) {
       COMPILE_ASSERT(Dispatcher::kTypeUnknown == 0,
                      value_of_Dispatcher_kTypeUnknown_must_be_zero);
       continue;
     }
 
 #if DCHECK_IS_ON
     size_t old_platform_handles_size =
         platform_handles_ ? platform_handles_->size() : 0;
 #endif
 
-    void* destination = static_cast<char*>(secondary_buffer_) + current_offset;
+    void* destination = secondary_buffer_.get() + current_offset;
     size_t actual_size = 0;
     if (Dispatcher::MessageInTransitAccess::EndSerializeAndClose(
             dispatcher, channel, destination, &actual_size,
             platform_handles_.get())) {
       handle_table[i].type = static_cast<int32_t>(dispatcher->GetType());
       handle_table[i].offset = static_cast<uint32_t>(current_offset);
       handle_table[i].size = static_cast<uint32_t>(actual_size);
 
 #if DCHECK_IS_ON
       DCHECK_LE(actual_size, all_max_sizes[i]);
       DCHECK_LE(platform_handles_ ? (platform_handles_->size() -
                                          old_platform_handles_size) : 0,
                 all_max_platform_handles[i]);
 #endif
     } else {
       // Nothing to do on failure, since |secondary_buffer_| was cleared, and
       // |kTypeUnknown| is zero. The handle was simply closed.
       LOG(ERROR) << "Failed to serialize handle to remote message pipe";
     }
 
     current_offset += RoundUpMessageAlignment(actual_size);
-    DCHECK_LE(current_offset, size);
+    DCHECK_LE(current_offset, estimated_size);
     DCHECK_LE(platform_handles_ ? platform_handles_->size() : 0,
               num_platform_handles);
   }
 
+  // There's no aligned realloc, so it's no good way to release unused space (if

[darin (slow to review), 2014/05/01 04:13:50]

would it be worth trying to invent an aligned realloc? is this a TODO?

+  // we overshot our estimated space requirements).
+  secondary_buffer_size_ = current_offset;
+
+  // The dispatchers (which we "owned") were closed. We can dispose of them now.
+  dispatchers_.reset();
+
+  // Update the sizes in the message header.
   UpdateTotalSize();
 }
 
 // Note: The message's secondary buffer should have been checked by calling
 // |View::IsValid()| (on the |View|) first.
 void MessageInTransit::DeserializeDispatchers(Channel* channel) {
   DCHECK(!dispatchers_);
 
   // Already checked by |View::IsValid()|:
   DCHECK_LE(num_handles(), kMaxMessageNumHandles);
 
   if (!num_handles())
     return;
 
   dispatchers_.reset(
       new std::vector<scoped_refptr<Dispatcher> >(num_handles()));
 
   size_t handle_table_size = num_handles() * sizeof(HandleTableEntry);
   // Already checked by |View::IsValid()|:
   DCHECK_LE(handle_table_size, secondary_buffer_size_);
 
   const HandleTableEntry* handle_table =
-      static_cast<const HandleTableEntry*>(secondary_buffer_);
+      reinterpret_cast<const HandleTableEntry*>(secondary_buffer_.get());
   for (size_t i = 0; i < num_handles(); i++) {
     size_t offset = handle_table[i].offset;
     size_t size = handle_table[i].size;
     // Already checked by |View::IsValid()|:
     DCHECK_EQ(offset % kMessageAlignment, 0u);
     DCHECK_LE(offset, secondary_buffer_size_);
     DCHECK_LE(offset + size, secondary_buffer_size_);
 
-    const void* source = static_cast<const char*>(secondary_buffer_) + offset;
+    const void* source = secondary_buffer_.get() + offset;
     (*dispatchers_)[i] = Dispatcher::MessageInTransitAccess::Deserialize(
         channel, handle_table[i].type, source, size);
   }
 }
 
 // Validates the secondary buffer. Returns null on success, or a human-readable
 // error message on error.
 // static
 const char* MessageInTransit::ValidateSecondaryBuffer(
     size_t num_handles,
@@ skipping 46 matching lines @@
 
 void MessageInTransit::UpdateTotalSize() {
   DCHECK_EQ(main_buffer_size_ % kMessageAlignment, 0u);
   DCHECK_EQ(secondary_buffer_size_ % kMessageAlignment, 0u);
   header()->total_size =
       static_cast<uint32_t>(main_buffer_size_ + secondary_buffer_size_);
 }
 
 }  // namespace system
 }  // namespace mojo