ipc: Add debugging to verify the source of ResourceDispatcher crashes.

This CL is mostly a reland of https://codereview.chromium.org/1354063002/. It adds a MessageVerifier that verifies specific IPC messages right before serialization, and right after deserialization. It also adds the same CHECKs to the application logic that uses Chrome IPC. BUG=527588 Review URL: https://codereview.chromium.org/1500013003 Cr-Commit-Position: refs/heads/master@{#363577}
Denger-Network · Dec 7, 2015 · ee68563 · ee68563
1 parent 8be7712
commit ee68563
Show file tree

Hide file tree

Showing 12 changed files with 79 additions and 0 deletions.
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc
@@ -54,6 +54,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/main_function_params.h"
 #include "content/public/common/result_codes.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"
@@ -1190,6 +1192,9 @@ 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::CheckContentsOfResourceMessage);
 }
 
 int BrowserMainLoop::BrowserThreadsStarted() {

diff --git a/content/browser/loader/async_resource_handler.cc b/content/browser/loader/async_resource_handler.cc
@@ -318,6 +318,9 @@ bool AsyncResourceHandler::OnReadCompleted(int bytes_read, bool* defer) {
     int size;
     if (!buffer_->ShareToProcess(filter->PeerHandle(), &handle, &size))
       return false;
+
+    // TODO(erikchen): Temporary debugging. http://crbug.com/527588.
+    CHECK_LE(size, kBufferSize);
     filter->Send(new ResourceMsg_SetDataBuffer(
         GetRequestID(), handle, size, filter->peer_pid()));
     sent_first_data_msg_ = true;
@@ -329,6 +332,9 @@ 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): Temporary debugging. http://crbug.com/527588.
+  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
@@ -52,9 +52,11 @@
 #include "content/common/child_process_messages.h"
 #include "content/common/in_process_child_thread_params.h"
 #include "content/common/mojo/mojo_messages.h"
+#include "content/common/resource_messages.h"
 #include "content/public/common/content_switches.h"
 #include "ipc/attachment_broker.h"
 #include "ipc/attachment_broker_unprivileged.h"
+#include "ipc/ipc_channel.h"
 #include "ipc/ipc_logging.h"
 #include "ipc/ipc_platform_file.h"
 #include "ipc/ipc_switches.h"
@@ -413,6 +415,8 @@ 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::CheckContentsOfResourceMessage);
 
   mojo_ipc_support_.reset(new IPC::ScopedIPCSupport(GetIOTaskRunner()));
   mojo_application_.reset(new MojoApplication(GetIOTaskRunner()));

diff --git a/content/child/resource_dispatcher.cc b/content/child/resource_dispatcher.cc
@@ -82,6 +82,9 @@ ResourceDispatcher::~ResourceDispatcher() {
 }
 
 bool ResourceDispatcher::OnMessageReceived(const IPC::Message& message) {
+  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
+  content::CheckContentsOfResourceMessage(&message);
+
   if (!IsResourceDispatcherMessage(message)) {
     return false;
   }

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"
 #include "third_party/WebKit/public/platform/WebTaskRunner.h"
@@ -49,6 +50,9 @@ ResourceSchedulingFilter::~ResourceSchedulingFilter() {
 }
 
 bool ResourceSchedulingFilter::OnMessageReceived(const IPC::Message& message) {
+  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
+  content::CheckContentsOfResourceMessage(&message);
+
   base::AutoLock lock(request_id_to_task_runner_map_lock_);
   int request_id;
 

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::CheckContentsOfResourceMessage(&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 CheckContentsOfResourceMessage(const IPC::Message* message) {
+  if (message->type() == ResourceMsg_DataReceived::ID) {
+    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
@@ -29,6 +29,10 @@ struct LoadTimingInfo;
 
 namespace content {
 struct ResourceDevToolsInfo;
+
+// TODO(erikchen): Temporary code to help track http://crbug.com/527588.
+// This method sanity checks the contents of the DataReceived resource message.
+void CheckContentsOfResourceMessage(const IPC::Message* m);
 }
 
 namespace IPC {

diff --git a/ipc/ipc_channel.h b/ipc/ipc_channel.h
@@ -157,6 +157,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,
@@ -47,6 +49,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
@@ -12,6 +12,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 {
@@ -163,6 +164,11 @@ bool ChannelReader::TranslateInputData(const char* input_data,
 bool ChannelReader::HandleTranslatedMessage(
     Message* translated_message,
     const AttachmentIdVector& attachment_ids) {
+  // TODO(erikchen): Temporary code to help track http://crbug.com/527588.
+  Channel::MessageVerifier verifier = Channel::GetMessageVerifier();
+  if (verifier)
+    verifier(translated_message);
+
   // Immediately handle internal messages.
   if (IsInternalMessage(*translated_message)) {
     EmitLogBeforeDispatch(*translated_message);

diff --git a/ipc/ipc_channel_win.cc b/ipc/ipc_channel_win.cc
@@ -489,6 +489,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(),