Fixing SecurityKeySocket unit test issues and cleanup

remoting/host/security_key/security_key_socket.cc

Index: remoting/host/security_key/security_key_socket.cc
diff --git a/remoting/host/security_key/security_key_socket.cc b/remoting/host/security_key/security_key_socket.cc
index 55af794becf1d98aa45cd58d31bdb1fc5aba8b08..85c979ad6f29570df94f03c5947e9db540a4b76f 100644
--- a/remoting/host/security_key/security_key_socket.cc
+++ b/remoting/host/security_key/security_key_socket.cc
@@ -42,10 +42,12 @@ bool SecurityKeySocket::GetAndClearRequestData(std::string* data_out) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(read_completed_);
 
-  if (!read_completed_)
+  if (!read_completed_) {
     return false;
-  if (!IsRequestComplete() || IsRequestTooLarge())
+  }
+  if (!IsRequestComplete() || IsRequestTooLarge()) {
     return false;
+  }
   // The request size is not part of the data; don't send it.
   data_out->assign(request_data_.begin() + kRequestSizeBytes,
                    request_data_.end());
@@ -53,23 +55,28 @@ bool SecurityKeySocket::GetAndClearRequestData(std::string* data_out) {
   return true;
 }
 
-void SecurityKeySocket::SendResponse(const std::string& response_data) {
+void SecurityKeySocket::SendResponse(
+    const std::string& response_data,
+    const base::Closure& response_written_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!write_buffer_);
 
+  response_written_callback_ = response_written_callback;

[Sergey Ulanov, 2016/12/07 20:41:36]

If SendResponse() is called before previous response is sent then the callback
for the previous response will never be called. In this CL the callback is only
used for the error in the end, so maybe we shouldn't allow
response_written_callback only for SsendSshError(). I.e. move this line to
SendSshError() and add DCHECK(!response_written_callback_).

[joedow, 2016/12/13 20:30:44]

Removed the callback mechanism.  Based on having running unit tests that
actually check the return values, I think the error is being sent correctly so
it isn't needed here.

   std::string response_length_string = GetResponseLengthAsBytes(response_data);
   int response_len = response_length_string.size() + response_data.size();
   std::unique_ptr<std::string> response(
       new std::string(response_length_string + response_data));
   write_buffer_ = new net::DrainableIOBuffer(
       new net::StringIOBuffer(std::move(response)), response_len);
+
+  DCHECK(write_buffer_->BytesRemaining());
   DoWrite();
 }
 
-void SecurityKeySocket::SendSshError() {
+void SecurityKeySocket::SendSshError(const base::Closure& complete_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  SendResponse(std::string(kSshError, arraysize(kSshError)));
+  SendResponse(std::string(kSshError, arraysize(kSshError)), complete_callback);
 }
 
 void SecurityKeySocket::StartReadingRequest(
@@ -87,34 +94,44 @@ void SecurityKeySocket::OnDataWritten(int result) {
 
   if (result < 0) {
     LOG(ERROR) << "Error sending response: " << result;
+    if (response_written_callback_) {
+      base::ResetAndReturn(&response_written_callback_).Run();
+    }
     return;
   }
   ResetTimer();
   write_buffer_->DidConsume(result);
-  DoWrite();
+
+  if (write_buffer_->BytesRemaining()) {
+    DoWrite();
+    return;
+  }
+
+  write_buffer_ = nullptr;
+  if (response_written_callback_) {
+    base::ResetAndReturn(&response_written_callback_).Run();
+  }
 }
 
 void SecurityKeySocket::DoWrite() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(write_buffer_);
 
-  if (!write_buffer_->BytesRemaining()) {
-    write_buffer_ = nullptr;
-    return;
-  }
   int result = socket_->Write(
       write_buffer_.get(), write_buffer_->BytesRemaining(),
       base::Bind(&SecurityKeySocket::OnDataWritten, base::Unretained(this)));
-  if (result != net::ERR_IO_PENDING)
+  if (result != net::ERR_IO_PENDING) {
     OnDataWritten(result);
+  }
 }
 
 void SecurityKeySocket::OnDataRead(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (result <= 0) {
-    if (result < 0)
+    if (result < 0) {
       LOG(ERROR) << "Error reading request: " << result;
+    }
     read_completed_ = true;
     base::ResetAndReturn(&request_received_callback_).Run();
     return;
@@ -138,23 +155,26 @@ void SecurityKeySocket::DoRead() {
   int result = socket_->Read(
       read_buffer_.get(), kRequestReadBufferLength,
       base::Bind(&SecurityKeySocket::OnDataRead, base::Unretained(this)));
-  if (result != net::ERR_IO_PENDING)
+  if (result != net::ERR_IO_PENDING) {
     OnDataRead(result);
+  }
 }
 
 bool SecurityKeySocket::IsRequestComplete() const {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (request_data_.size() < kRequestSizeBytes)
+  if (request_data_.size() < kRequestSizeBytes) {
     return false;
+  }
   return GetRequestLength() <= request_data_.size();
 }
 
 bool SecurityKeySocket::IsRequestTooLarge() const {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (request_data_.size() < kRequestSizeBytes)
+  if (request_data_.size() < kRequestSizeBytes) {
     return false;
+  }
   return GetRequestLength() > kMaxRequestLength;
 }
 
@@ -181,8 +201,9 @@ std::string SecurityKeySocket::GetResponseLengthAsBytes(
 }
 
 void SecurityKeySocket::ResetTimer() {
-  if (timer_->IsRunning())
+  if (timer_->IsRunning()) {
     timer_->Reset();
+  }
 }
 
 }  // namespace remoting