unittests for Chromoting native messaging host.

BUG=232200

Review URL: https://chromiumcodereview.appspot.com/14979008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@203084 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 893 insertions, 27 deletions

diff --git a/remoting/host/setup/native_messaging_host.cc b/remoting/host/setup/native_messaging_host.cc
index 7ce09ea..28a0b10 100644
--- a/remoting/host/setup/native_messaging_host.cc
+++ b/remoting/host/setup/native_messaging_host.cc
@@ -37,6 +37,7 @@ scoped_ptr<base::DictionaryValue> ConfigDictionaryFromMessage(
 namespace remoting {
 
 NativeMessagingHost::NativeMessagingHost(
+    scoped_ptr<DaemonController> daemon_controller,
     base::PlatformFile input,
     base::PlatformFile output,
     scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
@@ -45,7 +46,7 @@ NativeMessagingHost::NativeMessagingHost(
       quit_closure_(quit_closure),
       native_messaging_reader_(input),
       native_messaging_writer_(output),
-      daemon_controller_(DaemonController::Create()),
+      daemon_controller_(daemon_controller.Pass()),
       weak_factory_(this) {
   weak_ptr_ = weak_factory_.GetWeakPtr();
 }
@@ -70,10 +71,16 @@ void NativeMessagingHost::Shutdown() {
 
 void NativeMessagingHost::ProcessMessage(scoped_ptr<base::Value> message) {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
+
+  // Don't process any more messages if Shutdown() has been called.
+  if (quit_closure_.is_null())
+    return;
+
   const base::DictionaryValue* message_dict;
   if (!message->GetAsDictionary(&message_dict)) {
     LOG(ERROR) << "Expected DictionaryValue";
     Shutdown();
+    return;
   }
 
   scoped_ptr<base::DictionaryValue> response_dict(new base::DictionaryValue());
diff --git a/remoting/host/setup/native_messaging_host.h b/remoting/host/setup/native_messaging_host.h
index 4bfd09a..9c375ad 100644
--- a/remoting/host/setup/native_messaging_host.h
+++ b/remoting/host/setup/native_messaging_host.h
@@ -26,6 +26,7 @@ namespace remoting {
 class NativeMessagingHost {
  public:
   NativeMessagingHost(
+      scoped_ptr<DaemonController> daemon_controller,
       base::PlatformFile input,
       base::PlatformFile output,
       scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
@@ -79,11 +80,10 @@ class NativeMessagingHost {
                                 DaemonController::AsyncResult result);
   void SendConfigResponse(scoped_ptr<base::DictionaryValue> response,
                           scoped_ptr<base::DictionaryValue> config);
-  void SendUsageStatsConsentResponse(
-      scoped_ptr<base::DictionaryValue> response,
-      bool supported,
-      bool allowed,
-      bool set_by_policy);
+  void SendUsageStatsConsentResponse(scoped_ptr<base::DictionaryValue> response,
+                                     bool supported,
+                                     bool allowed,
+                                     bool set_by_policy);
   void SendAsyncResult(scoped_ptr<base::DictionaryValue> response,
                        DaemonController::AsyncResult result);
 
diff --git a/remoting/host/setup/native_messaging_host_main.cc b/remoting/host/setup/native_messaging_host_main.cc
index 49eee24..35cad5e 100644
--- a/remoting/host/setup/native_messaging_host_main.cc
+++ b/remoting/host/setup/native_messaging_host_main.cc
@@ -33,7 +33,8 @@ int main(int argc, char** argv) {
 
   base::MessageLoop message_loop(base::MessageLoop::TYPE_IO);
   base::RunLoop run_loop;
-  remoting::NativeMessagingHost host(read_file, write_file,
+  remoting::NativeMessagingHost host(remoting::DaemonController::Create(),
+                                     read_file, write_file,
                                      message_loop.message_loop_proxy(),
                                      run_loop.QuitClosure());
   host.Start();
diff --git a/remoting/host/setup/native_messaging_host_unittest.cc b/remoting/host/setup/native_messaging_host_unittest.cc
new file mode 100644
index 0000000..b6fc4ab
--- /dev/null
+++ b/remoting/host/setup/native_messaging_host_unittest.cc
@@ -0,0 +1,517 @@
+// Copyright 2013 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/setup/native_messaging_host.h"
+
+#include "base/compiler_specific.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/message_loop.h"
+#include "base/run_loop.h"
+#include "base/stl_util.h"
+#include "base/strings/stringize_macros.h"
+#include "base/values.h"
+#include "net/base/file_stream.h"
+#include "net/base/net_util.h"
+#include "remoting/host/pin_hash.h"
+#include "remoting/host/setup/test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+void VerifyHelloResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("helloResponse", value);
+  EXPECT_TRUE(response->GetString("version", &value));
+  EXPECT_EQ(STRINGIZE(VERSION), value);
+}
+
+void VerifyGetHostNameResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("getHostNameResponse", value);
+  EXPECT_TRUE(response->GetString("hostname", &value));
+  EXPECT_EQ(net::GetHostName(), value);
+}
+
+void VerifyGetPinHashResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("getPinHashResponse", value);
+  EXPECT_TRUE(response->GetString("hash", &value));
+  EXPECT_EQ(remoting::MakeHostPinHash("my_host", "1234"), value);
+}
+
+void VerifyGenerateKeyPairResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("generateKeyPairResponse", value);
+  EXPECT_TRUE(response->GetString("private_key", &value));
+  EXPECT_TRUE(response->GetString("public_key", &value));
+}
+
+void VerifyGetDaemonConfigResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("getDaemonConfigResponse", value);
+  const base::DictionaryValue* config = NULL;
+  EXPECT_TRUE(response->GetDictionary("config", &config));
+  EXPECT_TRUE(base::DictionaryValue().Equals(config));
+}
+
+void VerifyGetUsageStatsConsentResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("getUsageStatsConsentResponse", value);
+  bool supported, allowed, set_by_policy;
+  EXPECT_TRUE(response->GetBoolean("supported", &supported));
+  EXPECT_TRUE(response->GetBoolean("allowed", &allowed));
+  EXPECT_TRUE(response->GetBoolean("set_by_policy", &set_by_policy));
+  EXPECT_TRUE(supported);
+  EXPECT_TRUE(allowed);
+  EXPECT_TRUE(set_by_policy);
+}
+
+void VerifyStopDaemonResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("stopDaemonResponse", value);
+  int result;
+  EXPECT_TRUE(response->GetInteger("result", &result));
+  EXPECT_EQ(0, result);
+}
+
+void VerifyGetDaemonStateResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("getDaemonStateResponse", value);
+  int result;
+  EXPECT_TRUE(response->GetInteger("state", &result));
+  EXPECT_EQ(4, result);
+}
+
+void VerifyUpdateDaemonConfigResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("updateDaemonConfigResponse", value);
+  int result;
+  EXPECT_TRUE(response->GetInteger("result", &result));
+  EXPECT_EQ(0, result);
+}
+
+void VerifyStartDaemonResponse(const base::DictionaryValue* response) {
+  ASSERT_TRUE(response);
+  std::string value;
+  EXPECT_TRUE(response->GetString("type", &value));
+  EXPECT_EQ("startDaemonResponse", value);
+  int result;
+  EXPECT_TRUE(response->GetInteger("result", &result));
+  EXPECT_EQ(0, result);
+}
+
+}  // namespace
+
+namespace remoting {
+
+class MockDaemonController : public DaemonController {
+ public:
+  MockDaemonController();
+  virtual ~MockDaemonController();
+
+  virtual State GetState() OVERRIDE;
+  virtual void GetConfig(const GetConfigCallback& callback) OVERRIDE;
+  virtual void SetConfigAndStart(scoped_ptr<base::DictionaryValue> config,
+                                 bool consent,
+                                 const CompletionCallback& callback) OVERRIDE;
+  virtual void UpdateConfig(scoped_ptr<base::DictionaryValue> config,
+                            const CompletionCallback& callback) OVERRIDE;
+  virtual void Stop(const CompletionCallback& callback) OVERRIDE;
+  virtual void SetWindow(void* window_handle) OVERRIDE;
+  virtual void GetVersion(const GetVersionCallback& callback) OVERRIDE;
+  virtual void GetUsageStatsConsent(
+      const GetUsageStatsConsentCallback& callback) OVERRIDE;
+
+  // Returns a record of functions called, so that unittests can verify these
+  // were called in the proper sequence.
+  std::string call_log() { return call_log_; }
+
+ private:
+  std::string call_log_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockDaemonController);
+};
+
+MockDaemonController::MockDaemonController() {}
+
+MockDaemonController::~MockDaemonController() {}
+
+DaemonController::State MockDaemonController::GetState() {
+  call_log_ += "GetState:";
+  return DaemonController::STATE_STARTED;
+}
+
+void MockDaemonController::GetConfig(const GetConfigCallback& callback) {
+  call_log_ += "GetConfig:";
+  scoped_ptr<base::DictionaryValue> config(new base::DictionaryValue());
+  callback.Run(config.Pass());
+}
+
+void MockDaemonController::SetConfigAndStart(
+    scoped_ptr<base::DictionaryValue> config, bool consent,
+    const CompletionCallback& callback) {
+  call_log_ += "SetConfigAndStart:";
+
+  // Verify parameters passed in.
+  if (consent && config && config->HasKey("start")) {
+    callback.Run(DaemonController::RESULT_OK);
+  } else {
+    callback.Run(DaemonController::RESULT_FAILED);
+  }
+}
+
+void MockDaemonController::UpdateConfig(
+    scoped_ptr<base::DictionaryValue> config,
+    const CompletionCallback& callback) {
+  call_log_ += "UpdateConfig:";
+  if (config && config->HasKey("update")) {
+    callback.Run(DaemonController::RESULT_OK);
+  } else {
+    callback.Run(DaemonController::RESULT_FAILED);
+  }
+}
+
+void MockDaemonController::Stop(const CompletionCallback& callback) {
+  call_log_ += "Stop:";
+  callback.Run(DaemonController::RESULT_OK);
+}
+
+void MockDaemonController::SetWindow(void* window_handle) {}
+
+void MockDaemonController::GetVersion(const GetVersionCallback& callback) {
+  // Unused - NativeMessagingHost returns the compiled-in version string
+  // instead of calling this method.
+}
+
+void MockDaemonController::GetUsageStatsConsent(
+    const GetUsageStatsConsentCallback& callback) {
+  call_log_ += "GetUsageStatsConsent:";
+  callback.Run(true, true, true);
+}
+
+class NativeMessagingHostTest : public testing::Test {
+ public:
+  NativeMessagingHostTest();
+  virtual ~NativeMessagingHostTest();
+
+  virtual void SetUp() OVERRIDE;
+  virtual void TearDown() OVERRIDE;
+
+  void Run();
+
+  scoped_ptr<base::DictionaryValue> ReadMessageFromOutputPipe();
+
+  void WriteMessageToInputPipe(const base::Value& message);
+
+  // The Host process should shut down when it receives a malformed request.
+  // This is tested by sending a known-good request, followed by |message|,
+  // followed by the known-good request again. The response file should only
+  // contain a single response from the first good request.
+  void TestBadRequest(const base::Value& message);
+
+ protected:
+  // Reference to the MockDaemonController, which is owned by |host_|.
+  MockDaemonController* daemon_controller_;
+  std::string call_log_;
+
+ private:
+  // Each test creates two unidirectional pipes: "input" and "output".
+  // NativeMessagingHost reads from input_read_handle and writes to
+  // output_write_handle. The unittest supplies data to input_write_handle, and
+  // verifies output from output_read_handle.
+  //
+  // unittest -> [input] -> NativeMessagingHost -> [output] -> unittest
+  base::PlatformFile input_read_handle_;
+  base::PlatformFile input_write_handle_;
+  base::PlatformFile output_read_handle_;
+  base::PlatformFile output_write_handle_;
+
+  base::MessageLoop message_loop_;
+  base::RunLoop run_loop_;
+  scoped_ptr<remoting::NativeMessagingHost> host_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeMessagingHostTest);
+};
+
+NativeMessagingHostTest::NativeMessagingHostTest()
+    : message_loop_(base::MessageLoop::TYPE_IO) {}
+
+NativeMessagingHostTest::~NativeMessagingHostTest() {}
+
+void NativeMessagingHostTest::SetUp() {
+  ASSERT_TRUE(MakePipe(&input_read_handle_, &input_write_handle_));
+  ASSERT_TRUE(MakePipe(&output_read_handle_, &output_write_handle_));
+
+  daemon_controller_ = new MockDaemonController();
+  scoped_ptr<DaemonController> daemon_controller(daemon_controller_);
+  host_.reset(new NativeMessagingHost(daemon_controller.Pass(),
+                                      input_read_handle_, output_write_handle_,
+                                      message_loop_.message_loop_proxy(),
+                                      run_loop_.QuitClosure()));
+}
+
+void NativeMessagingHostTest::TearDown() {
+  // The NativeMessagingHost dtor closes the handles that are passed to it.
+  // |input_write_handle_| gets closed just before starting the host. So the
+  // only handle left to close is |output_read_handle_|.
+  base::ClosePlatformFile(output_read_handle_);
+}
+
+void NativeMessagingHostTest::Run() {
+  // Close the write-end of input, so that the host sees EOF after reading
+  // messages and won't block waiting for more input.
+  base::ClosePlatformFile(input_write_handle_);
+  host_->Start();
+  run_loop_.Run();
+
+  // Destroy |host_| so that it closes its end of the output pipe, so that
+  // TestBadRequest() will see EOF and won't block waiting for more data.
+  // Since |host_| owns |daemon_controller_|, capture its call log first.
+  call_log_ = daemon_controller_->call_log();
+  host_.reset(NULL);
+}
+
+scoped_ptr<base::DictionaryValue>
+NativeMessagingHostTest::ReadMessageFromOutputPipe() {
+  uint32 length;
+  int read_result = base::ReadPlatformFileAtCurrentPos(
+      output_read_handle_, reinterpret_cast<char*>(&length), sizeof(length));
+  if (read_result != sizeof(length)) {
+    return scoped_ptr<base::DictionaryValue>();
+  }
+
+  std::string message_json(length, '\0');
+  read_result = base::ReadPlatformFileAtCurrentPos(
+      output_read_handle_, string_as_array(&message_json), length);
+  if (read_result != static_cast<int>(length)) {
+    return scoped_ptr<base::DictionaryValue>();
+  }
+
+  scoped_ptr<base::Value> message(base::JSONReader::Read(message_json));
+  if (!message || !message->IsType(base::Value::TYPE_DICTIONARY)) {
+    return scoped_ptr<base::DictionaryValue>();
+  }
+
+  return scoped_ptr<base::DictionaryValue>(
+      static_cast<base::DictionaryValue*>(message.release()));
+}
+
+void NativeMessagingHostTest::WriteMessageToInputPipe(
+    const base::Value& message) {
+  std::string message_json;
+  base::JSONWriter::Write(&message, &message_json);
+
+  uint32 length = message_json.length();
+  base::WritePlatformFileAtCurrentPos(input_write_handle_,
+                                      reinterpret_cast<char*>(&length),
+                                      sizeof(length));
+  base::WritePlatformFileAtCurrentPos(input_write_handle_, message_json.data(),
+                                      length);
+}
+
+void NativeMessagingHostTest::TestBadRequest(const base::Value& message) {
+  base::DictionaryValue good_message;
+  good_message.SetString("type", "hello");
+
+  WriteMessageToInputPipe(good_message);
+  WriteMessageToInputPipe(message);
+  WriteMessageToInputPipe(good_message);
+
+  Run();
+
+  // Read from output pipe, and verify responses.
+  scoped_ptr<base::DictionaryValue> response =
+      ReadMessageFromOutputPipe();
+  VerifyHelloResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  EXPECT_FALSE(response);
+}
+
+// Test all valid request-types.
+TEST_F(NativeMessagingHostTest, All) {
+  base::DictionaryValue message;
+  message.SetString("type", "hello");
+  WriteMessageToInputPipe(message);
+
+  message.SetString("type", "getHostName");
+  WriteMessageToInputPipe(message);
+
+  message.SetString("type", "getPinHash");
+  message.SetString("hostId", "my_host");
+  message.SetString("pin", "1234");
+  WriteMessageToInputPipe(message);
+
+  message.Clear();
+  message.SetString("type", "generateKeyPair");
+  WriteMessageToInputPipe(message);
+
+  message.SetString("type", "getDaemonConfig");
+  WriteMessageToInputPipe(message);
+
+  message.SetString("type", "getUsageStatsConsent");
+  WriteMessageToInputPipe(message);
+
+  message.SetString("type", "stopDaemon");
+  WriteMessageToInputPipe(message);
+
+  message.SetString("type", "getDaemonState");
+  WriteMessageToInputPipe(message);
+
+  // Following messages require a "config" dictionary.
+  base::DictionaryValue config;
+  config.SetBoolean("update", true);
+  message.Set("config", config.DeepCopy());
+  message.SetString("type", "updateDaemonConfig");
+  WriteMessageToInputPipe(message);
+
+  config.Clear();
+  config.SetBoolean("start", true);
+  message.Set("config", config.DeepCopy());
+  message.SetBoolean("consent", true);
+  message.SetString("type", "startDaemon");
+  WriteMessageToInputPipe(message);
+
+  Run();
+
+  // Read from output pipe, and verify responses.
+  scoped_ptr<base::DictionaryValue> response = ReadMessageFromOutputPipe();
+  VerifyHelloResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyGetHostNameResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyGetPinHashResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyGenerateKeyPairResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyGetDaemonConfigResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyGetUsageStatsConsentResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyStopDaemonResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyGetDaemonStateResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyUpdateDaemonConfigResponse(response.get());
+
+  response = ReadMessageFromOutputPipe();
+  VerifyStartDaemonResponse(response.get());
+
+  // Verify that DaemonController methods were called in the correct sequence.
+  // This detects cases where NativeMessagingHost might call a wrong method
+  // that takes the same parameters and writes out the same response.
+  EXPECT_EQ("GetConfig:GetUsageStatsConsent:Stop:GetState:UpdateConfig:"
+            "SetConfigAndStart:", call_log_);
+}
+
+// Verify that response ID matches request ID.
+TEST_F(NativeMessagingHostTest, Id) {
+  base::DictionaryValue message;
+  message.SetString("type", "hello");
+  WriteMessageToInputPipe(message);
+  message.SetString("id", "42");
+  WriteMessageToInputPipe(message);
+
+  Run();
+
+  scoped_ptr<base::DictionaryValue> response =
+      ReadMessageFromOutputPipe();
+  EXPECT_TRUE(response);
+  std::string value;
+  EXPECT_FALSE(response->GetString("id", &value));
+
+  response = ReadMessageFromOutputPipe();
+  EXPECT_TRUE(response);
+  EXPECT_TRUE(response->GetString("id", &value));
+  EXPECT_EQ("42", value);
+}
+
+// Verify non-Dictionary requests are rejected.
+TEST_F(NativeMessagingHostTest, WrongFormat) {
+  base::ListValue message;
+  TestBadRequest(message);
+}
+
+// Verify requests with no type are rejected.
+TEST_F(NativeMessagingHostTest, MissingType) {
+  base::DictionaryValue message;
+  TestBadRequest(message);
+}
+
+// Verify rejection if type is unrecognized.
+TEST_F(NativeMessagingHostTest, InvalidType) {
+  base::DictionaryValue message;
+  message.SetString("type", "xxx");
+  TestBadRequest(message);
+}
+
+// Verify rejection if getPinHash request has no hostId.
+TEST_F(NativeMessagingHostTest, GetPinHashNoHostId) {
+  base::DictionaryValue message;
+  message.SetString("type", "getPinHash");
+  message.SetString("pin", "1234");
+  TestBadRequest(message);
+}
+
+// Verify rejection if getPinHash request has no pin.
+TEST_F(NativeMessagingHostTest, GetPinHashNoPin) {
+  base::DictionaryValue message;
+  message.SetString("type", "getPinHash");
+  message.SetString("hostId", "my_host");
+  TestBadRequest(message);
+}
+
+// Verify rejection if updateDaemonConfig request has invalid config.
+TEST_F(NativeMessagingHostTest, UpdateDaemonConfigInvalidConfig) {
+  base::DictionaryValue message;
+  message.SetString("type", "updateDaemonConfig");
+  message.SetString("config", "xxx");
+  TestBadRequest(message);
+}
+
+// Verify rejection if startDaemon request has invalid config.
+TEST_F(NativeMessagingHostTest, StartDaemonInvalidConfig) {
+  base::DictionaryValue message;
+  message.SetString("type", "startDaemon");
+  message.SetString("config", "xxx");
+  message.SetBoolean("consent", true);
+  TestBadRequest(message);
+}
+
+// Verify rejection if startDaemon request has no "consent" parameter.
+TEST_F(NativeMessagingHostTest, StartDaemonNoConsent) {
+  base::DictionaryValue message;
+  message.SetString("type", "startDaemon");
+  message.Set("config", base::DictionaryValue().DeepCopy());
+  TestBadRequest(message);
+}
+
+}  // namespace remoting
diff --git a/remoting/host/setup/native_messaging_reader.cc b/remoting/host/setup/native_messaging_reader.cc
index e98ac52..7151653 100644
--- a/remoting/host/setup/native_messaging_reader.cc
+++ b/remoting/host/setup/native_messaging_reader.cc
@@ -73,8 +73,7 @@ NativeMessagingReader::Core::Core(
       read_task_runner_(read_task_runner) {
 }
 
-NativeMessagingReader::Core::~Core() {
-}
+NativeMessagingReader::Core::~Core() {}
 
 void NativeMessagingReader::Core::ReadMessage() {
   DCHECK(read_task_runner_->RunsTasksOnCurrentThread());
@@ -85,8 +84,11 @@ void NativeMessagingReader::Core::ReadMessage() {
     int read_result = read_stream_.ReadUntilComplete(
         reinterpret_cast<char*>(&message_length), kMessageHeaderSize);
     if (read_result != kMessageHeaderSize) {
-      LOG(ERROR) << "Failed to read message header, read returned "
-                 << read_result;
+      // 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;
+      }
       NotifyEof();
       return;
     }
@@ -98,8 +100,8 @@ void NativeMessagingReader::Core::ReadMessage() {
     }
 
     std::string message_json(message_length, '\0');
-    read_result = read_stream_.ReadUntilComplete(
-        string_as_array(&message_json), message_length);
+    read_result = read_stream_.ReadUntilComplete(string_as_array(&message_json),
+                                                 message_length);
     if (read_result != static_cast<int>(message_length)) {
       LOG(ERROR) << "Failed to read message body, read returned "
                  << read_result;
@@ -115,16 +117,17 @@ void NativeMessagingReader::Core::ReadMessage() {
     }
 
     // Notify callback of new message.
-    caller_task_runner_->PostTask(FROM_HERE, base::Bind(
-        &NativeMessagingReader::InvokeMessageCallback, reader_,
-        base::Passed(&message)));
+    caller_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&NativeMessagingReader::InvokeMessageCallback,
+                              reader_, base::Passed(&message)));
   }
 }
 
 void NativeMessagingReader::Core::NotifyEof() {
   DCHECK(read_task_runner_->RunsTasksOnCurrentThread());
-  caller_task_runner_->PostTask(FROM_HERE, base::Bind(
-      &NativeMessagingReader::InvokeEofCallback, reader_));
+  caller_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&NativeMessagingReader::InvokeEofCallback, reader_));
 }
 
 NativeMessagingReader::NativeMessagingReader(base::PlatformFile handle)
