net: Implement canceling of all async operations in FileStream.

net: Implement canceling of all async operations in FileStream.

Canceling of async operations allows to not wait for their completion
in FileStream's destructor. Other related changes include:

- Got rid of FileStream::Close() and FileStream::CloseSync() methods because
reuse of FileStream object doesn't make much sense, it should be destroyed
instead.

- Changed FileStream to always acquire ownership of the PlatformFile it was
given. Fixed usages of FileStream where no ownership was assumed, introduced
new helper functions in base/platform_file.h on the way.

- FileStream's destructor now always closes the file. If file was opened with
PLATFORM_FILE_ASYNC then actual closing is done asynchronously, destructor
doesn't wait for that and returns immediately. When file was opened without
PLATFORM_FILE_ASYNC closing is done synchronously and the thread doing that
should be allowed to do IO operations.

- Implementation of FileStream is refactored. FileStream is now just a wrapper
around internal object that does all actual work and that can be easily
orphaned in the destructor to not block on the actual file descriptor closing.
All platform-independent code is extracted into a special file and amount of
platform-dependent code is minimized.


BUG=115067, 112474
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166091

[pivanof, 2012-06-30 23:29:31]

This patch looks like it's a huge change but it mostly consists of shuffling
code around to give it better structure, encapsulation and better unification
between windows and posix.

While preparing patch I noticed some issues with the current FileStream
implementation. I want to hear your take on whether we should do something about
that.

1) It looks like FileStream was never intended to be created from existing
base::PlatformFile, then closed and reopened again. Because in this case it
wouldn't automatically close opened file. I didn't fix this issue and wanted to
ask was it intentional?

2) Extrapolating the above problem I thought maybe Close*() methods were never
supposed to be called when FileStream was created from the existing file handle?
My implementation has a small issue in this use case now. Tell me if I should
fix that.

3) This is more serious issue: removing waiting on async operations in
FileStream destructor introduced sort of use-after-free problem which was even
exposed in FileStreamTest. AsyncRead_EarlyDelete_NoAutoClose test. I've put a
guard now against issue in this particular use case but it doesn't eliminate the
issue completely. So the scenario goes like this: FileStream is created from
existing file handle. Then user issues any async command (Read, Write or Seek).
Then user cancels async operation by destroying FileStream or calling
CloseAndCancelAsync(). Then user closes file handle used by FileStream while
async operation tries to access this file in another thread. There's no big deal
if async operations gets EBADF error, but if the same file descriptor gets
reused before async operation is executed we can get a problem in some
completely unrelated code. I've put a guard that makes this issue less probable,
but the window for race still exist and I'm not quite sure if I can do something
about it. What's your thoughts about this?

[willchan no longer on Chromium, 2012-07-02 08:18:50]

On Sat, Jun 30, 2012 at 4:29 PM, <paivanof@gmail.com> wrote:

> Reviewers: willchan, satorux1,
>
> Message:
> This patch looks like it's a huge change but it mostly consists of
> shuffling
> code around to give it better structure, encapsulation and better
> unification
> between windows and posix.
>
> While preparing patch I noticed some issues with the current FileStream
> implementation. I want to hear your take on whether we should do something
> about
> that.
>
> 1) It looks like FileStream was never intended to be created from existing
> base::PlatformFile, then closed and reopened again. Because in this case it
> wouldn't automatically close opened file. I didn't fix this issue and
> wanted to
> ask was it intentional?
>
> 2) Extrapolating the above problem I thought maybe Close*() methods were
> never
> supposed to be called when FileStream was created from the existing file
> handle?
> My implementation has a small issue in this use case now. Tell me if I
> should
> fix that.
>
> 3) This is more serious issue: removing waiting on async operations in
> FileStream destructor introduced sort of use-after-free problem which was
> even
> exposed in FileStreamTest. AsyncRead_EarlyDelete_**NoAutoClose test. I've
> put a
> guard now against issue in this particular use case but it doesn't
> eliminate the
> issue completely. So the scenario goes like this: FileStream is created
> from
> existing file handle. Then user issues any async command (Read, Write or
> Seek).
> Then user cancels async operation by destroying FileStream or calling
> CloseAndCancelAsync(). Then user closes file handle used by FileStream
> while
> async operation tries to access this file in another thread. There's no
> big deal
> if async operations gets EBADF error, but if the same file descriptor gets
> reused before async operation is executed we can get a problem in some
> completely unrelated code. I've put a guard that makes this issue less
> probable,
> but the window for race still exist and I'm not quite sure if I can do
> something
> about it. What's your thoughts about this?
>

It's a user bug as far as I'm concerned. Once you hand a file descriptor to
the FileStream, you've transferred ownership. It's not yours anymore. If we
need to transfer it back, then we need a new API for that to prevent races.
But I'd prefer to change users so they don't need this semantic.

PS: I'm on vacation through July 17th. So don't expect responses here.


>
> Description:
> net: Implement canceling of all async operations in FileStream.
>
> Canceling of async operations allows to not wait for their completion
> in FileStream's destructor or CloseSync() method.
>
>
> Patch from Pavel Ivanov <paivanof@gmail.com>
>
> BUG=115067
> TEST=net_unittests
>
> Please review this at
https://chromiumcodereview.**appspot.com/10701050/<https://chromiumcodereview...
>
> SVN Base:
https://src.chromium.org/**chrome/trunk/src/<https://src.chromium.org/chrome/...
>
> Affected files:
>   M     net/base/file_stream.h
>   M     net/base/file_stream.cc
>   M     net/base/file_stream_posix.h
>   M     net/base/file_stream_posix.cc
>   M     net/base/file_stream_unittest.**cc
>   M     net/base/file_stream_win.h
>   M     net/base/file_stream_win.cc
>
>
>

[pivanof, 2012-07-02 11:38:18]

On 2012/07/02 08:18:50, willchan wrote:
> On Sat, Jun 30, 2012 at 4:29 PM, <mailto:paivanof@gmail.com> wrote:
> > 3) This is more serious issue: removing waiting on async operations in
> > FileStream destructor introduced sort of use-after-free problem which was
> > even
> > exposed in FileStreamTest. AsyncRead_EarlyDelete_**NoAutoClose test. I've
> > put a
> > guard now against issue in this particular use case but it doesn't
> > eliminate the
> > issue completely. So the scenario goes like this: FileStream is created
> > from
> > existing file handle. Then user issues any async command (Read, Write or
> > Seek).
> > Then user cancels async operation by destroying FileStream or calling
> > CloseAndCancelAsync(). Then user closes file handle used by FileStream
> > while
> > async operation tries to access this file in another thread. There's no
> > big deal
> > if async operations gets EBADF error, but if the same file descriptor gets
> > reused before async operation is executed we can get a problem in some
> > completely unrelated code. I've put a guard that makes this issue less
> > probable,
> > but the window for race still exist and I'm not quite sure if I can do
> > something
> > about it. What's your thoughts about this?
> >
> 
> It's a user bug as far as I'm concerned. Once you hand a file descriptor to
> the FileStream, you've transferred ownership. It's not yours anymore. If we
> need to transfer it back, then we need a new API for that to prevent races.
> But I'd prefer to change users so they don't need this semantic.

As I understand this is not exactly true. Transferring ownership should mean
that FileStream closes this handle. But it was specifically written so that it
doesn't close it. And test FileStreamTest.AsyncRead_EarlyDelete_NoAutoClose has
special check that file is still open after FileStream destruction.

I searched where in the code FileStream is constructed from file handle. There's
not too many places.
1) There're several places in webkit/glue/webfileutilities_impl.cc -- they use
only sync operations, so they are safe.
2) content/browser/renderer_host/redirect_to_file_resource_handler.cc -- it
calls explicitly FileStream::CloseSync() when it doesn't need file handle
anymore. It means that I should fix this use case to work properly (or change
CloseSync() call to CloseAndCancleAsync() which will also fix crbug.com/112474).
3) chromeos/dbus/debug_daemon_client.cc -- specifically relies on the race
condition to somehow work as expected. I think it can be fixed by just calling
CloseAndCancelAsync() in the destructor. Should I include fix for that in this
CL?

[pivanof, 2012-07-05 01:51:55]

So I fixed Close*() methods to work correctly when FileStream created from
existing file descriptor and added comment to the constructor that when
FileStream is created like that it can only be used correctly when
1) only sync methods are used;
2) any async methods are always waited on and never canceled;
3) any of FileStream::Close*() methods is called before its destruction.

And I removed test FileStreamTest.AsyncRead_EarlyDelete_NoAutoClose because it
tested prohibited pattern not falling into 3 options above.

[pivanof, 2012-07-12 01:40:13]

wtc, could you review this patch while Will is on vacation?

[wtc, 2012-07-13 23:40:21]

Review comments on patch set 7:

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
File net/base/file_stream_posix.cc (right):

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
net/base/file_stream_posix.cc:52: 

Nit: delete a blank line.

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
net/base/file_stream_posix.cc:123: void OnAsyncCompleted(const
base::Callback<void(R)>& callback, R* result);

Since OnAsyncCompleted is only instantianted with R=int and
R=int64, it seems that we should be able to define OnAsyncCompleted
so that it takes "R result" instead of "R* result".  This
should avoid the need to do "new int(OK)".  I didn't study
the code well enough to know if this can be done.

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
File net/base/file_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
net/base/file_stream_unittest.cc:1213: MessageLoop::current()->RunAllPending();

Why do these unit tests need to call MessageLoop::current()->RunAllPending()?
Is it to make the unit tests more strict?  I guess the
RunAllPending() call eliminates any doubt that the callback
may still be called in the future (currently pending)?

[pivanof, 2012-07-14 00:13:58]

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
File net/base/file_stream_posix.cc (right):

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
net/base/file_stream_posix.cc:123: void OnAsyncCompleted(const
base::Callback<void(R)>& callback, R* result);
On 2012/07/13 23:40:21, wtc wrote:
> 
> Since OnAsyncCompleted is only instantianted with R=int and
> R=int64, it seems that we should be able to define OnAsyncCompleted
> so that it takes "R result" instead of "R* result".  This
> should avoid the need to do "new int(OK)".  I didn't study
> the code well enough to know if this can be done.

No, this can't be done because value of result is bound inside of Closure at the
time of scheduling of async task but async task during execution should be able
to change which result value OnAsyncCompleted() receives.

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
File net/base/file_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701050/diff/13/net/base/file_stream_...
net/base/file_stream_unittest.cc:1213: MessageLoop::current()->RunAllPending();
On 2012/07/13 23:40:21, wtc wrote:
> 
> Why do these unit tests need to call MessageLoop::current()->RunAllPending()?
> Is it to make the unit tests more strict?  I guess the
> RunAllPending() call eliminates any doubt that the callback
> may still be called in the future (currently pending)?

Correct. As callback can be called only on the current thread then it won't
execute until RunAllPending() is called and thus TestCompletionCallback won't be
able to record whether it received result or not.

Although personally I don't quite understand how this doesn't introduce race
here. By the time RunAllPending() is called open task in WorkerPool may not
finish yet and thus reply may not be in queue of current thread yet. Does
MessageLoop remembers somehow that it should get reply at some point in the
future and thus doesn't return from RunAllPending() until reply has been
executed? I don't know...

[willchan no longer on Chromium, 2012-07-18 18:20:38]

FYI, I'm back and plan on taking over this review again as I'm probably more
aware of the other interdependencies with other components here.

[willchan no longer on Chromium, 2012-07-19 22:10:22]

