Synchronize access to TestAutomationResourceMessageFilter's requests_ map. This fixes a race condition in Chrome Frame's net tests that could cause URLRequestJobs to be created and destroyed on different threads. This in turn leaks PowerObservers from SystemMonitor's observer list, resulting in an observer list full of dangling pointers. In debug mode this may eventually dcheck if heap addresses are reused.

BUG=109733
TEST=chrome_frame_net_tests.exe does not DCHECK.
Review URL: http://codereview.chromium.org/9158012

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

2 files changed, 25 insertions, 2 deletions

diff --git a/chrome_frame/test/net/test_automation_resource_message_filter.cc b/chrome_frame/test/net/test_automation_resource_message_filter.cc
index 655950c..1b3ae8d 100644
--- a/chrome_frame/test/net/test_automation_resource_message_filter.cc
+++ b/chrome_frame/test/net/test_automation_resource_message_filter.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -29,11 +29,26 @@ bool TestAutomationResourceMessageFilter::OnMessageReceived(
   bool handled = false;
   int request_id;
   if (URLRequestAutomationJob::MayFilterMessage(message, &request_id)) {
+    base::AutoLock lock(requests_lock_);
     RequestMap::iterator it = requests_.find(request_id);
     if (it != requests_.end()) {
       handled = true;
       IPC::Message* msg = new IPC::Message(message);
       RequestJob& job = it->second;
+
+      // SUBTLE: Why is this safe? We pass the URLRequestAutomationJob to a
+      // posted task which then takes a reference. We then release the lock,
+      // meaning we are no longer protecting access to the request_map_ which
+      // holds our last owned reference to the URLRequestAutomationJob. Thus
+      // the posted task could be the one holding the last reference.
+      //
+      // If the posted task were to be run on a thread other than the one the
+      // URLRequestAutomationJob was created on, we could destroy the job on
+      // the wrong thread (resulting in badness as URLRequestJobs must be
+      // created and destroyed on the same thread). The destruction will happen
+      // on the correct thread here since we post to job.loop_ which is set as
+      // the message loop of the current thread when RegisterRequest is invoked
+      // by URLRequestJob's constructor.
       job.loop_->PostTask(FROM_HERE,
                           base::Bind(OnRequestMessage, job.job_, msg));
     }
@@ -49,6 +64,7 @@ bool TestAutomationResourceMessageFilter::RegisterRequest(
   // Store the request in an internal map like the parent class
   // does, but also store the current loop pointer so we can send
   // request messages to that loop.
+  base::AutoLock lock(requests_lock_);
   DCHECK(requests_.end() == requests_.find(job->id()));
   RequestJob request_job = { MessageLoop::current(), job };
   requests_[job->id()] = request_job;
@@ -58,5 +74,6 @@ bool TestAutomationResourceMessageFilter::RegisterRequest(
 // Remove request from the list of outstanding requests.
 void TestAutomationResourceMessageFilter::UnRegisterRequest(
     URLRequestAutomationJob* job) {
+  base::AutoLock lock(requests_lock_);
   requests_.erase(job->id());
 }
diff --git a/chrome_frame/test/net/test_automation_resource_message_filter.h b/chrome_frame/test/net/test_automation_resource_message_filter.h
index 281a45a..4630401 100644
--- a/chrome_frame/test/net/test_automation_resource_message_filter.h
+++ b/chrome_frame/test/net/test_automation_resource_message_filter.h
@@ -1,9 +1,12 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 #ifndef CHROME_FRAME_TEST_NET_TEST_AUTOMATION_RESOURCE_MESSAGE_FILTER_H_
 #define CHROME_FRAME_TEST_NET_TEST_AUTOMATION_RESOURCE_MESSAGE_FILTER_H_
 
+#include <map>
+
+#include "base/synchronization/lock.h"
 #include "chrome/browser/automation/automation_provider.h"
 #include "chrome/browser/automation/automation_resource_message_filter.h"
 #include "chrome/browser/automation/url_request_automation_job.h"
@@ -46,6 +49,9 @@ class TestAutomationResourceMessageFilter
   };
   typedef std::map<int, RequestJob> RequestMap;
   RequestMap requests_;
+
+  // Protects access to requests_.
+  base::Lock requests_lock_;
 };
 
 #endif  // CHROME_FRAME_TEST_NET_TEST_AUTOMATION_RESOURCE_MESSAGE_FILTER_H_