Fix the ui test crashes on Linux. There is a race condition between the channel getting destroyed

and it being referenced in the background thread.

Fix is to use the underlying IPC::Channel pointer passed to the message filter when the
filter gets Added to send and receive messages. This ensures that the SyncChannel is not used across
threads.

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

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

4 files changed, 52 insertions, 16 deletions

diff --git a/chrome/test/automation/automation_handle_tracker.cc b/chrome/test/automation/automation_handle_tracker.cc
index 6ced518..5a1e623 100644
--- a/chrome/test/automation/automation_handle_tracker.cc
+++ b/chrome/test/automation/automation_handle_tracker.cc
@@ -18,6 +18,8 @@ AutomationResourceProxy::AutomationResourceProxy(
 }
 
 AutomationResourceProxy::~AutomationResourceProxy() {
+  if (tracker_)
+    tracker_->Remove(this);
 }
 
 AutomationHandleTracker::~AutomationHandleTracker() {
@@ -26,7 +28,6 @@ AutomationHandleTracker::~AutomationHandleTracker() {
   for (HandleToObjectMap::iterator iter = handle_to_object_.begin();
        iter != handle_to_object_.end(); ++iter) {
     iter->second->Invalidate();
-    iter->second->TrackerGone();
   }
 }
 
@@ -36,11 +37,13 @@ void AutomationHandleTracker::Add(AutomationResourceProxy* proxy) {
 }
 
 void AutomationHandleTracker::Remove(AutomationResourceProxy* proxy) {
+  AutoLock lock(map_lock_);
   HandleToObjectMap::iterator iter = handle_to_object_.find(proxy->handle());
   if (iter != handle_to_object_.end()) {
     AutomationHandle proxy_handle = proxy->handle();
     handle_to_object_.erase(iter);
-    sender_->Send(new AutomationMsg_HandleUnused(0, proxy_handle));
+    if (channel_)
+      channel_->Send(new AutomationMsg_HandleUnused(0, proxy_handle));
   }
 }
 
@@ -49,8 +52,9 @@ void AutomationHandleTracker::InvalidateHandle(AutomationHandle handle) {
   AutoLock lock(map_lock_);
   HandleToObjectMap::iterator iter = handle_to_object_.find(handle);
   if (iter != handle_to_object_.end()) {
-    iter->second->Invalidate();
-    Remove(iter->second);
+    scoped_refptr<AutomationResourceProxy> proxy = iter->second;
+    handle_to_object_.erase(iter);
+    proxy->Invalidate();
   }
 }
 
diff --git a/chrome/test/automation/automation_handle_tracker.h b/chrome/test/automation/automation_handle_tracker.h
index 1117710..fc33600 100644
--- a/chrome/test/automation/automation_handle_tracker.h
+++ b/chrome/test/automation/automation_handle_tracker.h
@@ -13,6 +13,7 @@
 #include "base/basictypes.h"
 #include "base/lock.h"
 #include "base/ref_counted.h"
+#include "ipc/ipc_channel.h"
 
 // This represents a value that the app's AutomationProvider returns
 // when asked for a resource (like a window or tab).
@@ -30,7 +31,11 @@ class AutomationResourceProxy
 
   // Marks this proxy object as no longer valid; this generally means
   // that the corresponding resource on the app side is gone.
-  void Invalidate() { is_valid_ = false; }
+  void Invalidate() {
+    is_valid_ = false;
+    tracker_ = NULL;
+  }
+
   bool is_valid() const { return is_valid_; }
 
   // Returns the handle that the app has generated to refer to this resource.
@@ -44,12 +49,6 @@ class AutomationResourceProxy
 
   AutomationHandle handle_;
 
-  // Called by the tracker when it is being destroyed so we know not to call
-  // it back.
-  void TrackerGone() {
-    tracker_ = NULL;
-  }
-
   // Not owned by us, owned by the AutomationProxy object. May be NULL if the
   // tracker has been destroyed (and hence the object is invalid).
   AutomationHandleTracker* tracker_;
@@ -74,8 +73,7 @@ class AutomationResourceProxy
 // discard the handle.
 class AutomationHandleTracker {
  public:
-  explicit AutomationHandleTracker(AutomationMessageSender* sender)
-      : sender_(sender) {}
+  explicit AutomationHandleTracker() : channel_(NULL) {}
   ~AutomationHandleTracker();
 
   // Adds the specified proxy object to the tracker.
@@ -94,6 +92,11 @@ class AutomationHandleTracker {
   void InvalidateHandle(AutomationHandle handle);
 
   AutomationResourceProxy* GetResource(AutomationHandle handle);
+
+  void put_channel(IPC::Channel* channel) {
+    channel_ = channel;
+  }
+
  private:
   typedef
     std::map<AutomationHandle, scoped_refptr<AutomationResourceProxy> >
@@ -102,8 +105,8 @@ class AutomationHandleTracker {
 
   HandleToObjectMap handle_to_object_;
 
-  AutomationMessageSender* sender_;
   Lock map_lock_;
+  IPC::Channel* channel_;
   DISALLOW_EVIL_CONSTRUCTORS(AutomationHandleTracker);
 };
 
diff --git a/chrome/test/automation/automation_proxy.cc b/chrome/test/automation/automation_proxy.cc
index 55ebc08..7c29ac4 100644
--- a/chrome/test/automation/automation_proxy.cc
+++ b/chrome/test/automation/automation_proxy.cc
@@ -55,6 +55,14 @@ class AutomationMessageFilter : public IPC::ChannelProxy::MessageFilter {
     return handled;
   }
 
+  virtual void OnFilterAdded(IPC::Channel* channel) {
+    server_->SetChannel(channel);
+  }
+
+  virtual void OnFilterRemoved() {
+    server_->ResetChannel();
+  }
+
   void NewTabLoaded(int load_time) {
     server_->SignalNewTabUITab(load_time);
   }
@@ -87,11 +95,13 @@ AutomationProxy::AutomationProxy(int command_execution_timeout_ms)
       app_launch_signaled_(0),
       perform_version_check_(false),
       command_execution_timeout_(
-          TimeDelta::FromMilliseconds(command_execution_timeout_ms)) {
+          TimeDelta::FromMilliseconds(command_execution_timeout_ms)),
+      listener_thread_id_(0) {
   // base::WaitableEvent::TimedWait() will choke if we give it a negative value.
   // Zero also seems unreasonable, since we need to wait for IPC, but at
   // least it is legal... ;-)
   DCHECK_GE(command_execution_timeout_ms, 0);
+  listener_thread_id_ = PlatformThread::CurrentId();
   InitializeChannelID();
   InitializeHandleTracker();
   InitializeThread();
@@ -151,7 +161,7 @@ void AutomationProxy::InitializeChannel() {
 }
 
 void AutomationProxy::InitializeHandleTracker() {
-  tracker_.reset(new AutomationHandleTracker(this));
+  tracker_.reset(new AutomationHandleTracker());
 }
 
 AutomationLaunchResult AutomationProxy::WaitForAppLaunch() {
@@ -462,6 +472,8 @@ bool AutomationProxy::Send(IPC::Message* message) {
 
 bool AutomationProxy::SendWithTimeout(IPC::Message* message, int timeout,
                                       bool* is_timeout) {
+  //DCHECK_EQ(listener_thread_id_, PlatformThread::CurrentId());
+
   if (is_timeout)
     *is_timeout = false;
 
@@ -534,3 +546,14 @@ template <class T> scoped_refptr<T> AutomationProxy::ProxyObjectFromHandle(
   result.swap(&p);
   return result;
 }
+
+void AutomationProxy::SetChannel(IPC::Channel* channel) {
+  if (tracker_.get())
+    tracker_->put_channel(channel);
+}
+
+void AutomationProxy::ResetChannel() {
+  if (tracker_.get())
+    tracker_->put_channel(NULL);
+}
+
diff --git a/chrome/test/automation/automation_proxy.h b/chrome/test/automation/automation_proxy.h
index 174dbb2..d012d01 100644
--- a/chrome/test/automation/automation_proxy.h
+++ b/chrome/test/automation/automation_proxy.h
@@ -230,6 +230,10 @@ class AutomationProxy : public IPC::Channel::Listener,
     perform_version_check_ = perform_version_check;
   }
 
+  // These functions set and reset the IPC::Channel pointer on the tracker.
+  void SetChannel(IPC::Channel* channel);
+  void ResetChannel();
+
  protected:
   template <class T> scoped_refptr<T> ProxyObjectFromHandle(int handle);
   void InitializeChannelID();
@@ -265,6 +269,8 @@ class AutomationProxy : public IPC::Channel::Listener,
   // Delay to let the browser execute the command.
   base::TimeDelta command_execution_timeout_;
 
+  PlatformThreadId listener_thread_id_;
+
   DISALLOW_COPY_AND_ASSIGN(AutomationProxy);
 };