Fix process output watcher not to exit when it sees \0

BUG=chromium-os:28831
TEST=unittest:*OutputWatcher*; browser_tests: *TerminalTest*, *ProcessProxy*

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

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

2 files changed, 31 insertions, 35 deletions

diff --git a/chrome/browser/chromeos/process_proxy/process_output_watcher.cc b/chrome/browser/chromeos/process_proxy/process_output_watcher.cc
index fc431d2..559b183 100644
--- a/chrome/browser/chromeos/process_proxy/process_output_watcher.cc
+++ b/chrome/browser/chromeos/process_proxy/process_output_watcher.cc
@@ -95,18 +95,13 @@ void ProcessOutputWatcher::ReadFromFd(ProcessOutputType type, int* fd) {
   if (bytes_read < 0)
     DPLOG(WARNING) << "read from buffer failed";
 
-  // TODO(tbarzic): We can probably avoid extra string creation.
   if (bytes_read > 0) {
-    // Ensure the string in buffer is NULL terminated. |read_buffer_size_| is
-    // is set to be smaller than real buffer capacity, so |read_buffer_| won't
-    // get overflown.
-    read_buffer_[bytes_read] = 0;
-    on_read_callback_.Run(type, std::string(read_buffer_));
+    on_read_callback_.Run(type, std::string(read_buffer_, bytes_read));
   }
-  // If there is nothing on the output or received data contains \0, we assume
-  // the watched process has exited (slave end of pty is closed).
-  if (bytes_read <= 0 ||
-      static_cast<size_t>(bytes_read) > strlen(read_buffer_)) {
+
+  // If there is nothing on the output the watched process has exited (slave end
+  // of pty is closed).
+  if (bytes_read <= 0) {
     // Slave pseudo terminal has been closed, we won't need master fd anymore.
     CloseFd(fd);
 
diff --git a/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc b/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc
index 30daf0b..778e905 100644
--- a/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc
+++ b/chrome/browser/chromeos/process_proxy/process_output_watcher_unittest.cc
@@ -21,7 +21,13 @@ struct TestCase {
   std::string str;
   ProcessOutputType type;
 
-  TestCase(std::string expected_string, ProcessOutputType expected_type)
+  TestCase(const char* expected_string,
+           size_t expected_string_length,
+           ProcessOutputType expected_type)
+      : str(expected_string, expected_string_length),
+        type(expected_type) {
+  }
+  TestCase(const std::string& expected_string, ProcessOutputType expected_type)
       : str(expected_string),
         type(expected_type) {
   }
@@ -33,44 +39,36 @@ class ProcessWatcherExpectations {
 
   void Init(const std::vector<TestCase>& expectations) {
     received_from_out_ = 0;
-    received_from_err_ = 0;
 
     for (size_t i = 0; i < expectations.size(); i++) {
-      if (expectations[i].type == PROCESS_OUTPUT_TYPE_OUT) {
-        out_expectations_.append(expectations[i].str);
-      } else {
-        err_expectations_.append(expectations[i].str);
-      }
+      out_expectations_.append(expectations[i].str.c_str(),
+                               expectations[i].str.length());
     }
   }
 
-  void CheckExpectations(const std::string& data, ProcessOutputType type) {
-    const std::string& expectations =
-        (type == PROCESS_OUTPUT_TYPE_OUT) ? out_expectations_
-                                          : err_expectations_;
-    size_t& received =
-        (type == PROCESS_OUTPUT_TYPE_OUT) ? received_from_out_
-                                          : received_from_err_;
+  bool CheckExpectations(const std::string& data, ProcessOutputType type) {
+    EXPECT_EQ(PROCESS_OUTPUT_TYPE_OUT, type);
+    if (!type == PROCESS_OUTPUT_TYPE_OUT)
+      return false;
 
-    EXPECT_LT(received, expectations.length());
-    if (received >= expectations.length())
-      return;
+    EXPECT_LT(received_from_out_, out_expectations_.length());
+    if (received_from_out_ >= out_expectations_.length())
+      return false;
 
-    EXPECT_EQ(received, expectations.find(data, received));
+    EXPECT_EQ(received_from_out_,
+              out_expectations_.find(data, received_from_out_));
 
-    received += data.length();
+    received_from_out_ += data.length();
+    return true;
   }
 
   bool IsDone() {
-    return received_from_out_ >= out_expectations_.length() &&
-           received_from_err_ >= err_expectations_.length();
+    return received_from_out_ >= out_expectations_.length();
   }
 
  private:
   std::string out_expectations_;
   size_t received_from_out_;
-  std::string err_expectations_;
-  size_t received_from_err_;
 };
 
 class ProcessOutputWatcherTest : public testing::Test {
@@ -86,8 +84,8 @@ public:
   }
 
   void OnRead(ProcessOutputType type, const std::string& output) {
-    expectations_.CheckExpectations(output, type);
-    if (expectations_.IsDone())
+    bool success = expectations_.CheckExpectations(output, type);
+    if (!success || expectations_.IsDone())
       all_data_received_->Signal();
   }
 
@@ -127,6 +125,9 @@ TEST_F(ProcessOutputWatcherTest, OutputWatcher) {
   test_cases.push_back(TestCase("testing output3\n", PROCESS_OUTPUT_TYPE_OUT));
   test_cases.push_back(TestCase(VeryLongString(), PROCESS_OUTPUT_TYPE_OUT));
   test_cases.push_back(TestCase("testing error2\n", PROCESS_OUTPUT_TYPE_OUT));
+  test_cases.push_back(TestCase("line with \0 in it\n",
+                                arraysize("line with \0 in it \n"),
+                                PROCESS_OUTPUT_TYPE_OUT));
 
   output_watch_thread.message_loop()->PostTask(FROM_HERE,
       base::Bind(&ProcessOutputWatcherTest::StartWatch, base::Unretained(this),