Revert of Reland chromium#2 "ipc: Add a new field num_brokered_attach…

…ments to the message header." (patchset chromium#5 id:80001 of https://codereview.chromium.org/1334593002/ ) Reason for revert: CL was only ever intended to be in 1 canary release. Original issue's description: > Reland chromium#2 "ipc: Add a new field num_brokered_attachments to the message header." > > This original version of this CL is causing an unusual crash in Canary. This CL > adds a message verifier to the ipc code so that it can dynamically verify the > contents of this message that is being corrupted. This CL also verifies the > message at several different points in the dispatch process. This will help > narrow down the range of code that is corrupting the message. > > I expect this CL to cause ~100 crashes in the next Chrome Canary. I intend to > revert this CL after a single Canary release. > > BUG=527588 > > Committed: https://crrev.com/a2e71be46dc4bdcbb544db479680f65a390ae8f3 > Cr-Commit-Position: refs/heads/master@{#349056} TBR=avi@chromium.org,tsepez@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=527588 Review URL: https://codereview.chromium.org/1350823002 Cr-Commit-Position: refs/heads/master@{#349152}
Denger-Network · Sep 16, 2015 · 1fa7dad · 1fa7dad
1 parent 152f7f2
commit 1fa7dad
Show file tree

Hide file tree

Showing 14 changed files with 2 additions and 111 deletions.
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc
@@ -53,7 +53,6 @@
 #include "content/common/content_switches_internal.h"
 #include "content/common/host_discardable_shared_memory_manager.h"
 #include "content/common/host_shared_bitmap_manager.h"
-#include "content/common/resource_messages.h"
 #include "content/public/browser/browser_main_parts.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host.h"
@@ -63,7 +62,6 @@
 #include "content/public/common/result_codes.h"
 #include "crypto/nss_util.h"
 #include "device/battery/battery_status_service.h"
-#include "ipc/ipc_channel.h"
 #include "media/audio/audio_manager.h"
 #include "media/base/media.h"
 #include "media/base/user_input_monitor.h"