@@ -147,9 +150,9 @@ void NativeMessagingReader::Start(MessageCallback message_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(
-      &NativeMessagingReader::Core::ReadMessage,
-      base::Unretained(core_.get())));
+  read_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&NativeMessagingReader::Core::ReadMessage,
+                            base::Unretained(core_.get())));
 }
 
 void NativeMessagingReader::InvokeMessageCallback(
diff --git a/remoting/host/setup/native_messaging_reader_unittest.cc b/remoting/host/setup/native_messaging_reader_unittest.cc
new file mode 100644
index 0000000..a33ca3b
--- /dev/null
+++ b/remoting/host/setup/native_messaging_reader_unittest.cc
@@ -0,0 +1,163 @@
+// Copyright 2013 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/setup/native_messaging_reader.h"
+
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/platform_file.h"
+#include "base/run_loop.h"
+#include "base/values.h"
+#include "remoting/host/setup/test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+class NativeMessagingReaderTest : public testing::Test {
+ public:
+  NativeMessagingReaderTest();
+  virtual ~NativeMessagingReaderTest();
+
+  virtual void SetUp() OVERRIDE;
+  virtual void TearDown() OVERRIDE;
+
+  // Starts the reader and runs the MessageLoop to completion.
+  void Run();
+
+  // MessageCallback passed to the Reader. Stores |message| so it can be
+  // verified by tests.
+  void OnMessage(scoped_ptr<base::Value> message);
+
+  // Writes a message (header+body) to the write-end of the pipe.
+  void WriteMessage(std::string message);
+
+  // Writes some data to the write-end of the pipe.
+  void WriteData(const char* data, int length);
+
+ protected:
+  scoped_ptr<NativeMessagingReader> reader_;
+  base::PlatformFile read_handle_;
+  base::PlatformFile write_handle_;
+  scoped_ptr<base::Value> message_;
+
+ private:
+  // MessageLoop declared here, since the NativeMessageReader ctor requires a
+  // MessageLoop to have been created.
+  base::MessageLoop message_loop_;
+  base::RunLoop run_loop_;
+};
+
+NativeMessagingReaderTest::NativeMessagingReaderTest()
+    : message_loop_(base::MessageLoop::TYPE_IO) {
+}
+
+NativeMessagingReaderTest::~NativeMessagingReaderTest() {}
+
+void NativeMessagingReaderTest::SetUp() {
+  ASSERT_TRUE(MakePipe(&read_handle_, &write_handle_));
+  reader_.reset(new NativeMessagingReader(read_handle_));
+}
+
+void NativeMessagingReaderTest::TearDown() {
+  // |read_handle_| is owned by NativeMessagingReader's FileStream, so don't
+  // try to close it here. Also, |write_handle_| gets closed by Run().
+}
+
+void NativeMessagingReaderTest::Run() {
+  // Close the write-end, so the reader doesn't block waiting for more data.
+  base::ClosePlatformFile(write_handle_);
+
+  // base::Unretained is safe since no further tasks can run after
+  // RunLoop::Run() returns.
+  reader_->Start(
+      base::Bind(&NativeMessagingReaderTest::OnMessage, base::Unretained(this)),
+      run_loop_.QuitClosure());
+  run_loop_.Run();
+}
+
+void NativeMessagingReaderTest::OnMessage(scoped_ptr<base::Value> message) {
+  message_ = message.Pass();
+}
+
+void NativeMessagingReaderTest::WriteMessage(std::string message) {
+  uint32 length = message.length();
+  WriteData(reinterpret_cast<char*>(&length), 4);
+  WriteData(message.data(), length);
+}
+
+void NativeMessagingReaderTest::WriteData(const char* data, int length) {
+  int written = base::WritePlatformFileAtCurrentPos(write_handle_, data,
+                                                    length);
+  ASSERT_EQ(length, written);
+}
+
+TEST_F(NativeMessagingReaderTest, GoodMessage) {
+  WriteMessage("{\"foo\": 42}");
+  Run();
+  EXPECT_TRUE(message_);
+  base::DictionaryValue* message_dict;
+  EXPECT_TRUE(message_->GetAsDictionary(&message_dict));
+  int result;
+  EXPECT_TRUE(message_dict->GetInteger("foo", &result));
+  EXPECT_EQ(42, result);
+}
+
+TEST_F(NativeMessagingReaderTest, InvalidLength) {
+  uint32 length = 0xffffffff;
+  WriteData(reinterpret_cast<char*>(&length), 4);
+  Run();
+  EXPECT_FALSE(message_);
+}
+
+TEST_F(NativeMessagingReaderTest, EmptyFile) {
+  Run();
+  EXPECT_FALSE(message_);
+}
+
+TEST_F(NativeMessagingReaderTest, ShortHeader) {
+  // Write only 3 bytes - the message length header is supposed to be 4 bytes.
+  WriteData("xxx", 3);
+  Run();
+  EXPECT_FALSE(message_);
+}
+
+TEST_F(NativeMessagingReaderTest, EmptyBody) {
+  uint32 length = 1;
+  WriteData(reinterpret_cast<char*>(&length), 4);
+  Run();
+  EXPECT_FALSE(message_);
+}
+
+TEST_F(NativeMessagingReaderTest, ShortBody) {
+  uint32 length = 2;
+  WriteData(reinterpret_cast<char*>(&length), 4);
+
+  // Only write 1 byte, where the header indicates there should be 2 bytes.
+  WriteData("x", 1);
+  Run();
+  EXPECT_FALSE(message_);
+}
+
+TEST_F(NativeMessagingReaderTest, InvalidJSON) {
+  std::string text = "{";
+  WriteMessage(text);
+  Run();
+  EXPECT_FALSE(message_);
+}
+
+TEST_F(NativeMessagingReaderTest, SecondMessage) {
+  WriteMessage("{}");
+  WriteMessage("{\"foo\": 42}");
+  Run();
+  EXPECT_TRUE(message_);
+  base::DictionaryValue* message_dict;
+  EXPECT_TRUE(message_->GetAsDictionary(&message_dict));
+  int result;
+  EXPECT_TRUE(message_dict->GetInteger("foo", &result));
+  EXPECT_EQ(42, result);
+}
+
+}  // namespace remoting
diff --git a/remoting/host/setup/native_messaging_writer_unittest.cc b/remoting/host/setup/native_messaging_writer_unittest.cc
new file mode 100644
index 0000000..c0cfbd1
--- /dev/null
+++ b/remoting/host/setup/native_messaging_writer_unittest.cc
@@ -0,0 +1,115 @@
+// Copyright 2013 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/setup/native_messaging_writer.h"
+
+#include "base/basictypes.h"
+#include "base/json/json_reader.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/platform_file.h"
+#include "base/stl_util.h"
+#include "base/values.h"
+#include "remoting/host/setup/test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+class NativeMessagingWriterTest : public testing::Test {
+ public:
+  NativeMessagingWriterTest();
+  virtual ~NativeMessagingWriterTest();
+
+  virtual void SetUp() OVERRIDE;
+  virtual void TearDown() OVERRIDE;
+
+ protected:
+  scoped_ptr<NativeMessagingWriter> writer_;
+  base::PlatformFile read_handle_;
+  base::PlatformFile write_handle_;
+  bool read_handle_open_;
+};
+
+NativeMessagingWriterTest::NativeMessagingWriterTest() {}
+
+NativeMessagingWriterTest::~NativeMessagingWriterTest() {}
+
+void NativeMessagingWriterTest::SetUp() {
+  ASSERT_TRUE(MakePipe(&read_handle_, &write_handle_));
+  writer_.reset(new NativeMessagingWriter(write_handle_));
+  read_handle_open_ = true;
+}
+
+void NativeMessagingWriterTest::TearDown() {
+  // |write_handle_| is owned by NativeMessagingWriter's FileStream, so don't
+  // try to close it here. And close |read_handle_| only if it hasn't
+  // already been closed.
+  if (read_handle_open_)
+    base::ClosePlatformFile(read_handle_);
+}
+
+TEST_F(NativeMessagingWriterTest, GoodMessage) {
+  base::DictionaryValue message;
+  message.SetInteger("foo", 42);
+  EXPECT_TRUE(writer_->WriteMessage(message));
+
+  // Read from the pipe and verify the content.
+  uint32 length;
+  int read = base::ReadPlatformFileAtCurrentPos(
+      read_handle_, reinterpret_cast<char*>(&length), 4);
+  EXPECT_EQ(4, read);
+  std::string content(length, '\0');
+  read = base::ReadPlatformFileAtCurrentPos(read_handle_,
+                                            string_as_array(&content), length);
+  EXPECT_EQ(static_cast<int>(length), read);
+
+  // |content| should now contain serialized |message|.
+  scoped_ptr<base::Value> written_message(base::JSONReader::Read(content));
+  EXPECT_TRUE(message.Equals(written_message.get()));
+
+  // Nothing more should have been written. Close the write-end of the pipe,
+  // and verify the read end immediately hits EOF.
+  writer_.reset(NULL);
+  char unused;
+  read = base::ReadPlatformFileAtCurrentPos(read_handle_, &unused, 1);
+  EXPECT_LE(read, 0);
+}
+
+TEST_F(NativeMessagingWriterTest, SecondMessage) {
+  base::DictionaryValue message1;
+  base::DictionaryValue message2;
+  message2.SetInteger("foo", 42);
+  EXPECT_TRUE(writer_->WriteMessage(message1));
+  EXPECT_TRUE(writer_->WriteMessage(message2));
+  writer_.reset(NULL);
+
+  // Read two messages.
+  uint32 length;
+  int read;
+  std::string content;
+  for (int i = 0; i < 2; i++) {
+    read = base::ReadPlatformFileAtCurrentPos(
+        read_handle_, reinterpret_cast<char*>(&length), 4);
+    EXPECT_EQ(4, read) << "i = " << i;
+    content.resize(length);
+    read = base::ReadPlatformFileAtCurrentPos(read_handle_,
+                                              string_as_array(&content),
+                                              length);
+    EXPECT_EQ(static_cast<int>(length), read) << "i = " << i;
+  }
+
+  // |content| should now contain serialized |message2|.
+  scoped_ptr<base::Value> written_message2(base::JSONReader::Read(content));
+  EXPECT_TRUE(message2.Equals(written_message2.get()));
+}
+
+TEST_F(NativeMessagingWriterTest, FailedWrite) {
+  // Close the read end so that writing fails immediately.
+  base::ClosePlatformFile(read_handle_);
+  read_handle_open_ = false;
+
+  base::DictionaryValue message;
+  EXPECT_FALSE(writer_->WriteMessage(message));
+}
+
+}  // namespace remoting
diff --git a/remoting/host/setup/test_util.cc b/remoting/host/setup/test_util.cc
new file mode 100644
index 0000000..9266c74
--- /dev/null
+++ b/remoting/host/setup/test_util.cc
@@ -0,0 +1,32 @@
+// Copyright 2013 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/setup/test_util.h"
+
+#if defined(OS_WIN)
+#include <windows.h>
+#elif defined(OS_POSIX)
+#include <unistd.h>
+#endif
+
+namespace remoting {
+
+bool MakePipe(base::PlatformFile* read_handle,
+              base::PlatformFile* write_handle) {
+#if defined(OS_WIN)
+  return CreatePipe(read_handle, write_handle, NULL, 0) != FALSE;
+#elif defined(OS_POSIX)
+  int fds[2];
+  if (pipe(fds) == 0) {
+    *read_handle = fds[0];
+    *write_handle = fds[1];
+    return true;
+  }
+  return false;
+#else
+#error Not implemented
+#endif
+}
+
+}  // namepsace remoting
diff --git a/remoting/host/setup/test_util.h b/remoting/host/setup/test_util.h
new file mode 100644
index 0000000..77a46ab
--- /dev/null
+++ b/remoting/host/setup/test_util.h
@@ -0,0 +1,20 @@
+// Copyright 2013 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.
+
+#ifndef REMOTING_HOST_SETUP_TEST_UTIL_H_
+#define REMOTING_HOST_SETUP_TEST_UTIL_H_
+
+#include "base/platform_file.h"
+
+namespace remoting {
+
+// Creates an anonymous, unidirectional pipe, returning true if successful. On
+// success, the caller is responsible for closing both ends using
+// base::ClosePlatformFile().
+bool MakePipe(base::PlatformFile* read_handle,
+              base::PlatformFile* write_handle);
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_SETUP_TEST_UTIL_H_
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index f99e399..7dfc95d 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -612,6 +612,9 @@
             '../google_apis/google_apis.gyp:google_apis',
             'remoting_host',
           ],
