ResourceLoader: Fix a bunch of double-cancellation/double-error notification cases.

ResourceLoader:  Fix a bunch of double-cancellation/double-error notification cases.

There are a number of cases where ResourceLoader ends up notifying ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709, 670400
Committed: https://crrev.com/94f1bd9459e17972e8e3f6f51a3d10fbf4a94df7
Cr-Commit-Position: refs/heads/master@{#437057}

[mmenke, 2016-12-01 19:25:42]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 19:26:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 19:47:44]

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

[commit-bot: I haz the power, 2016-12-01 19:47:45]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmenke, 2016-12-01 21:06:42]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 21:07:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 21:51:36]

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

[commit-bot: I haz the power, 2016-12-01 21:51:37]

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

[mmenke, 2016-12-01 22:49:32]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 22:49:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 23:50:03]

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

[commit-bot: I haz the power, 2016-12-01 23:50:04]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-02 15:15:43]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 15:15:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-12-02 15:17:26]

Description was changed from

==========
URLRequest:  No longer post notification on sync read errors.

URLRequest tells the caller of sync read errors synchronously, so no
need for the double notification. Also fix the three consumers that
depended on this behavior.

BUG=
==========

to

==========
URLRequest:  No longer post notification on sync read errors.

URLRequest tells the caller of sync read errors synchronously, so no
need for the double notification. Also fix the three consumers that
depended on this behavior.

BUG=669709
==========

[mmenke, 2016-12-02 15:20:07]

mmenke@chromium.org changed reviewers:
+ rdsmith@chromium.org

[mmenke, 2016-12-02 15:20:08]

rdsmith:  Sorry for sending all these reviews your way.  This one depends on the
new ResourceHandler test CL (Well, it enables the new disabled test, at least),
so review that one first.

Note that I manually audited all 24 non-test URLRequest::Delegates to make sure
that they handled synchronous read errors fine without the extra read error
message, and all but two of them did.

https://codereview.chromium.org/2542843006/diff/60001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2542843006/diff/60001/net/url_request/url_req...
net/url_request/url_request_job.cc:598: base::Bind(&URLRequestJob::NotifyDone,
weak_factory_.GetWeakPtr()));
Note that I want to get rid of NotifyDone entirely.  I have a CL that does it,
but it depends on the suspend mode stuff, which I still need to work out.  It's
not a coincidence that I'm working on the ResourceHandler stuff and the suspend
stuff at the same time.

https://codereview.chromium.org/2542843006/diff/60001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2542843006/diff/60001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:5628: http_test_server()->GetURL("/"),
DEFAULT_PRIORITY, &d));
Note that before the switch to the embedded test server, this URL returned a
response with a body, now it just returns headers with a 0-length response body,
so I added a test for what it used to do, above.

https://codereview.chromium.org/2542843006/diff/60001/remoting/host/token_val...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2542843006/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:132: OnReadCompleted(request_.get(),
net_result);
This is a bug fix.

https://codereview.chromium.org/2542843006/diff/60001/remoting/host/token_val...
remoting/host/token_validator_base.cc:137: if (bytes_read !=
net::ERR_IO_PENDING)
Note that doing this in the bytes_read == 0 case is a bug fix, and doing this in
the bytes_read < 0 case is due to the API change.

[mmenke, 2016-12-02 15:22:39]

On 2016/12/02 15:20:08, mmenke wrote:
> rdsmith:  Sorry for sending all these reviews your way.  This one depends on
the
> new ResourceHandler test CL (Well, it enables the new disabled test, at
least),
> so review that one first.
> 
> Note that I manually audited all 24 non-test URLRequest::Delegates to make
sure
> that they handled synchronous read errors fine without the extra read error
> message, and all but two of them did.

Sorry, that should be all but 3 (Forgot to count ResourceLoader itself)

[commit-bot: I haz the power, 2016-12-02 15:24:52]

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

[commit-bot: I haz the power, 2016-12-02 15:24:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2016-12-02 15:25:15]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 15:25:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 16:18:16]

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

[commit-bot: I haz the power, 2016-12-02 16:18:17]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-05 16:19:47]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 16:20:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 17:21:23]

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

[commit-bot: I haz the power, 2016-12-05 17:21:23]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-05 18:10:00]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 18:10:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 19:58:09]

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

[commit-bot: I haz the power, 2016-12-05 19:58:09]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-06 19:32:08]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 19:32:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 20:38:45]

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

[commit-bot: I haz the power, 2016-12-06 20:38:46]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-06 21:40:31]

Description was changed from