@@ -1117,10 +1115,6 @@ void BrowserMainLoop::InitializeMainThread() {
   // Register the main thread by instantiating it, but don't call any methods.
   main_thread_.reset(
       new BrowserThreadImpl(BrowserThread::UI, base::MessageLoop::current()));
-
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  IPC::Channel::SetMessageVerifier(
-      &content::CheckContentsOfDataReceivedMessage);
 }
 
 int BrowserMainLoop::BrowserThreadsStarted() {

diff --git a/content/browser/loader/async_resource_handler.cc b/content/browser/loader/async_resource_handler.cc
@@ -329,11 +329,6 @@ bool AsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
   int encoded_data_length = current_transfer_size - reported_transfer_size_;
   reported_transfer_size_ = current_transfer_size;
 
-  // TODO(erikchen): Remove me after finished debugging for
-  // http://crbug.com/527588.
-  // The data offset can't exceed the size of the buffer.
-  CHECK_LE(data_offset, kBufferSize);
-
   filter->Send(new ResourceMsg_DataReceived(
       GetRequestID(), data_offset, bytes_read, encoded_data_length));
   ++pending_data_count_;

diff --git a/content/child/child_thread_impl.cc b/content/child/child_thread_impl.cc
@@ -51,10 +51,8 @@
 #include "content/child/websocket_dispatcher.h"
 #include "content/common/child_process_messages.h"
 #include "content/common/in_process_child_thread_params.h"
-#include "content/common/resource_messages.h"
 #include "content/public/common/content_switches.h"
 #include "ipc/attachment_broker_unprivileged.h"
-#include "ipc/ipc_channel.h"
 #include "ipc/ipc_logging.h"
 #include "ipc/ipc_switches.h"
 #include "ipc/ipc_sync_channel.h"
@@ -398,9 +396,6 @@ void ChildThreadImpl::Init(const Options& options) {
   if (!IsInBrowserProcess())
     IPC::Logging::GetInstance()->SetIPCSender(this);
 #endif
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  IPC::Channel::SetMessageVerifier(
-      &content::CheckContentsOfDataReceivedMessage);
 
   mojo_application_.reset(new MojoApplication(GetIOTaskRunner()));
 

diff --git a/content/child/resource_dispatcher.cc b/content/child/resource_dispatcher.cc
@@ -80,9 +80,6 @@ ResourceDispatcher::~ResourceDispatcher() {
 }
 
 bool ResourceDispatcher::OnMessageReceived(const IPC::Message& message) {
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  content::CheckContentsOfDataReceivedMessage(&message);
-
   if (!IsResourceDispatcherMessage(message)) {
     return false;
   }
@@ -222,16 +219,9 @@ void ResourceDispatcher::OnReceivedData(int request_id,
   PendingRequestInfo* request_info = GetPendingRequestInfo(request_id);
   bool send_ack = true;
   if (request_info && data_length > 0) {
-    // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-    int buffer_size = request_info->buffer_size;
-
     CHECK(base::SharedMemory::IsHandleValid(request_info->buffer->handle()));
     CHECK_GE(request_info->buffer_size, data_offset + data_length);
 
-    // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-    base::debug::Alias(request_info);
-    base::debug::Alias(&buffer_size);
-
     // Ensure that the SHM buffer remains valid for the duration of this scope.
     // It is possible for Cancel() to be called before we exit this scope.
     // SharedMemoryReceivedDataFactory stores the SHM buffer inside it.

diff --git a/content/child/resource_scheduling_filter.cc b/content/child/resource_scheduling_filter.cc
@@ -7,7 +7,6 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "content/child/resource_dispatcher.h"
-#include "content/common/resource_messages.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_start.h"
 
@@ -27,9 +26,6 @@ ResourceSchedulingFilter::~ResourceSchedulingFilter() {
 }
 
 bool ResourceSchedulingFilter::OnMessageReceived(const IPC::Message& message) {
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  content::CheckContentsOfDataReceivedMessage(&message);
-
   main_thread_task_runner_->PostTask(
       FROM_HERE, base::Bind(&ResourceSchedulingFilter::DispatchMessage,
                             weak_ptr_factory_.GetWeakPtr(), message));

diff --git a/content/child/threaded_data_provider.cc b/content/child/threaded_data_provider.cc
@@ -81,9 +81,6 @@ void DataProviderMessageFilter::OnFilterAdded(IPC::Sender* sender) {
 
 bool DataProviderMessageFilter::OnMessageReceived(
     const IPC::Message& message) {
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  content::CheckContentsOfDataReceivedMessage(&message);
-
   DCHECK(io_task_runner_->BelongsToCurrentThread());
 
   if (message.type() != ResourceMsg_DataReceived::ID)

diff --git a/content/common/resource_messages.cc b/content/common/resource_messages.cc
@@ -4,25 +4,9 @@
 
 #include "content/common/resource_messages.h"
 
-#include "base/debug/alias.h"
-#include "ipc/ipc_message.h"
 #include "net/base/load_timing_info.h"
 #include "net/http/http_response_headers.h"
 
-namespace content {
-// TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-void CheckContentsOfDataReceivedMessage(const IPC::Message* message) {
-  if (message->type() != ResourceMsg_DataReceived::ID)
-    return;
-  ResourceMsg_DataReceived::Schema::Param arg;
-  bool success = ResourceMsg_DataReceived::Read(message, &arg);
-  CHECK(success);
-  int data_offset = base::get<1>(arg);
-  CHECK_LE(data_offset, 512 * 1024);
-  base::debug::Alias(&data_offset);
-}
-}
-
 namespace IPC {
 
 void ParamTraits<scoped_refptr<net::HttpResponseHeaders> >::Write(

diff --git a/content/common/resource_messages.h b/content/common/resource_messages.h
@@ -28,12 +28,6 @@ struct LoadTimingInfo;
 
 namespace content {
 struct ResourceDevToolsInfo;
-
-// TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-// If |m| does not have type ResourceMsg_DataReceived, this method has no
-// effect. Otherwise, this method checks that the |data_offset| member is less
-// than 512kB.
-void CheckContentsOfDataReceivedMessage(const IPC::Message* m);
 }
 
 namespace IPC {

diff --git a/ipc/ipc_channel.h b/ipc/ipc_channel.h
@@ -169,11 +169,6 @@ class IPC_EXPORT Channel : public Endpoint {
 
   ~Channel() override;
 
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  using MessageVerifier = void (*)(const Message*);
-  static void SetMessageVerifier(MessageVerifier verifier);
-  static MessageVerifier GetMessageVerifier();
-
   // Connect the pipe.  On the server side, this will initiate
   // waiting for connections.  On the client, it attempts to
   // connect to a pre-existing pipe.  Note, calling Connect()

diff --git a/ipc/ipc_channel_common.cc b/ipc/ipc_channel_common.cc
@@ -6,8 +6,6 @@
 
 namespace IPC {
 
-static Channel::MessageVerifier g_message_verifier = nullptr;
-
 // static
 scoped_ptr<Channel> Channel::CreateClient(
     const IPC::ChannelHandle& channel_handle,
@@ -58,16 +56,6 @@ scoped_ptr<Channel> Channel::CreateServer(
 Channel::~Channel() {
 }
 
-// static
-void Channel::SetMessageVerifier(MessageVerifier verifier) {
-  g_message_verifier = verifier;
-}
-
-// static
-Channel::MessageVerifier Channel::GetMessageVerifier() {
-  return g_message_verifier;
-}
-
 bool Channel::IsSendThreadSafe() const {
   return false;
 }

diff --git a/ipc/ipc_channel_reader.cc b/ipc/ipc_channel_reader.cc
@@ -11,7 +11,6 @@
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_attachment_set.h"
 #include "ipc/ipc_message_macros.h"
-#include "ipc/ipc_message_start.h"
 
 namespace IPC {
 namespace internal {
@@ -190,12 +189,6 @@ void ChannelReader::DispatchMessage(Message* m) {
     handled = GetAttachmentBroker()->OnMessageReceived(*m);
   }
 #endif  // USE_ATTACHMENT_BROKER
-
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  Channel::MessageVerifier verifier = Channel::GetMessageVerifier();
-  if (verifier)
-    verifier(m);
-
   if (!handled)
     listener_->OnMessageReceived(*m);
   if (m->dispatch_error())

diff --git a/ipc/ipc_channel_win.cc b/ipc/ipc_channel_win.cc
@@ -474,17 +474,6 @@ bool ChannelWin::ProcessOutgoingMessages(
 
   // Write to pipe...
   OutputElement* element = output_queue_.front();
-
-  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
-  {
-    const Message* m = element->get_message();
-    if (m) {
-      Channel::MessageVerifier verifier = Channel::GetMessageVerifier();
-      if (verifier)
-        verifier(m);
-    }
-  }
-
   DCHECK(element->size() <= INT_MAX);
   BOOL ok = WriteFile(pipe_.Get(),
                       element->data(),

diff --git a/ipc/ipc_message.cc b/ipc/ipc_message.cc
@@ -49,9 +49,6 @@ Message::~Message() {
 Message::Message() : base::Pickle(sizeof(Header)) {
   header()->routing = header()->type = 0;
   header()->flags = GetRefNumUpper24();
-#if USE_ATTACHMENT_BROKER
-  header()->num_brokered_attachments = 0;
-#endif
 #if defined(OS_POSIX)
   header()->num_fds = 0;
   header()->pad = 0;
@@ -65,9 +62,6 @@ Message::Message(int32_t routing_id, uint32_t type, PriorityValue priority)
   header()->type = type;
   DCHECK((priority & 0xffffff00) == 0);
   header()->flags = priority | GetRefNumUpper24();
-#if USE_ATTACHMENT_BROKER
-  header()->num_brokered_attachments = 0;
-#endif
 #if defined(OS_POSIX)
   header()->num_fds = 0;
   header()->pad = 0;
@@ -155,7 +149,7 @@ void Message::FindNext(const char* range_start,
   // The data is not copied.
   size_t pickle_len = static_cast<size_t>(pickle_end - range_start);
   Message message(range_start, static_cast<int>(pickle_len));
-  size_t num_attachments = message.header()->num_brokered_attachments;
+  int num_attachments = message.header()->num_brokered_attachments;
 
   // Check for possible overflows.
   size_t max_size_t = std::numeric_limits<size_t>::max();
@@ -171,7 +165,7 @@ void Message::FindNext(const char* range_start,
   if (buffer_length < attachment_length + pickle_len)
     return;
 
-  for (size_t i = 0; i < num_attachments; ++i) {
+  for (int i = 0; i < num_attachments; ++i) {
     const char* attachment_start =
         pickle_end + i * BrokerableAttachment::kNonceSize;
     BrokerableAttachment::AttachmentId id(attachment_start,
@@ -198,12 +192,6 @@ bool Message::WriteAttachment(scoped_refptr<MessageAttachment> attachment) {
   // We write the index of the descriptor so that we don't have to
   // keep the current descriptor as extra decoding state when deserialising.
   WriteInt(attachment_set()->size());
-
-#if USE_ATTACHMENT_BROKER
-  if (attachment->GetType() == MessageAttachment::TYPE_BROKERABLE_ATTACHMENT)
-    header()->num_brokered_attachments += 1;
-#endif
-
   return attachment_set()->AddAttachment(attachment);
 }
 

diff --git a/ipc/ipc_message.h b/ipc/ipc_message.h
@@ -12,7 +12,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/pickle.h"
 #include "base/trace_event/trace_event.h"
-#include "ipc/attachment_broker.h"
 #include "ipc/brokerable_attachment.h"
 #include "ipc/ipc_export.h"
 
@@ -252,12 +251,6 @@ class IPC_EXPORT Message : public base::Pickle {
     int32_t routing;  // ID of the view that this message is destined for
     uint32_t type;    // specifies the user-defined message type
     uint32_t flags;   // specifies control flags for the message
-#if USE_ATTACHMENT_BROKER
-    // The number of brokered attachments included with this message. The
-    // ids of the brokered attachment ids are sent immediately after the pickled
-    // message, before the next pickled message is sent.
-    uint32_t num_brokered_attachments;
-#endif
 #if defined(OS_POSIX)
     uint16_t num_fds; // the number of descriptors included with this message
     uint16_t pad;     // explicitly initialize this to appease valgrind