Fix a crash in the FileStream::Context::Read code path where we were invoking a NULL callback.

net/base/file_stream_context_win.cc

Index: net/base/file_stream_context_win.cc
diff --git a/net/base/file_stream_context_win.cc b/net/base/file_stream_context_win.cc
index 5ffbde72388293a339e46430846f4be6df62695c..261637890679f431ee624d10f5d450949a28484b 100644
--- a/net/base/file_stream_context_win.cc
+++ b/net/base/file_stream_context_win.cc
@@ -150,10 +150,12 @@ void FileStream::Context::OnIOCompleted(
     async_in_progress_ = false;
 
   if (orphaned_) {
-    async_in_progress_ = false;
-    callback_.Reset();
-    in_flight_buf_ = NULL;
-    CloseAndDelete();
+    io_complete_for_read_received_ = true;
+    // If we are called due to a pending read and the asynchronous read task
+    // has not completed we have to keep the context around until it completes.
+    if (async_read_initiated_ && !async_read_completed_)
+      return;
+    DeleteOrphanedContext();
     return;
   }
 
@@ -163,6 +165,8 @@ void FileStream::Context::OnIOCompleted(
     IOResult error_result = IOResult::FromOSError(error);
     result_ = static_cast<int>(error_result.result);
   } else {
+    if (result_)
+      DCHECK_EQ(result_, static_cast<int>(bytes_read));
     result_ = bytes_read;
     IncrementOffset(&io_context_.overlapped, bytes_read);
   }
@@ -174,6 +178,8 @@ void FileStream::Context::OnIOCompleted(
 }
 
 void FileStream::Context::InvokeUserCallback() {
+  if (callback_.is_null())

[rvargas (doing something else), 2015/02/13 02:05:57]

Should not need this.

[ananta, 2015/02/13 02:22:14]

This might be needed if we ever receive an io completion notification for a sync
read file. I know that this should never happen. But it is good to leave this
here for a safety net.

[rvargas (doing something else), 2015/02/13 02:26:05]

What do you mean for a sync read? We will receive notifications in that case.

I think it is more valuable to crash if we are here without a callback (which
should tell us that something is seriously wrong) than to have a safety net for
future changes.

+    return;
   // For an asynchonous Read operation don't invoke the user callback until
   // we receive the IO completion notification and the asynchronous Read
   // completion notification.
@@ -185,11 +191,20 @@ void FileStream::Context::InvokeUserCallback() {
     async_read_completed_ = false;
     async_in_progress_ = false;
   }
+  int result = result_;

[rvargas (doing something else), 2015/02/13 02:05:57]

This is not needed (I was going to comment before that line 198-99 should not be
needed either -I don't know why that was added but looks wrong-, but that was
unrelated to your change)

[ananta, 2015/02/13 02:22:14]

Done.

+  result_ = 0;
   CompletionCallback temp_callback = callback_;
   callback_.Reset();
   scoped_refptr<IOBuffer> temp_buf = in_flight_buf_;
   in_flight_buf_ = NULL;
-  temp_callback.Run(result_);
+  temp_callback.Run(result);
+}
+
+void FileStream::Context::DeleteOrphanedContext() {
+  async_in_progress_ = false;
+  callback_.Reset();
+  in_flight_buf_ = NULL;
+  CloseAndDelete();
 }
 
 // static
@@ -203,15 +218,28 @@ void FileStream::Context::ReadAsync(
   DWORD bytes_read = 0;
   BOOL ret = ::ReadFile(file, buf->data(), buf_len, &bytes_read, overlapped);
   origin_thread_loop->PostTask(
-      FROM_HERE, base::Bind(&FileStream::Context::ReadAsyncResult,
-                            base::Unretained(context), ret ? bytes_read : 0,
-                            ret ? 0 : ::GetLastError()));
+      FROM_HERE,
+      base::Bind(&FileStream::Context::ReadAsyncResult,
+                 base::Unretained(context), ret, bytes_read, ::GetLastError()));
 }
 
-void FileStream::Context::ReadAsyncResult(DWORD bytes_read, DWORD os_error) {
-  if (!os_error)
-    result_ = bytes_read;
+void FileStream::Context::ReadAsyncResult(BOOL read_file_ret,
+                                          DWORD bytes_read,
+                                          DWORD os_error) {
+  async_read_completed_ = true;

[rvargas (doing something else), 2015/02/13 02:05:57]

nit: I think this reads better if this line goes after the if (right before if
(read_file_ret)

[ananta, 2015/02/13 02:22:14]

Done.

+  // If the context is orphaned and we already received the io completion
+  // notification then we should delete the context and get out.
+  if (orphaned_ && io_complete_for_read_received_) {
+    DeleteOrphanedContext();
+    return;
+  }
 
+  if (read_file_ret) {
+    DCHECK(!os_error);

[rvargas (doing something else), 2015/02/13 02:05:57]

This is technically out of our control. (although expected)

[ananta, 2015/02/13 02:22:14]

ok. Removed

+    result_ = bytes_read;
+    InvokeUserCallback();
+    return;
+  }
   IOResult error = IOResult::FromOSError(os_error);
   if (error.os_error == ERROR_HANDLE_EOF) {
     // Report EOF by returning 0 bytes read.
@@ -224,8 +252,6 @@ void FileStream::Context::ReadAsyncResult(DWORD bytes_read, DWORD os_error) {
       OnIOCompleted(&io_context_, 0, error.os_error);

[rvargas (doing something else), 2015/02/13 02:05:57]

I'd really like to simplify this code. This is the same as line 246 so the only
difference is line 251... I'm not a fan of such logs, but if we are logging, we
should log from OnIOCompleted instead as that covers not only sync but also
async errors. Note that line 250 should be superfluous.

[ananta, 2015/02/13 02:22:14]

I removed the log and removed the multiple calls to OnIOCompleted

     }
   }
-  async_read_completed_ = true;
-  InvokeUserCallback();
 }
 
 }  // namespace net