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 135 matching lines @@
   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();

[rvargas (doing something else), 2015/02/12 23:53:35]

This looks wrong to me. Aren't we deleting the object potentially before
ReadAsyncResult?

We probably have to invert the logic from
complex OnIOCompleted
simple InvokeUserCallback

to
simple OnIOCompleted
complex Foo that does the real work when both OnIOCompleted and ReadAsyncResult
are received.

[ananta, 2015/02/13 00:17:00]

Reworked this a bit. Added a function DeleteOrphanedContext. We check here
whether there is a read pending and we have not received the read completion
signal before calling DeleteOrphanedContext. 

I also added a call to DeleteOrphanedContext in the ReadAsyncResult function
based on the two signals.

     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 {
     result_ = bytes_read;

[rvargas (doing something else), 2015/02/12 23:53:35]

if (result_)
dcheck_eq(result_, bytes_read) ?

[ananta, 2015/02/13 00:17:01]

Done.

     IncrementOffset(&io_context_.overlapped, bytes_read);

[rvargas (doing something else), 2015/02/12 23:53:35]

result_ ?

[ananta, 2015/02/13 00:17:01]

I added result_ = 0 here.

[ananta, 2015/02/13 00:36:38]

That is wrong :(. Removed it.

   }
 
   if (async_read_initiated_)
     io_complete_for_read_received_ = true;
 
   InvokeUserCallback();
 }
 
 void FileStream::Context::InvokeUserCallback() {
+  if (callback_.is_null())
+    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;
   }
   CompletionCallback temp_callback = callback_;
   callback_.Reset();
   scoped_refptr<IOBuffer> temp_buf = in_flight_buf_;
   in_flight_buf_ = NULL;
   temp_callback.Run(result_);
 }
 
 // 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) {
+  if (read_file_ret) {

[rvargas (doing something else), 2015/02/12 23:53:35]

Note that the original logic was:
       DWORD bytes_read;
  73   if (!ReadFile(file_.GetPlatformFile(), buf->data(), buf_len,	
  74                 &bytes_read, &io_context_.overlapped)) {	
  75     IOResult error = IOResult::FromOSError(GetLastError());	
  76     if (error.os_error == ERROR_HANDLE_EOF)	
  77       return 0;  // Report EOF by returning 0 bytes read.	
  78     if (error.os_error == ERROR_IO_PENDING)	
  79       IOCompletionIsPending(callback, buf);	
  80     else	
  81       LOG(WARNING) << "ReadFile failed: " << error.os_error;	
  82     return static_cast<int>(error.result);	
  83   }	
  85   IOCompletionIsPending(callback, buf);

We can avoid calling InvokeUserCallback twice.

if (read_file_ret) there is no need to look at os_error.

How about:

async_read_completed = true;
if (read_file_ret) {
  result_ = br
  InvokeUserCb
} else {
  if (!io_pending)
    OnIoCompleted(0) // and move the warning to line 163
}

[ananta, 2015/02/13 00:17:00]

Did something similar.

+    DCHECK(!os_error);
     result_ = bytes_read;
-
+  }
   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);
     }
   }
   async_read_completed_ = true;
   InvokeUserCallback();
 }
 
 }  // namespace net