[Draft] Fix canceling pin prompt causes host overload

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/strings/string_number_conversions.h"
@@ skipping 163 matching lines @@
                              auth_message.Pass()));
   SendMessage(message);
 
   // Update state.
   SetState(CONNECTED);
 
   if (authenticator_->state() == Authenticator::ACCEPTED) {
     SetState(AUTHENTICATED);
   } else {
     DCHECK_EQ(authenticator_->state(), Authenticator::WAITING_MESSAGE);
+    if (authenticator_->started()) {

[rmsousa, 2014/03/26 21:37:35]

Do we want to ensure that we pass through the AUTHENTICATING state between
CONNECTED and AUTHENTICATED? if so, this should be before the outer if block.

It might be useful to keep the backoff behavior consistent between
authenticators that start immediately or not, but I don't really see a practical
consequence if we don't do that.

[kelvinp, 2014/03/27 03:23:21]

After talking to Sergey, I think we can allow switching directly from connected
to authenticated.

+      SetState(AUTHENTICATING);
+    }
   }
 
   return;

[Sergey Ulanov, 2014/03/26 01:49:16]

while you are here, please remove this line

[kelvinp, 2014/03/27 03:23:21]

Done.

 }
 
 const std::string& JingleSession::jid() {
   DCHECK(CalledOnValidThread());
   return peer_jid_;
 }
 
 const CandidateSessionConfig* JingleSession::candidate_config() {
   DCHECK(CalledOnValidThread());
   return candidate_config_.get();
@@ skipping 281 matching lines @@
 }
 
 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 ||
+  if ((state_ != CONNECTED && state_ != AUTHENTICATING) ||
       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);
 
@@ skipping 12 matching lines @@
       // Transport info was received before the channel was created.
       // This could happen due to messages being reordered on the wire.
       pending_remote_candidates_.push_back(*it);
     }
   }
 }
 
 void JingleSession::OnTerminate(const JingleMessage& message,
                                 const ReplyCallback& reply_callback) {
   if (state_ != CONNECTING && state_ != ACCEPTING && state_ != CONNECTED &&
-      state_ != AUTHENTICATED) {
+      state_ != AUTHENTICATING && state_ != AUTHENTICATED) {
     LOG(WARNING) << "Received unexpected session-terminate message.";
     reply_callback.Run(JingleMessageReply::UNEXPECTED_REQUEST);
     return;
   }
 
   reply_callback.Run(JingleMessageReply::NONE);
 
   switch (message.reason) {
     case JingleMessage::SUCCESS:
       if (state_ == CONNECTING) {
@@ skipping 38 matching lines @@
     return false;
   }
 
   return true;
 }
 
 void JingleSession::ProcessAuthenticationStep() {
   DCHECK(CalledOnValidThread());
   DCHECK_NE(authenticator_->state(), Authenticator::PROCESSING_MESSAGE);
 
-  if (state_ != CONNECTED) {
+  if (state_ != CONNECTED && state_ != AUTHENTICATING) {
     DCHECK(state_ == FAILED || state_ == CLOSED);
     // The remote host closed the connection while the authentication was being
     // processed asynchronously, nothing to do.
     return;
   }
 
+  if (authenticator_->started()) {

[rmsousa, 2014/03/26 21:37:35]

authenticator may set the started bit in GetNextMessage, so this should be after
the if block below.

[Sergey Ulanov, 2014/03/27 00:00:03]

+1 I think it should be an else-if case in the end of this method.

[kelvinp, 2014/03/27 03:23:21]

Done.

[kelvinp, 2014/03/27 03:23:21]

Done.

+      SetState(AUTHENTICATING);

[Sergey Ulanov, 2014/03/26 01:49:16]

nit: 2-space indent relative to if

[kelvinp, 2014/03/27 03:23:21]

My bad. Fixed

Done.

+  }
+
   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);
 
   if (authenticator_->state() == Authenticator::ACCEPTED) {
     SetState(AUTHENTICATED);
   } else if (authenticator_->state() == Authenticator::REJECTED) {
     CloseInternal(AuthRejectionReasonToErrorCode(
         authenticator_->rejection_reason()));
   }
 }
 
 void JingleSession::CloseInternal(ErrorCode error) {
   DCHECK(CalledOnValidThread());
 
   if (state_ == CONNECTING || state_ == ACCEPTING || state_ == CONNECTED ||
-      state_ == AUTHENTICATED) {
+      state_ == AUTHENTICATING || state_ == AUTHENTICATED) {

[Sergey Ulanov, 2014/03/26 01:49:16]

this is duplicated in OnTerminate(). Maybe add is_session_active() to avoid
duplication.

[kelvinp, 2014/03/27 03:23:21]

Good Idea. Done.

     // Send session-terminate message with the appropriate error code.
     JingleMessage::Reason reason;
     switch (error) {
       case OK:
         reason = JingleMessage::SUCCESS;
         break;
       case SESSION_REJECTED:
       case AUTHENTICATION_FAILED:
         reason = JingleMessage::DECLINE;
         break;
@@ skipping 32 matching lines @@
     DCHECK_NE(state_, FAILED);
 
     state_ = new_state;
     if (event_handler_)
       event_handler_->OnSessionStateChange(new_state);
   }
 }
 
 }  // namespace protocol
 }  // namespace remoting