Fixing SecurityKeySocket unit test issues and cleanup

remoting/host/security_key/security_key_socket.cc

 // Copyright 2016 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/host/security_key/security_key_socket.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/macros.h"
@@ skipping 24 matching lines @@
   timer_.reset(new base::Timer(false, false));
   timer_->Start(FROM_HERE, timeout, timeout_callback);
 }
 
 SecurityKeySocket::~SecurityKeySocket() {}
 
 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());
   request_data_.clear();
   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(
     const base::Closure& request_received_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(request_received_callback_.is_null());
 
   request_received_callback_ = request_received_callback;
   DoRead();
 }
 
 void SecurityKeySocket::OnDataWritten(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(write_buffer_);
 
   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;
   }
 
   ResetTimer();
   request_data_.insert(request_data_.end(), read_buffer_->data(),
                        read_buffer_->data() + result);
   if (IsRequestComplete()) {
     read_completed_ = true;
     base::ResetAndReturn(&request_received_callback_).Run();
     return;
   }
 
   DoRead();
 }
 
 void SecurityKeySocket::DoRead() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   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;
 }
 
 size_t SecurityKeySocket::GetRequestLength() const {
   DCHECK(request_data_.size() >= kRequestSizeBytes);
 
   return ((request_data_[0] & 255) << 24) + ((request_data_[1] & 255) << 16) +
          ((request_data_[2] & 255) << 8) + (request_data_[3] & 255) +
          kRequestSizeBytes;
 }
 
 std::string SecurityKeySocket::GetResponseLengthAsBytes(
     const std::string& response) const {
   std::string response_len;
   response_len.reserve(kRequestSizeBytes);
   int len = response.size();
 
   response_len.push_back((len >> 24) & 255);
   response_len.push_back((len >> 16) & 255);
   response_len.push_back((len >> 8) & 255);
   response_len.push_back(len & 255);
 
   return response_len;
 }
 
 void SecurityKeySocket::ResetTimer() {
-  if (timer_->IsRunning())
+  if (timer_->IsRunning()) {
     timer_->Reset();
+  }
 }
 
 }  // namespace remoting