URLRequestHttpJob::StartTransaction should honour network delegate.

URLRequestHttpJob::StartTransaction should honour network delegate.

If a network delegate returns net::ERR_BLOCKED_BY_CLIENT inside URLRequestHttpJob::StartTransaction() the job is not cancelled. This does not happen, e.g., when ERR_BLOCKED_BY_CLIENT is returned within URLRequestHttpJob::OnStartCompleted(). This CL attempts to change the behaviour of StartTransaction() to cancel the job on the error signal from the delegate.

BUG=146816
TEST=This is not testable in the browser without custom code changes yet.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158984

[vabr (Chromium), 2012-09-07 17:05:35]

Hi Matt,

This CL is probably silly and wrong, because I'm not sure about the
notifications mechanism inside the request job.

Could you please take a look and tell me what you think?

Thanks,
Vaclav

P.S. If you don't feel like reviewing it yourself, could you please suggest
somebody else knowledgeable in this area of code?

[mmenke, 2012-09-07 17:27:50]

Your fix looks correct to me.  Mind adding a unit test for this?  Would be great
if you could test the async cancellation callback as well, as it doesn't look
like we have one.  Network delegate requests are currently all in
url_request_unittest.cc.

http://codereview.chromium.org/10911151/diff/3002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10911151/diff/3002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:344: // If an extension blocks the
request, we rely on the callback to
Need a complete sentence here.  "... the callback to
NotifyBeforeSendHeadersCallback()"?

[mmenke, 2012-09-07 17:28:32]

On 2012/09/07 17:27:50, Matt Menke wrote:
> Your fix looks correct to me.  Mind adding a unit test for this?  Would be
great
> if you could test the async cancellation callback as well, as it doesn't look
> like we have one.  Network delegate requests are currently all in
> url_request_unittest.cc.

Network delegate tests, rather, not requests.

[vabr (Chromium), 2012-09-10 15:32:58]

Thanks, Matt!

I added a unit test. I'm not done yet, because the unit test does not pass if I
uncomment one line (marked in a comment). I uploaded this CL for being able to
ask you:

1. Is the added unit test is going in the desired direction?
2. Do you know just straight out of your head what might be the problem with the
line marked with my comment?

If you don't know what the problem might be, don't worry and please do not
invest your time into finding out. I'll take a look tomorrow.

