Fix unitialized memory access in workers.

The primary issue was that OnDestroy didn't change the entangled port
to have its entangled port be none.

A secondary issues that came up is that in very rare circumstances (like a crash
happening early in a worker process), it seemed like it may be possible that one
of the message ports may think it is entangled and the other half may not, so
the Erase method guards against this.

Also, some code was added to verify the internal structure before running code
and after.

BUG=27839
TEST=valgrind on linux running ui tests, specifically WorkerTest.WorkerFastLayoutTests.

Review URL: http://codereview.chromium.org/402106

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@32586 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 71 insertions, 59 deletions

diff --git a/chrome/browser/worker_host/message_port_dispatcher.cc b/chrome/browser/worker_host/message_port_dispatcher.cc
index c6e992d..3592012 100644
--- a/chrome/browser/worker_host/message_port_dispatcher.cc
+++ b/chrome/browser/worker_host/message_port_dispatcher.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/worker_host/message_port_dispatcher.h"
 
 #include "base/singleton.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/renderer_host/resource_message_filter.h"
 #include "chrome/browser/worker_host/worker_process_host.h"
 #include "chrome/common/notification_service.h"
@@ -63,6 +64,7 @@ void MessagePortDispatcher::UpdateMessagePort(
     IPC::Message::Sender* sender,
     int routing_id,
     CallbackWithReturnValue<int>::Type* next_routing_id) {
+  DCHECK(CheckMessagePortMap(true));
   if (!message_ports_.count(message_port_id)) {
     NOTREACHED();
     return;
@@ -72,14 +74,17 @@ void MessagePortDispatcher::UpdateMessagePort(
   port.sender = sender;
   port.route_id = routing_id;
   port.next_routing_id = next_routing_id;
+  DCHECK(CheckMessagePortMap(true));
 }
 
 bool MessagePortDispatcher::Send(IPC::Message* message) {
+  DCHECK(CheckMessagePortMap(true));
   return sender_->Send(message);
 }
 
 void MessagePortDispatcher::OnCreate(int *route_id,
                                      int* message_port_id) {
+  DCHECK(CheckMessagePortMap(true));
   *message_port_id = ++next_message_port_id_;
   *route_id = next_routing_id_->Run();
 
@@ -91,20 +96,24 @@ void MessagePortDispatcher::OnCreate(int *route_id,
   port.entangled_message_port_id = MSG_ROUTING_NONE;
   port.queue_messages = false;
   message_ports_[*message_port_id] = port;
+  DCHECK(CheckMessagePortMap(true));
 }
 
 void MessagePortDispatcher::OnDestroy(int message_port_id) {
+  DCHECK(CheckMessagePortMap(true));
   if (!message_ports_.count(message_port_id)) {
     NOTREACHED();
     return;
   }
 
   DCHECK(message_ports_[message_port_id].queued_messages.empty());
-  message_ports_.erase(message_port_id);
+  Erase(message_port_id);
+  DCHECK(CheckMessagePortMap(true));
 }
 
 void MessagePortDispatcher::OnEntangle(int local_message_port_id,
                                        int remote_message_port_id) {
+  DCHECK(CheckMessagePortMap(false));
   if (!message_ports_.count(local_message_port_id) ||
       !message_ports_.count(remote_message_port_id)) {
     NOTREACHED();
@@ -115,12 +124,14 @@ void MessagePortDispatcher::OnEntangle(int local_message_port_id,
       MSG_ROUTING_NONE);
   message_ports_[remote_message_port_id].entangled_message_port_id =
       local_message_port_id;
+  DCHECK(CheckMessagePortMap(false));
 }
 
 void MessagePortDispatcher::OnPostMessage(
     int sender_message_port_id,
     const string16& message,
     const std::vector<int>& sent_message_port_ids) {
+  DCHECK(CheckMessagePortMap(true));
   if (!message_ports_.count(sender_message_port_id)) {
     NOTREACHED();
     return;
@@ -137,12 +148,14 @@ void MessagePortDispatcher::OnPostMessage(
   }
 
   PostMessageTo(entangled_message_port_id, message, sent_message_port_ids);
+  DCHECK(CheckMessagePortMap(true));
 }
 
 void MessagePortDispatcher::PostMessageTo(
     int message_port_id,
     const string16& message,
     const std::vector<int>& sent_message_port_ids) {
+  DCHECK(CheckMessagePortMap(true));
   if (!message_ports_.count(message_port_id)) {
     NOTREACHED();
     return;
@@ -184,9 +197,11 @@ void MessagePortDispatcher::PostMessageTo(
         new_routing_ids);
     entangled_port.sender->Send(ipc_msg);
   }
+  DCHECK(CheckMessagePortMap(true));
 }
 
 void MessagePortDispatcher::OnQueueMessages(int message_port_id) {
+  DCHECK(CheckMessagePortMap(true));
   if (!message_ports_.count(message_port_id)) {
     NOTREACHED();
     return;
@@ -196,11 +211,13 @@ void MessagePortDispatcher::OnQueueMessages(int message_port_id) {
   port.sender->Send(new WorkerProcessMsg_MessagesQueued(port.route_id));
   port.queue_messages = true;
   port.sender = NULL;
+  DCHECK(CheckMessagePortMap(true));
 }
 
 void MessagePortDispatcher::OnSendQueuedMessages(
     int message_port_id,
     const QueuedMessages& queued_messages) {
+  DCHECK(CheckMessagePortMap(true));
   if (!message_ports_.count(message_port_id)) {
     NOTREACHED();
     return;
@@ -214,9 +231,11 @@ void MessagePortDispatcher::OnSendQueuedMessages(
                               queued_messages.begin(),
                               queued_messages.end());
   SendQueuedMessagesIfPossible(message_port_id);
+  DCHECK(CheckMessagePortMap(true));
 }
 
 void MessagePortDispatcher::SendQueuedMessagesIfPossible(int message_port_id) {
+  DCHECK(CheckMessagePortMap(true));
   MessagePort& port = message_ports_[message_port_id];
   if (port.queue_messages || !port.sender)
     return;
@@ -226,11 +245,14 @@ void MessagePortDispatcher::SendQueuedMessagesIfPossible(int message_port_id) {
     PostMessageTo(message_port_id, iter->first, iter->second);
   }
   port.queued_messages.clear();
+  DCHECK(CheckMessagePortMap(true));
 }
 
 void MessagePortDispatcher::Observe(NotificationType type,
                                     const NotificationSource& source,
                                     const NotificationDetails& details) {
+  DCHECK(CheckMessagePortMap(true));
+
   IPC::Message::Sender* sender = NULL;
   if (type.value == NotificationType::RESOURCE_MESSAGE_FILTER_SHUTDOWN) {
     sender = Source<ResourceMessageFilter>(source).ptr();
@@ -245,11 +267,44 @@ void MessagePortDispatcher::Observe(NotificationType type,
        iter != message_ports_.end();) {
     MessagePorts::iterator cur_item = iter++;
     if (cur_item->second.sender == sender) {
-      if (cur_item->second.entangled_message_port_id != MSG_ROUTING_NONE) {
-        message_ports_[cur_item->second.entangled_message_port_id].
-            entangled_message_port_id = MSG_ROUTING_NONE;
-      }
-      message_ports_.erase(cur_item);
+      Erase(cur_item->first);
+    }
+  }
+
+  DCHECK(CheckMessagePortMap(true));
+}
+
+void MessagePortDispatcher::Erase(int message_port_id) {
+  MessagePorts::iterator erase_item = message_ports_.find(message_port_id);
+  DCHECK(erase_item != message_ports_.end());
+
+  int entangled_id = erase_item->second.entangled_message_port_id;
+  if (entangled_id != MSG_ROUTING_NONE) {
+    // Do the disentanglement (and be paranoid about the other side existing
+    // just in case something unusual happened during entanglement).
+    if (message_ports_.count(entangled_id)) {
+      message_ports_[entangled_id].entangled_message_port_id = MSG_ROUTING_NONE;
+    }
+  }
+  message_ports_.erase(erase_item);
+}
+
+#ifndef NDEBUG
+bool MessagePortDispatcher::CheckMessagePortMap(bool check_entanglements) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+
+  for (MessagePorts::iterator iter = message_ports_.begin();
+       iter != message_ports_.end(); iter++) {
+    DCHECK(iter->first <= next_message_port_id_);
+    DCHECK(iter->first == iter->second.message_port_id);
+
+    int entangled_id = iter->second.entangled_message_port_id;
+    if (check_entanglements && entangled_id != MSG_ROUTING_NONE) {
+      MessagePorts::iterator entangled_item = message_ports_.find(entangled_id);
+      DCHECK(entangled_item != message_ports_.end());
+      DCHECK(entangled_item->second.entangled_message_port_id == iter->first);
     }
   }
+  return true;
 }
+#endif  // NDEBUG
diff --git a/chrome/browser/worker_host/message_port_dispatcher.h b/chrome/browser/worker_host/message_port_dispatcher.h
index 21be0c6..16f4d22 100644
--- a/chrome/browser/worker_host/message_port_dispatcher.h
+++ b/chrome/browser/worker_host/message_port_dispatcher.h
@@ -67,6 +67,16 @@ class MessagePortDispatcher : public NotificationObserver {
                const NotificationSource& source,
                const NotificationDetails& details);
 
+  // Handles the details of removing a message port id. Before calling this,
+  // verify that the message port id exists.
+  void Erase(int message_port_id);
+
+#ifdef NDEBUG
+  bool CheckMessagePortMap(bool check_entanglements) { }
+#else
+  bool CheckMessagePortMap(bool check_entanglements);
+#endif
+
   struct MessagePort {
     // sender and route_id are what we need to send messages to the port.
     IPC::Message::Sender* sender;
diff --git a/tools/valgrind/memcheck/suppressions.txt b/tools/valgrind/memcheck/suppressions.txt
index 2a2c26e..0e9db9e 100644
--- a/tools/valgrind/memcheck/suppressions.txt
+++ b/tools/valgrind/memcheck/suppressions.txt
@@ -1139,16 +1139,6 @@
    fun:_ZN12RenderThread15OnCreateNewViewEiRK19RendererPreferencesRK14WebPreferencesi
 }
 {
-   bug_27318
-   Memcheck:Cond
-   fun:_ZN21MessagePortDispatcher7ObserveE16NotificationTypeRK18NotificationSourceRK19NotificationDetails
-   fun:_ZN19NotificationService6NotifyE16NotificationTypeRK18NotificationSourceRK19NotificationDetails
-   fun:_ZN17WorkerProcessHostD0Ev
-   fun:_ZN16ChildProcessHost11OnChildDiedEv
-   fun:_ZN16ChildProcessHost12ListenerHook14OnChannelErrorEv
-   fun:_ZN3IPC7Channel11ChannelImpl28OnFileCanReadWithoutBlockingEi
-}
-{
    # See comment in main_menu.cc
    main_menu_leak
    Memcheck:Leak
@@ -1203,49 +1193,6 @@
    fun:_ZN11MessageLoop10RunHandlerEv
 }
 {
-   bug_27839
-   Memcheck:Cond
-   fun:_ZN21MessagePortDispatcher7ObserveE16NotificationTypeRK18NotificationSourceRK19NotificationDetails
-   fun:_ZN19NotificationService6NotifyE16NotificationTypeRK18NotificationSourceRK19NotificationDetails
-   fun:_ZN21ResourceMessageFilterD0Ev
-   fun:_ZN12ChromeThread14DeleteOnThreadILNS_2IDE4EE8DestructI21ResourceMessageFilterEEvPT_
-   fun:_ZN21ResourceMessageFilter10OnDestructEv
-   fun:_ZN3IPC12ChannelProxy19MessageFilterTraits8DestructEPNS0_13MessageFilterE
-   fun:_ZN4base20RefCountedThreadSafeIN3IPC12ChannelProxy13MessageFilterENS2_19MessageFilterTraitsEE7ReleaseEv
-   fun:_ZN13scoped_refptrIN3IPC12ChannelProxy13MessageFilterEED1Ev
-   fun:_ZSt8_DestroyI13scoped_refptrIN3IPC12ChannelProxy13MessageFilterEEEvPT_
-   fun:_ZSt13__destroy_auxIP13scoped_refptrIN3IPC12ChannelProxy13MessageFilterEEEvT_S6_St12__false_type
-   fun:_ZSt8_DestroyIP13scoped_refptrIN3IPC12ChannelProxy13MessageFilterEEEvT_S6_
-   fun:_ZSt8_DestroyIP13scoped_refptrIN3IPC12ChannelProxy13MessageFilterEES4_EvT_S6_SaIT0_E
-   fun:_ZNSt6vectorI13scoped_refptrIN3IPC12ChannelProxy13MessageFilterEESaIS4_EE15_M_erase_at_endEPS4_
-   fun:_ZNSt6vectorI13scoped_refptrIN3IPC12ChannelProxy13MessageFilterEESaIS4_EE5clearEv
-   fun:_ZN3IPC12ChannelProxy7Context15OnChannelClosedEv
-   fun:_ZN3IPC11SyncChannel11SyncContext15OnChannelClosedEv
-   fun:_Z16DispatchToMethodIN3IPC12ChannelProxy7ContextEMS2_FvvEEvPT_T0_RK6Tuple0
-   fun:_ZN14RunnableMethodIN3IPC12ChannelProxy7ContextEMS2_FvvE6Tuple0E3RunEv
-   fun:_ZN11MessageLoop7RunTaskEP4Task
-   fun:_ZN11MessageLoop21DeferOrRunPendingTaskERKNS_11PendingTaskE
-   fun:_ZN11MessageLoop6DoWorkEv
-   fun:_ZN4base19MessagePumpLibevent3RunEPNS_11MessagePump8DelegateE
-}
-{
-   bug_27922
-   Memcheck:Cond
-   fun:_ZN21MessagePortDispatcher7ObserveE16NotificationTypeRK18NotificationSourceRK19NotificationDetails
-   fun:_ZN19NotificationService6NotifyE16NotificationTypeRK18NotificationSourceRK19NotificationDetails
-   fun:_ZN21ResourceMessageFilter14OnChannelErrorEv
-   fun:_ZN3IPC12ChannelProxy7Context14OnChannelErrorEv
-   fun:_ZN3IPC11SyncChannel11SyncContext14OnChannelErrorEv
-   fun:_ZN3IPC12ChannelProxy7Context13OnSendMessageEPNS_7MessageE
-   fun:_ZN3IPC8SendTask3RunEv
-   fun:_ZN11MessageLoop7RunTaskEP4Task
-   fun:_ZN11MessageLoop21DeferOrRunPendingTaskERKNS_11PendingTaskE
-   fun:_ZN11MessageLoop6DoWorkEv
-   fun:_ZN4base19MessagePumpLibevent3RunEPNS_11MessagePump8DelegateE
-   fun:_ZN11MessageLoop11RunInternalEv
-   fun:_ZN11MessageLoop10RunHandlerEv
-}
-{
    bug_27936
    Memcheck:Leak
    fun:_Znw*