Call MaybeNotifyNetworkBytes() in NotifyStartError().

Call MaybeNotifyNetworkBytes() in NotifyStartError().

This will cause OnNetworkBytesReceived() and OnNetworkBytesSent()
to be called before OnComplete() for this use case.

BUG=662050
Committed: https://crrev.com/8a1fa9e760fd98d596290825ef6d171f860f4d92
Cr-Commit-Position: refs/heads/master@{#430630}

[Not at Google. Contact bengr, 2016-11-03 16:05:25]

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

[commit-bot: I haz the power, 2016-11-03 16:06:04]

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

[Not at Google. Contact bengr, 2016-11-03 16:07:36]

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

[commit-bot: I haz the power, 2016-11-03 16:07:59]

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

[Not at Google. Contact bengr, 2016-11-03 16:08:43]

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

[commit-bot: I haz the power, 2016-11-03 16:09:05]

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

[Not at Google. Contact bengr, 2016-11-03 16:43:56]

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

[commit-bot: I haz the power, 2016-11-03 16:44: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-11-03 17:02:34]

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

[commit-bot: I haz the power, 2016-11-03 17:02:35]

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...)

[Not at Google. Contact bengr, 2016-11-03 18:19:03]

Patchset #4 (id:60001) has been deleted

[Not at Google. Contact bengr, 2016-11-03 18:26:55]

Description was changed from

==========
Fix NetworkDelegate life-cycle for data use.

Make NetworkDelegate bytes received and bytes sent updates before
OnComplete. Move MaybeNotifyNetworkBytes() out of NotifyDone().
In the failure case, call MaybeNotifyNetworkBytes() from
NotifyStartError() before NotifyResponseStarted().

BUG=662050
==========

to

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch NetworkDelegate bytes received and bytes sent updates before
OnComplete. Move MaybeNotifyNetworkBytes() out of NotifyDone().
In the failure case, call MaybeNotifyNetworkBytes() from
NotifyStartError() before NotifyResponseStarted().

BUG=662050
==========

[Not at Google. Contact bengr, 2016-11-03 18:28:51]

Description was changed from

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch NetworkDelegate bytes received and bytes sent updates before
OnComplete. Move MaybeNotifyNetworkBytes() out of NotifyDone().
In the failure case, call MaybeNotifyNetworkBytes() from
NotifyStartError() before NotifyResponseStarted().

BUG=662050
==========

to

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch OnNetworkBytesReceived() and OnNetworkBytesSent() before
OnComplete(). Move MaybeNotifyNetworkBytes() out of NotifyDone().
In the failure case, call MaybeNotifyNetworkBytes() from
NotifyStartError() before NotifyResponseStarted().

BUG=662050
==========

[Not at Google. Contact bengr, 2016-11-03 18:29:56]

Description was changed from

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch OnNetworkBytesReceived() and OnNetworkBytesSent() before
OnComplete(). Move MaybeNotifyNetworkBytes() out of NotifyDone().
In the failure case, call MaybeNotifyNetworkBytes() from
NotifyStartError() before NotifyResponseStarted().

BUG=662050
==========

to

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch OnNetworkBytesReceived() and OnNetworkBytesSent() before
OnComplete(). No calls to OnNetworkBytesReceived() or OnNetworkBytesSent() will
be made for a request once OnComplete()
is called for the same request.

BUG=662050
==========

[Not at Google. Contact bengr, 2016-11-03 18:30:32]

Description was changed from

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch OnNetworkBytesReceived() and OnNetworkBytesSent() before
OnComplete(). No calls to OnNetworkBytesReceived() or OnNetworkBytesSent() will
be made for a request once OnComplete()
is called for the same request.

BUG=662050
==========

to

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch OnNetworkBytesReceived() and OnNetworkBytesSent() before
OnComplete(). No calls to OnNetworkBytesReceived() or
OnNetworkBytesSent() will be made for a request once OnComplete()
is called for the same request.

BUG=662050
==========

[Not at Google. Contact bengr, 2016-11-03 18:44:03]

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

[Not at Google. Contact bengr, 2016-11-03 18:44:16]

PTAL.

[mmenke, 2016-11-03 19:07:01]

I'm not at all confident this ensures the API you're expecting the URLRequest to
implement.  Given the crazy complexity of this class, I'm not sure we want to
encourage external consumers to rely on behavior I'm not confident we can
guarantee.

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

https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.cc:669: MaybeNotifyNetworkBytes();
Not needed, we already call this in ReadRawDataComplete.

https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.cc:714: MaybeNotifyNetworkBytes();
This is a weird place to call this - presumably we called this when we parsed
the redirect header.

[Not at Google. Contact bengr, 2016-11-03 21:40:24]

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