+          'defines': [
+            'VERSION=<(version_full)',
+          ],
           'sources': [
             'host/setup/daemon_controller.h',
             'host/setup/daemon_controller_linux.cc',
@@ -621,10 +624,18 @@
             'host/setup/daemon_installer_win.h',
             'host/setup/host_starter.cc',
             'host/setup/host_starter.h',
+            'host/setup/native_messaging_host.cc',
+            'host/setup/native_messaging_host.h',
+            'host/setup/native_messaging_reader.cc',
+            'host/setup/native_messaging_reader.h',
+            'host/setup/native_messaging_writer.cc',
+            'host/setup/native_messaging_writer.h',
             'host/setup/oauth_helper.cc',
             'host/setup/oauth_helper.h',
             'host/setup/pin_validator.cc',
             'host/setup/pin_validator.h',
+            'host/setup/test_util.cc',
+            'host/setup/test_util.h',
             'host/setup/win/auth_code_getter.cc',
             'host/setup/win/auth_code_getter.h',
           ],
@@ -735,13 +746,7 @@
             'VERSION=<(version_full)',
           ],
           'sources': [
-            'host/setup/native_messaging_host.cc',
-            'host/setup/native_messaging_host.h',
             'host/setup/native_messaging_host_main.cc',
-            'host/setup/native_messaging_reader.cc',
-            'host/setup/native_messaging_reader.h',
-            'host/setup/native_messaging_writer.cc',
-            'host/setup/native_messaging_writer.h',
           ],
           'conditions': [
             ['OS=="linux" and linux_use_tcmalloc==1', {
@@ -2672,6 +2677,9 @@
         'host/resizing_host_observer_unittest.cc',
         'host/screen_resolution_unittest.cc',
         'host/server_log_entry_unittest.cc',
+        'host/setup/native_messaging_host_unittest.cc',
+        'host/setup/native_messaging_reader_unittest.cc',
+        'host/setup/native_messaging_writer_unittest.cc',
         'host/setup/oauth_helper_unittest.cc',
         'host/setup/pin_validator_unittest.cc',
         'host/token_validator_factory_impl_unittest.cc',