NQE: Use ResponseHeaders to get the HTTP status code

NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=663456
Committed: https://crrev.com/3f4b9da10b124cf841a5691abd4ede58f768be42
Cr-Commit-Position: refs/heads/master@{#430867}

[tbansal1, 2016-11-08 20:32:04]

Description was changed from

==========
Use ResponseHeaders to get the HTTP status code

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

to

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

BUG=663456
==========

[tbansal1, 2016-11-08 20:33:04]

Description was changed from

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

BUG=663456
==========

to

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of 
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

BUG=663456
==========

[tbansal1, 2016-11-08 20:33:32]

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

[tbansal1, 2016-11-08 20:33:33]

mmenke: ptal. Thanks.

[mmenke, 2016-11-08 20:46:56]

On 2016/11/08 20:33:33, tbansal1 wrote:
> mmenke: ptal. Thanks.

LGTM!

[mmenke, 2016-11-08 20:48:18]

On 2016/11/08 20:46:56, mmenke wrote:
> On 2016/11/08 20:33:33, tbansal1 wrote:
> > mmenke: ptal. Thanks.
> 
> LGTM!

Probably a good idea to add a test that would catch this (Make some TEST_P tests
that try different field trials, and make sure there's an NQE redirect test)

[tbansal1, 2016-11-08 21:15:29]

On 2016/11/08 20:48:18, mmenke wrote:
> On 2016/11/08 20:46:56, mmenke wrote:
> > On 2016/11/08 20:33:33, tbansal1 wrote:
> > > mmenke: ptal. Thanks.
> > 
> > LGTM!
> 
> Probably a good idea to add a test that would catch this (Make some TEST_P
tests
> that try different field trials, and make sure there's an NQE redirect test)

I added the test but it does not catch the regression.

[tbansal1, 2016-11-08 21:15:33]

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

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

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

[tbansal1, 2016-11-08 21:20:07]

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

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

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-08 21:41:14]

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

[commit-bot: I haz the power, 2016-11-08 21:41:15]

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

[tbansal1, 2016-11-08 21:50:18]

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

[commit-bot: I haz the power, 2016-11-08 21:51:00]

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

[mmenke, 2016-11-08 21:52:56]

https://codereview.chromium.org/2486033002/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator_test_util.cc (right):

https://codereview.chromium.org/2486033002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator_test_util.cc:88: return
embedded_test_server_.GetURL("/redirect302-to-https");
Doesn't look like the embedded test server is correctly configured to use the
directory with this file in it, so it's just returning the default response.

You need to use "PathService::Get(base::DIR_SOURCE_ROOT, &path);", and then
append net/data/url_request_unittest to that in your test_util code.

[commit-bot: I haz the power, 2016-11-08 23:07:34]

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

[commit-bot: I haz the power, 2016-11-08 23:07:35]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-11-09 01:26:34]

Description was changed from

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of 
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

BUG=663456
==========

to

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of 
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=663456
==========

[tbansal1, 2016-11-09 01:28:46]

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

[commit-bot: I haz the power, 2016-11-09 01:29:10]

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-09 02:23:13]

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

[commit-bot: I haz the power, 2016-11-09 02:23:14]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-11-09 03:56:20]

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

[commit-bot: I haz the power, 2016-11-09 03:56:48]

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-09 04:53:40]

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

[commit-bot: I haz the power, 2016-11-09 04:53:40]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-11-09 04:58:06]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-11-09 04:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2486033002/#ps80001
(title: "cleanup")

[commit-bot: I haz the power, 2016-11-09 04:58:13]

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

[tbansal1, 2016-11-09 04:59:18]

https://codereview.chromium.org/2486033002/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator_test_util.cc (right):

https://codereview.chromium.org/2486033002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator_test_util.cc:88: return
embedded_test_server_.GetURL("/redirect302-to-https");
On 2016/11/08 21:52:56, mmenke wrote:
> Doesn't look like the embedded test server is correctly configured to use the
> directory with this file in it, so it's just returning the default response.
> 
> You need to use "PathService::Get(base::DIR_SOURCE_ROOT, &path);", and then
> append net/data/url_request_unittest to that in your test_util code.

Fixed but in a different way.

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

Description was changed from

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of 
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=663456
==========

to

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of 
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=663456
==========

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-09 05:06:33]

Description was changed from

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of 
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=663456
==========

to

==========
NQE: Use ResponseHeaders to get the HTTP status code

After the request has completed, it is unsafe to access some of the
member functions of the request. This CL replaces invocation of
GetResponseCode() by looking up the response code in ResponseInfoHeaders.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=663456

Committed: https://crrev.com/3f4b9da10b124cf841a5691abd4ede58f768be42
Cr-Commit-Position: refs/heads/master@{#430867}
==========

[commit-bot: I haz the power, 2016-11-09 05:06:34]

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

[mmenke, 2016-11-09 15:05:21]

You probably should have waited for me to review the test.  So with the modified
test, can this code run into the crash, without the fix?

[tbansal1, 2016-11-09 22:38:37]

On 2016/11/09 15:05:21, mmenke wrote:
> You probably should have waited for me to review the test.  So with the
modified
> test, can this code run into the crash, without the fix?

Yes, the test crashes without the fix,
although only if I move the call to GetResponseCode() earlier.
Please see https://codereview.chromium.org/2486373002/ on 
how this test would crash if I move the call to the function earlier.
Moving the call earlier prevents the function
from returning earlier on Line 582:
(if (net_error != OK) {return;}).