On 2012/07/02 11:38:18, pivanof wrote:
> On 2012/07/02 08:18:50, willchan wrote:
> > On Sat, Jun 30, 2012 at 4:29 PM, <mailto:paivanof@gmail.com> wrote:
> > > 3) This is more serious issue: removing waiting on async operations in
> > > FileStream destructor introduced sort of use-after-free problem which was
> > > even
> > > exposed in FileStreamTest. AsyncRead_EarlyDelete_**NoAutoClose test. I've
> > > put a
> > > guard now against issue in this particular use case but it doesn't
> > > eliminate the
> > > issue completely. So the scenario goes like this: FileStream is created
> > > from
> > > existing file handle. Then user issues any async command (Read, Write or
> > > Seek).
> > > Then user cancels async operation by destroying FileStream or calling
> > > CloseAndCancelAsync(). Then user closes file handle used by FileStream
> > > while
> > > async operation tries to access this file in another thread. There's no
> > > big deal
> > > if async operations gets EBADF error, but if the same file descriptor gets
> > > reused before async operation is executed we can get a problem in some
> > > completely unrelated code. I've put a guard that makes this issue less
> > > probable,
> > > but the window for race still exist and I'm not quite sure if I can do
> > > something
> > > about it. What's your thoughts about this?
> > >
> > 
> > It's a user bug as far as I'm concerned. Once you hand a file descriptor to
> > the FileStream, you've transferred ownership. It's not yours anymore. If we
> > need to transfer it back, then we need a new API for that to prevent races.
> > But I'd prefer to change users so they don't need this semantic.
> 
> As I understand this is not exactly true. Transferring ownership should mean
> that FileStream closes this handle. But it was specifically written so that it
> doesn't close it. And test FileStreamTest.AsyncRead_EarlyDelete_NoAutoClose
has
> special check that file is still open after FileStream destruction.

I think you misunderstand me. Don't assume that any API you see is correct. I'm
positing it's flawed. This test is flawed too. I think that we should fix the
API so ownership is transferred. Does this sound reasonable to you?

> 
> I searched where in the code FileStream is constructed from file handle.
There's
> not too many places.
> 1) There're several places in webkit/glue/webfileutilities_impl.cc -- they use
> only sync operations, so they are safe.
> 2) content/browser/renderer_host/redirect_to_file_resource_handler.cc -- it
> calls explicitly FileStream::CloseSync() when it doesn't need file handle
> anymore. It means that I should fix this use case to work properly (or change
> CloseSync() call to CloseAndCancleAsync() which will also fix
crbug.com/112474).
> 3) chromeos/dbus/debug_daemon_client.cc -- specifically relies on the race
> condition to somehow work as expected. I think it can be fixed by just calling
> CloseAndCancelAsync() in the destructor. Should I include fix for that in this
> CL?

[pivanof, 2012-07-19 22:58:00]

On 2012/07/19 22:10:22, willchan wrote:
> On 2012/07/02 11:38:18, pivanof wrote:
> > On 2012/07/02 08:18:50, willchan wrote:
> > > It's a user bug as far as I'm concerned. Once you hand a file descriptor
to
> > > the FileStream, you've transferred ownership. It's not yours anymore. If
we
> > > need to transfer it back, then we need a new API for that to prevent
races.
> > > But I'd prefer to change users so they don't need this semantic.
> > 
> > As I understand this is not exactly true. Transferring ownership should mean
> > that FileStream closes this handle. But it was specifically written so that
it
> > doesn't close it. And test FileStreamTest.AsyncRead_EarlyDelete_NoAutoClose
> has
> > special check that file is still open after FileStream destruction.
> 
> I think you misunderstand me. Don't assume that any API you see is correct.
I'm
> positing it's flawed. This test is flawed too. I think that we should fix the
> API so ownership is transferred. Does this sound reasonable to you?

I think so. That's why my final patch set adds a comment to
FileStream(base::PlatformFile file, ...) which reads:

// Note: if you use this constructor you have 3 options of further usage:
// - Only sync operations are used on this stream;
// - You wait for completion of all async operations;
// - When you finished using stream you call any type of Close*() method
//   (including possibly CloseAndCancelAsync() or Close() with later
//   destruction without waiting for closing callback).
// If your usage doesn't fall into one of 3 options above you are introducing
// use-after-free bug when file descriptor could be used after you close it
// (and it possibly gets reopened to another file).

And I deleted FileStreamTest.AsyncRead_EarlyDelete_NoAutoClose because it tested
an invalid usage pattern.
Do you think that's enough?

[willchan no longer on Chromium, 2012-07-23 20:57:37]

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
File net/base/file_stream.h (right):

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
net/base/file_stream.h:47: // - You wait for completion of all async operations;
Why? Why doesn't deleting FileStream automatically cancel all async operations?

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
net/base/file_stream.h:82: virtual void CloseAndCancelAsync();
I don't understand this...when do you want to quickly destroy FileStream without
actually calling the destructor?

[pivanof, 2012-07-23 21:18:28]

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
File net/base/file_stream.h (right):

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
net/base/file_stream.h:47: // - You wait for completion of all async operations;
On 2012/07/23 20:57:37, willchan wrote:
> Why? Why doesn't deleting FileStream automatically cancel all async
operations?

Well, sure it will "cancel". But cancellation means that this async operation
can be in progress at the moment of destructor execution. And besides being in
progress it can be currently at the point between check "if operation was
canceled" and the actual execution of operation. And if at this point user won't
transfer file descriptor ownership to FileStream (by means of calling of one of
the Close*() methods) and will close file descriptor by himself after
destruction of FileStream async operation can be executed on a stale file
descriptor.
So basically what I meant by this comment is user should either transfer
ownership by calling one of the Close*() methods or (if he doesn't want to do
that) wait for all async operations to complete. Maybe as there's no such use
case in current Chromium code and it's really tricky to understand it, maybe
this should be removed from the comment? I.e. we will say that if user creates
FileStream from file descriptor and calls async methods he must call Close*()
before destruction.

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
net/base/file_stream.h:82: virtual void CloseAndCancelAsync();
On 2012/07/23 20:57:37, willchan wrote:
> I don't understand this...when do you want to quickly destroy FileStream
without
> actually calling the destructor?

Looking at it now I think maybe I should delete this last sentence.
What I meant is behavior of this method is basically equivalent to calling
destructor (in terms of cancellation of async operations, asynchronous closing
of file descriptor without notification to user of the completion, and all that
without actual waiting for those operations to complete) but FileStream object
is not actually destroyed and still usable after that (it could be reopened if
necessary).

[willchan no longer on Chromium, 2012-07-23 21:32:02]

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
File net/base/file_stream.h (right):

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
net/base/file_stream.h:47: // - You wait for completion of all async operations;
On 2012/07/23 21:18:29, pivanof wrote:
> On 2012/07/23 20:57:37, willchan wrote:
> > Why? Why doesn't deleting FileStream automatically cancel all async
> operations?
> 
> Well, sure it will "cancel". But cancellation means that this async operation
> can be in progress at the moment of destructor execution. And besides being in
> progress it can be currently at the point between check "if operation was
> canceled" and the actual execution of operation. And if at this point user
won't
> transfer file descriptor ownership to FileStream (by means of calling of one
of
> the Close*() methods) and will close file descriptor by himself after
> destruction of FileStream async operation can be executed on a stale file
> descriptor.

I think ownership is "transferred" when it is passed into the constructor. After
that point, it's a bug for the user to ever operate directly on the file
descriptor.

> So basically what I meant by this comment is user should either transfer
> ownership by calling one of the Close*() methods or (if he doesn't want to do
> that) wait for all async operations to complete. Maybe as there's no such use
> case in current Chromium code and it's really tricky to understand it, maybe
> this should be removed from the comment? I.e. we will say that if user creates
> FileStream from file descriptor and calls async methods he must call Close*()
> before destruction.

https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
net/base/file_stream.h:82: virtual void CloseAndCancelAsync();
On 2012/07/23 21:18:29, pivanof wrote:
> On 2012/07/23 20:57:37, willchan wrote:
> > I don't understand this...when do you want to quickly destroy FileStream
> without
> > actually calling the destructor?
> 
> Looking at it now I think maybe I should delete this last sentence.
> What I meant is behavior of this method is basically equivalent to calling
> destructor (in terms of cancellation of async operations, asynchronous closing
> of file descriptor without notification to user of the completion, and all
that
> without actual waiting for those operations to complete) but FileStream object
> is not actually destroyed and still usable after that (it could be reopened if
> necessary).

I'd rather get rid of this and have the user create a new FileStream if needed.

[pivanof, 2012-07-23 21:44:25]

On 2012/07/23 21:32:02, willchan wrote:
>
https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
> File net/base/file_stream.h (right):
> 
>
https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
> net/base/file_stream.h:47: // - You wait for completion of all async
operations;
> On 2012/07/23 21:18:29, pivanof wrote:
> > On 2012/07/23 20:57:37, willchan wrote:
> > > Why? Why doesn't deleting FileStream automatically cancel all async
> > operations?
> > 
> > Well, sure it will "cancel". But cancellation means that this async
operation
> > can be in progress at the moment of destructor execution. And besides being
in
> > progress it can be currently at the point between check "if operation was
> > canceled" and the actual execution of operation. And if at this point user
> won't
> > transfer file descriptor ownership to FileStream (by means of calling of one
> of
> > the Close*() methods) and will close file descriptor by himself after
> > destruction of FileStream async operation can be executed on a stale file
> > descriptor.
> 
> I think ownership is "transferred" when it is passed into the constructor.
After
> that point, it's a bug for the user to ever operate directly on the file
> descriptor.

There's usage of FileStream somewhere in webkit/ (I can't find at the moment
without eclipse and full tree at hand) where temporary FileStream is created on
file descriptor, sync operation is performed and then FileStream is destroyed.
Looks like pretty legitimate use and it won't work if ownership is transferred
in constructor.

I can post all such usages when I'll be at home later tonight.

>
https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
> net/base/file_stream.h:82: virtual void CloseAndCancelAsync();
> On 2012/07/23 21:18:29, pivanof wrote:
> > On 2012/07/23 20:57:37, willchan wrote:
> > > I don't understand this...when do you want to quickly destroy FileStream
> > without
> > > actually calling the destructor?
> > 
> > Looking at it now I think maybe I should delete this last sentence.
> > What I meant is behavior of this method is basically equivalent to calling
> > destructor (in terms of cancellation of async operations, asynchronous
closing
> > of file descriptor without notification to user of the completion, and all
> that
> > without actual waiting for those operations to complete) but FileStream
object
> > is not actually destroyed and still usable after that (it could be reopened
if
> > necessary).
> 
> I'd rather get rid of this and have the user create a new FileStream if
needed.

You mean remove this sentence from the comment as I said or you mean something
more here?

[willchan no longer on Chromium, 2012-07-23 21:58:31]

On 2012/07/23 21:44:25, pivanof wrote:
> On 2012/07/23 21:32:02, willchan wrote:
> >
>
https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
> > File net/base/file_stream.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
> > net/base/file_stream.h:47: // - You wait for completion of all async
> operations;
> > On 2012/07/23 21:18:29, pivanof wrote:
> > > On 2012/07/23 20:57:37, willchan wrote:
> > > > Why? Why doesn't deleting FileStream automatically cancel all async
> > > operations?
> > > 
> > > Well, sure it will "cancel". But cancellation means that this async
> operation
> > > can be in progress at the moment of destructor execution. And besides
being
> in
> > > progress it can be currently at the point between check "if operation was
> > > canceled" and the actual execution of operation. And if at this point user
> > won't
> > > transfer file descriptor ownership to FileStream (by means of calling of
one
> > of
> > > the Close*() methods) and will close file descriptor by himself after
> > > destruction of FileStream async operation can be executed on a stale file
> > > descriptor.
> > 
> > I think ownership is "transferred" when it is passed into the constructor.
> After
> > that point, it's a bug for the user to ever operate directly on the file
> > descriptor.
> 
> There's usage of FileStream somewhere in webkit/ (I can't find at the moment
> without eclipse and full tree at hand) where temporary FileStream is created
on
> file descriptor, sync operation is performed and then FileStream is destroyed.
> Looks like pretty legitimate use and it won't work if ownership is transferred
> in constructor.

Why are they using FileStream? Why not just FileUtil?

