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

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

This could happen if if the ReadFileAsync completion calls OnIOComplete for an orphanaed object.

Fix is to rework the way these callbacks work.

1. Handle the case where in an orphaned context could be
   deleted before the read async completion callback is received.

2. Pass the return of the ReadFile call to the ReadAsyncResult callback. The logic to call the IO completion routine or the user callback has been consolidated in this function.

BUG=458253
Committed: https://crrev.com/47f234d399caaa4fe270896ee5393265acf9ed8c
Cr-Commit-Position: refs/heads/master@{#316362}

[ananta, 2015-02-12 20:28:08]

ananta@chromium.org changed reviewers:
+ rvargas@chromium.org

[rvargas (doing something else), 2015-02-12 22:24:54]

https://codereview.chromium.org/920123002/diff/1/net/base/file_stream_context...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/1/net/base/file_stream_context...
net/base/file_stream_context_win.cc:208: FROM_HERE,
base::Bind(&FileStream::Context::ReadAsyncResult,
ret = TRUE + a completion callback is the expected behavior. I believe this
should be changes to explicitly communicate ret to the other thread because we
should not be accessing anything before looking at the return value of the call
(and that includes read_bytes, but also last error).

I should have mentioned that on the last review, but I expected that code to
last only one day in trunk before writing another CL based on the results (it
was a quick test). BTW, I'm seriously inclined to say that the DrMemory report
was bogus, but I don't really mind having to wait for two signals.

[ananta, 2015-02-12 22:47:59]

https://codereview.chromium.org/920123002/diff/1/net/base/file_stream_context...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/1/net/base/file_stream_context...
net/base/file_stream_context_win.cc:208: FROM_HERE,
base::Bind(&FileStream::Context::ReadAsyncResult,
On 2015/02/12 22:24:54, rvargas wrote:
> ret = TRUE + a completion callback is the expected behavior. I believe this
> should be changes to explicitly communicate ret to the other thread because we
> should not be accessing anything before looking at the return value of the
call
> (and that includes read_bytes, but also last error).
> 
> I should have mentioned that on the last review, but I expected that code to
> last only one day in trunk before writing another CL based on the results (it
> was a quick test). BTW, I'm seriously inclined to say that the DrMemory report
> was bogus, but I don't really mind having to wait for two signals.

Done.

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

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:156: CloseAndDelete();
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.

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:166: result_ = bytes_read;
if (result_)
dcheck_eq(result_, bytes_read) ?

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:167:
IncrementOffset(&io_context_.overlapped, bytes_read);
result_ ?

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:216: if (read_file_ret) {
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:01]

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:156: CloseAndDelete();
On 2015/02/12 23:53:35, rvargas wrote:
> 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.

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.

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:166: result_ = bytes_read;
On 2015/02/12 23:53:35, rvargas wrote:
> if (result_)
> dcheck_eq(result_, bytes_read) ?

Done.

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:167:
IncrementOffset(&io_context_.overlapped, bytes_read);
On 2015/02/12 23:53:35, rvargas wrote:
> result_ ?

I added result_ = 0 here.

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:216: if (read_file_ret) {
On 2015/02/12 23:53:35, rvargas wrote:
> 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
> }

Did something similar.

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

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/20001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:167:
IncrementOffset(&io_context_.overlapped, bytes_read);
On 2015/02/13 00:17:01, ananta wrote:
> On 2015/02/12 23:53:35, rvargas wrote:
> > result_ ?
> 
> I added result_ = 0 here.

That is wrong :(. Removed it.

[rvargas (doing something else), 2015-02-13 02:05:58]

Almost there

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:181: if (callback_.is_null())
Should not need this.

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:194: int result = result_;
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)

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:229: async_read_completed_ = true;
nit: I think this reads better if this line goes after the if (right before if
(read_file_ret)

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:238: DCHECK(!os_error);
This is technically out of our control. (although expected)

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:252: OnIOCompleted(&io_context_, 0,
error.os_error);
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]

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:181: if (callback_.is_null())
On 2015/02/13 02:05:57, rvargas wrote:
> Should not need this.

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.

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:194: int result = result_;
On 2015/02/13 02:05:57, rvargas wrote:
> 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)

Done.

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:229: async_read_completed_ = true;
On 2015/02/13 02:05:57, rvargas wrote:
> nit: I think this reads better if this line goes after the if (right before if
> (read_file_ret)

Done.

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:238: DCHECK(!os_error);
On 2015/02/13 02:05:57, rvargas wrote:
> This is technically out of our control. (although expected)

ok. Removed

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:252: OnIOCompleted(&io_context_, 0,
error.os_error);
On 2015/02/13 02:05:57, rvargas wrote:
> 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.

I removed the log and removed the multiple calls to OnIOCompleted

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

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/80001/net/base/file_stream_con...
net/base/file_stream_context_win.cc:181: if (callback_.is_null())
On 2015/02/13 02:22:14, ananta wrote:
> On 2015/02/13 02:05:57, rvargas wrote:
> > Should not need this.
> 
> 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.

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.

[rvargas (doing something else), 2015-02-13 02:38:51]

LGTM

[ananta, 2015-02-13 02:39:58]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-02-13 02:40:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/920123002/180001

[commit-bot: I haz the power, 2015-02-13 04:17:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-13 04:17:47]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[ananta, 2015-02-13 15:29:59]

New patchsets have been uploaded after l-g-t-m from rvargas@chromium.org

[rvargas (doing something else), 2015-02-13 18:22:44]

https://codereview.chromium.org/920123002/diff/200001/net/base/file_stream_co...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/200001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:236: if (read_file_ret ||
io_complete_for_read_received_) {
Sounds like an else to err != pending, no?

Which, btw, would mean inverting the logic of the if (if == pending)

https://codereview.chromium.org/920123002/diff/200001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:243: if (error.os_error != ERROR_IO_PENDING)
{
nit: don't use {} here

[ananta, 2015-02-13 19:35:05]

https://codereview.chromium.org/920123002/diff/200001/net/base/file_stream_co...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/200001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:236: if (read_file_ret ||
io_complete_for_read_received_) {
On 2015/02/13 18:22:44, rvargas wrote:
> Sounds like an else to err != pending, no?
> 
> Which, btw, would mean inverting the logic of the if (if == pending)

Done.

https://codereview.chromium.org/920123002/diff/200001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:243: if (error.os_error != ERROR_IO_PENDING)
{
On 2015/02/13 18:22:44, rvargas wrote:
> nit: don't use {} here

Reworked the if. PTAL

[rvargas (doing something else), 2015-02-13 19:53:06]

https://codereview.chromium.org/920123002/diff/220001/net/base/file_stream_co...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:237: InvokeUserCallback();
if we don't set result_ here, bytes_read becomes unused (and the whole point of
having result_ is lost)

https://codereview.chromium.org/920123002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:243: if (io_complete_for_read_received_)
why do we need to check this? That is the job of InvokeUserCallback().

[ananta, 2015-02-13 23:07:54]

https://codereview.chromium.org/920123002/diff/220001/net/base/file_stream_co...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/920123002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:237: InvokeUserCallback();
On 2015/02/13 19:53:06, rvargas wrote:
> if we don't set result_ here, bytes_read becomes unused (and the whole point
of
> having result_ is lost)

Sorry that was an oversight. I had a patchset waiting for upload and forgot
about it :(

https://codereview.chromium.org/920123002/diff/220001/net/base/file_stream_co...
net/base/file_stream_context_win.cc:243: if (io_complete_for_read_received_)
On 2015/02/13 19:53:06, rvargas wrote:
> why do we need to check this? That is the job of InvokeUserCallback().

Done.

[rvargas (doing something else), 2015-02-13 23:31:22]

lgtm

[ananta, 2015-02-14 00:01:01]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-02-14 00:02:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/920123002/240001

[commit-bot: I haz the power, 2015-02-14 01:03:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-14 01:03:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2015-02-14 01:09:26]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-02-14 01:10:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/920123002/240001

[commit-bot: I haz the power, 2015-02-14 02:11:04]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-14 02:11:50]

Patchset 13 (id:??) landed as
https://crrev.com/47f234d399caaa4fe270896ee5393265acf9ed8c
Cr-Commit-Position: refs/heads/master@{#316362}