URLFetcher won't call delegate until file is closed.

URLFetcher won't call delegate until file is closed.

Previously, URLFetchers closed the file in parallel with calling the
delegate, which causes races on whether or not the file has been closed.

Implementing this change required the addition of a Close() function to
the API of FileStream.

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

[waffles, 2013-09-05 17:49:01]

Hello willchan@,

Please take a look at this patch that prevents a URLFetcher from finishing until
it closes the file it is writing to.

[Sorin Jianu, 2013-09-05 19:59:12]

Thanks Josh.

https://codereview.chromium.org/23500006/diff/1/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/23500006/diff/1/net/base/file_stream.h#newcode84
net/base/file_stream.h:84: // Closes the file. If the file si open
asynchronously, returns ERR_IO_PENDING
s/si/is/

https://codereview.chromium.org/23500006/diff/1/net/url_request/url_fetcher_r...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/1/net/url_request/url_fetcher_r...
net/url_request/url_fetcher_response_writer.cc:107: if (result !=
ERR_IO_PENDING) {
Remove the full bracing for the if statement, per Chrome style guide.

[waffles, 2013-09-05 20:31:52]

Thanks!

https://codereview.chromium.org/23500006/diff/1/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/23500006/diff/1/net/base/file_stream.h#newcode84
net/base/file_stream.h:84: // Closes the file. If the file si open
asynchronously, returns ERR_IO_PENDING
On 2013/09/05 19:59:12, Sorin Jianu wrote:
> s/si/is/

Done.

https://codereview.chromium.org/23500006/diff/1/net/url_request/url_fetcher_r...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/1/net/url_request/url_fetcher_r...
net/url_request/url_fetcher_response_writer.cc:107: if (result !=
ERR_IO_PENDING) {
On 2013/09/05 19:59:12, Sorin Jianu wrote:
> Remove the full bracing for the if statement, per Chrome style guide.

Done.

[willchan no longer on Chromium, 2013-09-09 00:39:08]

Redirecting to mmenke

[mmenke, 2013-09-09 15:03:08]

Suggested unit tests for the FileStream:  Basic sync case w/ close, basic async
case w/ close, async case + orphaned while close pending.

Be great if we could verify the file was closed in the async case in a manner
that would fail consistently without the call to close() as well.

https://codereview.chromium.org/23500006/diff/15001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/23500006/diff/15001/net/base/file_stream.cc#n...
net/base/file_stream.cc:97: if (!is_async()) {
All the other sync calls are preceded by a:

base::ThreadRestrictions::AssertIOAllowed();

https://codereview.chromium.org/23500006/diff/15001/net/base/file_stream.cc#n...
net/base/file_stream.cc:99: context_.reset();
Erm....Doesn't this result in a crash on destruction?  ~FileStream always calls
into context_.

We definitely need some unit tests for this.

[waffles, 2013-09-09 22:50:55]

Thanks, mmenke!

Added 3 unit tests.
Split Close() into CloseSync() and Close() because dealing with the callback is
too annoying in the CloseSync() case.

"Be great if we could verify the file was closed in the async case in a manner
that would fail consistently without the call to close() as well." ::I thought
about this but wasn't sure where to go with it: the file does get closed no
matter what; it's just that it might still be open after the stream is destroyed
(for a short while). All that this change has introduced for FileStream is a
point (the call of the callback) that the file is guaranteed to be closed by.
The "Orphaned" test demonstrates this; let me know if you meant something
different.

https://codereview.chromium.org/23500006/diff/15001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/23500006/diff/15001/net/base/file_stream.cc#n...
net/base/file_stream.cc:97: if (!is_async()) {
On 2013/09/09 15:03:09, mmenke wrote:
> All the other sync calls are preceded by a:
> 
> base::ThreadRestrictions::AssertIOAllowed();

Done.

https://codereview.chromium.org/23500006/diff/15001/net/base/file_stream.cc#n...
net/base/file_stream.cc:99: context_.reset();
On 2013/09/09 15:03:09, mmenke wrote:
> Erm....Doesn't this result in a crash on destruction?  ~FileStream always
calls
> into context_.
> 
> We definitely need some unit tests for this.

Hm, yes, I agree.

[mmenke, 2013-09-10 18:32:52]

Your unit tests look great to me.

https://codereview.chromium.org/23500006/diff/34001/net/base/file_stream_unit...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/23500006/diff/34001/net/base/file_stream_unit...
net/base/file_stream_unittest.cc:101: FileStream stream(NULL);
nit:  I think it's generally a little clearer to use a scoped_ptr and reset() to
delete instead of a nested block of code.  Also, while one or two other tests in
this file also use this method, it looks to me like scoped_ptrs are the more
common method used to do this in this file as well.  Also seems like there's
less than can go wrong in future refactoring or reuse of the tests (If callback
were declared in the inner code block, for example, things would go badly).

Same goes for the next test as well, of course.

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:106:
&URLFetcherFileWriter::CloseComplete, base::Unretained(this), callback));
The base::Unretained changes the safe usage model of the URLFetcherFileWriter. 
Previously, it could safely be deleted at any point.  You should use
weak_factory_.GetWeakPtr() here.

If you look at URLFetcherCore::CancelURLRequest and the calls into it from
URLFetchImpl, this seems to be a property that is relevant.  I don't see any
tests for this property, but may be worth adding.

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:143:
base::Bind(base::IgnoreResult(&base::DeleteFile),
Kinda related to this CL:  Think it's worth a comment that this is safe because
file_stream_.reset() will close the file on file_task_runner_ before this task
runs (Was trying to figure out if this was safe).

[waffles, 2013-09-10 21:41:34]

https://codereview.chromium.org/23500006/diff/34001/net/base/file_stream_unit...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/23500006/diff/34001/net/base/file_stream_unit...
net/base/file_stream_unittest.cc:101: FileStream stream(NULL);
On 2013/09/10 18:32:52, mmenke wrote:
> nit:  I think it's generally a little clearer to use a scoped_ptr and reset()
to
> delete instead of a nested block of code.  Also, while one or two other tests
in
> this file also use this method, it looks to me like scoped_ptrs are the more
> common method used to do this in this file as well.  Also seems like there's
> less than can go wrong in future refactoring or reuse of the tests (If
callback
> were declared in the inner code block, for example, things would go badly).
> 
> Same goes for the next test as well, of course.

I agree that scoped_ptr is better. I had noticed the nested block in
BasicOpenClose, which is highly similar to these tests, so I used it.

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:106:
&URLFetcherFileWriter::CloseComplete, base::Unretained(this), callback));
On 2013/09/10 18:32:52, mmenke wrote:
> The base::Unretained changes the safe usage model of the URLFetcherFileWriter.

> Previously, it could safely be deleted at any point.  You should use
> weak_factory_.GetWeakPtr() here.
> 
> If you look at URLFetcherCore::CancelURLRequest and the calls into it from
> URLFetchImpl, this seems to be a property that is relevant.  I don't see any
> tests for this property, but may be worth adding.

Done.

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:143:
base::Bind(base::IgnoreResult(&base::DeleteFile),
On 2013/09/10 18:32:52, mmenke wrote:
> Kinda related to this CL:  Think it's worth a comment that this is safe
because
> file_stream_.reset() will close the file on file_task_runner_ before this task
> runs (Was trying to figure out if this was safe).

Hm, except that file_task_runner_ is only guaranteed to be a base::TaskRunner,
not a base::SequencedTaskRunner, and therefore may not perform tasks in FIFO
order, right?

So I think it's unsafe (and always has been).

[mmenke, 2013-09-10 22:25:15]

You forgot to upload the latest version.  :)

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:143:
base::Bind(base::IgnoreResult(&base::DeleteFile),
On 2013/09/10 21:41:34, waffles wrote:
> On 2013/09/10 18:32:52, mmenke wrote:
> > Kinda related to this CL:  Think it's worth a comment that this is safe
> because
> > file_stream_.reset() will close the file on file_task_runner_ before this
task
> > runs (Was trying to figure out if this was safe).
> 
> Hm, except that file_task_runner_ is only guaranteed to be a base::TaskRunner,
> not a base::SequencedTaskRunner, and therefore may not perform tasks in FIFO
> order, right?
> 
> So I think it's unsafe (and always has been).

Looks like you're right.  It actually used to use message loops.  And the CL
that switched it over to using TaskRunners a year ago got it right, so it's
regressed sometime since then.

I'm thinking I'll do some more spelunking to figure out why and upload a
separate patch, unless you have an urge to fix it yourself, either in this CL or
in another (And I'll want a unit test).

[waffles, 2013-09-10 22:33:02]

On 2013/09/10 22:25:15, mmenke wrote:
> You forgot to upload the latest version.  :)
> 

So I did; it's there now. (Too used to working directly in the cloud, I
guess...)

>
https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
> File net/url_request/url_fetcher_response_writer.cc (right):
> 
>
https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
> net/url_request/url_fetcher_response_writer.cc:143:
> base::Bind(base::IgnoreResult(&base::DeleteFile),
> On 2013/09/10 21:41:34, waffles wrote:
> > On 2013/09/10 18:32:52, mmenke wrote:
> > > Kinda related to this CL:  Think it's worth a comment that this is safe
> > because
> > > file_stream_.reset() will close the file on file_task_runner_ before this
> task
> > > runs (Was trying to figure out if this was safe).
> > 
> > Hm, except that file_task_runner_ is only guaranteed to be a
base::TaskRunner,
> > not a base::SequencedTaskRunner, and therefore may not perform tasks in FIFO
> > order, right?
> > 
> > So I think it's unsafe (and always has been).
> 
> Looks like you're right.  It actually used to use message loops.  And the CL
> that switched it over to using TaskRunners a year ago got it right, so it's
> regressed sometime since then.
> 
> I'm thinking I'll do some more spelunking to figure out why and upload a
> separate patch, unless you have an urge to fix it yourself, either in this CL
or
> in another (And I'll want a unit test).

[waffles, 2013-09-10 22:33:22]

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:143:
base::Bind(base::IgnoreResult(&base::DeleteFile),
On 2013/09/10 22:25:16, mmenke wrote:
> On 2013/09/10 21:41:34, waffles wrote:
> > On 2013/09/10 18:32:52, mmenke wrote:
> > > Kinda related to this CL:  Think it's worth a comment that this is safe
> > because
> > > file_stream_.reset() will close the file on file_task_runner_ before this
> task
> > > runs (Was trying to figure out if this was safe).
> > 
> > Hm, except that file_task_runner_ is only guaranteed to be a
base::TaskRunner,
> > not a base::SequencedTaskRunner, and therefore may not perform tasks in FIFO
> > order, right?
> > 
> > So I think it's unsafe (and always has been).
> 
> Looks like you're right.  It actually used to use message loops.  And the CL
> that switched it over to using TaskRunners a year ago got it right, so it's
> regressed sometime since then.
> 
> I'm thinking I'll do some more spelunking to figure out why and upload a
> separate patch, unless you have an urge to fix it yourself, either in this CL
or
> in another (And I'll want a unit test).

I'd rather not fix it in this CL; I definitely don't mind doing it in another
but I'm not sure exactly when I'll have the cycles.

[mmenke, 2013-09-10 22:38:19]

Want to give it a final once over tomorrow morning, but I expect to sign off on
it then.

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/34001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:143:
base::Bind(base::IgnoreResult(&base::DeleteFile),
On 2013/09/10 22:33:22, waffles wrote:
> On 2013/09/10 22:25:16, mmenke wrote:
> > On 2013/09/10 21:41:34, waffles wrote:
> > > On 2013/09/10 18:32:52, mmenke wrote:
> > > > Kinda related to this CL:  Think it's worth a comment that this is safe
> > > because
> > > > file_stream_.reset() will close the file on file_task_runner_ before
this
> > task
> > > > runs (Was trying to figure out if this was safe).
> > > 
> > > Hm, except that file_task_runner_ is only guaranteed to be a
> base::TaskRunner,
> > > not a base::SequencedTaskRunner, and therefore may not perform tasks in
FIFO
> > > order, right?
> > > 
> > > So I think it's unsafe (and always has been).
> > 
> > Looks like you're right.  It actually used to use message loops.  And the CL
> > that switched it over to using TaskRunners a year ago got it right, so it's
> > regressed sometime since then.
> > 
> > I'm thinking I'll do some more spelunking to figure out why and upload a
> > separate patch, unless you have an urge to fix it yourself, either in this
CL
> or
> > in another (And I'll want a unit test).
> 
> I'd rather not fix it in this CL; I definitely don't mind doing it in another
> but I'm not sure exactly when I'll have the cycles.

That's fine, I'll do it.  I want to investigate why it regressed, to make sure
that there wasn't any bug fix involved, anyways.

https://codereview.chromium.org/23500006/diff/49001/net/base/file_stream_unit...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/23500006/diff/49001/net/base/file_stream_unit...
net/base/file_stream_unittest.cc:115: stream.reset();
Shouldn't this go above the "base::PlatformFileInfo info;" line?

[waffles, 2013-09-10 22:58:33]

https://codereview.chromium.org/23500006/diff/49001/net/base/file_stream_unit...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/23500006/diff/49001/net/base/file_stream_unit...
net/base/file_stream_unittest.cc:115: stream.reset();
On 2013/09/10 22:38:19, mmenke wrote:
> Shouldn't this go above the "base::PlatformFileInfo info;" line?

Previously, it did, but while changing it, it seemed to me that this is better.

Before, we checked:
  1) IsOpen() returns false before destroying the stream and
  2) the PlatformFile is closed *after* destroying the stream.

Now, we check: 
  1) IsOpen() returns false before destroying the stream and
  2) the PlatformFile is closed *before* destroying the stream.

The case that we now fail on that passed before is:
* Close() causes IsOpen() to return false but doesn't actually close the file;
running the destructor actually closes the file.

The only case I can think of that we now pass on that failed before is:
* Running the destructor re-opens the file. (seems far-fetched)

Of course, now it occurs to me that I should just remove the reset() altogether,
since I moved it to the end of the scope... and then that the scoped_ptr is
unnecessary after all. So I've now removed it.

Of course, all of the above does not apply to the Orphan test, since we want to
explicitly orphan the stream context before we RunUntilIdle().

[mmenke, 2013-09-11 15:16:11]

LGTM, minor comments.

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.cc#n...
net/base/file_stream.cc:109: 
nit:  Remove extra line break.

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.h#ne...
net/base/file_stream.h:85: // |callback| when done.
Think it's worth mentioning that no other operations may be in progress when
this is called.

https://codereview.chromium.org/23500006/diff/56001/net/url_request/url_fetch...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/56001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:115: // Either we closed the file
or we can't; either way destroy the file_stream.
nit:  Some people strongly object to "We" in comments, due to ambiguity. 
Suggest something along the lines of "Either the close operation succeeded or
failed; ...."

https://codereview.chromium.org/23500006/diff/56001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:115: // Either we closed the file
or we can't; either way destroy the file_stream.
nit:  Either "destroy the FileStream" or "destroy |file_stream_|" (|| around
class members variables isn't required by Google style, but it's the style used
in the header, so should be consistent).

[waffles, 2013-09-11 19:54:48]

Thanks!

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.cc
File net/base/file_stream.cc (right):

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.cc#n...
net/base/file_stream.cc:109: 
On 2013/09/11 15:16:11, mmenke wrote:
> nit:  Remove extra line break.

Done.

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.h
File net/base/file_stream.h (right):

https://codereview.chromium.org/23500006/diff/56001/net/base/file_stream.h#ne...
net/base/file_stream.h:85: // |callback| when done.
On 2013/09/11 15:16:11, mmenke wrote:
> Think it's worth mentioning that no other operations may be in progress when
> this is called.

Done.

https://codereview.chromium.org/23500006/diff/56001/net/url_request/url_fetch...
File net/url_request/url_fetcher_response_writer.cc (right):

https://codereview.chromium.org/23500006/diff/56001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:115: // Either we closed the file
or we can't; either way destroy the file_stream.
On 2013/09/11 15:16:11, mmenke wrote:
> nit:  Some people strongly object to "We" in comments, due to ambiguity. 
> Suggest something along the lines of "Either the close operation succeeded or
> failed; ...."

Done.

https://codereview.chromium.org/23500006/diff/56001/net/url_request/url_fetch...
net/url_request/url_fetcher_response_writer.cc:115: // Either we closed the file
or we can't; either way destroy the file_stream.
On 2013/09/11 15:16:11, mmenke wrote:
> nit:  Either "destroy the FileStream" or "destroy |file_stream_|" (|| around
> class members variables isn't required by Google style, but it's the style
used
> in the header, so should be consistent).

Done.

[commit-bot: I haz the power, 2013-09-11 23:15:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/waffles@chromium.org/23500006/36001

[commit-bot: I haz the power, 2013-09-12 06:37:21]

Change committed as 222737