Fix Valgrind errors in remoting_unittests.

- FakeSession was used after it is destroyed.
- JingleThreadMessageLoop::Quit() was calling Thread::Stop() before all tasks are finished.
- JingleSessionManager::AcceptConnection() would destroy JingleSession when called by that JingleSession.

BUG=None
TEST=Valgrind is happy.

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

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

6 files changed, 70 insertions, 61 deletions

diff --git a/remoting/jingle_glue/jingle_thread.cc b/remoting/jingle_glue/jingle_thread.cc
index 2f3ba00..b0b409a 100644
--- a/remoting/jingle_glue/jingle_thread.cc
+++ b/remoting/jingle_glue/jingle_thread.cc
@@ -21,21 +21,28 @@ class JingleMessagePump : public base::MessagePump,
                           public talk_base::MessageHandler {
  public:
   JingleMessagePump(talk_base::Thread* thread)
-      : thread_(thread), delegate_(NULL) {
+      : thread_(thread), delegate_(NULL), stopping_(false) {
   }
 
   virtual void Run(Delegate* delegate) {
     delegate_ = delegate;
 
-    talk_base::Thread::Current()->Thread::Run();
+    thread_->Thread::Run();
     // Call Restart() so that we can run again.
-    talk_base::Thread::Current()->Restart();
+    thread_->Restart();
 
     delegate_ = NULL;
   }
 
   virtual void Quit() {
-    talk_base::Thread::Current()->Quit();
+    if (!stopping_) {
+      stopping_ = true;
+
+      // Shutdown gracefully: make sure that we excute all messages
+      // left in the queue before exiting. Thread::Quit() would not do
+      // that.
+      thread_->Post(this, kStopMessageId);
+    }
   }
 
   virtual void ScheduleWork() {
@@ -48,27 +55,43 @@ class JingleMessagePump : public base::MessagePump,
   }
 
   void OnMessage(talk_base::Message* msg) {
-    DCHECK(msg->message_id == kRunTasksMessageId);
-    DCHECK(delegate_);
-
-    // Clear currently pending messages in case there were delayed tasks.
-    // Will schedule it again from ScheduleNextDelayedTask() if neccessary.
-    thread_->Clear(this, kRunTasksMessageId);
-
-    // Process all pending tasks.
-    while (true) {
-      if (delegate_->DoWork())
-        continue;
-      if (delegate_->DoDelayedWork(&delayed_work_time_))
-        continue;
-      if (delegate_->DoIdleWork())
-        continue;
-      break;
-    }
+    if (msg->message_id == kRunTasksMessageId) {
+      DCHECK(delegate_);
+
+      // Clear currently pending messages in case there were delayed tasks.
+      // Will schedule it again from ScheduleNextDelayedTask() if neccessary.
+      thread_->Clear(this, kRunTasksMessageId);
+
+      // Process all pending tasks.
+      while (true) {
+        if (delegate_->DoWork())
+          continue;
+        if (delegate_->DoDelayedWork(&delayed_work_time_))
+          continue;
+        if (delegate_->DoIdleWork())
+          continue;
+        break;
+      }
 
-    ScheduleNextDelayedTask();
+      ScheduleNextDelayedTask();
+    } else if (msg->message_id == kStopMessageId) {
+      DCHECK(stopping_);
+      // Stop the thread only if there are no more non-delayed
+      // messages left in the queue, otherwise post another task to
+      // try again later.
+      int delay = thread_->GetDelay();
+      if (delay > 0 || delay == talk_base::kForever) {
+        stopping_ = false;
+        thread_->Quit();
+      } else {
+        thread_->Post(this, kStopMessageId);
+      }
+    } else {
+      NOTREACHED();
+    }
   }
 
+
  private:
   void ScheduleNextDelayedTask() {
     if (!delayed_work_time_.is_null()) {
@@ -85,6 +108,7 @@ class JingleMessagePump : public base::MessagePump,
   talk_base::Thread* thread_;
   Delegate* delegate_;
   base::TimeTicks delayed_work_time_;
+  bool stopping_;
 };
 
 }  // namespace
@@ -145,9 +169,7 @@ void JingleThread::Run() {
 }
 
 void JingleThread::Stop() {
-  // Shutdown gracefully: make sure that we excute all messages left in the
-  // queue before exiting. Thread::Stop() would not do that.
-  Post(this, kStopMessageId);
+  message_loop_->PostTask(FROM_HERE, new MessageLoop::QuitTask());
   stopped_event_.Wait();
 
   // This will wait until the thread is actually finished.
@@ -162,16 +184,4 @@ TaskPump* JingleThread::task_pump() {
   return task_pump_;
 }
 
-void JingleThread::OnMessage(talk_base::Message* msg) {
-  DCHECK(msg->message_id == kStopMessageId);
-
-  // Stop the thread only if there are no more messages left in the queue,
-  // otherwise post another task to try again later.
-  if (!msgq_.empty() || fPeekKeep_) {
-    Post(this, kStopMessageId);
-  } else {
-    MessageQueue::Quit();
-  }
-}
-
 }  // namespace remoting
diff --git a/remoting/jingle_glue/jingle_thread.h b/remoting/jingle_glue/jingle_thread.h
index 1db62f3..35b46f2 100644
--- a/remoting/jingle_glue/jingle_thread.h
+++ b/remoting/jingle_glue/jingle_thread.h
@@ -42,8 +42,7 @@ class JingleThreadMessageLoop : public MessageLoop {
 
 // TODO(sergeyu): This class should be changed to inherit from Chromiums
 // base::Thread instead of libjingle's thread.
-class JingleThread : public talk_base::Thread,
-                     public talk_base::MessageHandler {
+class JingleThread : public talk_base::Thread {
  public:
   JingleThread();
   virtual ~JingleThread();
@@ -65,8 +64,6 @@ class JingleThread : public talk_base::Thread,
   TaskPump* task_pump();
 
  private:
-  virtual void OnMessage(talk_base::Message* msg);
-
   TaskPump* task_pump_;
   base::WaitableEvent started_event_;
   base::WaitableEvent stopped_event_;
diff --git a/remoting/protocol/connection_to_client_unittest.cc b/remoting/protocol/connection_to_client_unittest.cc
index c5db404..3609511 100644
--- a/remoting/protocol/connection_to_client_unittest.cc
+++ b/remoting/protocol/connection_to_client_unittest.cc
@@ -97,8 +97,6 @@ TEST_F(ConnectionToClientTest, StateChange) {
 TEST_F(ConnectionToClientTest, Close) {
   viewer_->Disconnect();
   message_loop_.RunAllPending();
-  EXPECT_TRUE(session_->is_closed());
-
   viewer_->Disconnect();
   message_loop_.RunAllPending();
 }
diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index 4b35de6..f4b49fb 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -451,13 +451,20 @@ void JingleSession::OnInitiate() {
   video_channel_.reset(
       new jingle_glue::TransportChannelSocketAdapter(raw_video_channel_));
 
-  if (!cricket_session_->initiator())
-    jingle_session_manager_->AcceptConnection(this, cricket_session_);
-
-  if (!closed_) {
-    // Set state to CONNECTING if the session is being accepted.
-    SetState(CONNECTING);
+  if (!cricket_session_->initiator()) {
+    if (!jingle_session_manager_->AcceptConnection(this, cricket_session_)) {
+      Close();
+      // Release session so that
+      // JingleSessionManager::SessionDestroyed() doesn't try to call
+      // cricket::SessionManager::DestroySession() for it.
+      ReleaseSession();
+      delete this;
+      return;
+    }
   }
+
+  // Set state to CONNECTING if the session is being accepted.
+  SetState(CONNECTING);
 }
 
 bool JingleSession::EstablishPseudoTcp(
diff --git a/remoting/protocol/jingle_session_manager.cc b/remoting/protocol/jingle_session_manager.cc
index 93ae125..314f3a0 100644
--- a/remoting/protocol/jingle_session_manager.cc
+++ b/remoting/protocol/jingle_session_manager.cc
@@ -186,7 +186,7 @@ void JingleSessionManager::OnSessionDestroy(cricket::Session* cricket_session) {
   }
 }
 
-void JingleSessionManager::AcceptConnection(
+bool JingleSessionManager::AcceptConnection(
     JingleSession* jingle_session,
     cricket::Session* cricket_session) {
   DCHECK(CalledOnValidThread());
@@ -194,7 +194,7 @@ void JingleSessionManager::AcceptConnection(
   // Reject connection if we are closed.
   if (closed_) {
     cricket_session->Reject(cricket::STR_TERMINATE_DECLINE);
-    return;
+    return false;
   }
 
   const cricket::SessionDescription* session_description =
@@ -230,24 +230,20 @@ void JingleSessionManager::AcceptConnection(
 
     case protocol::SessionManager::INCOMPATIBLE: {
       cricket_session->Reject(cricket::STR_TERMINATE_INCOMPATIBLE_PARAMETERS);
-      jingle_session->ReleaseSession();
-      jingle_session->Close();
-      delete jingle_session;
-      break;
+      return false;
     }
 
     case protocol::SessionManager::DECLINE: {
       cricket_session->Reject(cricket::STR_TERMINATE_DECLINE);
-      jingle_session->ReleaseSession();
-      jingle_session->Close();
-      delete jingle_session;
-      break;
+      return false;
     }
 
     default: {
       NOTREACHED();
     }
   }
+
+  return true;
 }
 
 void JingleSessionManager::SessionDestroyed(JingleSession* jingle_session) {
diff --git a/remoting/protocol/jingle_session_manager.h b/remoting/protocol/jingle_session_manager.h
index 7dcc703..e629f40 100644
--- a/remoting/protocol/jingle_session_manager.h
+++ b/remoting/protocol/jingle_session_manager.h
@@ -76,8 +76,9 @@ class JingleSessionManager
  private:
   friend class JingleSession;
 
-  // Called by JingleSession when a new connection is initiated.
-  void AcceptConnection(JingleSession* jingle_session,
+  // Called by JingleSession when a new connection is
+  // initiated. Returns true if session is accepted.
+  bool AcceptConnection(JingleSession* jingle_session,
                         cricket::Session* cricket_session);
 
   // Called by JingleSession when it is being destroyed.