> 
> I can post all such usages when I'll be at home later tonight.
> 
> >
>
https://chromiumcodereview.appspot.com/10701050/diff/23002/net/base/file_stre...
> > net/base/file_stream.h:82: virtual void CloseAndCancelAsync();
> > On 2012/07/23 21:18:29, pivanof wrote:
> > > On 2012/07/23 20:57:37, willchan wrote:
> > > > I don't understand this...when do you want to quickly destroy FileStream
> > > without
> > > > actually calling the destructor?
> > > 
> > > Looking at it now I think maybe I should delete this last sentence.
> > > What I meant is behavior of this method is basically equivalent to calling
> > > destructor (in terms of cancellation of async operations, asynchronous
> closing
> > > of file descriptor without notification to user of the completion, and all
> > that
> > > without actual waiting for those operations to complete) but FileStream
> object
> > > is not actually destroyed and still usable after that (it could be
reopened
> if
> > > necessary).
> > 
> > I'd rather get rid of this and have the user create a new FileStream if
> needed.
> 
> You mean remove this sentence from the comment as I said or you mean something
> more here?

Get rid of the new function.

[pivanof, 2012-07-24 00:24:06]

On 2012/07/23 21:58:31, willchan wrote:
> On 2012/07/23 21:44:25, pivanof wrote:
> > There's usage of FileStream somewhere in webkit/ (I can't find at the moment
> > without eclipse and full tree at hand) where temporary FileStream is created
> on
> > file descriptor, sync operation is performed and then FileStream is
destroyed.
> > Looks like pretty legitimate use and it won't work if ownership is
transferred
> > in constructor.
> 
> Why are they using FileStream? Why not just FileUtil?

Do you mean stuff from base/platform_file.h? I think if I add SeekPlatformFile()
to platform_file.h I can put those functions instead of FileStream (BTW, I'm
talking about webkit/glue/webfileutilities_impl.cc).

Shouldn't we change FileStream to use functions from platform_file.h where
possible too?

> > > I'd rather get rid of this and have the user create a new FileStream if
> > needed.
> > 
> > You mean remove this sentence from the comment as I said or you mean
something
> > more here?
> 
> Get rid of the new function.

Then I'll need to change member variable in URLRequestFileJob to be
scoped_ptr<FileStream> instead of plain FileStream, because I'll need to delete
it in URLRequestFileJob::Kill(). But I guess that's not a big deal.

[pivanof, 2012-07-24 04:46:01]

I've updated the patch for FileStream to always maintain ownership of file
descriptor and removed all usages of FileStream that were not in line with new
paradigm. To maintain functionality of webkit/glue/webfileutilities_impl.cc I
had to add 3 new functions to base/platform_file.h (Seek and new versions of
Read and Write).

willchan: Please tell me what you think of this approach.

[willchan no longer on Chromium, 2012-07-27 21:19:47]

I need to review the FileStream implementation stuff next. I may need to hand
off the detailed portion of that review to wtc or mmenke or someone, since I
will be traveling again soon. But all the interface stuff (which is the
important stuff to me) is much better, thanks.

The main thing I want to review before I leave is your threading WRT WorkerPool.
I'll try to get that today.

https://chromiumcodereview.appspot.com/10701050/diff/24002/net/base/file_stre...
File net/base/file_stream.h (right):

https://chromiumcodereview.appspot.com/10701050/diff/24002/net/base/file_stre...
net/base/file_stream.h:67: // behavior of this method is equivalent to
CloseAndCancelAsync().
You can undo this comment change, right?

[pivanof, 2012-07-29 03:10:33]

On 2012/07/27 21:19:47, willchan wrote:
>
https://chromiumcodereview.appspot.com/10701050/diff/24002/net/base/file_stre...
> File net/base/file_stream.h (right):
> 
>
https://chromiumcodereview.appspot.com/10701050/diff/24002/net/base/file_stre...
> net/base/file_stream.h:67: // behavior of this method is equivalent to
> CloseAndCancelAsync().
> You can undo this comment change, right?

Yes, I forgot to change that. I've updated it (although it's not a complete
undo).

[pivanof, 2012-08-08 04:41:17]

willchan, wtc, mmenke: Is anybody reviewing this now?

[mmenke, 2012-08-08 16:53:00]

On 2012/08/08 04:41:17, pivanof wrote:
> willchan, wtc, mmenke: Is anybody reviewing this now?

Will's on vacation, not sure when he'll get back.  This review wasn't on my
radar since Will seemed to be doing it, and I'm not familiar with the file
stream code.

[wtc, 2012-08-08 18:28:54]

On 2012/08/08 16:53:00, Matt Menke wrote:
>
> Will's on vacation, not sure when he'll get back.  This review wasn't on my
> radar since Will seemed to be doing it, and I'm not familiar with the file
> stream code.

Yeah, I did review an earlier patch set of this CL, but I stopped
reviewing this CL after seeing willchan's message 8:
http://codereview.chromium.org/10701050/#msg8

willchan gets back on August 14th. Can you wait until then?

[pivanof, 2012-08-08 22:17:34]

On 2012/08/08 18:28:54, wtc wrote:
> willchan gets back on August 14th. Can you wait until then?

Well, I guess yes. I was just worried that this review hangs here for too long.

[willchan no longer on Chromium, 2012-08-14 17:55:22]

I'm back, sorry folks! Looking.


On Wed, Aug 8, 2012 at 3:17 PM, <paivanof@gmail.com> wrote:

> On 2012/08/08 18:28:54, wtc wrote:
>
>> willchan gets back on August 14th. Can you wait until then?
>>
>
> Well, I guess yes. I was just worried that this review hangs here for too
> long.
>
>
http://codereview.chromium.**org/10701050/<http://codereview.chromium.org/107...
>

[willchan no longer on Chromium, 2012-08-14 17:55:46]

Not sure why I didn't send these out earlier. Here's some leftover draft
comments from before vacation.

http://codereview.chromium.org/10701050/diff/24002/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/24002/net/base/file_stream_posix...
net/base/file_stream_posix.cc:62: bool record_uma() { return record_uma_; }
Should be const member function

http://codereview.chromium.org/10701050/diff/24002/net/base/file_stream_posix...
net/base/file_stream_posix.cc:64: base::PlatformFile file() { return file_; }
Ditto on const

http://codereview.chromium.org/10701050/diff/24002/net/base/file_stream_posix...
net/base/file_stream_posix.cc:65: bool async_in_progress() { return
async_in_progress_; }
Ditto on const

[willchan no longer on Chromium, 2012-08-14 19:23:26]

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream.h#new...
net/base/file_stream.h:44: // Note: new FileStream object takes ownership of the
PlatformFile and will
Nit: grammar is a bit off. Either it should be "the new FileStream object" or
"new FileStream objects take"

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:62: bool record_uma() { return record_uma_; }
All accessors should be const member functions.

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:120: void OnAsyncCompleted(const
base::Callback<void(R)>& callback, R* result);
net/ convention is to name this function OnIOComplete().

I'm running off to lunch now, but you may want to look at
PostTaskAndReplyWithResult() instead of using a R* output param.

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:128: bool destroyed_;
I think you should change it so that destroyed_ is not protected by the lock.
That's race prone. Only access it on the network thread.

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:129: base::Lock net_log_lock_;
This looks wrong. Note that the BoundNetLog is an object that writes to an
underlying NetLog object, which is a browser process wide singleton. So even if
you lock access to the BoundNetLog, we will have races on the underlying NetLog
object.

The code needs to be changed so we never access BoundNetLog except on the
network thread.

[mmenke, 2012-08-14 20:31:01]

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:129: base::Lock net_log_lock_;
On 2012/08/14 19:23:26, willchan wrote:
> This looks wrong. Note that the BoundNetLog is an object that writes to an
> underlying NetLog object, which is a browser process wide singleton. So even
if
> you lock access to the BoundNetLog, we will have races on the underlying
NetLog
> object.
> 
> The code needs to be changed so we never access BoundNetLog except on the
> network thread.

It's safe to use a BoundNetLog on any thread, at least until the context is
destroyed (Actually lasts until the browser process is destroyed, but shouldn't
be reasoning about that in net/).

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:135: : file_(base::kInvalidPlatformFileValue),
These should be using a 4-space indent.

[willchan no longer on Chromium, 2012-08-14 20:34:12]

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:129: base::Lock net_log_lock_;
On 2012/08/14 20:31:01, Matt Menke wrote:
> On 2012/08/14 19:23:26, willchan wrote:
> > This looks wrong. Note that the BoundNetLog is an object that writes to an
> > underlying NetLog object, which is a browser process wide singleton. So even
> if
> > you lock access to the BoundNetLog, we will have races on the underlying
> NetLog
> > object.
> > 
> > The code needs to be changed so we never access BoundNetLog except on the
> > network thread.
> 
> It's safe to use a BoundNetLog on any thread, at least until the context is
> destroyed (Actually lasts until the browser process is destroyed, but
shouldn't
> be reasoning about that in net/).

The problem is that the helper threads being used are unjoined WorkerPool
threads, by design, since we don't want to slow down shutdown on file IO. That
means during browser process shutdown, the BoundNetLog may be getting accessed
while/after the NetLog singleton is destroyed.

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:135: : file_(base::kInvalidPlatformFileValue),
google style guide says to indent 4 spaces for initializer lists

[willchan no longer on Chromium, 2012-08-14 20:40:18]

OK, I read the windows code now too. It looks like my same general comments
apply there as well. I'm a bit disappointed there's so much duplication. I'll
read more later, but it'd be nice to see if we can consolidate the
implementations a bit in a FileStreamBase object or something that the platform
specific ones can compose. I'm not sure if that's possible due to the use of the
IOContext on Windows, but I'd need to read the code more.

[pivanof, 2012-08-14 21:04:24]

On 2012/08/14 20:40:18, willchan wrote:
> OK, I read the windows code now too. It looks like my same general comments
> apply there as well. I'm a bit disappointed there's so much duplication. I'll
> read more later, but it'd be nice to see if we can consolidate the
> implementations a bit in a FileStreamBase object or something that the
platform
> specific ones can compose. I'm not sure if that's possible due to the use of
the
> IOContext on Windows, but I'd need to read the code more.

I thought about this too. I should look closer but probably with this code
structure it's possible to make platform-independent FileStream and only
AsyncContext will have dependency on the platform. I can try to adjust CL for
this.

[pivanof, 2012-08-14 21:06:25]

On 2012/08/14 20:34:12, willchan wrote:
>
http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc
> File net/base/file_stream_posix.cc (right):
> 
>
http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
> net/base/file_stream_posix.cc:129: base::Lock net_log_lock_;
> On 2012/08/14 20:31:01, Matt Menke wrote:
> > On 2012/08/14 19:23:26, willchan wrote:
> > > This looks wrong. Note that the BoundNetLog is an object that writes to an
> > > underlying NetLog object, which is a browser process wide singleton. So
even
> > if
> > > you lock access to the BoundNetLog, we will have races on the underlying
> > NetLog
> > > object.
> > > 
> > > The code needs to be changed so we never access BoundNetLog except on the
> > > network thread.
> > 
> > It's safe to use a BoundNetLog on any thread, at least until the context is
> > destroyed (Actually lasts until the browser process is destroyed, but
> shouldn't
> > be reasoning about that in net/).
> 
> The problem is that the helper threads being used are unjoined WorkerPool
> threads, by design, since we don't want to slow down shutdown on file IO. That
> means during browser process shutdown, the BoundNetLog may be getting accessed
> while/after the NetLog singleton is destroyed.

But shouldn't it be true that FileStream object must be destroyed during
shutdown before NetLog singleton? If that order is guaranteed then code is
protected to not use NetLog after destruction of FileStream.

[willchan no longer on Chromium, 2012-08-14 21:15:04]

On Tue, Aug 14, 2012 at 2:06 PM, <paivanof@gmail.com> wrote:

> On 2012/08/14 20:34:12, willchan wrote:
>
> http://codereview.chromium.**org/10701050/diff/38001/net/**
>
base/file_stream_posix.cc<http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc>
>
>> File net/base/file_stream_posix.cc (right):
>>
>
>
> http://codereview.chromium.**org/10701050/diff/38001/net/**
>
base/file_stream_posix.cc#**newcode129<http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc#newcode129>
>
>> net/base/file_stream_posix.cc:**129: base::Lock net_log_lock_;
>> On 2012/08/14 20:31:01, Matt Menke wrote:
>> > On 2012/08/14 19:23:26, willchan wrote:
>> > > This looks wrong. Note that the BoundNetLog is an object that writes
>> to an
>> > > underlying NetLog object, which is a browser process wide singleton.
>> So
>>
> even
>
>> > if
>> > > you lock access to the BoundNetLog, we will have races on the
>> underlying
>> > NetLog
>> > > object.
>> > >
>> > > The code needs to be changed so we never access BoundNetLog except on
>> the
>> > > network thread.
>> >
>> > It's safe to use a BoundNetLog on any thread, at least until the
>> context is
>> > destroyed (Actually lasts until the browser process is destroyed, but
>> shouldn't
>> > be reasoning about that in net/).
>>
>
>  The problem is that the helper threads being used are unjoined WorkerPool
>> threads, by design, since we don't want to slow down shutdown on file IO.
>> That
>> means during browser process shutdown, the BoundNetLog may be getting
>> accessed
>> while/after the NetLog singleton is destroyed.
>>
>
> But shouldn't it be true that FileStream object must be destroyed during
> shutdown before NetLog singleton? If that order is guaranteed then code is
> protected to not use NetLog after destruction of FileStream.
>

Good point. You are indeed correct. Still, I'd prefer you not to use locks,
unless you have a strong reason why. Chromium code strongly prefers message
passing over locking.


>
>
http://codereview.chromium.**org/10701050/<http://codereview.chromium.org/107...
>

[pivanof, 2012-08-14 21:54:59]

On 2012/08/14 21:15:04, willchan wrote:
> On Tue, Aug 14, 2012 at 2:06 PM, <mailto:paivanof@gmail.com> wrote:
> 
> > On 2012/08/14 20:34:12, willchan wrote:
> >  The problem is that the helper threads being used are unjoined WorkerPool
> >> threads, by design, since we don't want to slow down shutdown on file IO.
> >> That
> >> means during browser process shutdown, the BoundNetLog may be getting
> >> accessed
> >> while/after the NetLog singleton is destroyed.
> >>
> >
> > But shouldn't it be true that FileStream object must be destroyed during
> > shutdown before NetLog singleton? If that order is guaranteed then code is
> > protected to not use NetLog after destruction of FileStream.
> >
> 
> Good point. You are indeed correct. Still, I'd prefer you not to use locks,
> unless you have a strong reason why. Chromium code strongly prefers message
> passing over locking.