==========
URLRequest:  No longer post notification on sync read errors.

URLRequest tells the caller of sync read errors synchronously, so no
need for the double notification. Also fix the three consumers that
depended on this behavior.

BUG=669709
==========

to

==========
URLRequest:  No longer post notification on sync read errors.

URLRequest tells the caller of sync read errors synchronously, so no
need for the double notification.  Also no longer post a cancellation
notification after the request has already notified its delegate
that the request is complete/

Also fix the three consumers that depended on this behavior.

BUG=669709
==========

[Randy Smith (Not in Mondays), 2016-12-06 23:44:43]

nit: Could you deal with the "/" being used as a "." in the in the description?

Also, I'm not clear that I can do a reasonable job reviewing the token
validation test change.

https://codereview.chromium.org/2542843006/diff/160001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/content/browser/loader...
content/browser/loader/resource_loader.cc:557: void
ResourceLoader::ReadMore(bool is_continuation) {
This file looks like pure refactoring rather than adapting to the new
URLRequest::Read contract around not dispatching an asynchronous call to
OnReadCompleted on synchronous error returns.  What am I missing?

https://codereview.chromium.org/2542843006/diff/160001/content/browser/loader...
content/browser/loader/resource_loader.cc:573: // Cancel the request, which will
then call back into |this| to it of
nit: "to it"?

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.cc:603: // Check if we should notify the
delegate that we're done because of an error.
nit, suggestion, not relevant to this CL: I wince at referring to the request as
"the delegate" given that we're not accessing it through a delegate interface
and there are several delegates at the URLRequest level (network &
URLRequest::Delegate).  Using that language seems like an invitation to
confusion.

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.h:367: // If |notify_done| is true, will notify
the URLRequest of completion.
I believe this is only done if the completion is in error.   The non-error
completion looks to be done through NotifyReadCompleted directly (from
SourceStreamReadComplete).

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.h:373: // the URLRequest::Delegate.
I think some gesture to the PostTask or the asynchronous triggering of this
method by OnDone() belongs in this comment.

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5614: // or it could have been all the
bytes.
Sorry, what's the pathway that makes this true?  I don't see anything in
TestDelegate::* where data_received_ (which underlies bytes_received()) is
changed without received_bytes_count_ being changed.

https://codereview.chromium.org/2542843006/diff/160001/remoting/host/token_va...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/remoting/host/token_va...
remoting/host/token_validator_base.cc:131: // Treat all network errors read
errors.
nit: Treat?  Maybe process?

[mmenke, 2016-12-07 15:59:25]

Description was changed from

==========
URLRequest:  No longer post notification on sync read errors.

URLRequest tells the caller of sync read errors synchronously, so no
need for the double notification.  Also no longer post a cancellation
notification after the request has already notified its delegate
that the request is complete/

Also fix the three consumers that depended on this behavior.

BUG=669709
==========

to

==========
URLRequest:  No longer post notification on sync read errors.

URLRequest tells the caller of sync read errors synchronously, so no
need for the double notification.  Also no longer post a cancellation
notification after the request has already notified its delegate
that the request is complete.

Also fix the three consumers that depended on this behavior.

BUG=669709
==========

[mmenke, 2016-12-07 16:15:46]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 16:16:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-12-07 16:17:15]

Thanks for the careful review!  I'll look into the revert now.  Looks like just
ServiceWorker tests broke, so may be SW making certain assumptions about
lifecycle of URLRequests, or may just be a bug in my changes.

