Fix net::FileStream to handle POSIX errors correctly.

Fix net::FileStream to handle all errors correctly.

This CL adds FileStream::Context::IOResult struct that's used to pass 
results of IO operations inside FileStream::Context. Following bugs are 
fixes:

1. On POSIX net::FileStream::Read() and net::FileStream::Write() would
   return a positive result in case of an error. This happens because 
   POSIX errors are positive integers and FileStream was written with 
   assumption that they are negative.
2. On Windows Seek() and Flush() errors were not handled correctly.
   This is because CheckForIOError() was assuming that all error codes 
   are negative, but RecordAndMapError() was mapping positive errors.
3. On Windows results of asynchronous ReadFile() and WriteFile() were
   not handled correctly - ERR_IO_PENDING would be returned even when
   operation completes synchronously.

Also added unittests to check that error codes are handled correctly 
now.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184257

[willchan no longer on Chromium, 2013-02-20 22:15:31]

Sorry for the delay, I missed this earlier. Redirecting to mmenke.

[mmenke, 2013-02-21 19:41:05]

On 2013/02/20 22:15:31, willchan wrote:
> Sorry for the delay, I missed this earlier. Redirecting to mmenke.

Relying on the signs of external error codes just seems like a bad idea to me. 
Having some functions return ints-or-chrome-net-errors and others return
int64s-or-negative-system-errors also seems like a bad idea to me.

Seems like we could we just inline the calls to RecordAndMapError in the
relevant functions in *_posix.cc and *_win.cc, and get rid of CheckForIOError.

[mmenke, 2013-02-21 19:50:18]

On 2013/02/21 19:41:05, mmenke wrote:
> On 2013/02/20 22:15:31, willchan wrote:
> > Sorry for the delay, I missed this earlier. Redirecting to mmenke.
> 
> Relying on the signs of external error codes just seems like a bad idea to me.

> Having some functions return ints-or-chrome-net-errors and others return
> int64s-or-negative-system-errors also seems like a bad idea to me.
> 
> Seems like we could we just inline the calls to RecordAndMapError in the
> relevant functions in *_posix.cc and *_win.cc, and get rid of CheckForIOError.

Also, the new tests are failing on windows.

[mmenke, 2013-02-21 19:57:23]

On 2013/02/21 19:41:05, mmenke wrote:
> On 2013/02/20 22:15:31, willchan wrote:
> > Sorry for the delay, I missed this earlier. Redirecting to mmenke.
> 
> Relying on the signs of external error codes just seems like a bad idea to me.

> Having some functions return ints-or-chrome-net-errors and others return
> int64s-or-negative-system-errors also seems like a bad idea to me.
> 
> Seems like we could we just inline the calls to RecordAndMapError in the
> relevant functions in *_posix.cc and *_win.cc, and get rid of CheckForIOError.

Oh, right...There may be threading issues with that.  So we still need to do the
logging on the main thread.  Nevermind, then, though I still don't like the
negative error code approach, can live with it for now.

[Sergey Ulanov, 2013-02-21 22:23:51]

On 2013/02/21 19:57:23, mmenke wrote:
> On 2013/02/21 19:41:05, mmenke wrote:
> > On 2013/02/20 22:15:31, willchan wrote:
> > > Sorry for the delay, I missed this earlier. Redirecting to mmenke.
> > 
> > Relying on the signs of external error codes just seems like a bad idea to
me.
> 
> > Having some functions return ints-or-chrome-net-errors and others return
> > int64s-or-negative-system-errors also seems like a bad idea to me.
> > 
> > Seems like we could we just inline the calls to RecordAndMapError in the
> > relevant functions in *_posix.cc and *_win.cc, and get rid of
CheckForIOError.
> 
> Oh, right...There may be threading issues with that.  So we still need to do
the
> logging on the main thread.  Nevermind, then, though I still don't like the
> negative error code approach, can live with it for now.

Yeah. I'm not original author of this code, but as far as I understand it, it's
not possible to map the error on the blocking thread because the original OS
error code needs to be reported too, and that has to happen on the IO thread.
But I do agree it's very ugly. An alternative is to add some struct like this:
 struct IOResult {
   int64 result;
   int os_error; // Set only when result < 0.
 }