(Disclaimer: The current state of the CL is not meant to be land-able, patchset
#2 is here for discussion.)

Thanks,
Vaclav

http://codereview.chromium.org/10911151/diff/3002/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10911151/diff/3002/net/url_request/url_request...
net/url_request/url_request_http_job.cc:344: // If an extension blocks the
request, we rely on the callback to
On 2012/09/07 17:27:51, Matt Menke wrote:
> Need a complete sentence here.  "... the callback to
> NotifyBeforeSendHeadersCallback()"?

Oops, I deleted that by mistake. Restored the original line and updated
according to the current content of NotifyBeforeSendHeadersCallback().

http://codereview.chromium.org/10911151/diff/5003/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10911151/diff/5003/net/url_request/url_request...
net/url_request/url_request_unittest.cc:706:
//BlockingNetworkDelegateWithManualCallback::ON_BEFORE_URL_REQUEST,
I'm not sure why, but if blocking ON_BEFORE_URL_REQUEST then the request gets
not cancelled. Any ideas?

[mmenke, 2012-09-10 15:53:18]

It looks to me like your test is indeed going in the right direction.  Great
that you're testing the other cases, too.

I think your test isn't working because OnBeforeURLRequest is handled at the
URLRequest level, so ends up creating and starting a new URLRequestErrorJob. 
Starting the error job results in posting a task to pass along the error code. 
As a result, you need a call to MessageLoop::current()->RunAllPending();

On 2012/09/10 15:32:58, vabr (Chromium) wrote:
> Thanks, Matt!
> 
> I added a unit test. I'm not done yet, because the unit test does not pass if
I
> uncomment one line (marked in a comment). I uploaded this CL for being able to
> ask you:
> 
> 1. Is the added unit test is going in the desired direction?
> 2. Do you know just straight out of your head what might be the problem with
the
> line marked with my comment?
> 
> If you don't know what the problem might be, don't worry and please do not
> invest your time into finding out. I'll take a look tomorrow.
> 
> (Disclaimer: The current state of the CL is not meant to be land-able,
patchset
> #2 is here for discussion.)
> 
> Thanks,
> Vaclav
> 
>
http://codereview.chromium.org/10911151/diff/3002/net/url_request/url_request...
> File net/url_request/url_request_http_job.cc (right):
> 
>
http://codereview.chromium.org/10911151/diff/3002/net/url_request/url_request...
> net/url_request/url_request_http_job.cc:344: // If an extension blocks the
> request, we rely on the callback to
> On 2012/09/07 17:27:51, Matt Menke wrote:
> > Need a complete sentence here.  "... the callback to
> > NotifyBeforeSendHeadersCallback()"?
> 
> Oops, I deleted that by mistake. Restored the original line and updated
> according to the current content of NotifyBeforeSendHeadersCallback().
> 
>
http://codereview.chromium.org/10911151/diff/5003/net/url_request/url_request...
> File net/url_request/url_request_unittest.cc (right):
> 
>
http://codereview.chromium.org/10911151/diff/5003/net/url_request/url_request...
> net/url_request/url_request_unittest.cc:706:
> //BlockingNetworkDelegateWithManualCallback::ON_BEFORE_URL_REQUEST,
> I'm not sure why, but if blocking ON_BEFORE_URL_REQUEST then the request gets
> not cancelled. Any ideas?

[vabr (Chromium), 2012-09-12 12:40:48]

Matt,
Big thanks for your speedy answers and for nailing the problem I had with the
unit-test.

Is there anything more in the CL you would like me to change? Including missing
comments for strangely-looking parts of the added code?

Cheers,
Vaclav

[mmenke, 2012-09-12 12:50:24]

On 2012/09/12 12:40:48, vabr (Chromium) wrote:
> Matt,
> Big thanks for your speedy answers and for nailing the problem I had with the
> unit-test.
> 
> Is there anything more in the CL you would like me to change? Including
missing
> comments for strangely-looking parts of the added code?
> 
> Cheers,
> Vaclav

Could you also add a test for synchronous cancellation, at least in the
NotifyBeforeSendHeaders case?  Thanks.

[vabr (Chromium), 2012-09-12 13:08:32]

> Could you also add a test for synchronous cancellation, at least in the
> NotifyBeforeSendHeaders case?  Thanks.

Sure!

I'm thinking of using BlockingNetworkDelegate to perform the synchronous cancel,
but currently it only can do that in onBeforeURLRequest.
There are a couple of options I can change that (in order of my preference):
1. Extend BlockingNetworkDelegate to allow specifying other stage to do the
cancel.
2. Modify BlockingNetworkDelegateWithManualCallback to allow synchronous actions
as well.
3. Make a different custom NetworkDelegate sub-class.

Do you have other preferences for this, or can I use option 1?

Thanks,
Vaclav

[mmenke, 2012-09-12 14:06:09]

On 2012/09/12 13:08:32, vabr (Chromium) wrote:
> > Could you also add a test for synchronous cancellation, at least in the
> > NotifyBeforeSendHeaders case?  Thanks.
> 
> Sure!
> 
> I'm thinking of using BlockingNetworkDelegate to perform the synchronous
cancel,
> but currently it only can do that in onBeforeURLRequest.
> There are a couple of options I can change that (in order of my preference):
> 1. Extend BlockingNetworkDelegate to allow specifying other stage to do the
> cancel.
> 2. Modify BlockingNetworkDelegateWithManualCallback to allow synchronous
actions
> as well.
> 3. Make a different custom NetworkDelegate sub-class.
> 
> Do you have other preferences for this, or can I use option 1?
> 
> Thanks,
> Vaclav

I'm fine with any of the 3 approaches.

[vabr (Chromium), 2012-09-26 13:44:11]

Hi Matt,

Getting back to the first CL -- I added both the synchronous and asynchronous
tests, as requested.

They shared most of the code, so I pulled that out in a separate function. Let
me know what you think about that.

Also, adding the other stages caused again problems in the Chrome Frame test. I
had to disable the tests there as well.
But that removed, in particular, the combination of SYNCHRONOUS and
OnBeforeRequest (covered by the original
NetworkDelegateCancelRequestSynchronously), which was originally run in the
Chrome Frame tests.
An alternative would be to separate the tests for OnBeforeRequest from those for
other stages, and only disable the latter in Chrome Frame tests.

The question is -- is it worth it?

Looking forward to your comments!

Vaclav

P.S. Starting tomorrow I'll have one week off. So first -- don't rush if you
don't have time to answer today, and second, don't be surprised if I'm late with
a response. :)

[mmenke, 2012-09-26 15:25:27]

LGTM.  Think it's reasonable just to not run them on Chrome Frame (Though,
admittedly, I'm not responsible for maintaining Chrome Frame).

[vabr (Chromium), 2012-09-26 15:31:48]

Thanks, Matt!

Erik, could you please review chrome_frame/test/net/fake_external_tab.cc as
OWNER?
(This is a different CL from yesterday, but the principle is the same.)

Thanks,
Vaclav

[erikwright (departed), 2012-09-26 15:41:42]

http://codereview.chromium.org/10911151/diff/21001/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10911151/diff/21001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:2073:
BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST,
Could you just make the blocking stage a parameter to this function and then
define six separate test-cases instead of two?

Or, alternatively, refactor this to be a parameterized test.

http://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Par...

[vabr (Chromium), 2012-09-26 16:04:08]

I split the tests as Erik suggested.

Erik, is that better now?

Matt, do you still like it?

Thanks,
Vaclav

http://codereview.chromium.org/10911151/diff/21001/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10911151/diff/21001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:2073:
BlockingNetworkDelegate::ON_BEFORE_URL_REQUEST,
On 2012/09/26 15:41:42, erikwright wrote:
> Could you just make the blocking stage a parameter to this function and then
> define six separate test-cases instead of two?

Done.
I disabled all of them in Chrome Frame, except for the OnBeforeRequest ones.

> 
> Or, alternatively, refactor this to be a parameterized test.
> 
>
http://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Par...

I did not go for the alternative, as the first option seems to be a pattern
already (see other URLRequestTestHTTP tests with numbers at the end of name). I
guess the refactoring would be significant enough to have a separate CL and
crbug dedicated to it.

But thanks for the pointer to the docs, they are quite interesting.

[mmenke, 2012-09-26 16:05:28]

Still LGTM

[erikwright (departed), 2012-09-26 16:07:03]

chrome_frame/* LGTM

[commit-bot: I haz the power, 2012-09-26 19:26:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10911151/26001

[commit-bot: I haz the power, 2012-09-26 21:09:54]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-27 05:01:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10911151/26001

[commit-bot: I haz the power, 2012-09-27 06:15:32]

Change committed as 158984

[battre, 2012-09-27 07:24:30]

https://chromiumcodereview.appspot.com/10911151/diff/26001/chrome_frame/test/...
File chrome_frame/test/net/fake_external_tab.cc (right):

https://chromiumcodereview.appspot.com/10911151/diff/26001/chrome_frame/test/...
chrome_frame/test/net/fake_external_tab.cc:212: // OnHeadersReceived, which do
not seem supported by CF.
Why are they not supported? This looks like a weak argument.

[vabr (Chromium), 2012-09-27 18:15:12]

https://chromiumcodereview.appspot.com/10911151/diff/26001/chrome_frame/test/...
File chrome_frame/test/net/fake_external_tab.cc (right):

https://chromiumcodereview.appspot.com/10911151/diff/26001/chrome_frame/test/...
chrome_frame/test/net/fake_external_tab.cc:212: // OnHeadersReceived, which do
not seem supported by CF.
On 2012/09/27 07:24:30, battre wrote:
> Why are they not supported? This looks like a weak argument.

I was re-using the argument from other parts of this file (see below).
Note that there is still actually one more test run for Chrome Frame after this
CL, compared to the state before.
I don't think there is a way to run these 4 tests under Chrome Frame, the stages
in which they block do not seem to be ever visited.