https://codereview.chromium.org/2542843006/diff/160001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/content/browser/loader...
content/browser/loader/resource_loader.cc:557: void
ResourceLoader::ReadMore(bool is_continuation) {
On 2016/12/06 23:44:43, Randy Smith - Not in Mondays wrote:
> This file looks like pure refactoring rather than adapting to the new
> URLRequest::Read contract around not dispatching an asynchronous call to
> OnReadCompleted on synchronous error returns.  What am I missing?

In the cancel path from the old ReadMore(), we want to exit immediately and let
ResourceLoader::Cancel() handle it, but in the sync read error path, we need to
pass the error on.  So either we'd have to have ReadMore return a value to
distinguish those two paths, or we need to just merge the methods.  Since there
was only one call site, went with the latter option.

I should have mentioned when I sent this CL out - the reason for this change is
completely non-obvious.

https://codereview.chromium.org/2542843006/diff/160001/content/browser/loader...
content/browser/loader/resource_loader.cc:573: // Cancel the request, which will
then call back into |this| to it of
On 2016/12/06 23:44:43, Randy Smith - Not in Mondays wrote:
> nit: "to it"?

Done.

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.cc:603: // Check if we should notify the
delegate that we're done because of an error.
On 2016/12/06 23:44:43, Randy Smith - Not in Mondays wrote:
> nit, suggestion, not relevant to this CL: I wince at referring to the request
as
> "the delegate" given that we're not accessing it through a delegate interface
> and there are several delegates at the URLRequest level (network &
> URLRequest::Delegate).  Using that language seems like an invitation to
> confusion.

It's actually referring to the URLRequest::Delegate, I bet.  Anyhow, I switched
it to URLRequest (The URLRequestJob shouldn't know about the
URLRequest::Delegate).

Could do more cleanup here, but this is something I'd rather just remove, in the
future.

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.h:367: // If |notify_done| is true, will notify
the URLRequest of completion.
On 2016/12/06 23:44:43, Randy Smith - Not in Mondays wrote:
> I believe this is only done if the completion is in error.   The non-error
> completion looks to be done through NotifyReadCompleted directly (from
> SourceStreamReadComplete).
> 

Done.

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.h:373: // the URLRequest::Delegate.
On 2016/12/06 23:44:43, Randy Smith - Not in Mondays wrote:
> I think some gesture to the PostTask or the asynchronous triggering of this
> method by OnDone() belongs in this comment.

Done.

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:5614: // or it could have been all the
bytes.
On 2016/12/06 23:44:43, Randy Smith - Not in Mondays wrote:
> Sorry, what's the pathway that makes this true?  I don't see anything in
> TestDelegate::* where data_received_ (which underlies bytes_received()) is
> changed without received_bytes_count_ being changed.

You're right, the comment is false.  Was just blindly copied from old test. 
Removed the comment (Also removed the bytes_received check, since it's logically
equivalent to the received_bytes check - we should merge the values, but I bet
it would touch a ton of tests)

https://codereview.chromium.org/2542843006/diff/160001/remoting/host/token_va...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/2542843006/diff/160001/remoting/host/token_va...
remoting/host/token_validator_base.cc:131: // Treat all network errors read
errors.
On 2016/12/06 23:44:43, Randy Smith - Not in Mondays wrote:
> nit: Treat?  Maybe process?

Done.

[commit-bot: I haz the power, 2016-12-07 16:19:06]

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

[commit-bot: I haz the power, 2016-12-07 16:19:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2016-12-07 16:20:30]

Think all that trybot red is due to the revert.

[mmenke, 2016-12-07 17:06:24]

On 2016/12/07 16:20:30, mmenke wrote:
> Think all that trybot red is due to the revert.

Randy:  Do you mind if I combine this CL with the reverted CL, and land them
together?

The reason for the revert looks to be that ServiceWorker doesn't handle the
double-cancellation correctly.  A URLRequestJob subclass that sits on top of its
own URLRequest reads from the socket, gets an error synchronously, passes the
error upstream, then gets the async notification of the error again, sends the
result upstream again, and URLRequestJob DCHECKs due to the double notification.
 We aren't seeing crashes here because ResourceLoader generally tears down the
URLRequest before the async notification occurs.  However, the reverted CL
doesn't synchronously tear down the ResourceLoader, but instead waits for the
async notification.  So to make a long story short, landing both CLs together
fixes that issue.

[Randy Smith (Not in Mondays), 2016-12-07 17:14:43]

On 2016/12/07 17:06:24, mmenke wrote:
> On 2016/12/07 16:20:30, mmenke wrote:
> > Think all that trybot red is due to the revert.
> 
> Randy:  Do you mind if I combine this CL with the reverted CL, and land them
> together?
> 
> The reason for the revert looks to be that ServiceWorker doesn't handle the
> double-cancellation correctly.  A URLRequestJob subclass that sits on top of
its
> own URLRequest reads from the socket, gets an error synchronously, passes the
> error upstream, then gets the async notification of the error again, sends the
> result upstream again, and URLRequestJob DCHECKs due to the double
notification.
>  We aren't seeing crashes here because ResourceLoader generally tears down the
> URLRequest before the async notification occurs.  However, the reverted CL
> doesn't synchronously tear down the ResourceLoader, but instead waits for the
> async notification.  So to make a long story short, landing both CLs together
> fixes that issue.

Nope, that's fine by me, but I'd be grateful if you set it up so that I know
precise patchset correspondences and hence can easily review from previously
known points.

[mmenke, 2016-12-07 17:27:21]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[mmenke, 2016-12-07 17:27:22]

On 2016/12/07 17:14:43, Randy Smith - Not in Mondays wrote:
> On 2016/12/07 17:06:24, mmenke wrote:
> > On 2016/12/07 16:20:30, mmenke wrote:
> > > Think all that trybot red is due to the revert.
> > 
> > Randy:  Do you mind if I combine this CL with the reverted CL, and land them
> > together?
> > 
> > The reason for the revert looks to be that ServiceWorker doesn't handle the
> > double-cancellation correctly.  A URLRequestJob subclass that sits on top of
> its
> > own URLRequest reads from the socket, gets an error synchronously, passes
the
> > error upstream, then gets the async notification of the error again, sends
the
> > result upstream again, and URLRequestJob DCHECKs due to the double
> notification.
> >  We aren't seeing crashes here because ResourceLoader generally tears down
the
> > URLRequest before the async notification occurs.  However, the reverted CL
> > doesn't synchronously tear down the ResourceLoader, but instead waits for
the
> > async notification.  So to make a long story short, landing both CLs
together
> > fixes that issue.
> 
> Nope, that's fine by me, but I'd be grateful if you set it up so that I know
> precise patchset correspondences and hence can easily review from previously
> known points.

Done.  Set 11 is the reverted patch set (It's identical, no changes, just a
trivial merge), and Set 12 is what this patch set used to be, on top of that
patch set.  Again, no changes (Other than response to your comments).

If you want to review my response to your comments, I suggest you look at the
diff between sets 9 to 10 (You can also compare 9 to 12 - there's a merge in
there, too, though it doesn't look like too much changed there).

[mmenke, 2016-12-07 17:29:38]

Description was changed from

==========
URLRequest:  No longer post notification on sync read errors.

URLRequest tells the caller of sync read errors synchronously, so no
need for the double notification.  Also no longer post a cancellation
notification after the request has already notified its delegate
that the request is complete.

Also fix the three consumers that depended on this behavior.

BUG=669709
==========

to

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two URLRequest consumers that depended on that behavior.

BUG=669709
==========

[mmenke, 2016-12-07 17:30:03]

Description was changed from

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two URLRequest consumers that depended on that behavior.

BUG=669709
==========

to

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709
==========

[mmenke, 2016-12-07 17:31:21]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 17:32:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 17:46:27]

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

[commit-bot: I haz the power, 2016-12-07 17:46:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2016-12-07 18:01:44]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 18:02:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-12-07 18:33:03]