And then use that struct to return results from async IO calls. WDYT?

[mmenke, 2013-02-21 22:45:22]

On 2013/02/21 22:23:51, sergeyu wrote:
> On 2013/02/21 19:57:23, mmenke wrote:
> > On 2013/02/21 19:41:05, mmenke wrote:
> > > On 2013/02/20 22:15:31, willchan wrote:
> > > > Sorry for the delay, I missed this earlier. Redirecting to mmenke.
> > > 
> > > Relying on the signs of external error codes just seems like a bad idea to
> me.
> > 
> > > Having some functions return ints-or-chrome-net-errors and others return
> > > int64s-or-negative-system-errors also seems like a bad idea to me.
> > > 
> > > Seems like we could we just inline the calls to RecordAndMapError in the
> > > relevant functions in *_posix.cc and *_win.cc, and get rid of
> CheckForIOError.
> > 
> > Oh, right...There may be threading issues with that.  So we still need to do
> the
> > logging on the main thread.  Nevermind, then, though I still don't like the
> > negative error code approach, can live with it for now.
> 
> Yeah. I'm not original author of this code, but as far as I understand it,
it's
> not possible to map the error on the blocking thread because the original OS
> error code needs to be reported too, and that has to happen on the IO thread.
> But I do agree it's very ugly. An alternative is to add some struct like this:
>  struct IOResult {
>    int64 result;
>    int os_error; // Set only when result < 0.
>  }
> 
> And then use that struct to return results from async IO calls. WDYT?

SGTM.

The logging code that forces us to make the conversion on the IO thread can
actually be called on any thread...Until the main thread shuts down.  Since we
leak worker threads, that's not good enough, unfortunately.

[mmenke, 2013-02-21 22:47:18]

On 2013/02/21 22:45:22, mmenke wrote:
> On 2013/02/21 22:23:51, sergeyu wrote:
> > On 2013/02/21 19:57:23, mmenke wrote:
> > > On 2013/02/21 19:41:05, mmenke wrote:
> > > > On 2013/02/20 22:15:31, willchan wrote:
> > > > > Sorry for the delay, I missed this earlier. Redirecting to mmenke.
> > > > 
> > > > Relying on the signs of external error codes just seems like a bad idea
to
> > me.
> > > 
> > > > Having some functions return ints-or-chrome-net-errors and others return
> > > > int64s-or-negative-system-errors also seems like a bad idea to me.
> > > > 
> > > > Seems like we could we just inline the calls to RecordAndMapError in the
> > > > relevant functions in *_posix.cc and *_win.cc, and get rid of
> > CheckForIOError.
> > > 
> > > Oh, right...There may be threading issues with that.  So we still need to
do
> > the
> > > logging on the main thread.  Nevermind, then, though I still don't like
the
> > > negative error code approach, can live with it for now.
> > 
> > Yeah. I'm not original author of this code, but as far as I understand it,
> it's
> > not possible to map the error on the blocking thread because the original OS
> > error code needs to be reported too, and that has to happen on the IO
thread.
> > But I do agree it's very ugly. An alternative is to add some struct like
this:
> >  struct IOResult {
> >    int64 result;
> >    int os_error; // Set only when result < 0.
> >  }
> > 
> > And then use that struct to return results from async IO calls. WDYT?
> 
> SGTM.
> 
> The logging code that forces us to make the conversion on the IO thread can
> actually be called on any thread...Until the main thread shuts down.  Since we
> leak worker threads, that's not good enough, unfortunately.

May want to say "set only when result == -1", just so we have a well-defined
error value in result.  May also want to name it bytes_read, just because
IOResult::result is a little redundant.

[Sergey Ulanov, 2013-02-21 22:54:40]

On Thu, Feb 21, 2013 at 2:47 PM, <mmenke@chromium.org> wrote:

> On 2013/02/21 22:45:22, mmenke wrote:
>
>> On 2013/02/21 22:23:51, sergeyu wrote:
>> > On 2013/02/21 19:57:23, mmenke wrote:
>> > > On 2013/02/21 19:41:05, mmenke wrote:
>> > > > On 2013/02/20 22:15:31, willchan wrote:
>> > > > > Sorry for the delay, I missed this earlier. Redirecting to mmenke.
>> > > >
>> > > > Relying on the signs of external error codes just seems like a bad
>> idea
>>
> to
>
>> > me.
>> > >
>> > > > Having some functions return ints-or-chrome-net-errors and others
>> return
>> > > > int64s-or-negative-system-**errors also seems like a bad idea to
>> me.
>> > > >
>> > > > Seems like we could we just inline the calls to RecordAndMapError
>> in the
>> > > > relevant functions in *_posix.cc and *_win.cc, and get rid of
>> > CheckForIOError.
>> > >
>> > > Oh, right...There may be threading issues with that.  So we still
>> need to
>>
> do
>
>> > the
>> > > logging on the main thread.  Nevermind, then, though I still don't
>> like
>>
> the
>
>> > > negative error code approach, can live with it for now.
>> >
>> > Yeah. I'm not original author of this code, but as far as I understand
>> it,
>> it's
>> > not possible to map the error on the blocking thread because the
>> original OS
>> > error code needs to be reported too, and that has to happen on the IO
>>
> thread.
>
>> > But I do agree it's very ugly. An alternative is to add some struct like
>>
> this:
>
>> >  struct IOResult {
>> >    int64 result;
>> >    int os_error; // Set only when result < 0.
>> >  }
>> >
>> > And then use that struct to return results from async IO calls. WDYT?
>>
>
>  SGTM.
>>
>
>  The logging code that forces us to make the conversion on the IO thread
>> can
>> actually be called on any thread...Until the main thread shuts down.
>>  Since we
>> leak worker threads, that's not good enough, unfortunately.
>>
>
> May want to say "set only when result == -1", just so we have a
> well-defined
> error value in result.  May also want to name it bytes_read, just because
> IOResult::result is a little redundant.
>

I think the result should be a chrome error code (i.e. net::Error value)
returned by net::MapSystemError(), so it can take negative values other
than -1.


>
>
https://codereview.chromium.**org/12320003/<https://codereview.chromium.org/1...
>

[mmenke, 2013-02-21 22:57:00]

On 2013/02/21 22:54:40, sergeyu wrote:
> On Thu, Feb 21, 2013 at 2:47 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > On 2013/02/21 22:45:22, mmenke wrote:
> >
> >> On 2013/02/21 22:23:51, sergeyu wrote:
> >> > On 2013/02/21 19:57:23, mmenke wrote:
> >> > > On 2013/02/21 19:41:05, mmenke wrote:
> >> > > > On 2013/02/20 22:15:31, willchan wrote:
> >> > > > > Sorry for the delay, I missed this earlier. Redirecting to mmenke.
> >> > > >
> >> > > > Relying on the signs of external error codes just seems like a bad
> >> idea
> >>
> > to
> >
> >> > me.
> >> > >
> >> > > > Having some functions return ints-or-chrome-net-errors and others
> >> return
> >> > > > int64s-or-negative-system-**errors also seems like a bad idea to
> >> me.
> >> > > >
> >> > > > Seems like we could we just inline the calls to RecordAndMapError
> >> in the
> >> > > > relevant functions in *_posix.cc and *_win.cc, and get rid of
> >> > CheckForIOError.
> >> > >
> >> > > Oh, right...There may be threading issues with that.  So we still
> >> need to
> >>
> > do
> >
> >> > the
> >> > > logging on the main thread.  Nevermind, then, though I still don't
> >> like
> >>
> > the
> >
> >> > > negative error code approach, can live with it for now.
> >> >
> >> > Yeah. I'm not original author of this code, but as far as I understand
> >> it,
> >> it's
> >> > not possible to map the error on the blocking thread because the
> >> original OS
> >> > error code needs to be reported too, and that has to happen on the IO
> >>
> > thread.
> >
> >> > But I do agree it's very ugly. An alternative is to add some struct like
> >>
> > this:
> >
> >> >  struct IOResult {
> >> >    int64 result;
> >> >    int os_error; // Set only when result < 0.
> >> >  }
> >> >
> >> > And then use that struct to return results from async IO calls. WDYT?
> >>
> >
> >  SGTM.
> >>
> >
> >  The logging code that forces us to make the conversion on the IO thread
> >> can
> >> actually be called on any thread...Until the main thread shuts down.
> >>  Since we
> >> leak worker threads, that's not good enough, unfortunately.
> >>
> >
> > May want to say "set only when result == -1", just so we have a
> > well-defined
> > error value in result.  May also want to name it bytes_read, just because
> > IOResult::result is a little redundant.
> >
> 
> I think the result should be a chrome error code (i.e. net::Error value)
> returned by net::MapSystemError(), so it can take negative values other
> than -1.