Yes, I know that message passing is much more preferable in Chromimum than
locking. But passing messages back and forth just for a couple of calls to
NetLog sounds like overkill. Unless NetLog calls are pretty expensive of course.
Are they (it seems they shouldn't)?

[willchan no longer on Chromium, 2012-08-14 22:04:31]

On Tue, Aug 14, 2012 at 2:54 PM, <paivanof@gmail.com> wrote:

> On 2012/08/14 21:15:04, willchan wrote:
>
>  On Tue, Aug 14, 2012 at 2:06 PM, <mailto:paivanof@gmail.com> wrote:
>>
>
>  > On 2012/08/14 20:34:12, willchan wrote:
>> >  The problem is that the helper threads being used are unjoined
>> WorkerPool
>> >> threads, by design, since we don't want to slow down shutdown on file
>> IO.
>> >> That
>> >> means during browser process shutdown, the BoundNetLog may be getting
>> >> accessed
>> >> while/after the NetLog singleton is destroyed.
>> >>
>> >
>> > But shouldn't it be true that FileStream object must be destroyed during
>> > shutdown before NetLog singleton? If that order is guaranteed then code
>> is
>> > protected to not use NetLog after destruction of FileStream.
>> >
>>
>
>  Good point. You are indeed correct. Still, I'd prefer you not to use
>> locks,
>> unless you have a strong reason why. Chromium code strongly prefers
>> message
>> passing over locking.
>>
>
> Yes, I know that message passing is much more preferable in Chromimum than
> locking. But passing messages back and forth just for a couple of calls to
> NetLog sounds like overkill. Unless NetLog calls are pretty expensive of
> course.
> Are they (it seems they shouldn't)?
>

They are not expensive. And yeah, passing messages back and forth for just
a couple of calls may seem like overkill, but that's what we do. And we're
already passing messages between the network thread and the worker pool
threads, so all you have to do is add another param (probably a bound
callback that will do the write to the BoundNetLog).


>
>
http://codereview.chromium.**org/10701050/<http://codereview.chromium.org/107...
>

[pivanof, 2012-08-25 07:35:53]

Sorry for the long delay. I've put some of code shared between Win and Posix
into one place and got rid of the base::Lock. It turned out that code looks even
cleaner with this change, so thanks willchan for insisting on this idea. Please
take a look at the updated patch.

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:120: void OnAsyncCompleted(const
base::Callback<void(R)>& callback, R* result);
On 2012/08/14 19:23:26, willchan wrote:
> net/ convention is to name this function OnIOComplete().

There is already a function with this name in Win implementation and there is a
function with the same meaning as this one in Win implementation too. I wanted
to keep things symmetrical, so I needed to name the function here and in Win
with some different name.
But with new patch this should be moot as I needed to add one more function on
Linux that could be conveniently named OnIOCompleted().

> I'm running off to lunch now, but you may want to look at
> PostTaskAndReplyWithResult() instead of using a R* output param.

It looks like I can't use PostTaskAndReplyWithResult because there's no way to
get TaskRunner* from WorkerPool.

[willchan no longer on Chromium, 2012-08-26 22:33:59]

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/38001/net/base/file_stream_posix...
net/base/file_stream_posix.cc:120: void OnAsyncCompleted(const
base::Callback<void(R)>& callback, R* result);
On 2012/08/25 07:35:53, pivanof wrote:
> On 2012/08/14 19:23:26, willchan wrote:
> > net/ convention is to name this function OnIOComplete().
> 
> There is already a function with this name in Win implementation and there is
a
> function with the same meaning as this one in Win implementation too. I wanted
> to keep things symmetrical, so I needed to name the function here and in Win
> with some different name.
> But with new patch this should be moot as I needed to add one more function on
> Linux that could be conveniently named OnIOCompleted().
> 
> > I'm running off to lunch now, but you may want to look at
> > PostTaskAndReplyWithResult() instead of using a R* output param.
> 
> It looks like I can't use PostTaskAndReplyWithResult because there's no way to
> get TaskRunner* from WorkerPool.

http://code.google.com/searchframe#OAMlx_jo-ck/src/base/threading/worker_pool...
has a base::WorkerPool::GetTaskRunner().

[pivanof, 2012-08-27 00:13:00]

On 2012/08/26 22:33:59, willchan wrote:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/threading/worker_pool...
> has a base::WorkerPool::GetTaskRunner().

I thought TaskRunner is bound to some particular thread and can't be created for
WorkerPool...
I've updated CL to use PostTaskAndReplyWithResult().

[willchan no longer on Chromium, 2012-08-27 05:24:52]

On 2012/08/27 00:13:00, pivanof wrote:
> On 2012/08/26 22:33:59, willchan wrote:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/threading/worker_pool...
> > has a base::WorkerPool::GetTaskRunner().
> 
> I thought TaskRunner is bound to some particular thread and can't be created
for
> WorkerPool...
> I've updated CL to use PostTaskAndReplyWithResult().

TaskRunner is just an abstract interface for running tasks that get posted to
it. It has no constraints on threads. There are subclasses that do have
constraints, like SequencedTaskRunner or SingleThreadTaskRunner and what not.

PS: I'm traveling in Japan right now and will be going back to SF tomorrow. So
sorry for the delay.

[willchan no longer on Chromium, 2012-08-27 06:24:06]

http://codereview.chromium.org/10701050/diff/61002/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/10701050/diff/61002/net/base/file_stream.cc#ne...
net/base/file_stream.cc:6: #include "net/base/file_stream.h"
foo.h always goes first in foo.cc according to the style guide.

http://codereview.chromium.org/10701050/diff/61002/net/base/file_stream.cc#ne...
net/base/file_stream.cc:7: #if defined(OS_WIN)
Anything with platform specific definitions goes after the rest of the includes,
according to the chromium style guide.

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.cc#ne...
net/base/file_stream.cc:60: AsyncContext* old_ctx = context_;
s/old_ctx/old_context/

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.cc#ne...
net/base/file_stream.cc:80: int FileStream::OpenSync(const FilePath& path, int
open_flags) {
Since all the cross platform code is in file_stream.cc now (nice!), can you move
all the base::ThreadRestrictions::AssertIOAllowed() calls to the top of the
FooSync() methods? Once that's done, I believe we won't need AssertIOAllowed()
at all in the _posix and _win files, right?

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.h#new...
net/base/file_stream.h:215: class AsyncContext;
How about we just call it Context. It's not necessarily async, as you noted
below. Also, how about we wrap it within a scoped_ptr. Then if it's in sync
mode, just delete it. If it's in async mode, use
scoped_ptr<T>::release()->Orphan(). Rename |Context::destroyed_| to
|Context::orphaned_|.

I prefer this to Destroy(), since Destroy() has different behaviors based on
sync/async. I'd rather convey that FileStream generally owns the Context, by
putting it in a scoped_ptr and explicitly Orphan()'ing it when necessary. This
is done elsewhere in the codebase too, so there's precedence for it.

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.cc:262: if (destroyed_)
Remove this. It's arguably a benign race, but as Hans Boehm taught us, no data
race is truly benign since compilers can miscompile them:
http://static.usenix.org/event/hotpar11/tech/final_files/Boehm.pdf.

Once it's been posted to the WorkerPool thread, don't bother trying to cancel
it. It's OK to just check |orphaned_| in the reply callback.

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.cc:366: if (destroyed_) {
Change this to:
if (orphaned_) {
  delete this;
} ...

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.cc:375: void
FileStream::AsyncContext::DeleteAbandoned() {
One good thing about removing DeleteAbandoned is this is racy, since WorkerPool
is not tied to a single thread. It can spawn multiple threads, so this posted
task may run while other async tasks are running for the same context.

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix.h
File net/base/file_stream_posix.h (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.h:30: 
Define a destructor. The compiler generated destructor will mildly lead to code
bloat, since it must be inlined.

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.h:55: int64 GetFileSize();
const?

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.h:126: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.h
File net/base/file_stream_win.h (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.h...
net/base/file_stream_win.h:30: int open_flags);
Define the destructor here too.

[willchan no longer on Chromium, 2012-08-27 06:59:10]

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.cc:70: void FileStream::AsyncContext::Destroy() {
Oops, earlier I neglected to mention how this bit would have to change if we
switched to Orphan(). Here, we wouldn't do DeleteAbandoned() anymore. But in all
the places below where the async IO completes, we would need to check
|orphaned_| and then, if the file were still open, begin a CloseAsync()
operation. If the file is not open, then we can just delete this. Note that when
we post the CloseAsync() operation, and that completes, then we'll again check
|orphaned_| and then delete this.

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.c...
net/base/file_stream_win.cc:75: CancelIo(file_);
Is this correct? What happens when |file_| is invalid? Is there a test that
covers this case?

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.c...
net/base/file_stream_win.cc:360: if (destroyed_)
Remove this

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.h
File net/base/file_stream_win.h (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.h...
net/base/file_stream_win.h:56: int64 GetFileSize();
make this const too

[pivanof, 2012-08-27 08:43:30]

willchan, I'm not sure that I understood your idea with orphaning exactly as you
intended but I implemented what I understood and agreed with. Also I implemented
all other changes. Please take a look.
Also could you please submit a try on Win to see that at least everything
compiles correctly in there? I don't have access to any Windows machine ATM.

http://codereview.chromium.org/10701050/diff/61002/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/10701050/diff/61002/net/base/file_stream.cc#ne...
net/base/file_stream.cc:7: #if defined(OS_WIN)
On 2012/08/27 06:24:06, willchan wrote:
> Anything with platform specific definitions goes after the rest of the
includes,
> according to the chromium style guide.

I did exactly as you said initially but lint said here that it's an error, so I
tried to move it. Let's move it back. :)

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.cc#ne...
net/base/file_stream.cc:80: int FileStream::OpenSync(const FilePath& path, int
open_flags) {
On 2012/08/27 06:24:06, willchan wrote:
> Since all the cross platform code is in file_stream.cc now (nice!),

There are still some similarities between _posix and _win which probably are
better to be in one place. And I just got an idea: will it be okay if
FileStream::Context will be declared in _posix.h and _win.h, platform dependent
methods of Context will be defined in _posix.cc and _win.cc but common methods
will be defined in file_stream.cc?

> can you move
> all the base::ThreadRestrictions::AssertIOAllowed() calls to the top of the
> FooSync() methods? Once that's done, I believe we won't need AssertIOAllowed()
> at all in the _posix and _win files, right?

Hm... I agree that it's certainly a good idea to put AssertIOAllowed() in front
of all FooSync() methods here. But if we remove all calls to AssertIOAllowed()
from platform files then we will rely on the fact that certain methods are
called either from FooSync() or from WorkerPool. Is it okay to rely that there
won't be some accidental change later breaking this assumption?

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.cc:375: void
FileStream::AsyncContext::DeleteAbandoned() {
On 2012/08/27 06:24:06, willchan wrote:
> One good thing about removing DeleteAbandoned is this is racy, since
WorkerPool
> is not tied to a single thread. It can spawn multiple threads, so this posted
> task may run while other async tasks are running for the same context.

I don't follow. There are DCHECKs everywhere to ensure that at least in the
debug build there is only one async operation in progress. If there's several
async operations in progress there will be several races here and there. Are you
really talking about such situation?

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_win.c...
net/base/file_stream_win.cc:75: CancelIo(file_);
On 2012/08/27 06:59:10, willchan wrote:
> Is this correct? What happens when |file_| is invalid? Is there a test that
> covers this case?

Any test working in sync mode should trip over this. I think nothing bad will
happen though -- just CancelIo() will return error which is not checked anyway.
But I agree that it's better be fixed.

[willchan no longer on Chromium, 2012-09-05 00:03:38]

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream.cc#ne...
net/base/file_stream.cc:80: int FileStream::OpenSync(const FilePath& path, int
open_flags) {
On 2012/08/27 08:43:31, pivanof wrote:
> On 2012/08/27 06:24:06, willchan wrote:
> > Since all the cross platform code is in file_stream.cc now (nice!),
> 
> There are still some similarities between _posix and _win which probably are
> better to be in one place. And I just got an idea: will it be okay if
> FileStream::Context will be declared in _posix.h and _win.h, platform
dependent
> methods of Context will be defined in _posix.cc and _win.cc but common methods
> will be defined in file_stream.cc?

I see. We generally try to prevent separation of code like this, although we do
do it in base/ in a few places. That's primarily because lots of that code is
not object based. That said, if you think it will make the code cleaner, I'm
open to it.

> 
> > can you move
> > all the base::ThreadRestrictions::AssertIOAllowed() calls to the top of the
> > FooSync() methods? Once that's done, I believe we won't need
AssertIOAllowed()
> > at all in the _posix and _win files, right?
> 
> Hm... I agree that it's certainly a good idea to put AssertIOAllowed() in
front
> of all FooSync() methods here. But if we remove all calls to AssertIOAllowed()
> from platform files then we will rely on the fact that certain methods are
> called either from FooSync() or from WorkerPool. Is it okay to rely that there
> won't be some accidental change later breaking this assumption?

Please correct me if I'm misunderstanding, but all the entrypoints to the
platform specific files are from FileStream, right? If that's true, and there's
no other way to enter this code, I'd rather have the assertion done at the
obvious entrypoints in FileStream, since it makes it more easily verifiable that
all the sync methods have AssertIOAllowed().

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/60006/net/base/file_stream_posix...
net/base/file_stream_posix.cc:375: void
FileStream::AsyncContext::DeleteAbandoned() {
On 2012/08/27 08:43:31, pivanof wrote:
> On 2012/08/27 06:24:06, willchan wrote:
> > One good thing about removing DeleteAbandoned is this is racy, since
> WorkerPool
> > is not tied to a single thread. It can spawn multiple threads, so this
posted
> > task may run while other async tasks are running for the same context.
> 
> I don't follow. There are DCHECKs everywhere to ensure that at least in the
> debug build there is only one async operation in progress. If there's several
> async operations in progress there will be several races here and there. Are
you
> really talking about such situation?

You're right, there should not be multiple async operations at the same time.
The thing though is, DeleteAbandoned() may be triggered by the deletion of the
FileStream object while an async operation is in program. This means this
CloseFileImpl() and an async operation may be concurrently executing on the
WorkerPool, which is a race condition.

[willchan no longer on Chromium, 2012-09-05 00:15:48]

http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream.cc#ne...
net/base/file_stream.cc:36: CloseSync();
Can you change this to:
if (IsOpen() && !is_async()) {
  CloseSync();
  context_.reset();
} else {
  context_.release()->Orphan();
}

http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream.cc#ne...
net/base/file_stream.cc:48: // CloseSync() should be called on the correct
thread even if it eventually
I think this comment is unnecessary.

http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream_posix...
net/base/file_stream_posix.cc:73: void FileStream::Context::Orphan() {
I would change this to DCHECK(async_in_progress);

http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream_posix.h
File net/base/file_stream_posix.h (right):

http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream_posix...
net/base/file_stream_posix.h:28: const BoundNetLog& bound_net_log,
Indentation is off

[pivanof, 2012-09-05 08:41:18]

On 2012/09/05 00:03:38, willchan wrote:
> I see. We generally try to prevent separation of code like this, although we
do
> do it in base/ in a few places. That's primarily because lots of that code is
> not object based. That said, if you think it will make the code cleaner, I'm
> open to it.

I think it will avoid lots of code duplication and copy-pasting. See my last
patch set with an attempt to do this. There are still Context::SeekAsync() and
SeekSync() methods that I'd like to extract into multi-platform code but I can't
figure out how to do that yet (unfortunately it's impossible to define templates
in one .cc file and instantiate them in another).

> Please correct me if I'm misunderstanding, but all the entrypoints to the
> platform specific files are from FileStream, right? If that's true, and
there's
> no other way to enter this code, I'd rather have the assertion done at the
> obvious entrypoints in FileStream, since it makes it more easily verifiable
that
> all the sync methods have AssertIOAllowed().

Yes, you are right. My concern was that later code of FileStream will change and
something will get messed up with threads. But probably you are right, cleaner
code here is better than protection from incorrect changes that should be caught
during review.

> > I don't follow. There are DCHECKs everywhere to ensure that at least in the
> > debug build there is only one async operation in progress. If there's
several
> > async operations in progress there will be several races here and there. Are
> you
> > really talking about such situation?
> 
> You're right, there should not be multiple async operations at the same time.
> The thing though is, DeleteAbandoned() may be triggered by the deletion of the
> FileStream object while an async operation is in program. This means this
> CloseFileImpl() and an async operation may be concurrently executing on the
> WorkerPool, which is a race condition.

Actually the code was protected and DeleteAbandoned() couldn't be called from
FileStream destructor if some async operation is in progress... But anyway
there's no such function now and this argument should be moot.

On 2012/09/05 00:15:48, willchan wrote:
> http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream.cc
> File net/base/file_stream.cc (right):
> 
>
http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream.cc#ne...
> net/base/file_stream.cc:36: CloseSync();
> Can you change this to:
> if (IsOpen() && !is_async()) {
>   CloseSync();
>   context_.reset();
> } else {
>   context_.release()->Orphan();
> }

After some thought I've realized I can do that indeed.

>
http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream_posix.cc
> File net/base/file_stream_posix.cc (right):
> 
>
http://codereview.chromium.org/10701050/diff/61006/net/base/file_stream_posix...
> net/base/file_stream_posix.cc:73: void FileStream::Context::Orphan() {
> I would change this to DCHECK(async_in_progress);

No, Orphan() can be entered when no async operation is in progress. It will
happen when FileStream is opened in async mode and destroyed without explicit
call to Close(). I.e. FileStream should close itself but destructor cannot wait
for that operation to complete and thus will orphan the Context.

[willchan no longer on Chromium, 2012-09-05 23:20:31]

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
net/base/file_stream.cc:22: void FileStream::Context::Orphan() {
DCHECK(!orphaned_);

We shouldn't call this twice.

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
net/base/file_stream.cc:54: int FileStream::Context::OpenSync(const FilePath&
path, int open_flags) {
DCHECK(!async_in_progress_);

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
net/base/file_stream.cc:71: MessageLoop::current()->PostTask(
base::MessageLoopProxy::current()->PostTask(

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
net/base/file_stream.cc:129: void FileStream::Context::CheckForOpenError(int*
result) {
What do you think about, instead of having a single input / output param, having
a return value and an input param? That'd be my personal stylistic preference,
but I'm fine with what you want.

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
net/base/file_stream.cc:140: RegisterInMessageLoop();
Can we name this differently? Perhaps something like OnOpenCompletedInternal().
I would have said OnOpenCompletedImpl(), but that has a different meaning here.
I basically want a platform specific function name, that doesn't reference
implementation details.

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
net/base/file_stream.cc:141: OnAsyncCompleted(callback, result);
Why isn't OnAsyncCompleted defined here in file_stream.cc? It looks duplicated
in the platform specific files.

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
net/base/file_stream.cc:144: void FileStream::Context::CloseFileImpl() {
How would you feel about getting rid of CloseFileImpl() and just using
base::ClosePlatformFile() instead? Then the assignment of file_ to
base::kInvalidPlatformFileValue would be done in CloseSync() and
OnCloseCompleted(). The reason I prefer this is because that keeps the use of
|file_| to the network thread. The less we access the |this| pointer on the
WorkerPool, the better, as it will reduce confusion and likelihood of race
conditions.

Now that I think about this, a similar comment may also apply to OpenFileImpl
and OnOpenCompleted. Perhaps we can change the result to be std::pair<int,
base::PlatformFile>? That way we can similarly avoid dereferencing the |this|
pointer on the WorkerPool.

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.h#new...
net/base/file_stream.h:223: // to perform asynchronous operations correctly.
This should explain why it's necessary for correctness. In particular, when
FileStream itself is destroyed, an asynchronous operation may still be running.

[pivanof, 2012-09-05 23:35:46]

On 2012/09/05 23:20:31, willchan wrote:
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> net/base/file_stream.cc:129: void FileStream::Context::CheckForOpenError(int*
> result) {
> What do you think about, instead of having a single input / output param,
having
> a return value and an input param? That'd be my personal stylistic preference,
> but I'm fine with what you want.

Sounds good to me. Probably I'll need to name the function differently though in
this case. I'll think about it.

>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> net/base/file_stream.cc:141: OnAsyncCompleted(callback, result);
> Why isn't OnAsyncCompleted defined here in file_stream.cc? It looks duplicated
> in the platform specific files.

Because OnAsyncCompleted is template. And I can't define template in one .cc
file and instantiate it in another.
I want this function to be unified too but I didn't come with an idea how to do
that yet.

>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> net/base/file_stream.cc:144: void FileStream::Context::CloseFileImpl() {
> How would you feel about getting rid of CloseFileImpl() and just using
> base::ClosePlatformFile() instead? Then the assignment of file_ to
> base::kInvalidPlatformFileValue would be done in CloseSync() and
> OnCloseCompleted(). The reason I prefer this is because that keeps the use of
> |file_| to the network thread. The less we access the |this| pointer on the
> WorkerPool, the better, as it will reduce confusion and likelihood of race
> conditions.
> 
> Now that I think about this, a similar comment may also apply to OpenFileImpl
> and OnOpenCompleted. Perhaps we can change the result to be std::pair<int,
> base::PlatformFile>? That way we can similarly avoid dereferencing the |this|
> pointer on the WorkerPool.

Won't it be too difficult to read the code with such return type? I'm not
strongly opposed to such solution (although I'd prefer some specifically defined
struct instead of generic std::pair) but I thought accessing |this| is much
easier to read and understand than passing such complicated values. Although I
agree it can cause some confusion and doubt about thread-safety (as was the case
e.g. with call to CancelIo() ).

[willchan no longer on Chromium, 2012-09-06 00:25:02]

On 2012/09/05 23:35:46, pivanof wrote:
> On 2012/09/05 23:20:31, willchan wrote:
> >
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> > net/base/file_stream.cc:129: void
FileStream::Context::CheckForOpenError(int*
> > result) {
> > What do you think about, instead of having a single input / output param,
> having
> > a return value and an input param? That'd be my personal stylistic
preference,
> > but I'm fine with what you want.
> 
> Sounds good to me. Probably I'll need to name the function differently though
in
> this case. I'll think about it.
> 
> >
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> > net/base/file_stream.cc:141: OnAsyncCompleted(callback, result);
> > Why isn't OnAsyncCompleted defined here in file_stream.cc? It looks
duplicated
> > in the platform specific files.
> 
> Because OnAsyncCompleted is template. And I can't define template in one .cc
> file and instantiate it in another.
> I want this function to be unified too but I didn't come with an idea how to
do
> that yet.
> 
> >
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> > net/base/file_stream.cc:144: void FileStream::Context::CloseFileImpl() {
> > How would you feel about getting rid of CloseFileImpl() and just using
> > base::ClosePlatformFile() instead? Then the assignment of file_ to
> > base::kInvalidPlatformFileValue would be done in CloseSync() and
> > OnCloseCompleted(). The reason I prefer this is because that keeps the use
of
> > |file_| to the network thread. The less we access the |this| pointer on the
> > WorkerPool, the better, as it will reduce confusion and likelihood of race
> > conditions.
> > 
> > Now that I think about this, a similar comment may also apply to
OpenFileImpl
> > and OnOpenCompleted. Perhaps we can change the result to be std::pair<int,
> > base::PlatformFile>? That way we can similarly avoid dereferencing the
|this|
> > pointer on the WorkerPool.
> 
> Won't it be too difficult to read the code with such return type? I'm not
> strongly opposed to such solution (although I'd prefer some specifically
defined
> struct instead of generic std::pair) but I thought accessing |this| is much
> easier to read and understand than passing such complicated values. Although I
> agree it can cause some confusion and doubt about thread-safety (as was the
case
> e.g. with call to CancelIo() ).

I concede it will be more difficult to read the code with such a return type,
but I think the win in terms of easier understanding of threading is worth it.
Having debugged many threading issues in Chromium, my I've grown very wary of
accessing variables on multiple threads and am willing to trade off a fair
amount of readability in order to make threading correctness more easily
verifiable.

[willchan no longer on Chromium, 2012-09-06 00:45:20]

On 2012/09/05 23:35:46, pivanof wrote:
> On 2012/09/05 23:20:31, willchan wrote:
> >
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> > net/base/file_stream.cc:129: void
FileStream::Context::CheckForOpenError(int*
> > result) {
> > What do you think about, instead of having a single input / output param,
> having
> > a return value and an input param? That'd be my personal stylistic
preference,
> > but I'm fine with what you want.
> 
> Sounds good to me. Probably I'll need to name the function differently though
in
> this case. I'll think about it.
> 
> >
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> > net/base/file_stream.cc:141: OnAsyncCompleted(callback, result);
> > Why isn't OnAsyncCompleted defined here in file_stream.cc? It looks
duplicated
> > in the platform specific files.
> 
> Because OnAsyncCompleted is template. And I can't define template in one .cc
> file and instantiate it in another.
> I want this function to be unified too but I didn't come with an idea how to
do
> that yet.

Your explanation mostly makes sense, although I don't see how file_stream.cc is
able to instantiate OnAsyncCompleted() since it's defined in
file_stream_{posix|win}.cc.

Are there any other bits of code that we'd want to share? If there are
significantly more, then perhaps it's worth it to introduce more composition
(have a FileStreamContextBase that FileStream::Context composes). Otherwise, the
minor code duplication here might be the best tradeoff.

> 
> >
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> > net/base/file_stream.cc:144: void FileStream::Context::CloseFileImpl() {
> > How would you feel about getting rid of CloseFileImpl() and just using
> > base::ClosePlatformFile() instead? Then the assignment of file_ to
> > base::kInvalidPlatformFileValue would be done in CloseSync() and
> > OnCloseCompleted(). The reason I prefer this is because that keeps the use
of
> > |file_| to the network thread. The less we access the |this| pointer on the
> > WorkerPool, the better, as it will reduce confusion and likelihood of race
> > conditions.
> > 
> > Now that I think about this, a similar comment may also apply to
OpenFileImpl
> > and OnOpenCompleted. Perhaps we can change the result to be std::pair<int,
> > base::PlatformFile>? That way we can similarly avoid dereferencing the
|this|
> > pointer on the WorkerPool.
> 
> Won't it be too difficult to read the code with such return type? I'm not
> strongly opposed to such solution (although I'd prefer some specifically
defined
> struct instead of generic std::pair) but I thought accessing |this| is much
> easier to read and understand than passing such complicated values. Although I
> agree it can cause some confusion and doubt about thread-safety (as was the
case
> e.g. with call to CancelIo() ).

[pivanof, 2012-09-06 08:08:23]

On 2012/09/06 00:45:20, willchan wrote:
>
http://codereview.chromium.org/10701050/diff/77001/net/base/file_stream.cc#ne...
> > > net/base/file_stream.cc:141: OnAsyncCompleted(callback, result);
> > > Why isn't OnAsyncCompleted defined here in file_stream.cc? It looks
> duplicated
> > > in the platform specific files.
> > 
> > Because OnAsyncCompleted is template. And I can't define template in one .cc
> > file and instantiate it in another.
> > I want this function to be unified too but I didn't come with an idea how to
> do
> > that yet.
> 
> Your explanation mostly makes sense, although I don't see how file_stream.cc
is
> able to instantiate OnAsyncCompleted() since it's defined in
> file_stream_{posix|win}.cc.

I guess I was generating a link in file_stream.cc to a particular instantiation
of OnAsyncCompleted() and I was lucky that file_stream_posix.cc needed the same
instantiation too. So it generated its implementation and thus file_stream.cc
was able to successfully link in the end. I'd bet file_stream.cc wasn't able to
link on Windows...

But anyway I found a way to get rid of these templates, unify final pieces and
remove duplication from file_stream_{posix|win}.cc completely. Please take a
look (I addressed all your suggestions as well).

And could you please submit a try at least for Windows to see if it compiles
correctly?

[willchan no longer on Chromium, 2012-09-11 23:05:02]

OK, there are definitely some bugs that the tests uncovered when I hit the
tryservers. Check them out :)

http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream.h
File net/base/file_stream.h (right):

http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream.h#new...
net/base/file_stream.h:217: bool is_async() { return !!(open_flags_ &
base::PLATFORM_FILE_ASYNC); }
make this const?

http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_posix.h
File net/base/file_stream_posix.h (right):

http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_posix...
net/base/file_stream_posix.h:84: // Stubs to make more of the Context code
multi-platform.
Can you use banner comments to indicate which section is used by file_stream.cc
and which section is local to this specific implementation? I thought a lot
about this, and I've come around to your point of view that things are clearer
when we share more stuff in file_stream.cc, but it was a bit confusing when I
saw GetLastErrno() and was trying to figure out who called it.

http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_win.h
File net/base/file_stream_win.h (right):

http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_win.h...
net/base/file_stream_win.h:29: const BoundNetLog& bound_net_log,
Indentation

http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_win.h...
net/base/file_stream_win.h:75: kErrorBadFile = ERROR_INVALID_HANDLE
There's a fun thread on chromium-dev about enum style right now :) I think you
should switch it to MACRO_NAMING_STYLE in absence of any decision on the thread,
since that's what the Chromium style guide mandates.

[pivanof, 2012-09-12 06:52:44]

On 2012/09/11 23:05:02, willchan wrote:
> OK, there are definitely some bugs that the tests uncovered when I hit the
> tryservers. Check them out :)

Boo... Now I have to chase all the tests that destroy FileStream and don't wait
for the Task posted from destructor to complete. All these tests report leak in
valgrind now. :(
Other than that I've fixed "obvious" problem in FileStream and build error on
Win. A couple of failures don't look related in any way, hopefully they will be
gone with next run. Could you please run it?

I have a question related to this: can base::MessageLoopProxy::current() be NULL
when FileStream::CloseAsync() is called? If yes then should I just forget about
callback I'm supposed to call in this case?

>
http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_posix.h
> File net/base/file_stream_posix.h (right):
> 
>
http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_posix...
> net/base/file_stream_posix.h:84: // Stubs to make more of the Context code
> multi-platform.
> Can you use banner comments to indicate which section is used by
file_stream.cc
> and which section is local to this specific implementation? I thought a lot
> about this, and I've come around to your point of view that things are clearer
> when we share more stuff in file_stream.cc, but it was a bit confusing when I
> saw GetLastErrno() and was trying to figure out who called it.

Please look at the current format, how does that look? It violates style guide
slightly but looks much more readable.

> http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_win.h
> File net/base/file_stream_win.h (right):
> 
>
http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_win.h...
> net/base/file_stream_win.h:75: kErrorBadFile = ERROR_INVALID_HANDLE
> There's a fun thread on chromium-dev about enum style right now :) I think you
> should switch it to MACRO_NAMING_STYLE in absence of any decision on the
thread,
> since that's what the Chromium style guide mandates.

Yes, that's a "fun" thread. :) I've changed the style of this constant for now.
But honestly with base::kInvalidPlatformFileValue all over the place
kErrorBadFile looks much more consistent... Although kInvalidPlatformFileValue
is not an enum, it's constant...

[willchan no longer on Chromium, 2012-09-12 17:49:04]

On 2012/09/12 06:52:44, pivanof wrote:
> On 2012/09/11 23:05:02, willchan wrote:
> > OK, there are definitely some bugs that the tests uncovered when I hit the
> > tryservers. Check them out :)
> 
> Boo... Now I have to chase all the tests that destroy FileStream and don't
wait
> for the Task posted from destructor to complete. All these tests report leak
in
> valgrind now. :(

Suppress those. FileStream by design uses WorkerPool, which uses unjoined
threads. Anything running on an unjoined thread on shutdown is a "leak"
according to valgrind. It looks to me like we aren't correctly suppressing the
WorkerPool leaks for unit_tests and content_unittests like we do for
net_unittests and others.

> Other than that I've fixed "obvious" problem in FileStream and build error on
> Win. A couple of failures don't look related in any way, hopefully they will
be
> gone with next run. Could you please run it?

I've kicked off the next run.

> 
> I have a question related to this: can base::MessageLoopProxy::current() be
NULL
> when FileStream::CloseAsync() is called? If yes then should I just forget
about
> callback I'm supposed to call in this case?

Assume it can't be NULL.

> 
> >
>
http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_posix.h
> > File net/base/file_stream_posix.h (right):
> > 
> >
>
http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_posix...
> > net/base/file_stream_posix.h:84: // Stubs to make more of the Context code
> > multi-platform.
> > Can you use banner comments to indicate which section is used by
> file_stream.cc
> > and which section is local to this specific implementation? I thought a lot
> > about this, and I've come around to your point of view that things are
clearer
> > when we share more stuff in file_stream.cc, but it was a bit confusing when
I
> > saw GetLastErrno() and was trying to figure out who called it.
> 
> Please look at the current format, how does that look? It violates style guide
> slightly but looks much more readable.

I know you've done a lot of work splitting things up in this way, but I'm
starting to get doubts about inconsistency with the codebase now. I'm going to
consult wtc for his opinion. I don't want to singlehandedly approve something
that others will find frustrating to maintain. If I do ask you to change it back
to basically having code duplication in the posix and windows implementations,
I'm sorry for having you redo it yet again. I'll get back to you on whether
that's necessary.

> 
> >
http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_win.h
> > File net/base/file_stream_win.h (right):
> > 
> >
>
http://codereview.chromium.org/10701050/diff/81002/net/base/file_stream_win.h...
> > net/base/file_stream_win.h:75: kErrorBadFile = ERROR_INVALID_HANDLE
> > There's a fun thread on chromium-dev about enum style right now :) I think
you
> > should switch it to MACRO_NAMING_STYLE in absence of any decision on the
> thread,
> > since that's what the Chromium style guide mandates.
> 
> Yes, that's a "fun" thread. :) I've changed the style of this constant for
now.
> But honestly with base::kInvalidPlatformFileValue all over the place
> kErrorBadFile looks much more consistent... Although kInvalidPlatformFileValue
> is not an enum, it's constant...

Yeah, I know what you mean, but let's not go there :)

[pivanof, 2012-09-12 22:31:17]

willchan, could you please take a look at test failures in try jobs (except
valgrind leaks)? I see AsyncRead_EarlyClose on win_rel bot that should be fixed,
and I don't understand everything else. What's wrong with
RtlInitializeExceptionChain on win_rel? What this failure is about? Are all
other failures unrelated and are safe to ignore?

[willchan no longer on Chromium, 2012-09-12 23:05:13]

RtlInitializeExceptionChain() is the Windows exception handler. We
unfortunately do not get symbolized stacktraces on Windows. It's unclear to
me if this is expected or not. Oftentimes developers just re-run tryjobs
and see if it goes away, or just look at the tryserver waterfall (
http://build.chromium.org/p/tryserver.chromium/builders/win_rel and
http://build.chromium.org/p/tryserver.chromium/waterfall) to see if others
are encountering this.

If no one else is encountering this and it doesn't go away, that's a signal
that it's an issue with your patch, at which point you need to fire up a
Windows build yourself to debug, or ask a nice windows dev on irc to do it
for you.


On Wed, Sep 12, 2012 at 3:31 PM, <paivanof@gmail.com> wrote:

> willchan, could you please take a look at test failures in try jobs (except
> valgrind leaks)? I see AsyncRead_EarlyClose on win_rel bot that should be
> fixed,
> and I don't understand everything else. What's wrong with
> RtlInitializeExceptionChain on win_rel? What this failure is about? Are all
> other failures unrelated and are safe to ignore?
>
>
http://codereview.chromium.**org/10701050/<http://codereview.chromium.org/107...
>

[willchan no longer on Chromium, 2012-09-12 23:28:06]

So, I talked to wtc. We came to the conclusion that we'd rather use a
single class called FileStream::Context, declared in file_stream_context.h,
with implementations in file_stream_context.cc,
file_stream_context_posix.cc, and file_stream_context_win.cc.
file_stream_context.h obviously would need a number of platform ifdefs.

I looked at the Windows implementation and realized that we're actually
doing Windows async I/O suboptimally. We should use CancelIo() to cancel
the async operations instead of waiting for them to complete before we do
CloseAsync(). This would clearly have some implications on the cross
platform code, primarily with regard to Orphan(), since in the Windows
case, we'd CancelIo() and CloseAsync(), rather than possibly wait for an
async operation to complete. This would require distinguishing cancelable
async operations vs close operations. Alternatively, we could completely
remove the Close() API, and make you always delete FileStream. I don't
think there's much advantage to reusing the existing FileStream object
rather than just creating a new one.


On Wed, Sep 12, 2012 at 4:05 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> RtlInitializeExceptionChain() is the Windows exception handler. We
> unfortunately do not get symbolized stacktraces on Windows. It's unclear to
> me if this is expected or not. Oftentimes developers just re-run tryjobs
> and see if it goes away, or just look at the tryserver waterfall (
> http://build.chromium.org/p/tryserver.chromium/builders/win_rel and
> http://build.chromium.org/p/tryserver.chromium/waterfall) to see if
> others are encountering this.
>
> If no one else is encountering this and it doesn't go away, that's a
> signal that it's an issue with your patch, at which point you need to fire
> up a Windows build yourself to debug, or ask a nice windows dev on irc to
> do it for you.
>
>
> On Wed, Sep 12, 2012 at 3:31 PM, <paivanof@gmail.com> wrote:
>
>> willchan, could you please take a look at test failures in try jobs
>> (except
>> valgrind leaks)? I see AsyncRead_EarlyClose on win_rel bot that should be
>> fixed,
>> and I don't understand everything else. What's wrong with
>> RtlInitializeExceptionChain on win_rel? What this failure is about? Are
>> all
>> other failures unrelated and are safe to ignore?
>>
>>
http://codereview.chromium.**org/10701050/<http://codereview.chromium.org/107...
>>
>
>

[pivanof, 2012-09-16 04:59:55]

On 2012/09/12 23:28:06, willchan wrote:
> Alternatively, we could completely
> remove the Close() API, and make you always delete FileStream. I don't
> think there's much advantage to reusing the existing FileStream object
> rather than just creating a new one.

That's a very interesting idea. Did you mean just Close() here or CloseSync()
too? I've checked and to my great surprise I didn't find any usage of
FileStream::Close() method outside of file_stream_unittests.cc. CloseSync() is
used either right before stream deletion or in a place where stream is
apparently not needed anymore, so if FileStream member is changed to
scoped_ptr<FileStream> then it looks like it can be safely deleted.
Do you want me to make all these changes in this CL? It doesn't look like there
will be too many of them.

[willchan no longer on Chromium, 2012-09-17 17:40:45]

On 2012/09/16 04:59:55, pivanof wrote:
> On 2012/09/12 23:28:06, willchan wrote:
> > Alternatively, we could completely
> > remove the Close() API, and make you always delete FileStream. I don't
> > think there's much advantage to reusing the existing FileStream object
> > rather than just creating a new one.
> 
> That's a very interesting idea. Did you mean just Close() here or CloseSync()
> too? I've checked and to my great surprise I didn't find any usage of
> FileStream::Close() method outside of file_stream_unittests.cc. CloseSync() is
> used either right before stream deletion or in a place where stream is
> apparently not needed anymore, so if FileStream member is changed to
> scoped_ptr<FileStream> then it looks like it can be safely deleted.
> Do you want me to make all these changes in this CL? It doesn't look like
there
> will be too many of them.

Yeah, I think it'd be great to kill off both of them.

[pivanof, 2012-09-18 23:34:44]

willchan, I've updated the patch (why doesn't it show file deletions?). Please
take a look and submit a try. Also while submitting tries could you please add
more Win trybots? Maybe others will show me more info about what's wrong with
win side of the patch.

[willchan no longer on Chromium, 2012-09-20 18:33:42]

On 2012/09/18 23:34:44, pivanof wrote:
> willchan, I've updated the patch (why doesn't it show file deletions?). Please
> take a look and submit a try. Also while submitting tries could you please add
> more Win trybots? Maybe others will show me more info about what's wrong with
> win side of the patch.

The tryjobs are failing to patch. You probably need to rebase your patch.

[pivanof, 2012-09-20 18:39:44]

On 2012/09/20 18:33:42, willchan wrote:
> The tryjobs are failing to patch. You probably need to rebase your patch.

I've just did 'gclient sync' and I didn't get any conflicts. Could it be that my
'svn cp' was made on some older revision and after updating it doesn't redo that
while trybots are doing 'svn cp' on current revision and get conflict? Should I
make a clean checkout and then reapply the patch on top of it to fix that? Or
maybe I need just to forget about svn hostiry preservation?

[willchan no longer on Chromium, 2012-09-20 18:46:38]

On 2012/09/20 18:39:44, pivanof wrote:
> On 2012/09/20 18:33:42, willchan wrote:
> > The tryjobs are failing to patch. You probably need to rebase your patch.
> 
> I've just did 'gclient sync' and I didn't get any conflicts. Could it be that
my
> 'svn cp' was made on some older revision and after updating it doesn't redo
that
> while trybots are doing 'svn cp' on current revision and get conflict? Should
I
> make a clean checkout and then reapply the patch on top of it to fix that? Or
> maybe I need just to forget about svn hostiry preservation?

No, we want to preserve the history. I don't know, I'm not an svn wizard (I use
git). If you have issues, you could probably ask maruel on irc. I'm going to
kick off another attempt. The output seems to indicate that the source file of
the patch doesn't exist, so it indeed seems not to pick up the svn cp or
something (even though that metadata is at the top of the patch).

[pivanof, 2012-09-20 20:43:00]

I rebased, fixed conflict in suppressions.txt and uploaded the patch again. It
looks much better now (Rietveld actually shows necessary file deletions). Maybe
it will work now. willchan, could you try it? And maybe you should try to do it
from Rietveld UI?

[pivanof, 2012-09-30 23:26:06]

Okay, the rest of tryjob failures look completely unrelated. So willchan, please
review it.

[willchan no longer on Chromium, 2012-10-01 04:45:18]

FYI I'm going to be out at the chrome networking offsite all week! That
means all the reviewers are going to disappear too :) I'll try to find some
time in between to review this.


On Sun, Sep 30, 2012 at 4:26 PM, <paivanof@gmail.com> wrote:

> Okay, the rest of tryjob failures look completely unrelated. So willchan,
> please
> review it.
>
>
http://codereview.chromium.**org/10701050/<http://codereview.chromium.org/107...
>

[willchan no longer on Chromium, 2012-10-28 05:58:14]

Sorry Pavel, put away your shotgun, I'll look at this on Monday for sure :)

http://codereview.chromium.org/10701050/diff/111001/net/base/file_stream_cont...
File net/base/file_stream_context.h (right):

http://codereview.chromium.org/10701050/diff/111001/net/base/file_stream_cont...
net/base/file_stream_context.h:5: // This file implements FileStream for
Windows.
No longer true

[willchan no longer on Chromium, 2012-10-30 18:01:26]

OK, I'm basically done with this review. Do you remember all the things we
discussed for future followup work here? I'd like to consolidate everything in
bugs if possible, especially if you get busy and can't finish everything. Do you
mind filing bugs if they don't already exist?

I really appreciate all your help here Pavel, thanks :)

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
File net/base/file_stream_context.cc (right):

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
net/base/file_stream_context.cc:30:
bound_net_log_.EndEvent(net::NetLog::TYPE_FILE_STREAM_OPEN);
No need to use net:: when within net already

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
File net/base/file_stream_context.h (right):

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
net/base/file_stream_context.h:5: // This file implements FileStream for
Windows.
Wrong :)

Please update this with a large explanation of the implementation architecture.
Explain that this contains the bulk of the implementation for FileStream, in a
detached class so that the FileStream object can go away while async operations
complete. Also discuss the cross platform structure of this file and its
implementation and the tradeoffs that we discussed in the code review for how
best to lay this out. I want to explain why things are done in this weird way :)

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
net/base/file_stream_context.h:19: #include "errno.h"
<errno.h>

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
net/base/file_stream_context.h:141: #if defined(OS_WIN)
Can you move the ERROR_BAD_FILE enum up to the top of the private section? I
know it breaks the flow, but I'd rather be consistent with the style guide.

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_unit...
File net/base/file_stream_unittest.cc (left):

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_unit...
net/base/file_stream_unittest.cc:1132: TEST_F(FileStreamTest,
AsyncBasicOpenClose) {
Why are all of these deleted again? I forget :(

http://codereview.chromium.org/10701050/diff/135001/net/url_request/url_reque...
File net/url_request/url_request_file_job.h (right):

http://codereview.chromium.org/10701050/diff/135001/net/url_request/url_reque...
net/url_request/url_request_file_job.h:12: #include "net/base/file_stream.h"
forward declare instead now?

[pivanof, 2012-10-30 18:54:21]

> Do you remember all the things we
> discussed for future followup work here? I'd like to consolidate everything in
> bugs if possible, especially if you get busy and can't finish everything. Do
you
> mind filing bugs if they don't already exist?

Hm... I didn't recall right away what followups you mean here. But I'll walk
through the logs and refresh my memory.

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
File net/base/file_stream_context.h (right):

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
net/base/file_stream_context.h:19: #include "errno.h"
On 2012/10/30 18:01:27, willchan wrote:
> <errno.h>

I wonder how did it work this way. :)

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_cont...
net/base/file_stream_context.h:141: #if defined(OS_WIN)
On 2012/10/30 18:01:27, willchan wrote:
> Can you move the ERROR_BAD_FILE enum up to the top of the private section? I
> know it breaks the flow, but I'd rather be consistent with the style guide.

Yes, I can do that. Unfortunately it will add one more #if..#elif.

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_unit...
File net/base/file_stream_unittest.cc (left):

http://codereview.chromium.org/10701050/diff/135001/net/base/file_stream_unit...
net/base/file_stream_unittest.cc:1132: TEST_F(FileStreamTest,
AsyncBasicOpenClose) {
On 2012/10/30 18:01:27, willchan wrote:
> Why are all of these deleted again? I forget :(

Basically everything except AsyncRead_EarlyDelete_NoAutoClose is deleted because
it tests different situations involving Close*() methods which are deleted now.
AsyncRead_EarlyDelete_NoAutoClose is deleted because it tests stream not
acquiring ownership when created from fd -- we changed that now.

http://codereview.chromium.org/10701050/diff/135001/net/url_request/url_reque...
File net/url_request/url_request_file_job.h (right):

http://codereview.chromium.org/10701050/diff/135001/net/url_request/url_reque...
net/url_request/url_request_file_job.h:12: #include "net/base/file_stream.h"
On 2012/10/30 18:01:27, willchan wrote:
> forward declare instead now?

Yep, will do.

[willchan no longer on Chromium, 2012-10-31 06:11:44]

I can't seem to find any references to future followup work here. I must have
misrecollected. I do have ideas for future work if you're interested :) I'll
look into filing them at some point.

[pivanof, 2012-11-02 16:52:06]

On 2012/10/31 06:11:44, willchan wrote:
> I can't seem to find any references to future followup work here. I must have
> misrecollected. I do have ideas for future work if you're interested :) I'll
> look into filing them at some point.

Sure, I'm interested. CC me on bugs.
And take a look at the updated CL.

[willchan no longer on Chromium, 2012-11-02 19:06:04]

base/ && net/ LGTM. Thanks for all the cleanup!

Good luck landing this, I suspect you may hit some try failures or other things
:P

PS: Please update the changelist description too with other details, like the
implementation refactoring. Also, explain the change in semantics for the
~FileStream.

http://codereview.chromium.org/10701050/diff/194002/net/base/file_stream_cont...
File net/base/file_stream_context.h (right):

http://codereview.chromium.org/10701050/diff/194002/net/base/file_stream_cont...
net/base/file_stream_context.h:8: // specific implementation details. It
re-reoutes all its method calls to
re-routes

http://codereview.chromium.org/10701050/diff/194002/net/base/file_stream_cont...
net/base/file_stream_context.h:9: // the instance of FileStream::Context
(FileStream holds scoped_ptr to
holds a scoped_ptr

http://codereview.chromium.org/10701050/diff/194002/net/base/file_stream_cont...
net/base/file_stream_context.h:10: // FileStream::Context instance). Context was
extracted into different class
into a different

http://codereview.chromium.org/10701050/diff/194002/net/base/file_stream_cont...
net/base/file_stream_context.h:18: // header file contains complete definition
of Context class including all
contains the complete definition

[pivanof, 2012-11-03 07:31:52]

sky:
Please take a quick look at content/browser/, chrome/common/,
chrome/browser/sessions/ and chrome/browser/bookmarks/

hashimoto:
Please take a quick look at chrome/browser/chromeos/drive/.

satorux:
Please take a quick look at chromeos/.

jamesr:
Please take a quick look at webkit/.

willchan:
Please take a quick look at the current description.

[willchan no longer on Chromium, 2012-11-03 16:52:08]

CL desc looks great to me, thanks.


On Sat, Nov 3, 2012 at 12:31 AM, <paivanof@gmail.com> wrote:

> sky:
> Please take a quick look at content/browser/, chrome/common/,
> chrome/browser/sessions/ and chrome/browser/bookmarks/
>
> hashimoto:
> Please take a quick look at chrome/browser/chromeos/drive/**.
>
> satorux:
> Please take a quick look at chromeos/.
>
> jamesr:
> Please take a quick look at webkit/.
>
> willchan:
> Please take a quick look at the current description.
>
>
http://codereview.chromium.**org/10701050/<http://codereview.chromium.org/107...
>

[jamesr, 2012-11-03 20:25:03]

webkit/ lgtm

[hashimoto, 2012-11-05 02:29:19]

chrome/browser/chromeos/drve lgtm with a nit.

https://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

https://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:832: if (!stream_.get())
nit: Since we no longer need to NULL check here, could you remove this 'if'
statement too?

[satorux1, 2012-11-05 03:18:34]

http://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

http://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:832: if (!stream_.get())
On 2012/11/05 02:29:19, hashimoto wrote:
> nit: Since we no longer need to NULL check here, could you remove this 'if'
> statement too?

+1

http://codereview.chromium.org/10701050/diff/169004/chromeos/dbus/debug_daemo...
File chromeos/dbus/debug_daemon_client.cc (right):

http://codereview.chromium.org/10701050/diff/169004/chromeos/dbus/debug_daemo...
chromeos/dbus/debug_daemon_client.cc:56: PLOG(ERROR) << "close[0]";
Can we remove the line 54-56?

pipe_fd_[0] is closed by data_stream_, right? Maybe we should leave a comment
like:

// Don't close pipe_fd_[0] as it's closed by data_stream_.

[pivanof, 2012-11-05 05:17:46]

http://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):

http://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_protocol_handler.cc:832: if (!stream_.get())
On 2012/11/05 02:29:19, hashimoto wrote:
> nit: Since we no longer need to NULL check here, could you remove this 'if'
> statement too?

Done.

http://codereview.chromium.org/10701050/diff/169004/chromeos/dbus/debug_daemo...
File chromeos/dbus/debug_daemon_client.cc (right):

http://codereview.chromium.org/10701050/diff/169004/chromeos/dbus/debug_daemo...
chromeos/dbus/debug_daemon_client.cc:56: PLOG(ERROR) << "close[0]";
On 2012/11/05 03:18:35, satorux1 wrote:
> Can we remove the line 54-56?
> 
> pipe_fd_[0] is closed by data_stream_, right? Maybe we should leave a comment
> like:
> 
> // Don't close pipe_fd_[0] as it's closed by data_stream_.

For some reason I thought that there can be situations when pipe_fd_[0] is not
bound to data_stream_, i.e. pipe_fd_ exists but data_stream_ doesn't. Looking at
the code now I see that it's impossible, so indeed I removed these lines.

[hashimoto, 2012-11-05 05:20:45]

chrome/browser/chromeos/drve lgtm, thanks.

On 2012/11/05 05:17:46, pivanof wrote:
>
http://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/d...
> File chrome/browser/chromeos/drive/drive_protocol_handler.cc (right):
> 
>
http://codereview.chromium.org/10701050/diff/169004/chrome/browser/chromeos/d...
> chrome/browser/chromeos/drive/drive_protocol_handler.cc:832: if
(!stream_.get())
> On 2012/11/05 02:29:19, hashimoto wrote:
> > nit: Since we no longer need to NULL check here, could you remove this 'if'
> > statement too?
> 
> Done.
> 
>
http://codereview.chromium.org/10701050/diff/169004/chromeos/dbus/debug_daemo...
> File chromeos/dbus/debug_daemon_client.cc (right):
> 
>
http://codereview.chromium.org/10701050/diff/169004/chromeos/dbus/debug_daemo...
> chromeos/dbus/debug_daemon_client.cc:56: PLOG(ERROR) << "close[0]";
> On 2012/11/05 03:18:35, satorux1 wrote:
> > Can we remove the line 54-56?
> > 
> > pipe_fd_[0] is closed by data_stream_, right? Maybe we should leave a
comment
> > like:
> > 
> > // Don't close pipe_fd_[0] as it's closed by data_stream_.
> 
> For some reason I thought that there can be situations when pipe_fd_[0] is not
> bound to data_stream_, i.e. pipe_fd_ exists but data_stream_ doesn't. Looking
at
> the code now I see that it's impossible, so indeed I removed these lines.

[satorux1, 2012-11-05 05:30:37]

chromeos/ LGTM with a nit:

http://codereview.chromium.org/10701050/diff/199063/chromeos/dbus/debug_daemo...
File chromeos/dbus/debug_daemon_client.cc (right):

http://codereview.chromium.org/10701050/diff/199063/chromeos/dbus/debug_daemo...
chromeos/dbus/debug_daemon_client.cc:53: virtual ~PipeReader() {
I'd suggest to leave a comment:

// Don't close pipe_fd_[0] as it's closed by data_stream_.

[pivanof, 2012-11-05 06:11:34]

On 2012/11/05 05:30:37, satorux1 wrote:
>
http://codereview.chromium.org/10701050/diff/199063/chromeos/dbus/debug_daemo...
> File chromeos/dbus/debug_daemon_client.cc (right):
> 
>
http://codereview.chromium.org/10701050/diff/199063/chromeos/dbus/debug_daemo...
> chromeos/dbus/debug_daemon_client.cc:53: virtual ~PipeReader() {
> I'd suggest to leave a comment:
> 
> // Don't close pipe_fd_[0] as it's closed by data_stream_.

Sorry, forgot this one. Done.

[sky, 2012-11-05 16:12:52]

https://codereview.chromium.org/10701050/diff/169006/content/browser/download...
File content/browser/download/base_file.cc (right):

https://codereview.chromium.org/10701050/diff/169006/content/browser/download...
content/browser/download/base_file.cc:202: DCHECK(deleted);
Why the DCHECK? We know delete can fail for various reasons.

[pivanof, 2012-11-05 18:51:53]

On 2012/11/05 16:12:52, sky wrote:
>
https://codereview.chromium.org/10701050/diff/169006/content/browser/download...
> File content/browser/download/base_file.cc (right):
> 
>
https://codereview.chromium.org/10701050/diff/169006/content/browser/download...
> content/browser/download/base_file.cc:202: DCHECK(deleted);
> Why the DCHECK? We know delete can fail for various reasons.

My understanding is delete fails very rarely and probably in debugging
environment this failure could mean some bug somewhere. That's exactly why I
added it back then -- to catch a bug in base_file_unittest.cc (see patch set
31). But the test was already changed since then and thus doesn't have any fix
in the latest patch set. So if you insist I can remove that, but it could help
somebody later.

[sky, 2012-11-05 21:57:44]

My rationale is that the failure can happen for reasons outside of
Chromes control. Because of that it should not be a DCHECK.

  -Scott

On Mon, Nov 5, 2012 at 10:51 AM,  <paivanof@gmail.com> wrote:
> On 2012/11/05 16:12:52, sky wrote:
>
>
https://codereview.chromium.org/10701050/diff/169006/content/browser/download...
>>
>> File content/browser/download/base_file.cc (right):
>
>
>
>
https://codereview.chromium.org/10701050/diff/169006/content/browser/download...
>>
>> content/browser/download/base_file.cc:202: DCHECK(deleted);
>> Why the DCHECK? We know delete can fail for various reasons.
>
>
> My understanding is delete fails very rarely and probably in debugging
> environment this failure could mean some bug somewhere. That's exactly why I
> added it back then -- to catch a bug in base_file_unittest.cc (see patch set
> 31). But the test was already changed since then and thus doesn't have any
> fix
> in the latest patch set. So if you insist I can remove that, but it could
> help
> somebody later.
>
> http://codereview.chromium.org/10701050/

[pivanof, 2012-11-05 22:01:00]

On 2012/11/05 21:57:44, sky wrote:
> My rationale is that the failure can happen for reasons outside of
> Chromes control. Because of that it should not be a DCHECK.

OK. Removed DCHECK.

[sky, 2012-11-05 22:14:50]

LGTM

[commit-bot: I haz the power, 2012-11-05 22:19:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10701050/204035

[commit-bot: I haz the power, 2012-11-06 00:29:54]

Change committed as 166091

[Lei Zhang, 2012-11-06 01:58:27]

https://chromiumcodereview.appspot.com/10701050/diff/204035/tools/valgrind/me...
File tools/valgrind/memcheck/suppressions.txt (right):

https://chromiumcodereview.appspot.com/10701050/diff/204035/tools/valgrind/me...
tools/valgrind/memcheck/suppressions.txt:5886: FileStream::Context can leak
through WorkerPool by design
This is in the wrong section, and you also need to suppress the same leaks for
Heapchecker. I'll take care of it.