Refactor of Authenticator to allow it to ProcessMessage asynchronously and then call a callback

remoting/protocol/jingle_session.cc

Index: remoting/protocol/jingle_session.cc
diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index bfab845debed86b9f7b98c032acd653953f14679..1dbc92776b0773304cb7d6b2f8d74e6be3566ef0 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -147,7 +147,13 @@ void JingleSession::AcceptIncomingConnection(
   }
 
   DCHECK_EQ(authenticator_->state(), Authenticator::WAITING_MESSAGE);
-  authenticator_->ProcessMessage(first_auth_message);
+  authenticator_->ProcessMessage(base::Bind(
+      &JingleSession::ContinueAcceptIncomingConnection,
+      // Authenticator is owned by JingleSession, and cannot outlive it.
+      base::Unretained(this)), first_auth_message);
+}
+
+void JingleSession::ContinueAcceptIncomingConnection() {

[Sergey Ulanov, 2013/02/28 08:05:16]

This change introduces another state that wasn't possible before, and we keep
state_ set to INITIALIZING before this method is called. We need to make sure
that we handle the case when another message is received while in that state. I
suggest adding another state value in Session::State enum and then look through
this class to make sure it's handled properly everywhere. Specifically I see
that OnTerminate() needs to be updated, but there may be some other places.

[rmsousa, 2013/02/28 23:29:23]

AcceptIncomingConnection is called after InitializeIncomingConnection, so it's
already in the CONNECTING stage.

Looking through the code, we seem to mostly already do the right thing in a
"CONNECTING" state: New SessionInfo messages for that session are rejected with
UNEXPECTED_REQUEST, CloseInternal sends a TERMINATE message, and OnTerminate
closes the session.

The only problem is that CONNECTING doesn't differentiate whether this is the
initiator or acceptor, so we could indeed get some interesting interactions
where we get confused: e.g. If we get an ACCEPT or TERMINATE message while we're
still processing the INITIATE, the acceptor suddenly thinks it was the one
trying to connect, and assumes the session was ACCEPTED/REJECTED.

I separated the states in CONNECTING and ACCEPTING, to avoid that kind of issue.
The Session doesn't need an explicit PROCESSING_MESSAGE state per se (receiving
a message while the authenticator is processing another is perfectly valid, as
long it's not another authentication message), that state must live in the
authenticator.

   if (authenticator_->state() == Authenticator::REJECTED) {
     CloseInternal(AuthRejectionReasonToErrorCode(
         authenticator_->rejection_reason()));
@@ -435,9 +441,6 @@ void JingleSession::OnAccept(const JingleMessage& message,
     return;
   }
 
-  DCHECK(authenticator_->state() == Authenticator::WAITING_MESSAGE);
-  authenticator_->ProcessMessage(auth_message);
-
   if (!InitializeConfigFromDescription(message.description.get())) {
     CloseInternal(INCOMPATIBLE_PROTOCOL);
     return;
@@ -448,12 +451,11 @@ void JingleSession::OnAccept(const JingleMessage& message,
 
   SetState(CONNECTED);
 
-  // Process authentication.
-  if (authenticator_->state() == Authenticator::ACCEPTED) {
-    SetState(AUTHENTICATED);
-  } else {
-    ProcessAuthenticationStep();
-  }
+  DCHECK(authenticator_->state() == Authenticator::WAITING_MESSAGE);
+  authenticator_->ProcessMessage(base::Bind(
+      &JingleSession::ProcessAuthenticationStep,
+      // Authenticator is owned by JingleSession, and cannot outlive it.
+      base::Unretained(this)), auth_message);
 }
 
 void JingleSession::OnSessionInfo(const JingleMessage& message,
@@ -475,8 +477,10 @@ void JingleSession::OnSessionInfo(const JingleMessage& message,
 
   reply_callback.Run(JingleMessageReply::NONE);
 
-  authenticator_->ProcessMessage(message.info.get());
-  ProcessAuthenticationStep();
+  authenticator_->ProcessMessage(base::Bind(
+      &JingleSession::ProcessAuthenticationStep,
+      // Authenticator is owned by JingleSession, and cannot outlive it.
+      base::Unretained(this)), message.info.get());
 }
 
 void JingleSession::ProcessTransportInfo(const JingleMessage& message) {
@@ -550,6 +554,7 @@ bool JingleSession::InitializeConfigFromDescription(
 }
 
 void JingleSession::ProcessAuthenticationStep() {
+  DCHECK(CalledOnValidThread());
   DCHECK_EQ(state_, CONNECTED);
 
   if (authenticator_->state() == Authenticator::MESSAGE_READY) {