mmenke@chromium.org changed reviewers:
+ jkarlin@chromium.org, sergeyu@chromium.org

[mmenke, 2016-12-07 18:33:04]

[+sergeyu]:  Please review remoting/ (Changes are both a bugfix and updates to
work with the slightly modified API).

[+jkarlin]:  Please review content/browser/cache_storage.  Didn't add a test for
the API change, as it looked a bit too involved.

[commit-bot: I haz the power, 2016-12-07 19:02:56]

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

[commit-bot: I haz the power, 2016-12-07 19:02:57]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-12-07 19:03:32]

LGTM.  Thanks again for doing this.

[jkarlin, 2016-12-07 19:32:39]

cache_storage/ lgtm, thanks for the fix!

[Sergey Ulanov, 2016-12-07 19:40:52]

lgtm

[mmenke, 2016-12-07 20:52:42]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-12-07 20:53:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-12-07 20:55:50]

Description was changed from

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709
==========

to

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709,670400
==========

[commit-bot: I haz the power, 2016-12-07 21:13:09]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1481143962018290,
"parent_rev": "74d52b960d16eb94ac2388f8746a9ceb910dba5f", "commit_rev":
"86cb3f8c3604b88ad8d8c0108289124db69598e2"}

[commit-bot: I haz the power, 2016-12-07 21:13:53]

Description was changed from

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709,670400
==========

to

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709,670400
==========

[commit-bot: I haz the power, 2016-12-07 21:13:54]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-12-07 21:17:48]

Description was changed from

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709,670400
==========

to

==========
ResourceLoader:  Fix a bunch of double-cancellation/double-error notification
cases.

There are a number of cases where ResourceLoader ends up notifying
ResourceHandlers
twice of the same failure. This CL fixes them and adds some tests.

It also makes URLRequest no longer post notification on sync read errors, which
was
causing double notifications in one of those cases.

Also fixes two other URLRequest consumers that depended on that behavior.

BUG=669709,670400

Committed: https://crrev.com/94f1bd9459e17972e8e3f6f51a3d10fbf4a94df7
Cr-Commit-Position: refs/heads/master@{#437057}
==========

[commit-bot: I haz the power, 2016-12-07 21:17:49]

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