Only notify extensions about the end of a network request when it started

Only notify extensions about the end of a network request when it started

This CL makes it so that
ChromeExtensionsNetworkDelegate::ForwardDoneRequestStatus is only called when a
request actually started (and a corresponding
ChromeExtensionsNetworkDelegate::ForwardStartRequestStatus has been
called). This is preventing https://codereview.chromium.org/1363483007/
from landing.

BUG=504347

[clamy, 2015-09-28 14:36:20]

clamy@chromium.org changed reviewers:
+ rockot@chromium.org

[clamy, 2015-09-28 14:36:40]

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

[clamy, 2015-09-28 14:37:00]

@rockot, mmemnke: PTAL

[Ken Rockot(use gerrit already), 2015-09-28 15:26:26]

I also sent out https://codereview.chromium.org/1366393002/ to address the
problem, but this lgtm

[mmenke, 2015-09-28 15:45:21]

On 2015/09/28 15:26:26, Ken Rockot wrote:
> I also sent out https://codereview.chromium.org/1366393002/ to address the
> problem, but this lgtm

I don't think this works.

If the URLRequest is cancelled or destroyed by the ResourceLoader (Like due to a
RenderView being destroyed, or the renderer crashing), OnCompleted will be
called with a false |started| value.  What we actually want to know if if
OnBeforeStarted was invoked, not if the request was actually started.

[mmenke, 2015-09-28 17:47:41]

https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:394:
extensions_delegate_->ForwardStartRequestStatus(request);
I think it we always called this in wrapped callback (regardless of
force_safe_search) or when rv below was net::OK, everything would be fine,
though that does seem a bit ugly.

[mmenke, 2015-09-28 17:48:34]

https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:394:
extensions_delegate_->ForwardStartRequestStatus(request);
On 2015/09/28 17:47:40, mmenke wrote:
> I think it we always called this in wrapped callback (regardless of
> force_safe_search) or when rv below was net::OK, everything would be fine,
> though that does seem a bit ugly.

That "or" means "and" (Since doing both will make sure it's invoked exactly once
for every request that's actually started)

[Ken Rockot(use gerrit already), 2015-09-28 17:50:01]

On 2015/09/28 17:47:41, mmenke wrote:
>
https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
> chrome/browser/net/chrome_network_delegate.cc:394:
> extensions_delegate_->ForwardStartRequestStatus(request);
> I think it we always called this in wrapped callback (regardless of
> force_safe_search) or when rv below was net::OK, everything would be fine,
> though that does seem a bit ugly.

Another option is to not change NetworkDelegate behavior at all here. We can
instead change extensions' assumption about symmetry between start & complete,
which is what https://codereview.chromium.org/1366393002/ does.

[mmenke, 2015-09-28 18:02:16]

On 2015/09/28 17:50:01, Ken Rockot wrote:
> On 2015/09/28 17:47:41, mmenke wrote:
> >
>
https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
> > File chrome/browser/net/chrome_network_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/1368383003/diff/1/chrome/browser/net/chrome_n...
> > chrome/browser/net/chrome_network_delegate.cc:394:
> > extensions_delegate_->ForwardStartRequestStatus(request);
> > I think it we always called this in wrapped callback (regardless of
> > force_safe_search) or when rv below was net::OK, everything would be fine,
> > though that does seem a bit ugly.
> 
> Another option is to not change NetworkDelegate behavior at all here. We can
> instead change extensions' assumption about symmetry between start & complete,
> which is what https://codereview.chromium.org/1366393002/ does.

That option sounds much more robust to me.

[clamy, 2015-09-29 09:56:46]

Closing this since rockot@ patch landed.