Improve process output watcher's handling of multi-byte UTF8 characters

chromeos/process_proxy/process_output_watcher_unittest.cc

Index: chromeos/process_proxy/process_output_watcher_unittest.cc
diff --git a/chromeos/process_proxy/process_output_watcher_unittest.cc b/chromeos/process_proxy/process_output_watcher_unittest.cc
index 5eeaf1a662f271606ef6f32a85d3518e548d7843..8e4c166516edf6f00052feb5890e7b9cb81a3bc1 100644
--- a/chromeos/process_proxy/process_output_watcher_unittest.cc
+++ b/chromeos/process_proxy/process_output_watcher_unittest.cc
@@ -8,40 +8,48 @@
 #include <string>
 #include <vector>
 
-#include <sys/wait.h>
-
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/file_util.h"
+#include "base/message_loop/message_loop.h"
 #include "base/posix/eintr_wrapper.h"
-#include "base/synchronization/waitable_event.h"
+#include "base/run_loop.h"
+#include "base/strings/string_util.h"
 #include "base/threading/thread.h"
 #include "chromeos/process_proxy/process_output_watcher.h"
 
 namespace chromeos {
 
 struct TestCase {
-  std::string str;
+  TestCase(const std::string& input, bool send_terminating_null)
+      : input(input),
+        should_send_terminating_null(send_terminating_null),
+        expected_output(input) {}
+
+  // Conctructor for cases where the output is not expected to be the same as
+  // input.
+  TestCase(const std::string& input,
+           bool send_terminating_null,
+           const std::string& expected_output)
+      : input(input),
+        should_send_terminating_null(send_terminating_null),
+        expected_output(expected_output) {}
+
+  std::string input;
   bool should_send_terminating_null;
-
-  TestCase(const std::string& expected_string,
-           bool send_terminating_null)
-      : str(expected_string),
-        should_send_terminating_null(send_terminating_null) {
-  }
+  std::string expected_output;
 };
 
 class ProcessWatcherExpectations {
  public:
   ProcessWatcherExpectations() {}
 
-  void Init(const std::vector<TestCase>& expectations) {
+  void SetTestCase(const TestCase& test_case) {
     received_from_out_ = 0;
 
-    for (size_t i = 0; i < expectations.size(); i++) {
-      out_expectations_.append(expectations[i].str);
-      if (expectations[i].should_send_terminating_null)
-        out_expectations_.append(std::string("", 1));
-    }
+    out_expectations_ = test_case.expected_output;
+    if (test_case.should_send_terminating_null)
+      out_expectations_.append(std::string("", 1));
   }
 
   bool CheckExpectations(const std::string& data, ProcessOutputType type) {
@@ -49,12 +57,17 @@ class ProcessWatcherExpectations {
     if (type != PROCESS_OUTPUT_TYPE_OUT)
       return false;
 
+    if (out_expectations_.length() == 0 && data.length() == 0)
+      return true;
+
     EXPECT_LT(received_from_out_, out_expectations_.length());
     if (received_from_out_ >= out_expectations_.length())
       return false;
 
     EXPECT_EQ(received_from_out_,
               out_expectations_.find(data, received_from_out_));
+    if (received_from_out_ != out_expectations_.find(data, received_from_out_))
+      return false;
 
     received_from_out_ += data.length();
     return true;
@@ -70,11 +83,19 @@ class ProcessWatcherExpectations {
 };
 
 class ProcessOutputWatcherTest : public testing::Test {
-public:
-  void StartWatch(int pt, int stop,
-                  const std::vector<TestCase>& expectations) {
-    expectations_.Init(expectations);
+ public:
+  ProcessOutputWatcherTest() : output_watch_thread_started_(false),
+                               failed_(false) {
+  }
+
+  virtual ~ProcessOutputWatcherTest() {}
 
+  virtual void TearDown() OVERRIDE {
+    if (output_watch_thread_started_)
+      output_watch_thread_->Stop();
+  }
+
+  void StartWatch(int pt, int stop) {
     // This will delete itself.
     ProcessOutputWatcher* crosh_watcher = new ProcessOutputWatcher(pt, stop,
         base::Bind(&ProcessOutputWatcherTest::OnRead, base::Unretained(this)));
@@ -82,9 +103,13 @@ public:
   }
 
   void OnRead(ProcessOutputType type, const std::string& output) {
-    bool success = expectations_.CheckExpectations(output, type);
-    if (!success || expectations_.IsDone())
-      all_data_received_->Signal();
+    ASSERT_FALSE(failed_);
+    failed_ = !expectations_.CheckExpectations(output, type);
+    if (failed_ || expectations_.IsDone()) {
+      ASSERT_FALSE(test_case_done_callback_.is_null());
+      message_loop_.PostTask(FROM_HERE, test_case_done_callback_);
+      test_case_done_callback_.Reset();
+    }
   }
 
  protected:
@@ -96,22 +121,30 @@ public:
   }
 
   void RunTest(const std::vector<TestCase>& test_cases) {
-    all_data_received_.reset(new base::WaitableEvent(true, false));
-
-    base::Thread output_watch_thread("ProcessOutpuWatchThread");
-    ASSERT_TRUE(output_watch_thread.Start());
+    ASSERT_FALSE(output_watch_thread_started_);
+    output_watch_thread_.reset(new base::Thread("ProcessOutpuWatchThread"));
+    output_watch_thread_started_ = output_watch_thread_->Start();
+    ASSERT_TRUE(output_watch_thread_started_);
 
     int pt_pipe[2], stop_pipe[2];
     ASSERT_FALSE(HANDLE_EINTR(pipe(pt_pipe)));
     ASSERT_FALSE(HANDLE_EINTR(pipe(stop_pipe)));
 
-    output_watch_thread.message_loop()->PostTask(FROM_HERE,
+    output_watch_thread_->message_loop()->PostTask(
+        FROM_HERE,
         base::Bind(&ProcessOutputWatcherTest::StartWatch,
                    base::Unretained(this),
-                   pt_pipe[0], stop_pipe[0], test_cases));
+                   pt_pipe[0],
+                   stop_pipe[0]));
 
     for (size_t i = 0; i < test_cases.size(); i++) {
-      const std::string& test_str = test_cases[i].str;
+      expectations_.SetTestCase(test_cases[i]);
+
+      base::RunLoop run_loop;
+      ASSERT_TRUE(test_case_done_callback_.is_null());
+      test_case_done_callback_ = run_loop.QuitClosure();
+
+      const std::string& test_str = test_cases[i].input;
       // Let's make inputs not NULL terminated, unless other is specified in
       // the test case.
       ssize_t test_size = test_str.length() * sizeof(*test_str.c_str());
@@ -120,22 +153,26 @@ public:
       EXPECT_EQ(test_size,
                 base::WriteFileDescriptor(pt_pipe[1], test_str.c_str(),
                                           test_size));
-    }
 
-    all_data_received_->Wait();
+      run_loop.Run();
+      EXPECT_TRUE(expectations_.IsDone());
+      if (failed_)
+        break;
+    }
 
     // Send stop signal. It is not important which string we send.
     EXPECT_EQ(1, base::WriteFileDescriptor(stop_pipe[1], "q", 1));
 
     EXPECT_NE(-1, IGNORE_EINTR(close(stop_pipe[1])));
     EXPECT_NE(-1, IGNORE_EINTR(close(pt_pipe[1])));
-
-    output_watch_thread.Stop();
   }
 
-  scoped_ptr<base::WaitableEvent> all_data_received_;
-
  private:
+  base::Closure test_case_done_callback_;
+  base::MessageLoop message_loop_;
+  scoped_ptr<base::Thread> output_watch_thread_;
+  bool output_watch_thread_started_;
+  bool failed_;
   ProcessWatcherExpectations expectations_;
   std::vector<TestCase> exp;
 };
@@ -143,6 +180,7 @@ public:
 
 TEST_F(ProcessOutputWatcherTest, OutputWatcher) {
   std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("t", false));
   test_cases.push_back(TestCase("testing output\n", false));
   test_cases.push_back(TestCase("testing error\n", false));
   test_cases.push_back(TestCase("testing error1\n", false));
@@ -155,11 +193,112 @@ TEST_F(ProcessOutputWatcherTest, OutputWatcher) {
   RunTest(test_cases);
 };
 
+TEST_F(ProcessOutputWatcherTest, SplitUTF8Character) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("test1\xc2", false, "test1"));
+  test_cases.push_back(TestCase("\xb5test1", false, "\xc2\xb5test1"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, SplitSoleUTF8Character) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("\xc2", false, ""));
+  test_cases.push_back(TestCase("\xb5", false, "\xc2\xb5"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, SplitUTF8CharacterLength3) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("test3\xe2\x82", false, "test3"));
+  test_cases.push_back(TestCase("\xac", false, "\xe2\x82\xac"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, SplitSoleUTF8CharacterThreeWays) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("\xe2", false, ""));
+  test_cases.push_back(TestCase("\x82", false, ""));
+  test_cases.push_back(TestCase("\xac", false, "\xe2\x82\xac"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, EndsWithThreeByteUTF8Character) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("test\xe2\x82\xac", false, "test\xe2\x82\xac"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, SoleThreeByteUTF8Character) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("\xe2\x82\xac", false, "\xe2\x82\xac"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, HasThreeByteUTF8Character) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(
+      TestCase("test\xe2\x82\xac_", false, "test\xe2\x82\xac_"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, MulitByteUTF8CharNullTerminated) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("test\xe2\x82\xac", true, "test\xe2\x82\xac"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, MultipleMultiByteUTF8Characters) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(
+      TestCase("test\xe2\x82\xac\xc2", false, "test\xe2\x82\xac"));
+  test_cases.push_back(TestCase("\xb5", false, "\xc2\xb5"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, ContainsInvalidUTF8) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("\xc2_", false, "\xc2_"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, InvalidUTF8SeriesOfTrailingBytes) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("\x82\x82\x82", false, "\x82\x82\x82"));
+  test_cases.push_back(TestCase("\x82\x82\x82", false, "\x82\x82\x82"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, EndsWithInvalidUTF8) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("\xff", false, "\xff"));
+
+  RunTest(test_cases);
+}
+
+TEST_F(ProcessOutputWatcherTest, FourByteUTF8) {
+  std::vector<TestCase> test_cases;
+  test_cases.push_back(TestCase("\xf0\xa4\xad", false, ""));
+  test_cases.push_back(TestCase("\xa2", false, "\xf0\xa4\xad\xa2"));
+
+  RunTest(test_cases);
+};
+
 // Verifies that sending '\0' generates PROCESS_OUTPUT_TYPE_OUT event and does
 // not terminate output watcher.
 TEST_F(ProcessOutputWatcherTest, SendNull) {
   std::vector<TestCase> test_cases;
-  // This will send '\0' to output wathcer.
+  // This will send '\0' to output watcher.
   test_cases.push_back(TestCase("", true));
   // Let's verify that next input also gets detected (i.e. output watcher does
   // not exit after seeing '\0' from previous test case).