Add Cronet experimental option to disable ipv6

Add Cronet experimental option to disable ipv6

An embedder has a specific problem that can't be worked around in any
other way :(

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2711153007
Cr-Commit-Position: refs/heads/master@{#453682}
Committed: https://chromium.googlesource.com/chromium/src/+/b3fe808c00e4716ff92a9629c8dea953689ba78e

[mgersh, 2017-02-24 23:14:38]

Description was changed from

==========
Add Cronet experimental option to disable ipv6

An embedder has a specific problem that can't be worked around in any
other way :(
==========

to

==========
Add Cronet experimental option to disable ipv6

An embedder has a specific problem that can't be worked around in any
other way :(
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-02-24 23:14:56]

mgersh@chromium.org changed reviewers:
+ xunjieli@chromium.org

[mgersh, 2017-02-24 23:14:57]

PTAL

[xunjieli, 2017-02-27 15:37:40]

https://codereview.chromium.org/2711153007/diff/1/components/cronet/stale_hos...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/2711153007/diff/1/components/cronet/stale_hos...
components/cronet/stale_host_resolver.cc:401: void
StaleHostResolver::SetDefaultAddressFamily(
Suggest removing the overrides and relying on the default implementation for the
StaleDns resolver.

Mixing two experiments makes reasoning difficult. If we have one bug in one
experiment, we can't isolate it. In the case of StaleDns, we can potentially
persist ipv6-disabled dns results. In subsequent sessions, we might revert to
default behavior (who knows maybe ipv4 is broken for some reason.) and find out
we may or may not use the default behavior... Suggest adding a check to make
sure this and "ipv6 disable" are not enabled at the same time.

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
components/cronet/url_request_context_config.cc:87: // reporting a bug.
nit: Open a bug and link it here.

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
components/cronet/url_request_context_config.cc:268: stale_dns_options));
nit: suggest add a DCHECK(disable_ipv6) here (see my previous comment)

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
components/cronet/url_request_context_config_unittest.cc:55:
"\"disable_ipv6\":true}",
nit: Add a comment before this line and link a crbug.

https://codereview.chromium.org/2711153007/diff/1/net/dns/mapped_host_resolve...
File net/dns/mapped_host_resolver.cc (right):

https://codereview.chromium.org/2711153007/diff/1/net/dns/mapped_host_resolve...
net/dns/mapped_host_resolver.cc:59: void
MappedHostResolver::SetDefaultAddressFamily(AddressFamily address_family) {
Is this needed? Cronet only use mapped host resolver in testing. You did not add
an integration/java test, so i assume this is not strictly necessary.

[mgersh, 2017-02-28 18:29:57]

https://codereview.chromium.org/2711153007/diff/1/components/cronet/stale_hos...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/2711153007/diff/1/components/cronet/stale_hos...
components/cronet/stale_host_resolver.cc:401: void
StaleHostResolver::SetDefaultAddressFamily(
On 2017/02/27 15:37:39, xunjieli wrote:
> Suggest removing the overrides and relying on the default implementation for
the
> StaleDns resolver.
> 
> Mixing two experiments makes reasoning difficult. If we have one bug in one
> experiment, we can't isolate it. In the case of StaleDns, we can potentially
> persist ipv6-disabled dns results. In subsequent sessions, we might revert to
> default behavior (who knows maybe ipv4 is broken for some reason.) and find
out
> we may or may not use the default behavior... Suggest adding a check to make
> sure this and "ipv6 disable" are not enabled at the same time. 

Done.

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
components/cronet/url_request_context_config.cc:87: // reporting a bug.
On 2017/02/27 15:37:39, xunjieli wrote:
> nit: Open a bug and link it here.

Done.

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
components/cronet/url_request_context_config.cc:268: stale_dns_options));
On 2017/02/27 15:37:39, xunjieli wrote:
> nit: suggest add a DCHECK(disable_ipv6) here (see my previous comment)

Done.

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/2711153007/diff/1/components/cronet/url_reque...
components/cronet/url_request_context_config_unittest.cc:55:
"\"disable_ipv6\":true}",
On 2017/02/27 15:37:39, xunjieli wrote:
> nit: Add a comment before this line and link a crbug.

Done.

https://codereview.chromium.org/2711153007/diff/1/net/dns/mapped_host_resolve...
File net/dns/mapped_host_resolver.cc (right):

https://codereview.chromium.org/2711153007/diff/1/net/dns/mapped_host_resolve...
net/dns/mapped_host_resolver.cc:59: void
MappedHostResolver::SetDefaultAddressFamily(AddressFamily address_family) {
On 2017/02/27 15:37:39, xunjieli wrote:
> Is this needed? Cronet only use mapped host resolver in testing. You did not
add
> an integration/java test, so i assume this is not strictly necessary.

It's needed for the unit test to work the way it's currently structured. I could
structure it differently but this seems more useful.

[xunjieli, 2017-02-28 18:39:59]

lgtm

https://codereview.chromium.org/2711153007/diff/40001/components/cronet/stale...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/2711153007/diff/40001/components/cronet/stale...
components/cronet/stale_host_resolver.cc:13: #include
"net/base/address_family.h"
can be reverted.

[xunjieli, 2017-02-28 18:40:30]

On 2017/02/28 18:39:59, xunjieli wrote:
> lgtm
> 
>
https://codereview.chromium.org/2711153007/diff/40001/components/cronet/stale...
> File components/cronet/stale_host_resolver.cc (right):
> 
>
https://codereview.chromium.org/2711153007/diff/40001/components/cronet/stale...
> components/cronet/stale_host_resolver.cc:13: #include
> "net/base/address_family.h"
> can be reverted.

Please also link this CL to the bug that you just filed.

[mgersh, 2017-02-28 18:42:33]

Description was changed from

==========
Add Cronet experimental option to disable ipv6

An embedder has a specific problem that can't be worked around in any
other way :(
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add Cronet experimental option to disable ipv6

An embedder has a specific problem that can't be worked around in any
other way :(

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-02-28 18:42:46]

https://codereview.chromium.org/2711153007/diff/40001/components/cronet/stale...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/2711153007/diff/40001/components/cronet/stale...
components/cronet/stale_host_resolver.cc:13: #include
"net/base/address_family.h"
On 2017/02/28 18:39:59, xunjieli wrote:
> can be reverted.

Done.

[mgersh, 2017-02-28 18:52:06]

The CQ bit was checked by mgersh@chromium.org

[mgersh, 2017-02-28 18:52:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2711153007/#ps60001
(title: "oops")

[commit-bot: I haz the power, 2017-02-28 18:52:32]

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, 2017-02-28 20:09:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488307926418520,
"parent_rev": "fdf249c81c06c4e679f4f1bedd1c5c030da029fa", "commit_rev":
"b3fe808c00e4716ff92a9629c8dea953689ba78e"}

[commit-bot: I haz the power, 2017-02-28 20:10:01]

Description was changed from

==========
Add Cronet experimental option to disable ipv6

An embedder has a specific problem that can't be worked around in any
other way :(

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add Cronet experimental option to disable ipv6

An embedder has a specific problem that can't be worked around in any
other way :(

BUG=696569
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2711153007
Cr-Commit-Position: refs/heads/master@{#453682}
Committed:
https://chromium.googlesource.com/chromium/src/+/b3fe808c00e4716ff92a9629c8de...
==========

[commit-bot: I haz the power, 2017-02-28 20:10:03]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b3fe808c00e4716ff92a9629c8de...