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

remoting/protocol/jingle_session.cc

 // 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.
 
 #include "remoting/protocol/jingle_session.h"
 
 #include "base/bind.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
@@ skipping 129 matching lines @@
   // Process the first authentication message.
   const buzz::XmlElement* first_auth_message =
       initiate_message.description->authenticator_message();
 
   if (!first_auth_message) {
     CloseInternal(INCOMPATIBLE_PROTOCOL);
     return;
   }
 
   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()));
     return;
   }
 
   // Send the session-accept message.
   JingleMessage message(peer_jid_, JingleMessage::SESSION_ACCEPT,
                         session_id_);
 
@@ skipping 267 matching lines @@
 
   const buzz::XmlElement* auth_message =
       message.description->authenticator_message();
   if (!auth_message) {
     DLOG(WARNING) << "Received session-accept without authentication message "
                   << auth_message->Str();
     CloseInternal(INCOMPATIBLE_PROTOCOL);
     return;
   }
 
-  DCHECK(authenticator_->state() == Authenticator::WAITING_MESSAGE);
-  authenticator_->ProcessMessage(auth_message);
-
   if (!InitializeConfigFromDescription(message.description.get())) {
     CloseInternal(INCOMPATIBLE_PROTOCOL);
     return;
   }
 
   // In case there is transport information in the accept message.
   ProcessTransportInfo(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,
                                   const ReplyCallback& reply_callback) {
   if (!message.info.get() ||
       !Authenticator::IsAuthenticatorMessage(message.info.get())) {
     reply_callback.Run(JingleMessageReply::UNSUPPORTED_INFO);
     return;
   }
 
   if (state_ != CONNECTED ||
       authenticator_->state() != Authenticator::WAITING_MESSAGE) {
     LOG(WARNING) << "Received unexpected authenticator message "
                  << message.info->Str();
     reply_callback.Run(JingleMessageReply::UNEXPECTED_REQUEST);
     CloseInternal(INCOMPATIBLE_PROTOCOL);
     return;
   }
 
   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) {
   for (std::list<JingleMessage::NamedCandidate>::const_iterator it =
            message.candidates.begin();
        it != message.candidates.end(); ++it) {
     ChannelsMap::iterator channel = channels_.find(it->name);
     if (channel == channels_.end()) {
       LOG(WARNING) << "Received candidate for unknown channel " << it->name;
       continue;
@@ skipping 53 matching lines @@
   }
   if (!candidate_config()->IsSupported(config_)) {
     LOG(ERROR) << "session-accept specifies an invalid configuration";
     return false;
   }
 
   return true;
 }
 
 void JingleSession::ProcessAuthenticationStep() {
+  DCHECK(CalledOnValidThread());
   DCHECK_EQ(state_, CONNECTED);
 
   if (authenticator_->state() == Authenticator::MESSAGE_READY) {
     JingleMessage message(peer_jid_, JingleMessage::SESSION_INFO, session_id_);
     message.info = authenticator_->GetNextMessage();
     DCHECK(message.info.get());
     SendMessage(message);
   }
   DCHECK_NE(authenticator_->state(), Authenticator::MESSAGE_READY);
 
@@ skipping 54 matching lines @@
     DCHECK_NE(state_, FAILED);
 
     state_ = new_state;
     if (event_handler_)
       event_handler_->OnSessionStateChange(new_state);
   }
 }
 
 }  // namespace protocol
 }  // namespace remoting