NetworkService: Notify SocketObserver of a graceful close.

Previously, there was no way for a consumer to distinguish a graceful
socket close from the data pipe being closed for something other than a
network error (Network Service crash, NetworkContext shutdown, etc).

Bug: 878136
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ibf0f1bbe0603a5db2d40945b12d59f243e7e34e9
Reviewed-on: https://chromium-review.googlesource.com/1196964Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587842}

services/network/public/mojom/tcp_socket.mojom

@@ -60,12 +60,12 @@ interface TCPConnectedSocket {
 // error, if a network error happens during a read or a write, consumer can find
 // out about it by implementing this interface.
 interface SocketObserver {
-  // Called when an error occurs during reading from the network. The producer
-  // side of |receive_stream| will be closed.
+  // Called when a network read fails. Called with net::OK if the socket was
+  // closed gracefully. The producer side of |receive_stream| will be closed.
   OnReadError(int32 net_error);
 
-  // Called when an error occurs during sending to the network. The consumer
-  // side of |send_stream| will be closed.
+  // Called when a network write fails. The consumer side of |send_stream| will
+  // be closed.
   OnWriteError(int32 net_error);
 };
 

services/network/socket_data_pump.cc

@@ -89,8 +89,13 @@ void SocketDataPump::ReceiveMore() {
     receive_stream_close_watcher_.ArmOrNotify();
     return;
   }
-  // Handle EOF.
+  // Handle EOF. Has to be done here rather than in
+  // OnNetworkReadIfReadyCompleted because net::OK in the sync completion case
+  // means EOF, but in the async case just means the socket is ready to be read
+  // from again.
   if (read_result == net::OK) {
+    if (delegate_)
+      delegate_->OnNetworkReadError(read_result);
     ShutdownReceive();
     return;
   }
@@ -113,16 +118,20 @@ void SocketDataPump::OnReceiveStreamWritable(MojoResult result) {
 }
 
 void SocketDataPump::OnNetworkReadIfReadyCompleted(int result) {
+  // This method is called either on ReadIfReady sync completion, except in the
+  // EOF case, or on async completion. In the sync case, result is < 0 on error,
+  // or > 0 on success. In the async case, result is < 0 on error, or 0 if we
+  // should try to read from the socket again (And possibly get any of more
+  // data, an EOF, or an error).
   DCHECK(receive_stream_.is_valid());
   if (read_if_ready_pending_) {
     DCHECK_GE(net::OK, result);
     read_if_ready_pending_ = false;
   }
 
-  if (result < 0 && delegate_)
-    delegate_->OnNetworkReadError(result);
-
   if (result < 0) {
+    if (delegate_)
+      delegate_->OnNetworkReadError(result);
     ShutdownReceive();
     return;
   }
@@ -188,15 +197,14 @@ void SocketDataPump::OnNetworkWriteCompleted(int result) {
   DCHECK(pending_send_buffer_);
   DCHECK(!send_stream_.is_valid());
 
-  if (result < 0 && delegate_)
-    delegate_->OnNetworkWriteError(result);
-
   // Partial write is possible.
   pending_send_buffer_->CompleteRead(result >= 0 ? result : 0);
   send_stream_ = pending_send_buffer_->ReleaseHandle();
   pending_send_buffer_ = nullptr;
 
   if (result <= 0) {
+    if (delegate_)
+      delegate_->OnNetworkWriteError(result);
     ShutdownSend();
     return;
   }

services/network/socket_data_pump_unittest.cc

@@ -234,6 +234,27 @@ TEST_P(SocketDataPumpTest, PartialStreamSocketWrite) {
   EXPECT_TRUE(data_provider.AllWriteDataConsumed());
 }
 
+TEST_P(SocketDataPumpTest, ReadEof) {
+  net::IoMode mode = GetParam();
+  net::MockRead reads[] = {net::MockRead(mode, net::OK)};
+  const char kTestMsg[] = "hello!";
+  net::MockWrite writes[] = {
+      net::MockWrite(mode, kTestMsg, strlen(kTestMsg), 0)};
+  net::StaticSocketDataProvider data_provider(reads, writes);
+  Init(&data_provider);
+  EXPECT_EQ("", Read(&receive_handle_, 1));
+  EXPECT_EQ(net::OK, delegate()->WaitForReadError());
+  // Writes can proceed even though there is a read error.
+  uint32_t num_bytes = strlen(kTestMsg);
+  EXPECT_EQ(MOJO_RESULT_OK, send_handle_->WriteData(&kTestMsg, &num_bytes,
+                                                    MOJO_WRITE_DATA_FLAG_NONE));
+  EXPECT_EQ(strlen(kTestMsg), num_bytes);
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(data_provider.AllReadDataConsumed());
+  EXPECT_TRUE(data_provider.AllWriteDataConsumed());
+}
+
 TEST_P(SocketDataPumpTest, ReadError) {
   net::IoMode mode = GetParam();
   net::MockRead reads[] = {net::MockRead(mode, net::ERR_FAILED)};
@@ -255,6 +276,27 @@ TEST_P(SocketDataPumpTest, ReadError) {
   EXPECT_TRUE(data_provider.AllWriteDataConsumed());
 }
 
+TEST_P(SocketDataPumpTest, WriteEof) {
+  net::IoMode mode = GetParam();
+  const char kTestMsg[] = "hello!";
+  net::MockRead reads[] = {net::MockRead(mode, kTestMsg, strlen(kTestMsg), 0),
+                           net::MockRead(mode, net::OK)};
+  net::MockWrite writes[] = {net::MockWrite(mode, net::OK)};
+  net::StaticSocketDataProvider data_provider(reads, writes);
+  Init(&data_provider);
+  uint32_t num_bytes = strlen(kTestMsg);
+  EXPECT_EQ(MOJO_RESULT_OK, send_handle_->WriteData(&kTestMsg, &num_bytes,
+                                                    MOJO_WRITE_DATA_FLAG_NONE));
+  EXPECT_EQ(strlen(kTestMsg), num_bytes);
+  EXPECT_EQ(net::OK, delegate()->WaitForWriteError());
+  // Reads can proceed even though there is a read error.
+  EXPECT_EQ(kTestMsg, Read(&receive_handle_, strlen(kTestMsg)));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(data_provider.AllReadDataConsumed());
+  EXPECT_TRUE(data_provider.AllWriteDataConsumed());
+}
+
 TEST_P(SocketDataPumpTest, WriteError) {
   net::IoMode mode = GetParam();
   const char kTestMsg[] = "hello!";