Adding the message reading class used for remote_security_key STDIN communication.

remoting/host/security_key/remote_security_key_message_reader.cc

Index: remoting/host/security_key/remote_security_key_message_reader.cc
diff --git a/remoting/host/security_key/remote_security_key_message_reader.cc b/remoting/host/security_key/remote_security_key_message_reader.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7bc3ba9d05aaad0c9c4b95e6fb041c712c63a7a0
--- /dev/null
+++ b/remoting/host/security_key/remote_security_key_message_reader.cc
@@ -0,0 +1,163 @@
+// 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/remote_security_key_message_reader.h"
+
+#include <stdint.h>
+
+#include <string>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/files/file.h"
+#include "base/macros.h"
+#include "base/sequenced_task_runner.h"
+#include "base/single_thread_task_runner.h"
+#include "base/stl_util.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/threading/thread_checker.h"
+#include "remoting/host/security_key/security_key_message.h"
+
+namespace remoting {
+
+class RemoteSecurityKeyMessageReader::Core {
+ public:
+  Core(base::File input_file,
+       scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
+       base::WeakPtr<RemoteSecurityKeyMessageReader> reader_);
+  ~Core();
+
+  // Reads a message from the remote security key process and passes it to
+  // |message_callback_| on the originating thread. Called on the reader thread.
+  void ReadMessage();
+
+ private:
+  // Notify the owning class when an error occurs or EOF is reached.
+  void NotifyError();
+
+  base::File read_stream_;
+
+  base::WeakPtr<RemoteSecurityKeyMessageReader> reader_;
+
+  // Used to post callbacks on the caller thread.
+  scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+RemoteSecurityKeyMessageReader::Core::Core(
+    base::File input_file,
+    scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
+    base::WeakPtr<RemoteSecurityKeyMessageReader> reader)
+    : read_stream_(std::move(input_file)),
+      reader_(reader),
+      caller_task_runner_(caller_task_runner) {
+  thread_checker_.DetachFromThread();
+}
+
+RemoteSecurityKeyMessageReader::Core::~Core() {}
+
+void RemoteSecurityKeyMessageReader::Core::ReadMessage() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  while (true) {
+    if (!read_stream_.IsValid()) {
+      LOG(ERROR) << "Cannot read from invalid stream.";
+      NotifyError();
+      return;
+    }
+
+    // Read the message header to retrieve the remaining message length.
+    uint32_t total_message_size_bytes;
+    int read_result = read_stream_.ReadAtCurrentPos(
+        reinterpret_cast<char*>(&total_message_size_bytes),
+        SecurityKeyMessage::kHeaderSizeBytes);
+    if (read_result != SecurityKeyMessage::kHeaderSizeBytes) {
+      // 0 means EOF which is normal and should not be logged as an error.
+      if (read_result != 0) {
+        LOG(ERROR) << "Failed to read message header, read returned "
+                   << read_result;
+      }
+      NotifyError();
+      return;
+    }
+
+    if (!SecurityKeyMessage::IsValidMessageSize(total_message_size_bytes)) {
+      LOG(ERROR) << "Message size too large: " << total_message_size_bytes;
+      NotifyError();
+      return;
+    }
+
+    std::string message_data(total_message_size_bytes, '\0');
+    read_result = read_stream_.ReadAtCurrentPos(string_as_array(&message_data),
+                                                total_message_size_bytes);
+    // The static cast is safe as we know the value is smaller than max int.
+    if (read_result != static_cast<int>(total_message_size_bytes)) {
+      LOG(ERROR) << "Failed to read message: " << read_result;
+      NotifyError();
+      return;
+    }
+
+    scoped_ptr<SecurityKeyMessage> message(new SecurityKeyMessage());
+    if (!message->ParseMessage(message_data)) {
+      LOG(ERROR) << "Invalid message data received.";
+      NotifyError();
+      return;
+    }
+
+    // Notify callback of the new message received.
+    caller_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&RemoteSecurityKeyMessageReader::InvokeMessageCallback,
+                   reader_, base::Passed(&message)));
+  }
+}
+
+void RemoteSecurityKeyMessageReader::Core::NotifyError() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  caller_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RemoteSecurityKeyMessageReader::InvokeErrorCallback,
+                 reader_));
+}
+
+RemoteSecurityKeyMessageReader::RemoteSecurityKeyMessageReader(
+    base::File input_file)
+    : reader_thread_("RemoteSecurityKeyMessageReader"), weak_factory_(this) {
+  reader_thread_.Start();
+  read_task_runner_ = reader_thread_.task_runner();
+  core_.reset(new Core(std::move(input_file),
+                       base::ThreadTaskRunnerHandle::Get(),
+                       weak_factory_.GetWeakPtr()));
+}
+
+RemoteSecurityKeyMessageReader::~RemoteSecurityKeyMessageReader() {
+  read_task_runner_->DeleteSoon(FROM_HERE, core_.release());
+}

[Sergey Ulanov, 2016/03/28 18:52:45]

Thread destructor will join the thread and it will block while the thread is
still running ReadMessage(), which may be forever. Native Messaging works the
same, but it may be less of an issue there because chrome normally closes the
pipe when NM host should be stopped and it's the correct behavior there is
arguable, see
https://chromiumcodereview.appspot.com/12852005/diff2/67001:95001/remoting/ho...
Are the requirements different here? Do we want to be able to stop the process
while the pipe is still open?

[joedow, 2016/03/30 19:39:00]

I don't think I have the same problem here.  I close the read_stream_ handle in
the D'tor which breaks the ReadMessage() loop on the reader thread.  Does that
alleviate the concern or is there another scenario I'm missing?  I've added a
test to make sure this scenario works correctly and does not regress.

+
+void RemoteSecurityKeyMessageReader::Start(
+    SecurityKeyMessage::Callback message_callback,
+    base::Closure error_callback) {
+  message_callback_ = message_callback;
+  error_callback_ = error_callback;
+
+  // base::Unretained is safe since |core_| is only deleted via the
+  // DeleteSoon task which is posted from this class's dtor.
+  read_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&RemoteSecurityKeyMessageReader::Core::ReadMessage,
+                            base::Unretained(core_.get())));
+}
+
+void RemoteSecurityKeyMessageReader::InvokeMessageCallback(
+    scoped_ptr<SecurityKeyMessage> message) {
+  message_callback_.Run(std::move(message));
+}
+
+void RemoteSecurityKeyMessageReader::InvokeErrorCallback() {
+  error_callback_.Run();
+}
+
+}  // namespace remoting