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

net/base/file_stream_context_win.cc

 // Copyright (c) 2012 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 "net/base/file_stream_context.h"
 
 #include <windows.h>
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
@@ skipping 132 matching lines @@
     DWORD bytes_read,
     DWORD error) {
   DCHECK_EQ(&io_context_, context);
   DCHECK(!callback_.is_null());
   DCHECK(async_in_progress_);
 
   if (!async_read_initiated_)
     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;
   }
 
   if (error == ERROR_HANDLE_EOF) {
     result_ = 0;
   } else if (error) {
     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);
   }
 
   if (async_read_initiated_)
     io_complete_for_read_received_ = true;
 
   InvokeUserCallback();
 }
 
 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.
   if (async_read_initiated_) {
     if (!io_complete_for_read_received_ || !async_read_completed_)
       return;
     async_read_initiated_ = false;
     io_complete_for_read_received_ = false;
     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
 void FileStream::Context::ReadAsync(
     FileStream::Context* context,
     HANDLE file,
     scoped_refptr<net::IOBuffer> buf,
     int buf_len,
     OVERLAPPED* overlapped,
     scoped_refptr<base::MessageLoopProxy> origin_thread_loop) {
   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)
+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.
     OnIOCompleted(&io_context_, 0, error.os_error);
   } else if (error.os_error != ERROR_IO_PENDING) {
     // We don't need to inform the caller about ERROR_PENDING_IO as that was
     // already done when the ReadFile call was queued to the worker pool.
     if (error.os_error) {
       LOG(WARNING) << "ReadFile failed: " << error.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