FileWriterDelegate should not call URLRequest::Start() after Cancel().

FileWriterDelegate should not call URLRequest::Start() after Cancel().

- This should also fix one of the flaky crashes in FileWriterDelegate::OnDataWritten after cancel
- This patch also removes Start/End quota update from FileWriterDelegate since now we do that in FileSystemOperation

BUG=122160, 116639
TEST=file-writer-abort-continue.html

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

[kinuko, 2012-04-06 10:42:33]

Locally verified that it works in debug build while the test crashes without the
fix.

[ericu, 2012-04-09 16:14:22]

http://codereview.chromium.org/10008047/diff/1/webkit/fileapi/file_writer_del...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10008047/diff/1/webkit/fileapi/file_writer_del...
webkit/fileapi/file_writer_delegate.cc:117: if (request_->status().status() ==
net::URLRequestStatus::CANCELED) {
This looks more like "Don't call Start after Cancel" than "Don't call Cancel
before Start".  Is that what you wanted?  To prevent a call of Cancel before
Start I think you'd want to put code in OnError instead.

[ericu, 2012-04-09 16:16:22]

On 2012/04/09 16:14:22, ericu wrote:
>
http://codereview.chromium.org/10008047/diff/1/webkit/fileapi/file_writer_del...
> File webkit/fileapi/file_writer_delegate.cc (right):
> 
>
http://codereview.chromium.org/10008047/diff/1/webkit/fileapi/file_writer_del...
> webkit/fileapi/file_writer_delegate.cc:117: if (request_->status().status() ==
> net::URLRequestStatus::CANCELED) {
> This looks more like "Don't call Start after Cancel" than "Don't call Cancel
> before Start".  Is that what you wanted?  To prevent a call of Cancel before
> Start I think you'd want to put code in OnError instead.

From url_request.h:

  // This method may be called at any time after Start() has been called to
  // cancel the request.  This method may be called many times, and it has
  // no effect once the response has completed.  It is guaranteed that no
  // methods of the delegate will be called after the request has been
  // cancelled, except that this may call the delegate's OnReadCompleted()
  // during the call to Cancel itself.
  void Cancel();

[kinuko, 2012-04-10 10:26:00]

Updated (or totally revised the change).  I was somehow mistaken the issue...
thanks for pointing it out.

On 2012/04/09 16:16:22, ericu wrote:
> On 2012/04/09 16:14:22, ericu wrote:
> >
>
http://codereview.chromium.org/10008047/diff/1/webkit/fileapi/file_writer_del...
> > File webkit/fileapi/file_writer_delegate.cc (right):
> > 
> >
>
http://codereview.chromium.org/10008047/diff/1/webkit/fileapi/file_writer_del...
> > webkit/fileapi/file_writer_delegate.cc:117: if (request_->status().status()
==
> > net::URLRequestStatus::CANCELED) {
> > This looks more like "Don't call Start after Cancel" than "Don't call Cancel
> > before Start".  Is that what you wanted?  To prevent a call of Cancel before
> > Start I think you'd want to put code in OnError instead.

Changed the code (along with a crash fix) not to call URLRequest::Cancel before
Start.

(try jobs are failing but due to the same server-side error as the one on the
waterfall)

[ericu, 2012-04-10 22:10:32]

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation.cc:401: // abort, this->DidWrite is called
later and there we delete this object.
I'm not sure this is still true; see comments in file_writer_delegate.cc

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:114: if (state_ ==
FILE_WRITER_STATE_CANCELED) {
If we cancel while starting up, we call OnError(ABORT) here.  But if we cancel
later, we don't call OnError.  Is that right?

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:163: state_ = FILE_WRITER_STATE_CANCELED;
This doesn't call quota_util()->proxy()->EndUpdateOrigin(path_.origin(),
path_.type()); should it?

What about file_system_operation->didWrite()?

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:282: // (in
FileWriterDelegate::DidWrite).
s/FileWriterDelete/FileSystemOperation/?

[kinuko, 2012-04-11 01:11:46]

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:114: if (state_ ==
FILE_WRITER_STATE_CANCELED) {
On 2012/04/10 22:10:32, ericu wrote:
> If we cancel while starting up, we call OnError(ABORT) here.  But if we cancel
> later, we don't call OnError.  Is that right?

We will eventually call OnError (or OnProgress(complete=true)) in either case.

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:163: state_ = FILE_WRITER_STATE_CANCELED;
On 2012/04/10 22:10:32, ericu wrote:
> This doesn't call quota_util()->proxy()->EndUpdateOrigin(path_.origin(),
> path_.type()); should it?
>
> What about file_system_operation->didWrite()?

We'll eventually call it.

If we've already started the request state_ must be
FILE_WRITER_STATE_REQUEST_STARTED (if not _CANCELED) and we call
request_->Cancel() which will in turn calls FWD::OnReadCompleted() where we hook
the quota code in OnError if request_->state().is_success() is false (and it
is).

If we're cancelled between Write() and OnDataWritten() we update the quota in
OnProgress (and bail out).

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:282: // (in
FileWriterDelegate::DidWrite).
On 2012/04/10 22:10:32, ericu wrote:
> s/FileWriterDelete/FileSystemOperation/?

Done.

[ericu, 2012-04-11 03:24:13]

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:163: state_ = FILE_WRITER_STATE_CANCELED;
On 2012/04/11 01:11:46, kinuko wrote:
> On 2012/04/10 22:10:32, ericu wrote:
> > This doesn't call quota_util()->proxy()->EndUpdateOrigin(path_.origin(),
> > path_.type()); should it?
> >
> > What about file_system_operation->didWrite()?
> 
> We'll eventually call it.
> 
> If we've already started the request state_ must be
> FILE_WRITER_STATE_REQUEST_STARTED (if not _CANCELED) and we call
> request_->Cancel() which will in turn calls FWD::OnReadCompleted() where we
hook
> the quota code in OnError if request_->state().is_success() is false (and it
> is).