[commit-bot: I haz the power, 2016-11-03 21:41:02]

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-11-03 23:20:42]

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

[commit-bot: I haz the power, 2016-11-03 23:20:43]

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_...)

[Not at Google. Contact bengr, 2016-11-04 00:08:12]

Without this cl, OnNetworkBytesReceived() and OnNetworkBytesSent() can called
after OnURLRequestDestroyed() and not just OnCompleted(). This seemed broken so
I thought I'd attempt to fix it. If fixing w.r.t. OnCompleted() is hard, I could
also look into fixing it w.r.t.  OnURLRequestDestroyed(), which might be easier.
Please let me know if this behavior is intentional and I am better off using
workarounds outside //net to compensate for it.

All this cl does is to move the call to MaybeNotifyNetworkBytes() outside of
NotifyDone().

There are 5 occurrences of NotifyDone across 4 methods in url_request_job.cc
(https://cs.chromium.org/chromium/src/net/url_request/url_request_job.cc?l=564):

498 NotifyHeadersComplete()
614 NotifyCanceled()
670 SourceStreamReadComplete(synchronous, result)
683 SourceStreamReadComplete(synchronous, result)
714 FollowRedirect(redirect_info)

Apart from NotifyCanceled(), none of these locations can be executed after
OnCompleted() as far as I understand. In each of those locations, I simply added
a call to MaybeNotifyNetworkBytes() just before NotifyDone(), while taking it
out from within NotifyDone().

As for the NotifyCanceled() case, is there a reason to delay reporting data use
bytes until a cancel? Could we add calls to MaybeNotifyNetworkBytes()  so that
there are never unreported bytes when NotifyCanceled() is called?

To fix the specific case in the bug, the cl calls MaybeNotifyNetworkBytes() in
NotifyStartError().

The DCHECK(request_->is_pending()) in MaybeNotifyNetworkBytes() ensures that
OnNetworkBytesReceived() and OnNetworkBytesSent() will not be called after
OnComplete().

My main concern is that this cl is missing bytes because
MaybeNotifyNetworkBytes() is no longer being called from NotifyCanceled(). I can
find these cases by adding DCHECKS to NotifyDone:
  if (network_delegate_) {
    DCHECK_LE(GetTotalReceivedBytes(), last_notified_total_received_bytes_);
    DCHECK_LE(GetTotalSentBytes(), last_notified_total_sent_bytes_);
  }

However, I am not quite sure where these bytes are coming from given that we
already report bytes from the raw reads. I could continue investigating unless
you think this behavior would be hard to fix. WDYT?

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

https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.cc:669: MaybeNotifyNetworkBytes();
On 2016/11/03 19:07:01, mmenke wrote:
> Not needed, we already call this in ReadRawDataComplete.

Done.

https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.cc:714: MaybeNotifyNetworkBytes();
On 2016/11/03 19:07:01, mmenke wrote:
> This is a weird place to call this - presumably we called this when we parsed
> the redirect header.

Done.

[mmenke, 2016-11-04 17:58:58]

On 2016/11/04 00:08:12, kundaji wrote:
> Without this cl, OnNetworkBytesReceived() and OnNetworkBytesSent() can called
> after OnURLRequestDestroyed() and not just OnCompleted(). This seemed broken
so
> I thought I'd attempt to fix it. If fixing w.r.t. OnCompleted() is hard, I
could
> also look into fixing it w.r.t.  OnURLRequestDestroyed(), which might be
easier.
> Please let me know if this behavior is intentional and I am better off using
> workarounds outside //net to compensate for it.
> 
> All this cl does is to move the call to MaybeNotifyNetworkBytes() outside of
> NotifyDone().
> 
> There are 5 occurrences of NotifyDone across 4 methods in url_request_job.cc
>
(https://cs.chromium.org/chromium/src/net/url_request/url_request_job.cc?l=564):
> 
> 498 NotifyHeadersComplete()
> 614 NotifyCanceled()
> 670 SourceStreamReadComplete(synchronous, result)
> 683 SourceStreamReadComplete(synchronous, result)
> 714 FollowRedirect(redirect_info)
> 
> Apart from NotifyCanceled(), none of these locations can be executed after
> OnCompleted() as far as I understand. In each of those locations, I simply
added
> a call to MaybeNotifyNetworkBytes() just before NotifyDone(), while taking it
> out from within NotifyDone().
> 
> As for the NotifyCanceled() case, is there a reason to delay reporting data
use
> bytes until a cancel? Could we add calls to MaybeNotifyNetworkBytes()  so that
> there are never unreported bytes when NotifyCanceled() is called?

There is a reason.  Maybe we're writing to the cache, and haven't informed the
URLRequestJob layer of the bytes yet.  Maybe we're in the middle of an SSL
handshake, and have received bytes, but no useful data yet, or talking to a
proxy, etc.  This CL doesn't cover any of those cases.  It's impossible to get
these bytes on cancellation without calling MaybeNotifyNetworkBytes() in
response to Cancel().

More generally, this is a complicated, fragile state machine, and I'm not sure
code should be relying on byte notifications being received before the
destruction notification, because of that complexity.

> To fix the specific case in the bug, the cl calls MaybeNotifyNetworkBytes() in
> NotifyStartError().
> 
> The DCHECK(request_->is_pending()) in MaybeNotifyNetworkBytes() ensures that
> OnNetworkBytesReceived() and OnNetworkBytesSent() will not be called after
> OnComplete().

I actually want to remove is_pending() because the only two consumers of it in
production code have a number of bugs in how they're using it.

> My main concern is that this cl is missing bytes because
> MaybeNotifyNetworkBytes() is no longer being called from NotifyCanceled(). I
can
> find these cases by adding DCHECKS to NotifyDone:
>   if (network_delegate_) {
>     DCHECK_LE(GetTotalReceivedBytes(), last_notified_total_received_bytes_);
>     DCHECK_LE(GetTotalSentBytes(), last_notified_total_sent_bytes_);
>   }
> 
> However, I am not quite sure where these bytes are coming from given that we
> already report bytes from the raw reads. I could continue investigating unless
> you think this behavior would be hard to fix. WDYT?
> 
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> File net/url_request/url_request_job.cc (right):
> 
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> net/url_request/url_request_job.cc:669: MaybeNotifyNetworkBytes();
> On 2016/11/03 19:07:01, mmenke wrote:
> > Not needed, we already call this in ReadRawDataComplete.
> 
> Done.
> 
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> net/url_request/url_request_job.cc:714: MaybeNotifyNetworkBytes();
> On 2016/11/03 19:07:01, mmenke wrote:
> > This is a weird place to call this - presumably we called this when we
parsed
> > the redirect header.
> 
> Done.

[RyanSturm, 2016-11-04 19:03:18]

On 2016/11/04 17:58:58, mmenke wrote:
> On 2016/11/04 00:08:12, kundaji wrote:
> > Without this cl, OnNetworkBytesReceived() and OnNetworkBytesSent() can
called
> > after OnURLRequestDestroyed() and not just OnCompleted(). This seemed broken
> so
> > I thought I'd attempt to fix it. If fixing w.r.t. OnCompleted() is hard, I
> could
> > also look into fixing it w.r.t.  OnURLRequestDestroyed(), which might be
> easier.
> > Please let me know if this behavior is intentional and I am better off using
> > workarounds outside //net to compensate for it.
> > 
> > All this cl does is to move the call to MaybeNotifyNetworkBytes() outside of
> > NotifyDone().
> > 
> > There are 5 occurrences of NotifyDone across 4 methods in url_request_job.cc
> >
>
(https://cs.chromium.org/chromium/src/net/url_request/url_request_job.cc?l=564):
> > 
> > 498 NotifyHeadersComplete()
> > 614 NotifyCanceled()
> > 670 SourceStreamReadComplete(synchronous, result)
> > 683 SourceStreamReadComplete(synchronous, result)
> > 714 FollowRedirect(redirect_info)
> > 
> > Apart from NotifyCanceled(), none of these locations can be executed after
> > OnCompleted() as far as I understand. In each of those locations, I simply
> added
> > a call to MaybeNotifyNetworkBytes() just before NotifyDone(), while taking
it
> > out from within NotifyDone().
> > 
> > As for the NotifyCanceled() case, is there a reason to delay reporting data
> use
> > bytes until a cancel? Could we add calls to MaybeNotifyNetworkBytes()  so
that
> > there are never unreported bytes when NotifyCanceled() is called?
> 
> There is a reason.  Maybe we're writing to the cache, and haven't informed the
> URLRequestJob layer of the bytes yet.  Maybe we're in the middle of an SSL
> handshake, and have received bytes, but no useful data yet, or talking to a
> proxy, etc.  This CL doesn't cover any of those cases.  It's impossible to get
> these bytes on cancellation without calling MaybeNotifyNetworkBytes() in
> response to Cancel().
> 
> More generally, this is a complicated, fragile state machine, and I'm not sure
> code should be relying on byte notifications being received before the
> destruction notification, because of that complexity.
> 
> > To fix the specific case in the bug, the cl calls MaybeNotifyNetworkBytes()
in
> > NotifyStartError().
> > 
> > The DCHECK(request_->is_pending()) in MaybeNotifyNetworkBytes() ensures that
> > OnNetworkBytesReceived() and OnNetworkBytesSent() will not be called after
> > OnComplete().
> 
> I actually want to remove is_pending() because the only two consumers of it in
> production code have a number of bugs in how they're using it.
> 
> > My main concern is that this cl is missing bytes because
> > MaybeNotifyNetworkBytes() is no longer being called from NotifyCanceled(). I
> can
> > find these cases by adding DCHECKS to NotifyDone:
> >   if (network_delegate_) {
> >     DCHECK_LE(GetTotalReceivedBytes(), last_notified_total_received_bytes_);
> >     DCHECK_LE(GetTotalSentBytes(), last_notified_total_sent_bytes_);
> >   }
> > 
> > However, I am not quite sure where these bytes are coming from given that we
> > already report bytes from the raw reads. I could continue investigating
unless
> > you think this behavior would be hard to fix. WDYT?
> > 
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > File net/url_request/url_request_job.cc (right):
> > 
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > net/url_request/url_request_job.cc:669: MaybeNotifyNetworkBytes();
> > On 2016/11/03 19:07:01, mmenke wrote:
> > > Not needed, we already call this in ReadRawDataComplete.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > net/url_request/url_request_job.cc:714: MaybeNotifyNetworkBytes();
> > On 2016/11/03 19:07:01, mmenke wrote:
> > > This is a weird place to call this - presumably we called this when we
> parsed
> > > the redirect header.
> > 
> > Done.

Does it make sense to at least guarantee that the bytes received and bytes sent
callbacks are called before OnBeforeUrlRequestDestroyed? Something like:
https://codereview.chromium.org/2467323002/

Seems like for the destroyed case we can safely reason about any execution after
this point.

[mmenke, 2016-11-04 19:07:36]

On 2016/11/04 19:03:18, Ryan Sturm wrote:
> On 2016/11/04 17:58:58, mmenke wrote:
> > On 2016/11/04 00:08:12, kundaji wrote:
> > > Without this cl, OnNetworkBytesReceived() and OnNetworkBytesSent() can
> called
> > > after OnURLRequestDestroyed() and not just OnCompleted(). This seemed
broken
> > so
> > > I thought I'd attempt to fix it. If fixing w.r.t. OnCompleted() is hard, I
> > could
> > > also look into fixing it w.r.t.  OnURLRequestDestroyed(), which might be
> > easier.
> > > Please let me know if this behavior is intentional and I am better off
using
> > > workarounds outside //net to compensate for it.
> > > 
> > > All this cl does is to move the call to MaybeNotifyNetworkBytes() outside
of
> > > NotifyDone().
> > > 
> > > There are 5 occurrences of NotifyDone across 4 methods in
url_request_job.cc
> > >
> >
>
(https://cs.chromium.org/chromium/src/net/url_request/url_request_job.cc?l=564):
> > > 
> > > 498 NotifyHeadersComplete()
> > > 614 NotifyCanceled()
> > > 670 SourceStreamReadComplete(synchronous, result)
> > > 683 SourceStreamReadComplete(synchronous, result)
> > > 714 FollowRedirect(redirect_info)
> > > 
> > > Apart from NotifyCanceled(), none of these locations can be executed after
> > > OnCompleted() as far as I understand. In each of those locations, I simply
> > added
> > > a call to MaybeNotifyNetworkBytes() just before NotifyDone(), while taking
> it
> > > out from within NotifyDone().
> > > 
> > > As for the NotifyCanceled() case, is there a reason to delay reporting
data
> > use
> > > bytes until a cancel? Could we add calls to MaybeNotifyNetworkBytes()  so
> that
> > > there are never unreported bytes when NotifyCanceled() is called?
> > 
> > There is a reason.  Maybe we're writing to the cache, and haven't informed
the
> > URLRequestJob layer of the bytes yet.  Maybe we're in the middle of an SSL
> > handshake, and have received bytes, but no useful data yet, or talking to a
> > proxy, etc.  This CL doesn't cover any of those cases.  It's impossible to
get
> > these bytes on cancellation without calling MaybeNotifyNetworkBytes() in
> > response to Cancel().
> > 
> > More generally, this is a complicated, fragile state machine, and I'm not
sure
> > code should be relying on byte notifications being received before the
> > destruction notification, because of that complexity.
> > 
> > > To fix the specific case in the bug, the cl calls
MaybeNotifyNetworkBytes()
> in
> > > NotifyStartError().
> > > 
> > > The DCHECK(request_->is_pending()) in MaybeNotifyNetworkBytes() ensures
that
> > > OnNetworkBytesReceived() and OnNetworkBytesSent() will not be called after
> > > OnComplete().
> > 
> > I actually want to remove is_pending() because the only two consumers of it
in
> > production code have a number of bugs in how they're using it.
> > 
> > > My main concern is that this cl is missing bytes because
> > > MaybeNotifyNetworkBytes() is no longer being called from NotifyCanceled().
I
> > can
> > > find these cases by adding DCHECKS to NotifyDone:
> > >   if (network_delegate_) {
> > >     DCHECK_LE(GetTotalReceivedBytes(),
last_notified_total_received_bytes_);
> > >     DCHECK_LE(GetTotalSentBytes(), last_notified_total_sent_bytes_);
> > >   }
> > > 
> > > However, I am not quite sure where these bytes are coming from given that
we
> > > already report bytes from the raw reads. I could continue investigating
> unless
> > > you think this behavior would be hard to fix. WDYT?
> > > 
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > File net/url_request/url_request_job.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > net/url_request/url_request_job.cc:669: MaybeNotifyNetworkBytes();
> > > On 2016/11/03 19:07:01, mmenke wrote:
> > > > Not needed, we already call this in ReadRawDataComplete.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > net/url_request/url_request_job.cc:714: MaybeNotifyNetworkBytes();
> > > On 2016/11/03 19:07:01, mmenke wrote:
> > > > This is a weird place to call this - presumably we called this when we
> > parsed
> > > > the redirect header.
> > > 
> > > Done.
> 
> Does it make sense to at least guarantee that the bytes received and bytes
sent
> callbacks are called before OnBeforeUrlRequestDestroyed? Something like:
> https://codereview.chromium.org/2467323002/
> 
> Seems like for the destroyed case we can safely reason about any execution
after
> this point.

That approach seem better than this one, though I'd also like to get rid of
URLRequestHttpJob::NotifyURLRequestDestroyed().

Having three calls into URLRequestJob on destruction seems like way too many
(Cancel(), NotifyURLRequestDestroyed(), and finally destructions). I'd really
like to get it down to just one.

[RyanSturm, 2016-11-04 19:18:04]

On 2016/11/04 19:07:36, mmenke wrote:
> On 2016/11/04 19:03:18, Ryan Sturm wrote:
> > On 2016/11/04 17:58:58, mmenke wrote:
> > > On 2016/11/04 00:08:12, kundaji wrote:
> > > > Without this cl, OnNetworkBytesReceived() and OnNetworkBytesSent() can
> > called
> > > > after OnURLRequestDestroyed() and not just OnCompleted(). This seemed
> broken
> > > so
> > > > I thought I'd attempt to fix it. If fixing w.r.t. OnCompleted() is hard,
I
> > > could
> > > > also look into fixing it w.r.t.  OnURLRequestDestroyed(), which might be
> > > easier.
> > > > Please let me know if this behavior is intentional and I am better off
> using
> > > > workarounds outside //net to compensate for it.
> > > > 
> > > > All this cl does is to move the call to MaybeNotifyNetworkBytes()
outside
> of
> > > > NotifyDone().
> > > > 
> > > > There are 5 occurrences of NotifyDone across 4 methods in
> url_request_job.cc
> > > >
> > >
> >
>
(https://cs.chromium.org/chromium/src/net/url_request/url_request_job.cc?l=564):
> > > > 
> > > > 498 NotifyHeadersComplete()
> > > > 614 NotifyCanceled()
> > > > 670 SourceStreamReadComplete(synchronous, result)
> > > > 683 SourceStreamReadComplete(synchronous, result)
> > > > 714 FollowRedirect(redirect_info)
> > > > 
> > > > Apart from NotifyCanceled(), none of these locations can be executed
after
> > > > OnCompleted() as far as I understand. In each of those locations, I
simply
> > > added
> > > > a call to MaybeNotifyNetworkBytes() just before NotifyDone(), while
taking
> > it
> > > > out from within NotifyDone().
> > > > 
> > > > As for the NotifyCanceled() case, is there a reason to delay reporting
> data
> > > use
> > > > bytes until a cancel? Could we add calls to MaybeNotifyNetworkBytes() 
so
> > that
> > > > there are never unreported bytes when NotifyCanceled() is called?
> > > 
> > > There is a reason.  Maybe we're writing to the cache, and haven't informed
> the
> > > URLRequestJob layer of the bytes yet.  Maybe we're in the middle of an SSL
> > > handshake, and have received bytes, but no useful data yet, or talking to
a
> > > proxy, etc.  This CL doesn't cover any of those cases.  It's impossible to
> get
> > > these bytes on cancellation without calling MaybeNotifyNetworkBytes() in
> > > response to Cancel().
> > > 
> > > More generally, this is a complicated, fragile state machine, and I'm not
> sure
> > > code should be relying on byte notifications being received before the
> > > destruction notification, because of that complexity.
> > > 
> > > > To fix the specific case in the bug, the cl calls
> MaybeNotifyNetworkBytes()
> > in
> > > > NotifyStartError().
> > > > 
> > > > The DCHECK(request_->is_pending()) in MaybeNotifyNetworkBytes() ensures
> that
> > > > OnNetworkBytesReceived() and OnNetworkBytesSent() will not be called
after
> > > > OnComplete().
> > > 
> > > I actually want to remove is_pending() because the only two consumers of
it
> in
> > > production code have a number of bugs in how they're using it.
> > > 
> > > > My main concern is that this cl is missing bytes because
> > > > MaybeNotifyNetworkBytes() is no longer being called from
NotifyCanceled().
> I
> > > can
> > > > find these cases by adding DCHECKS to NotifyDone:
> > > >   if (network_delegate_) {
> > > >     DCHECK_LE(GetTotalReceivedBytes(),
> last_notified_total_received_bytes_);
> > > >     DCHECK_LE(GetTotalSentBytes(), last_notified_total_sent_bytes_);
> > > >   }
> > > > 
> > > > However, I am not quite sure where these bytes are coming from given
that
> we
> > > > already report bytes from the raw reads. I could continue investigating
> > unless
> > > > you think this behavior would be hard to fix. WDYT?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > > File net/url_request/url_request_job.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > > net/url_request/url_request_job.cc:669: MaybeNotifyNetworkBytes();
> > > > On 2016/11/03 19:07:01, mmenke wrote:
> > > > > Not needed, we already call this in ReadRawDataComplete.
> > > > 
> > > > Done.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > > net/url_request/url_request_job.cc:714: MaybeNotifyNetworkBytes();
> > > > On 2016/11/03 19:07:01, mmenke wrote:
> > > > > This is a weird place to call this - presumably we called this when we
> > > parsed
> > > > > the redirect header.
> > > > 
> > > > Done.
> > 
> > Does it make sense to at least guarantee that the bytes received and bytes
> sent
> > callbacks are called before OnBeforeUrlRequestDestroyed? Something like:
> > https://codereview.chromium.org/2467323002/
> > 
> > Seems like for the destroyed case we can safely reason about any execution
> after
> > this point.
> 
> That approach seem better than this one, though I'd also like to get rid of
> URLRequestHttpJob::NotifyURLRequestDestroyed().
> 
> Having three calls into URLRequestJob on destruction seems like way too many
> (Cancel(), NotifyURLRequestDestroyed(), and finally destructions). I'd really
> like to get it down to just one.

That's a good point, but if the job is destroyed before the URLRequestDestroyed
callback many consumers couldn't access fields in the job that they might want
to.

I think the most valuable part of the the way kundaji@ is trying to implement
this, is that all of the byte callbacks occur while the URLRequest is not in its
destructor. I don't think there are any virtual methods to worry about right
now, but at some point there might be and an unsuspecting consumer of
OnNetworkBytesReceived might try to access a virtual method of the request. It
actually seems sort of odd that some of the time OnNetworkBytesReceived is
called, the request can be in it's destructor.

[mmenke, 2016-11-04 19:51:31]

On 2016/11/04 19:18:04, Ryan Sturm wrote:
> On 2016/11/04 19:07:36, mmenke wrote:
> > On 2016/11/04 19:03:18, Ryan Sturm wrote:
> > > On 2016/11/04 17:58:58, mmenke wrote:
> > > > On 2016/11/04 00:08:12, kundaji wrote:
> > > > > Without this cl, OnNetworkBytesReceived() and OnNetworkBytesSent() can
> > > called
> > > > > after OnURLRequestDestroyed() and not just OnCompleted(). This seemed
> > broken
> > > > so
> > > > > I thought I'd attempt to fix it. If fixing w.r.t. OnCompleted() is
hard,
> I
> > > > could
> > > > > also look into fixing it w.r.t.  OnURLRequestDestroyed(), which might
be
> > > > easier.
> > > > > Please let me know if this behavior is intentional and I am better off
> > using
> > > > > workarounds outside //net to compensate for it.
> > > > > 
> > > > > All this cl does is to move the call to MaybeNotifyNetworkBytes()
> outside
> > of
> > > > > NotifyDone().
> > > > > 
> > > > > There are 5 occurrences of NotifyDone across 4 methods in
> > url_request_job.cc
> > > > >
> > > >
> > >
> >
>
(https://cs.chromium.org/chromium/src/net/url_request/url_request_job.cc?l=564):
> > > > > 
> > > > > 498 NotifyHeadersComplete()
> > > > > 614 NotifyCanceled()
> > > > > 670 SourceStreamReadComplete(synchronous, result)
> > > > > 683 SourceStreamReadComplete(synchronous, result)
> > > > > 714 FollowRedirect(redirect_info)
> > > > > 
> > > > > Apart from NotifyCanceled(), none of these locations can be executed
> after
> > > > > OnCompleted() as far as I understand. In each of those locations, I
> simply
> > > > added
> > > > > a call to MaybeNotifyNetworkBytes() just before NotifyDone(), while
> taking
> > > it
> > > > > out from within NotifyDone().
> > > > > 
> > > > > As for the NotifyCanceled() case, is there a reason to delay reporting
> > data
> > > > use
> > > > > bytes until a cancel? Could we add calls to MaybeNotifyNetworkBytes() 
> so
> > > that
> > > > > there are never unreported bytes when NotifyCanceled() is called?
> > > > 
> > > > There is a reason.  Maybe we're writing to the cache, and haven't
informed
> > the
> > > > URLRequestJob layer of the bytes yet.  Maybe we're in the middle of an
SSL
> > > > handshake, and have received bytes, but no useful data yet, or talking
to
> a
> > > > proxy, etc.  This CL doesn't cover any of those cases.  It's impossible
to
> > get
> > > > these bytes on cancellation without calling MaybeNotifyNetworkBytes() in
> > > > response to Cancel().
> > > > 
> > > > More generally, this is a complicated, fragile state machine, and I'm
not
> > sure
> > > > code should be relying on byte notifications being received before the
> > > > destruction notification, because of that complexity.
> > > > 
> > > > > To fix the specific case in the bug, the cl calls
> > MaybeNotifyNetworkBytes()
> > > in
> > > > > NotifyStartError().
> > > > > 
> > > > > The DCHECK(request_->is_pending()) in MaybeNotifyNetworkBytes()
ensures
> > that
> > > > > OnNetworkBytesReceived() and OnNetworkBytesSent() will not be called
> after
> > > > > OnComplete().
> > > > 
> > > > I actually want to remove is_pending() because the only two consumers of
> it
> > in
> > > > production code have a number of bugs in how they're using it.
> > > > 
> > > > > My main concern is that this cl is missing bytes because
> > > > > MaybeNotifyNetworkBytes() is no longer being called from
> NotifyCanceled().
> > I
> > > > can
> > > > > find these cases by adding DCHECKS to NotifyDone:
> > > > >   if (network_delegate_) {
> > > > >     DCHECK_LE(GetTotalReceivedBytes(),
> > last_notified_total_received_bytes_);
> > > > >     DCHECK_LE(GetTotalSentBytes(), last_notified_total_sent_bytes_);
> > > > >   }
> > > > > 
> > > > > However, I am not quite sure where these bytes are coming from given
> that
> > we
> > > > > already report bytes from the raw reads. I could continue
investigating
> > > unless
> > > > > you think this behavior would be hard to fix. WDYT?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > > > File net/url_request/url_request_job.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > > > net/url_request/url_request_job.cc:669: MaybeNotifyNetworkBytes();
> > > > > On 2016/11/03 19:07:01, mmenke wrote:
> > > > > > Not needed, we already call this in ReadRawDataComplete.
> > > > > 
> > > > > Done.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2471533006/diff/40001/net/url_request/url_req...
> > > > > net/url_request/url_request_job.cc:714: MaybeNotifyNetworkBytes();
> > > > > On 2016/11/03 19:07:01, mmenke wrote:
> > > > > > This is a weird place to call this - presumably we called this when
we
> > > > parsed
> > > > > > the redirect header.
> > > > > 
> > > > > Done.
> > > 
> > > Does it make sense to at least guarantee that the bytes received and bytes
> > sent
> > > callbacks are called before OnBeforeUrlRequestDestroyed? Something like:
> > > https://codereview.chromium.org/2467323002/
> > > 
> > > Seems like for the destroyed case we can safely reason about any execution
> > after
> > > this point.
> > 
> > That approach seem better than this one, though I'd also like to get rid of
> > URLRequestHttpJob::NotifyURLRequestDestroyed().
> > 
> > Having three calls into URLRequestJob on destruction seems like way too many
> > (Cancel(), NotifyURLRequestDestroyed(), and finally destructions). I'd
really
> > like to get it down to just one.
> 
> That's a good point, but if the job is destroyed before the
URLRequestDestroyed
> callback many consumers couldn't access fields in the job that they might want
> to.

Right, so ideally we'd tell them about destruction, and then destroy the
URLRequest (Which does indeed result in the weird ordering).

> I think the most valuable part of the the way kundaji@ is trying to implement
> this, is that all of the byte callbacks occur while the URLRequest is not in
its
> destructor. I don't think there are any virtual methods to worry about right
> now, but at some point there might be and an unsuspecting consumer of
> OnNetworkBytesReceived might try to access a virtual method of the request. It
> actually seems sort of odd that some of the time OnNetworkBytesReceived is
> called, the request can be in it's destructor.

This is true.  We may want to rethink how things work here, either on the
URLRequestJob side (Or move them to the ResourceLoader, which is really what's
managing lifetimes here), or in the consumers of this method.

[Not at Google. Contact bengr, 2016-11-07 18:32:30]

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

[commit-bot: I haz the power, 2016-11-07 18:32:53]

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-11-07 19:23:20]

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

[commit-bot: I haz the power, 2016-11-07 19:23:21]

Dry run: This issue passed the CQ dry run.

[Not at Google. Contact bengr, 2016-11-07 19:42:34]

Description was changed from

==========
Fix NetworkDelegate life-cycle for data use.

Dispatch OnNetworkBytesReceived() and OnNetworkBytesSent() before
OnComplete(). No calls to OnNetworkBytesReceived() or
OnNetworkBytesSent() will be made for a request once OnComplete()
is called for the same request.

BUG=662050
==========

to

==========
Call MaybeNotifyNetworkBytes() in NotifyStartError().

This will cause OnNetworkBytesReceived() and OnNetworkBytesSent()
to be called before OnComplete() for this use case.

BUG=662050
==========

[Not at Google. Contact bengr, 2016-11-07 19:45:37]

Reporting data use perfectly in the context of a URLRequest is just not possible
since even after a URLRequest is destroyed packets can continue to be
transported over the network, and  users charged for it. Given this limitation,
my intent with the original cl was to fix the callback ordering at the cost of
reduced accuracy.

Would be great to get rid of NotifyURLRequestDestroyed() if possible and fixing
the life cycle in the long run.

For now, I've changed the cl to just add a call to MaybeNotifyNetworkBytes() in
NotifyStartError() itself rather than waiting for the destructor to trigger it.
For my purpose, all data use after OnComplete (or OnDestroyed) will be ignored.
We can track missed data use and prioritize issues accordingly.

PTAL.

[mmenke, 2016-11-08 15:37:03]

On 2016/11/07 19:45:37, kundaji wrote:
> Reporting data use perfectly in the context of a URLRequest is just not
possible
> since even after a URLRequest is destroyed packets can continue to be
> transported over the network, and  users charged for it. Given this
limitation,
> my intent with the original cl was to fix the callback ordering at the cost of
> reduced accuracy.
> 
> Would be great to get rid of NotifyURLRequestDestroyed() if possible and
fixing
> the life cycle in the long run.
> 
> For now, I've changed the cl to just add a call to MaybeNotifyNetworkBytes()
in
> NotifyStartError() itself rather than waiting for the destructor to trigger
it.
> For my purpose, all data use after OnComplete (or OnDestroyed) will be
ignored.
> We can track missed data use and prioritize issues accordingly.
> 
> PTAL.

LGTM

[Not at Google. Contact bengr, 2016-11-08 16:21:21]

The CQ bit was checked by kundaji@chromium.org

[commit-bot: I haz the power, 2016-11-08 16:21:49]

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-11-08 16:26:44]

Description was changed from

==========
Call MaybeNotifyNetworkBytes() in NotifyStartError().

This will cause OnNetworkBytesReceived() and OnNetworkBytesSent()
to be called before OnComplete() for this use case.

BUG=662050
==========

to

==========
Call MaybeNotifyNetworkBytes() in NotifyStartError().

This will cause OnNetworkBytesReceived() and OnNetworkBytesSent()
to be called before OnComplete() for this use case.

BUG=662050
==========

[commit-bot: I haz the power, 2016-11-08 16:26:45]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-11-08 16:38:35]

Description was changed from

==========
Call MaybeNotifyNetworkBytes() in NotifyStartError().

This will cause OnNetworkBytesReceived() and OnNetworkBytesSent()
to be called before OnComplete() for this use case.

BUG=662050
==========

to

==========
Call MaybeNotifyNetworkBytes() in NotifyStartError().

This will cause OnNetworkBytesReceived() and OnNetworkBytesSent()
to be called before OnComplete() for this use case.

BUG=662050

Committed: https://crrev.com/8a1fa9e760fd98d596290825ef6d171f860f4d92
Cr-Commit-Position: refs/heads/master@{#430630}
==========

[commit-bot: I haz the power, 2016-11-08 16:38:37]

Patchset 5 (id:??) landed as
https://crrev.com/8a1fa9e760fd98d596290825ef6d171f860f4d92
Cr-Commit-Position: refs/heads/master@{#430630}