Fix and enable JingleSessionTest.*. Disable these tests under TSan.

BUG=70225
TEST=None

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

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

7 files changed, 70 insertions, 62 deletions

diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index 3293396..713dc06 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -86,6 +86,7 @@ JingleSession::JingleSession(
       server_cert_(server_cert),
       state_(INITIALIZING),
       closed_(false),
+      closing_(false),
       cricket_session_(NULL),
       event_channel_(NULL),
       video_channel_(NULL),
@@ -113,17 +114,16 @@ void JingleSession::Init(cricket::Session* cricket_session) {
   cert_verifier_.reset(new net::CertVerifier());
   cricket_session_->SignalState.connect(
       this, &JingleSession::OnSessionState);
+  cricket_session_->SignalError.connect(
+      this, &JingleSession::OnSessionError);
 }
 
-void JingleSession::CloseInternal(Task* closed_task, int result) {
-  if (MessageLoop::current() != jingle_session_manager_->message_loop()) {
-    jingle_session_manager_->message_loop()->PostTask(
-        FROM_HERE, NewRunnableMethod(this, &JingleSession::CloseInternal,
-                                     closed_task, result));
-    return;
-  }
+void JingleSession::CloseInternal(int result, bool failed) {
+  DCHECK_EQ(jingle_session_manager_->message_loop(), MessageLoop::current());
+
+  if (!closed_ && !closing_) {
+    closing_ = true;
 
-  if (!closed_) {
     if (control_ssl_socket_.get())
       control_ssl_socket_.reset();
 
@@ -165,16 +165,14 @@ void JingleSession::CloseInternal(Task* closed_task, int result) {
     if (cricket_session_)
       cricket_session_->Terminate();
 
-    SetState(CLOSED);
+    if (failed)
+      SetState(FAILED);
+    else
+      SetState(CLOSED);
 
     closed_ = true;
   }
   cert_verifier_.reset();
-
-  if (closed_task) {
-    closed_task->Run();
-    delete closed_task;
-  }
 }
 
 bool JingleSession::HasSession(cricket::Session* cricket_session) {
@@ -279,13 +277,29 @@ void JingleSession::set_receiver_token(const std::string& receiver_token) {
 }
 
 void JingleSession::Close(Task* closed_task) {
-  CloseInternal(closed_task, net::ERR_CONNECTION_CLOSED);
+  if (MessageLoop::current() != jingle_session_manager_->message_loop()) {
+    jingle_session_manager_->message_loop()->PostTask(
+        FROM_HERE, NewRunnableMethod(this, &JingleSession::Close, closed_task));
+    return;
+  }
+
+  CloseInternal(net::ERR_CONNECTION_CLOSED, false);
+
+  if (closed_task) {
+    closed_task->Run();
+    delete closed_task;
+  }
 }
 
 void JingleSession::OnSessionState(
     BaseSession* session, BaseSession::State state) {
   DCHECK_EQ(cricket_session_, session);
 
+  if (closed_) {
+    // Don't do anything if we already closed.
+    return;
+  }
+
   switch (state) {
     case cricket::Session::STATE_SENTINITIATE:
     case cricket::Session::STATE_RECEIVEDINITIATE:
@@ -315,6 +329,15 @@ void JingleSession::OnSessionState(
   }
 }
 
+void JingleSession::OnSessionError(
+    BaseSession* session, BaseSession::Error error) {
+  DCHECK_EQ(cricket_session_, session);
+
+  if (error != cricket::Session::ERROR_NONE) {
+    CloseInternal(net::ERR_CONNECTION_ABORTED, true);
+  }
+}
+
 void JingleSession::OnInitiate() {
   jid_ = cricket_session_->remote_name();
 
@@ -362,7 +385,10 @@ void JingleSession::OnInitiate() {
   if (!cricket_session_->initiator())
     jingle_session_manager_->AcceptConnection(this, cricket_session_);
 
-  SetState(CONNECTING);
+  if (!closed_) {
+    // Set state to CONNECTING if the session is being accepted.
+    SetState(CONNECTING);
+  }
 }
 
 bool JingleSession::EstablishSSLConnection(
@@ -450,44 +476,14 @@ void JingleSession::OnAccept() {
 }
 
 void JingleSession::OnTerminate() {
-  if (control_channel_adapter_.get())
-    control_channel_adapter_->Close(net::ERR_CONNECTION_ABORTED);
-  if (control_channel_) {
-    control_channel_->OnSessionTerminate(cricket_session_);
-    control_channel_ = NULL;
-  }
-
-  if (event_channel_adapter_.get())
-    event_channel_adapter_->Close(net::ERR_CONNECTION_ABORTED);
-  if (event_channel_) {
-    event_channel_->OnSessionTerminate(cricket_session_);
-    event_channel_ = NULL;
-  }
-
-  if (video_channel_adapter_.get())
-    video_channel_adapter_->Close(net::ERR_CONNECTION_ABORTED);
-  if (video_channel_) {
-    video_channel_->OnSessionTerminate(cricket_session_);
-    video_channel_ = NULL;
-  }
-
-  if (video_rtp_channel_.get())
-    video_rtp_channel_->Close(net::ERR_CONNECTION_ABORTED);
-  if (video_rtcp_channel_.get())
-    video_rtcp_channel_->Close(net::ERR_CONNECTION_ABORTED);
-
-  SetState(CLOSED);
-
-  closed_ = true;
+  CloseInternal(net::ERR_CONNECTION_ABORTED, false);
 }
 
 void JingleSession::OnSSLConnect(int result) {
   DCHECK(!closed_);
   if (result != net::OK) {
     LOG(ERROR) << "Error during SSL connection: " << result;
-    // TODO(hclam): Just setting the state is not enough. Need to invoke a
-    // callback to report failure.
-    CloseInternal(NULL, result);
+    CloseInternal(result, true);
     return;
   }
 
@@ -503,6 +499,9 @@ void JingleSession::OnSSLConnect(int result) {
 
 void JingleSession::SetState(State new_state) {
   if (new_state != state_) {
+    DCHECK_NE(state_, CLOSED);
+    DCHECK_NE(state_, FAILED);
+
     state_ = new_state;
     if (!closed_ && state_change_callback_.get())
       state_change_callback_->Run(new_state);
diff --git a/remoting/protocol/jingle_session.h b/remoting/protocol/jingle_session.h
index 94c28f6..617ded8 100644
--- a/remoting/protocol/jingle_session.h
+++ b/remoting/protocol/jingle_session.h
@@ -92,7 +92,7 @@ class JingleSession : public protocol::Session,
   void Init(cricket::Session* cricket_session);
 
   // Close all the channels and terminate the session.
-  void CloseInternal(Task* closed_task, int result);
+  void CloseInternal(int result, bool failed);
 
   bool HasSession(cricket::Session* cricket_session);
   cricket::Session* ReleaseSession();
@@ -106,6 +106,9 @@ class JingleSession : public protocol::Session,
   // Used for Session.SignalState sigslot.
   void OnSessionState(cricket::BaseSession* session,
                       cricket::BaseSession::State state);
+  // Used for Session.SignalError sigslot.
+  void OnSessionError(cricket::BaseSession* session,
+                      cricket::BaseSession::Error error);
 
   void OnInitiate();
   void OnAccept();
@@ -129,6 +132,7 @@ class JingleSession : public protocol::Session,
   scoped_ptr<StateChangeCallback> state_change_callback_;
 
   bool closed_;
+  bool closing_;
 
   // JID of the other side. Set when the connection is initialized,
   // and never changed after that.
diff --git a/remoting/protocol/jingle_session_unittest.cc b/remoting/protocol/jingle_session_unittest.cc
index 18bf2b0..692b2f0 100644
--- a/remoting/protocol/jingle_session_unittest.cc
+++ b/remoting/protocol/jingle_session_unittest.cc
@@ -587,15 +587,13 @@ TEST_F(JingleSessionTest, RejectConnection) {
 }
 
 // Verify that we can connect two endpoints.
-// Disabled, http://crbug.com/70225.
-TEST_F(JingleSessionTest, DISABLED_Connect) {
+TEST_F(JingleSessionTest, Connect) {
   CreateServerPair();
   ASSERT_TRUE(InitiateConnection());
 }
 
 // Verify that data can be transmitted over the event channel.
-// Disabled, http://crbug.com/70225.
-TEST_F(JingleSessionTest, DISABLED_TestControlChannel) {
+TEST_F(JingleSessionTest, TestControlChannel) {
   CreateServerPair();
   ASSERT_TRUE(InitiateConnection());
   scoped_refptr<TCPChannelTester> tester(
@@ -625,8 +623,7 @@ TEST_F(JingleSessionTest, TestVideoChannel) {
 }
 
 // Verify that data can be transmitted over the event channel.
-// Disabled, http://crbug.com/70225.
-TEST_F(JingleSessionTest, DISABLED_TestEventChannel) {
+TEST_F(JingleSessionTest, TestEventChannel) {
   CreateServerPair();
   ASSERT_TRUE(InitiateConnection());
   scoped_refptr<TCPChannelTester> tester(
diff --git a/remoting/protocol/session.h b/remoting/protocol/session.h
index 286dbe7..3051a68 100644
--- a/remoting/protocol/session.h
+++ b/remoting/protocol/session.h
@@ -82,8 +82,9 @@ class Session : public base::RefCountedThreadSafe<Session> {
   virtual const std::string& receiver_token() = 0;
   virtual void set_receiver_token(const std::string& receiver_token) = 0;
 
-  // Closes connection. Callbacks are guaranteed not to be called after
-  // |closed_task| is executed.
+  // Closes connection. Callbacks are guaranteed not to be called
+  // after |closed_task| is executed. Must be called before the object
+  // is destroyed, unless the state is set to FAILED or CLOSED.
   virtual void Close(Task* closed_task) = 0;
 
  protected:
diff --git a/tools/valgrind/gtest_exclude/remoting_unittests.gtest-drmemory_win32.txt b/tools/valgrind/gtest_exclude/remoting_unittests.gtest-drmemory_win32.txt
index 3b09ec1..4e0696a 100644
--- a/tools/valgrind/gtest_exclude/remoting_unittests.gtest-drmemory_win32.txt
+++ b/tools/valgrind/gtest_exclude/remoting_unittests.gtest-drmemory_win32.txt
@@ -1,10 +1,10 @@
-# These tests fail under Dr. Memory; see http://crbug.com/57832
+# Following tests create real libjingle connections, and libjingle has
+# hardcoded timeouts, so these tests fail under TSan.
+JingleSessionTest.Connect
 JingleSessionTest.TestControlChannel
 JingleSessionTest.TestEventChannel
 JingleSessionTest.TestVideoChannel
 JingleSessionTest.TestVideoRtpChannel
-# http://crbug.com/70225
-JingleSessionTest.Connect
 
 # This test fails on an assertion, see http://crbug.com/57266
 DecoderVp8Test.EncodeAndDecode
diff --git a/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan.txt b/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan.txt
index 0703323..5a8a7cb 100644
--- a/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan.txt
+++ b/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan.txt
@@ -1,4 +1,6 @@
-# Fail under TSan, see http://crbug.com/57832
+# Following tests create real libjingle connections, and libjingle has
+# hardcoded timeouts, so these tests fail under TSan.
+JingleSessionTest.Connect
 JingleSessionTest.TestControlChannel
 JingleSessionTest.TestEventChannel
 JingleSessionTest.TestVideoChannel
diff --git a/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan_win32.txt b/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan_win32.txt
index 6b0a66f..5a8a7cb 100644
--- a/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan_win32.txt
+++ b/tools/valgrind/gtest_exclude/remoting_unittests.gtest-tsan_win32.txt
@@ -1,2 +1,7 @@
-# Fails under TSan/Win, see http://crbug.com/70225
+# Following tests create real libjingle connections, and libjingle has
+# hardcoded timeouts, so these tests fail under TSan.
 JingleSessionTest.Connect
+JingleSessionTest.TestControlChannel
+JingleSessionTest.TestEventChannel
+JingleSessionTest.TestVideoChannel
+JingleSessionTest.TestVideoRtpChannel