The URLRequest header comments say
  // ...It is guaranteed that no
  // methods of the delegate will be called after the request has been
  // cancelled, except that this may call the delegate's OnReadCompleted()
  // during the call to Cancel itself.

That "may" doesn't sound like a guarantee, more like a warning.  If we're going
to assume that it's a contract, we should 1) audit the code to make sure it's
always true, and that we won't want to change it; 2) change this comment.

> If we're cancelled between Write() and OnDataWritten() we update the quota in
> OnProgress (and bail out).

If we Cancel between Read and OnDataReceived, we won't notice the cancel for a
while, though.  Should we check in OnDataReceived as well?

[kinuko, 2012-04-11 09:06:02]

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate.cc (right):

http://codereview.chromium.org/10008047/diff/14011/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate.cc:163: state_ = FILE_WRITER_STATE_CANCELED;
On 2012/04/11 03:24:13, ericu wrote:
> On 2012/04/11 01:11:46, kinuko wrote:
> > On 2012/04/10 22:10:32, ericu wrote:
> > > This doesn't call quota_util()->proxy()->EndUpdateOrigin(path_.origin(),
> > > path_.type()); should it?
> > >
> > > What about file_system_operation->didWrite()?
> > 
> > We'll eventually call it.
> > 
> > If we've already started the request state_ must be
> > FILE_WRITER_STATE_REQUEST_STARTED (if not _CANCELED) and we call
> > request_->Cancel() which will in turn calls FWD::OnReadCompleted() where we
> hook
> > the quota code in OnError if request_->state().is_success() is false (and it
> > is).
> 
> The URLRequest header comments say
>   // ...It is guaranteed that no
>   // methods of the delegate will be called after the request has been
>   // cancelled, except that this may call the delegate's OnReadCompleted()
>   // during the call to Cancel itself.
> 
> That "may" doesn't sound like a guarantee, more like a warning.  If we're
going
> to assume that it's a contract, we should 1) audit the code to make sure it's
> always true, and that we won't want to change it; 2) change this comment.

Oh ok, I dived into the code before looking at the comment but probably we
should rather trust what the comment says.  It sounds like we'll need to make
some more changes in the Cancel flow together with this one though...

