Fix crashing bug in EngineAuthenticationHandler on token mismatch.

* Modify logic to only invoke the read callback on authentication
  success. Previously, the callback was invoked even if the connection
  was terminated due to auth failure. This lead to a segfault because
  the callback was referencing the message pump that belonged to the
  now-destroyed connection.
* Restructure auth logic for clarity - success case at the end of the
  function, with error cases returning early.

R=haibinlu@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1712083002

Cr-Commit-Position: refs/heads/master@{#376499}

2 files changed, 22 insertions, 13 deletions

diff --git a/blimp/net/engine_authentication_handler.cc b/blimp/net/engine_authentication_handler.cc
index 41813fc..39037b5 100644
--- a/blimp/net/engine_authentication_handler.cc
+++ b/blimp/net/engine_authentication_handler.cc
@@ -4,6 +4,8 @@
 
 #include "blimp/net/engine_authentication_handler.h"
 
+#include <string>
+
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/timer/timer.h"
@@ -108,23 +110,30 @@ void Authenticator::OnConnectionError(int error) {
 
 void Authenticator::ProcessMessage(scoped_ptr<BlimpMessage> message,
                                    const net::CompletionCallback& callback) {
-  if (message->type() == BlimpMessage::PROTOCOL_CONTROL &&
-      message->protocol_control().type() ==
+  if (message->type() != BlimpMessage::PROTOCOL_CONTROL ||
+      message->protocol_control().type() !=
           ProtocolControlMessage::START_CONNECTION) {
-    bool token_match =
-        client_token_ ==
-        message->protocol_control().start_connection().client_token();
-    DVLOG(1) << "Authentication challenge received: "
-             << message->protocol_control().start_connection().client_token()
-             << ", and token "
-             << (token_match ? " matches" : " does not match");
-    OnConnectionAuthenticated(token_match);
-  } else {
     DVLOG(1) << "Expected START_CONNECTION message, got " << *message
              << " instead.";
     OnConnectionAuthenticated(false);
+    return;
+  }
+
+  bool token_matches =
+      client_token_ ==
+      message->protocol_control().start_connection().client_token();
+  DVLOG(1) << "Authentication challenge received: "
+           << message->protocol_control().start_connection().client_token()
+           << ", and token "
+           << (token_matches ? " matches" : " does not match");
+  if (!token_matches) {
+    OnConnectionAuthenticated(false);
+    return;
   }
 
+  // Authentication succeeded!
+  // Run |callback| to signal the pump to read more messages.
+  OnConnectionAuthenticated(true);
   callback.Run(net::OK);
 }
 
diff --git a/blimp/net/engine_authentication_handler_unittest.cc b/blimp/net/engine_authentication_handler_unittest.cc
index dd8943c..9dc52b3 100644
--- a/blimp/net/engine_authentication_handler_unittest.cc
+++ b/blimp/net/engine_authentication_handler_unittest.cc
@@ -84,7 +84,7 @@ TEST_F(EngineAuthenticationHandlerTest, AuthenticationFailed) {
   net::TestCompletionCallback process_message_cb;
   incoming_message_processor_->ProcessMessage(std::move(blimp_message),
                                               process_message_cb.callback());
-  EXPECT_EQ(net::OK, process_message_cb.WaitForResult());
+  EXPECT_FALSE(process_message_cb.have_result());
 }
 
 TEST_F(EngineAuthenticationHandlerTest, WrongMessageReceived) {
@@ -96,7 +96,7 @@ TEST_F(EngineAuthenticationHandlerTest, WrongMessageReceived) {
   net::TestCompletionCallback process_message_cb;
   incoming_message_processor_->ProcessMessage(std::move(blimp_message),
                                               process_message_cb.callback());
-  EXPECT_EQ(net::OK, process_message_cb.WaitForResult());
+  EXPECT_FALSE(process_message_cb.have_result());
 }
 
 TEST_F(EngineAuthenticationHandlerTest, ConnectionError) {