I was thinking we'd do the mapping in the same place we do it now, but that
sounds better (And result seems a reasonable name).

[Sergey Ulanov, 2013-02-22 08:39:25]

PTAL - added IOResult struct that's now used to pass results of IO operations

[mmenke, 2013-02-22 16:24:59]

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:26: : result(0),
Maybe result(OK)?

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:196: return OpenResult(file, IOResult(0, 0));
Are the default constructors used anywhere?  If not, suggest getting rid of them
and replacing the first 0 with OK.  If so, suggest just using the default
constructor here.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:211: OnAsyncCompleted(IntToInt64(callback),
result.error_code.result);
Hmm...  This looks a little weird, though suppose it's better than
result.result.result.  No suggestions on better naming scheme.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:105: return bytes_read;
Skimming over the MSDN docs, the old behavior looks wrong, but could we do this
in a followup CL?  Think this change is a bit too far removed from the purpose
of this CL.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:142: return bytes_written;
See above comment.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:216: IOResult result;
Don't think we need this here.  Can be scoped within the error block.

[Sergey Ulanov, 2013-02-22 21:04:06]

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:26: : result(0),
On 2013/02/22 16:24:59, mmenke wrote:
> Maybe result(OK)?

Done.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:196: return OpenResult(file, IOResult(0, 0));
On 2013/02/22 16:24:59, mmenke wrote:
> Are the default constructors used anywhere?  If not, suggest getting rid of
them
> and replacing the first 0 with OK.  If so, suggest just using the default
> constructor here.

