Reland of land debugging for ipc related crash. (patchset chromium#1 …

…id:1 of https://codereview.chromium.org/1354863004/ ) Reason for revert: Relanding this CL, as I don't think it will actually cause crashes. https://code.google.com/p/chromium/issues/detail?id=524032#c27 Original issue's description: > Revert of Reland debugging for ipc related crash. (patchset chromium#1 id:1 of https://codereview.chromium.org/1354063002/ ) > > Reason for revert: > Supposedly, this CL only changes the signature of the crash, not the presence of the crash. There is some (but not totally clear) evidence that this CL is causing a crash itself. > > https://code.google.com/p/chromium/issues/detail?id=527588#c32 > > Original issue's description: > > Reland debugging for ipc related crash. > > > > This CL is almost identical to https://codereview.chromium.org/1350823002/. The > > only difference is that the part of the CL that causes the crash has been > > omitted. > > > > BUG=527588 > > > > Committed: https://crrev.com/a7502c6dff276bce3c76d680c5d613cb6ece19be > > Cr-Commit-Position: refs/heads/master@{#349828} > > TBR=avi@chromium.org,tsepez@chromium.org > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=527588 > > Committed: https://crrev.com/dbc8c1869a4fd0493150c008106be720eba86205 > Cr-Commit-Position: refs/heads/master@{#350062} TBR=avi@chromium.org,tsepez@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=527588 Review URL: https://codereview.chromium.org/1359933002 Cr-Commit-Position: refs/heads/master@{#350203}
Denger-Network · Sep 22, 2015 · a8b611f · a8b611f
1 parent 94c1a3d
commit a8b611f
Show file tree

Hide file tree

Showing 13 changed files with 92 additions and 2 deletions.
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc
@@ -53,6 +53,7 @@
 #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"
@@ -62,6 +63,7 @@
 #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"
@@ -1121,6 +1123,10 @@ 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,6 +329,11 @@ 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,8 +51,10 @@
 #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"
@@ -396,6 +398,9 @@ 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,6 +80,9 @@ 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;
   }
@@ -219,9 +222,16 @@ 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,6 +7,7 @@
 #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"
 
@@ -26,6 +27,9 @@ 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,6 +81,9 @@ 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,9 +4,25 @@
 
 #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,6 +28,12 @@ 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,6 +169,11 @@ 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,6 +6,8 @@
 
 namespace IPC {
 
+static Channel::MessageVerifier g_message_verifier = nullptr;
+
 // static
 scoped_ptr<Channel> Channel::CreateClient(
     const IPC::ChannelHandle& channel_handle,
@@ -56,6 +58,16 @@ 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,6 +11,7 @@
 #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 {
@@ -189,6 +190,12 @@ 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,6 +474,17 @@ 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
@@ -149,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));
-  int num_attachments = message.header()->num_brokered_attachments;
+  size_t num_attachments = message.header()->num_brokered_attachments;
 
   // Check for possible overflows.
   size_t max_size_t = std::numeric_limits<size_t>::max();
@@ -165,7 +165,7 @@ void Message::FindNext(const char* range_start,
   if (buffer_length < attachment_length + pickle_len)
     return;
 
-  for (int i = 0; i < num_attachments; ++i) {
+  for (size_t i = 0; i < num_attachments; ++i) {
     const char* attachment_start =
         pickle_end + i * BrokerableAttachment::kNonceSize;
     BrokerableAttachment::AttachmentId id(attachment_start,