> > If we're cancelled between Write() and OnDataWritten() we update the quota
in
> > OnProgress (and bail out).
> 
> If we Cancel between Read and OnDataReceived, we won't notice the cancel for a
> while, though.  Should we check in OnDataReceived as well?

Sounds good, will do.

[kinuko, 2012-05-08 17:55:44]

After a long pause I resumed working on this.  Can you take a look again? 
Thanks,

[ericu, 2012-05-08 22:12:48]

LGTM, unless you can improve it by adding a test.  I'm not sure how possible
that'll be, though, due to the timing.

http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate_unittest.cc:227:
TEST_F(FileWriterDelegateTest, WriteSuccessWithoutQuotaLimit) {
Is it possible to write a test that will deterministically crash without this
change?

[kinuko, 2012-05-09 06:01:34]

http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer...
File webkit/fileapi/file_writer_delegate_unittest.cc (right):

http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer...
webkit/fileapi/file_writer_delegate_unittest.cc:227:
TEST_F(FileWriterDelegateTest, WriteSuccessWithoutQuotaLimit) {
On 2012/05/08 22:12:48, ericu wrote:
> Is it possible to write a test that will deterministically crash without this
> change?

Done!  I've added TestCancelWithInflightTasks[1-5], which certainly crashes
without this fix.

[ericu, 2012-05-09 22:33:41]

On 2012/05/09 06:01:34, kinuko wrote:
>
http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer...
> File webkit/fileapi/file_writer_delegate_unittest.cc (right):
> 
>
http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer...
> webkit/fileapi/file_writer_delegate_unittest.cc:227:
> TEST_F(FileWriterDelegateTest, WriteSuccessWithoutQuotaLimit) {
> On 2012/05/08 22:12:48, ericu wrote:
> > Is it possible to write a test that will deterministically crash without
this
> > change?
> 
> Done!  I've added TestCancelWithInflightTasks[1-5], which certainly crashes
> without this fix.

Actually, it doesn't.  I just patched just the new test in on a clean client,
and it passes fine without your fix.

[kinuko, 2012-05-10 02:24:57]

Ok then it looks to be still just flaky... I'll poke around the test code,
but I'm not sure if I could add a deterministic test.
For now I'd like to submit the change itself with a TODO comment.

On Thu, May 10, 2012 at 7:33 AM, <ericu@chromium.org> wrote:

> On 2012/05/09 06:01:34, kinuko wrote:
>
> http://codereview.chromium.**org/10008047/diff/27007/**
>
webkit/fileapi/file_writer_**delegate_unittest.cc<http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer_delegate_unittest.cc>
>
>> File webkit/fileapi/file_writer_**delegate_unittest.cc (right):
>>
>
>
> http://codereview.chromium.**org/10008047/diff/27007/**
>
webkit/fileapi/file_writer_**delegate_unittest.cc#**newcode227<http://codereview.chromium.org/10008047/diff/27007/webkit/fileapi/file_writer_delegate_unittest.cc#newcode227>
>
>> webkit/fileapi/file_writer_**delegate_unittest.cc:227:
>> TEST_F(FileWriterDelegateTest, WriteSuccessWithoutQuotaLimit) {
>> On 2012/05/08 22:12:48, ericu wrote:
>> > Is it possible to write a test that will deterministically crash without
>>
> this
>
>> > change?
>>
>
>  Done!  I've added TestCancelWithInflightTasks[1-**5], which certainly
>> crashes
>> without this fix.
>>
>
> Actually, it doesn't.  I just patched just the new test in on a clean
> client,
> and it passes fine without your fix.
>
>
>
http://codereview.chromium.**org/10008047/<http://codereview.chromium.org/100...
>

[Eric U., 2012-05-10 02:59:09]

On Wed, May 9, 2012 at 7:24 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> Ok then it looks to be still just flaky... I'll poke around the test code,
> but I'm not sure if I could add a deterministic test.
> For now I'd like to submit the change itself with a TODO comment.

When you add the TODO, remove the test code; we don't want people to
think it's testing something it's not.

[kinuko, 2012-05-10 03:10:56]

Sure, I meant reverting the test code.

On Thu, May 10, 2012 at 11:58 AM, Eric U <ericu@google.com> wrote:

> On Wed, May 9, 2012 at 7:24 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> > Ok then it looks to be still just flaky... I'll poke around the test
> code,
> > but I'm not sure if I could add a deterministic test.
> > For now I'd like to submit the change itself with a TODO comment.
>
> When you add the TODO, remove the test code; we don't want people to
> think it's testing something it's not.
>

[Eric U., 2012-05-10 03:30:23]

Okeydoke; go for it.

On Wed, May 9, 2012 at 8:10 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
> Sure, I meant reverting the test code.
>
>
> On Thu, May 10, 2012 at 11:58 AM, Eric U <ericu@google.com> wrote:
>>
>> On Wed, May 9, 2012 at 7:24 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:
>> > Ok then it looks to be still just flaky... I'll poke around the test
>> > code,
>> > but I'm not sure if I could add a deterministic test.
>> > For now I'd like to submit the change itself with a TODO comment.
>>
>> When you add the TODO, remove the test code; we don't want people to
>> think it's testing something it's not.
>
>

[kinuko, 2012-05-10 05:08:43]

Weird, this test actually crashes on my local clean build almost 100%.
(I'm testing on linux btw)

Could I just submit together with the test?

On 2012/05/10 03:30:23, Eric U. wrote:
> Okeydoke; go for it.
> 
> On Wed, May 9, 2012 at 8:10 PM, Kinuko Yasuda <mailto:kinuko@chromium.org>
wrote:
> > Sure, I meant reverting the test code.
> >
> >
> > On Thu, May 10, 2012 at 11:58 AM, Eric U <mailto:ericu@google.com> wrote:
> >>
> >> On Wed, May 9, 2012 at 7:24 PM, Kinuko Yasuda <mailto:kinuko@chromium.org>
wrote:
> >> > Ok then it looks to be still just flaky...&nbsp;I'll poke around the test
> >> > code,
> >> > but I'm not sure if I could add a deterministic test.
> >> > For now I'd like to submit the change itself with a TODO comment.
> >>
> >> When you add the TODO, remove the test code; we don't want people to
> >> think it's testing something it's not.
> >
> >

[kinuko, 2012-05-10 05:55:29]

Confirmed: it's still flaky but have a higher possibility to let it crash
(without the fix).

Anyway for now I'm submitting the patch without the test but with TODO as
we agreed.

On Thu, May 10, 2012 at 2:08 PM, <kinuko@chromium.org> wrote:

> Weird, this test actually crashes on my local clean build almost 100%.
> (I'm testing on linux btw)
>
> Could I just submit together with the test?
>
>
> On 2012/05/10 03:30:23, Eric U. wrote:
>
>> Okeydoke; go for it.
>>
>
>  On Wed, May 9, 2012 at 8:10 PM, Kinuko Yasuda <mailto:kinuko@chromium.org
>> >
>>
> wrote:
>
>> > Sure, I meant reverting the test code.
>> >
>> >
>> > On Thu, May 10, 2012 at 11:58 AM, Eric U <mailto:ericu@google.com>
>> wrote:
>> >>
>> >> On Wed, May 9, 2012 at 7:24 PM, Kinuko Yasuda <mailto:
>> kinuko@chromium.org>
>>
> wrote:
>
>> >> > Ok then it looks to be still just flaky...&nbsp;I'll poke around the
>> test
>>
>> >> > code,
>> >> > but I'm not sure if I could add a deterministic test.
>> >> > For now I'd like to submit the change itself with a TODO comment.
>> >>
>> >> When you add the TODO, remove the test code; we don't want people to
>> >> think it's testing something it's not.
>> >
>> >
>>
>
>
>
>
http://codereview.chromium.**org/10008047/<http://codereview.chromium.org/100...
>

[commit-bot: I haz the power, 2012-05-10 16:06:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/10008047/29003

[commit-bot: I haz the power, 2012-05-10 18:47:20]

Try job failure for 10008047-29003 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...