They are used in base::PostTaskAndReplyWithResult() (see
base/task_runner_util.h), and so this code wouldn't compile without default
constructors :(

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:211: OnAsyncCompleted(IntToInt64(callback),
result.error_code.result);
On 2013/02/22 16:24:59, mmenke wrote:
> Hmm...  This looks a little weird, though suppose it's better than
> result.result.result.  No suggestions on better naming scheme.

Renamed |result| to |open_result| in this function - IMHO it's a bit more
readable this way.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:105: return bytes_read;
On 2013/02/22 16:24:59, mmenke wrote:
> Skimming over the MSDN docs, the old behavior looks wrong, but could we do
this
> in a followup CL?  Think this change is a bit too far removed from the purpose
> of this CL.

This method would have to be updated either way (for RecordError()), so not
fixing this bug in this CL would not make this CL smaller. I updated CL
description to list all bugs fixed by this CL.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:142: return bytes_written;
On 2013/02/22 16:24:59, mmenke wrote:
> See above comment.

Same here.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:216: IOResult result;
On 2013/02/22 16:24:59, mmenke wrote:
> Don't think we need this here.  Can be scoped within the error block.

Done.

[mmenke, 2013-02-22 21:13:17]

LGTM

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:196: return OpenResult(file, IOResult(0, 0));
On 2013/02/22 21:04:06, sergeyu wrote:
> On 2013/02/22 16:24:59, mmenke wrote:
> > Are the default constructors used anywhere?  If not, suggest getting rid of
> them
> > and replacing the first 0 with OK.  If so, suggest just using the default
> > constructor here.
> 
> They are used in base::PostTaskAndReplyWithResult() (see
> base/task_runner_util.h), and so this code wouldn't compile without default
> constructors :(

Still suggest using IOResult(OK, 0), or even just IOResult().

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:105: return bytes_read;
On 2013/02/22 21:04:06, sergeyu wrote:
> On 2013/02/22 16:24:59, mmenke wrote:
> > Skimming over the MSDN docs, the old behavior looks wrong, but could we do
> this
> > in a followup CL?  Think this change is a bit too far removed from the
purpose
> > of this CL.
> 
> This method would have to be updated either way (for RecordError()), so not
> fixing this bug in this CL would not make this CL smaller. I updated CL
> description to list all bugs fixed by this CL.

It's not about the size, it's about the unrelated change in behavior.  If the
change unexpectedly causes a breakage, then the preferred behavior would be to
revert the entire CL.

[mmenke, 2013-02-22 21:16:27]

On 2013/02/22 21:13:17, mmenke wrote:
> LGTM
> 
>
https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
> File net/base/file_stream_context.cc (right):
> 
>
https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
> net/base/file_stream_context.cc:196: return OpenResult(file, IOResult(0, 0));
> On 2013/02/22 21:04:06, sergeyu wrote:
> > On 2013/02/22 16:24:59, mmenke wrote:
> > > Are the default constructors used anywhere?  If not, suggest getting rid
of
> > them
> > > and replacing the first 0 with OK.  If so, suggest just using the default
> > > constructor here.
> > 
> > They are used in base::PostTaskAndReplyWithResult() (see
> > base/task_runner_util.h), and so this code wouldn't compile without default
> > constructors :(
> 
> Still suggest using IOResult(OK, 0), or even just IOResult().
> 
>
https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
> File net/base/file_stream_context_win.cc (right):
> 
>
https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
> net/base/file_stream_context_win.cc:105: return bytes_read;
> On 2013/02/22 21:04:06, sergeyu wrote:
> > On 2013/02/22 16:24:59, mmenke wrote:
> > > Skimming over the MSDN docs, the old behavior looks wrong, but could we do
> > this
> > > in a followup CL?  Think this change is a bit too far removed from the
> purpose
> > > of this CL.
> > 
> > This method would have to be updated either way (for RecordError()), so not
> > fixing this bug in this CL would not make this CL smaller. I updated CL
> > description to list all bugs fixed by this CL.
> 
> It's not about the size, it's about the unrelated change in behavior.  If the
> change unexpectedly causes a breakage, then the preferred behavior would be to
> revert the entire CL.

And to be clear - that is a signoff as-is, though I do think it would be a good
idea to separate out the unrelated behavioral fix, not something I'm going to
block you on.

[Sergey Ulanov, 2013-02-22 21:51:47]

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context.cc:196: return OpenResult(file, IOResult(0, 0));
On 2013/02/22 21:13:17, mmenke wrote:
> On 2013/02/22 21:04:06, sergeyu wrote:
> > On 2013/02/22 16:24:59, mmenke wrote:
> > > Are the default constructors used anywhere?  If not, suggest getting rid
of
> > them
> > > and replacing the first 0 with OK.  If so, suggest just using the default
> > > constructor here.
> > 
> > They are used in base::PostTaskAndReplyWithResult() (see
> > base/task_runner_util.h), and so this code wouldn't compile without default
> > constructors :(
> 
> Still suggest using IOResult(OK, 0), or even just IOResult().

Done.

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
File net/base/file_stream_context_win.cc (right):

https://codereview.chromium.org/12320003/diff/19003/net/base/file_stream_cont...
net/base/file_stream_context_win.cc:105: return bytes_read;
On 2013/02/22 21:13:17, mmenke wrote:
> On 2013/02/22 21:04:06, sergeyu wrote:
> > On 2013/02/22 16:24:59, mmenke wrote:
> > > Skimming over the MSDN docs, the old behavior looks wrong, but could we do
> > this
> > > in a followup CL?  Think this change is a bit too far removed from the
> purpose
> > > of this CL.
> > 
> > This method would have to be updated either way (for RecordError()), so not
> > fixing this bug in this CL would not make this CL smaller. I updated CL
> > description to list all bugs fixed by this CL.
> 
> It's not about the size, it's about the unrelated change in behavior.  If the
> change unexpectedly causes a breakage, then the preferred behavior would be to
> revert the entire CL.

Yeah, I understand and agree that in general it would be better to have a
separate CL for this. It's just this looked like an obvious bug and it's not
independent of my other changes in this CL.

[commit-bot: I haz the power, 2013-02-22 21:52:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12320003/6009

[commit-bot: I haz the power, 2013-02-22 22:01:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12320003/20017

[commit-bot: I haz the power, 2013-02-22 23:01:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-22 23:26:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12320003/11007

[commit-bot: I haz the power, 2013-02-23 02:08:05]

Change committed as 184257