Add host resolver rules experimental flag for Cronet

Add host resolver rules experimental flag for Cronet

This option allows Cronet embedders to do end-to-end testing which
requires pointing requests to a particular test server.

BUG=594601
Committed: https://crrev.com/af2c12c7e4968c60f39b33e8244429146447f3a5
Cr-Commit-Position: refs/heads/master@{#413454}

[mgersh, 2016-03-18 18:09:40]

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

[mgersh, 2016-03-18 18:09:41]

PTAL. Not sure if this is still needed right now, but it was close to done
anyway.

[xunjieli, 2016-03-18 18:15:30]

On 2016/03/18 18:09:41, mgersh wrote:
> PTAL. Not sure if this is still needed right now, but it was close to done
> anyway.

Could you add a java test where we resolve a random host name to ip address of
embedded test server? and make sure the urlrequest succeeds.

[cbentzel, 2016-03-31 10:02:48]

On 2016/03/18 18:15:30, xunjieli wrote:
> On 2016/03/18 18:09:41, mgersh wrote:
> > PTAL. Not sure if this is still needed right now, but it was close to done
> > anyway.
> 
> Could you add a java test where we resolve a random host name to ip address of
> embedded test server? and make sure the urlrequest succeeds.

Thanks for getting this together - is there any progress on the test request?

[mgersh, 2016-04-01 19:17:41]

Sorry about the delay--here's the new test.

[xunjieli, 2016-04-01 19:26:04]

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

[xunjieli, 2016-04-01 19:26:04]

Paul, could you review this? you're most knowledgeable in DNS resolver.  It
sounds like in the email thread that we want to expose this functionality as a
stable API, so some API design decision is probably needed here as well.

[pauljensen, 2016-04-04 11:38:00]

On 2016/04/01 19:26:04, xunjieli wrote:
> Paul, could you review this? you're most knowledgeable in DNS resolver.  It
> sounds like in the email thread that we want to expose this functionality as a
> stable API, so some API design decision is probably needed here as well.

I briefly looked it over and it seems fine.  I'll take a more thorough look
soon.  As for exposing a stable API, I think we may want to first start with
this experimental flag, and if it becomes popular we can move to a stable API. 
It's a bit of a footgun so I'd like to see if embedders can use it safely.  If
not used correctly it can break connectivity in many ways and can impair
performance (actual resolvers know the closest server).

[xunjieli, 2016-04-05 16:37:01]

Description was changed from

==========
Add host resolver rules experimental flag for Cronet

This option allows Cronet embedders to do end-to-end testing which
requires pointing requests to a particular test server.

BUG=594601
==========

to

==========
Add host resolver rules experimental flag for Cronet

This option allows Cronet embedders to do end-to-end testing which
requires pointing requests to a particular test server.

BUG=594601
==========

[xunjieli, 2016-04-05 16:37:01]

xunjieli@chromium.org changed reviewers:
- xunjieli@chromium.org

[pauljensen, 2016-04-07 16:29:15]

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:45:
private static final String RESOLVER_TEST_HOSTNAME = "some-weird-hostname";
if this is just used for one test, how about we move it into that test.

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1030:
"host_resolver_rules", "MAP " + RESOLVER_TEST_HOSTNAME + " 127.0.0.1:8000");
I think port 8000 is the python test server running on the host, which we're
trying to avoid, can this instead use mUrl which points at the
EmbeddedTestServer?

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1039:
new UrlRequest.Builder("http://" + RESOLVER_TEST_HOSTNAME, callback,
can this also use mUrl?

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:57: const char
kHostResolverRulesFieldTrialName[] = "HostResolverRules";
can we add a comment above here documenting what this does? preferably with a
link to another comment explaining how to write such rules?

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:217: }
I think we need to simplify some things in this function by by adding the
precondition that net_log is non-null:
1. make net_log non-null, we can do this by making the three call sites other
than CronetUrlRequestContext just call "new NetLog()".
2. initialize |host_resolver| to CreateDefaultResolver(net_log)
3. remove lines 197-199,202,212-214,222,226,228,221
4. changing line 218 to "host_resolver.reset("

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:103:
TEST(URLRequestContextConfigTest, SetDNSExperimentalOptions) {
how about we just merge this into SetQuicExperimentalOptions by adding the JSON
config and EXPECT_TRUE from this function to that one?  we should also rename
SetQuicExperimentalOptions to something more general (it's already testing
things other than QUIC stuff), perhaps TestExperimentalOptionPassing

[mgersh, 2016-06-08 18:31:14]

I'm finally getting back to finishing this one. Sorry it took forever.

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:45:
private static final String RESOLVER_TEST_HOSTNAME = "some-weird-hostname";
On 2016/04/07 16:29:15, pauljensen wrote:
> if this is just used for one test, how about we move it into that test.

Done.

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1030:
"host_resolver_rules", "MAP " + RESOLVER_TEST_HOSTNAME + " 127.0.0.1:8000");
On 2016/04/07 16:29:15, pauljensen wrote:
> I think port 8000 is the python test server running on the host, which we're
> trying to avoid, can this instead use mUrl which points at the
> EmbeddedTestServer?

Done, with some fiddling to get the right parts of mUrl in the right places.

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1039:
new UrlRequest.Builder("http://" + RESOLVER_TEST_HOSTNAME, callback,
On 2016/04/07 16:29:15, pauljensen wrote:
> can this also use mUrl?

Done.

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:57: const char
kHostResolverRulesFieldTrialName[] = "HostResolverRules";
On 2016/04/07 16:29:15, pauljensen wrote:
> can we add a comment above here documenting what this does? preferably with a
> link to another comment explaining how to write such rules?

Done.

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:217: }
On 2016/04/07 16:29:15, pauljensen wrote:
> I think we need to simplify some things in this function by by adding the
> precondition that net_log is non-null:
> 1. make net_log non-null, we can do this by making the three call sites other
> than CronetUrlRequestContext just call "new NetLog()".
> 2. initialize |host_resolver| to CreateDefaultResolver(net_log)
> 3. remove lines 197-199,202,212-214,222,226,228,221
> 4. changing line 218 to "host_resolver.reset("

Done.

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/1817553002/diff/40001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:103:
TEST(URLRequestContextConfigTest, SetDNSExperimentalOptions) {
On 2016/04/07 16:29:15, pauljensen wrote:
> how about we just merge this into SetQuicExperimentalOptions by adding the
JSON
> config and EXPECT_TRUE from this function to that one?  we should also rename
> SetQuicExperimentalOptions to something more general (it's already testing
> things other than QUIC stuff), perhaps TestExperimentalOptionPassing

Done.

[pauljensen, 2016-06-10 14:19:37]

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1184:
String[] testUrlParts = mUrl.split("/");
this is pretty confusing, why not:
URL testUrl = new URL(mUrl);
....
testUrl.getHost()
....
testUrl.getPath()

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1196:
new UrlRequest.Builder("http://" + resolverTestHostname + "/" + testUrlParts[3],
nit: maybe build the URL with:
         new URL("http", resolverHostname, testUrl.getPath())

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:154: new net::NetLog(),
nullptr);
looks like a leak

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:74: DCHECK(net_log != nullptr);
remove " != nullptr" so it matches other DCHECKs in this file

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:295: DCHECK(net_log) << "A valid
NetLog is required";
Can we remove the error message?  This is an internal API that I don't think we
regularly add call sites for, and I'd rather not increase the size of Cronet for
an error message we're unlikely to ever see.

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:65:
config.ConfigureURLRequestContextBuilder(&builder, new net::NetLog(),
why the change to create NetLog on the heap?  looks like a leak

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:103:
EXPECT_TRUE(context->host_resolver()->GetDnsConfigAsValue());
Please add a test that the rules were installed in the host resolver

[mgersh, 2016-06-29 17:43:32]

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1184:
String[] testUrlParts = mUrl.split("/");
On 2016/06/10 14:19:37, pauljensen wrote:
> this is pretty confusing, why not:
> URL testUrl = new URL(mUrl);
> ....
> testUrl.getHost()
> ....
> testUrl.getPath()

I didn't know that existed. Done.

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:1196:
new UrlRequest.Builder("http://" + resolverTestHostname + "/" + testUrlParts[3],
On 2016/06/10 14:19:37, pauljensen wrote:
> nit: maybe build the URL with:
>          new URL("http", resolverHostname, testUrl.getPath())

Done.

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:154: new net::NetLog(),
nullptr);
On 2016/06/10 14:19:37, pauljensen wrote:
> looks like a leak

Added a global netlog similar to the one in
cronet_url_request_context_adapter.cc, which I think is what we discussed before
I left.

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:74: DCHECK(net_log != nullptr);
On 2016/06/10 14:19:37, pauljensen wrote:
> remove " != nullptr" so it matches other DCHECKs in this file

Done.

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config.cc:295: DCHECK(net_log) << "A valid
NetLog is required";
On 2016/06/10 14:19:37, pauljensen wrote:
> Can we remove the error message?  This is an internal API that I don't think
we
> regularly add call sites for, and I'd rather not increase the size of Cronet
for
> an error message we're unlikely to ever see.

Done.

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:65:
config.ConfigureURLRequestContextBuilder(&builder, new net::NetLog(),
On 2016/06/10 14:19:37, pauljensen wrote:
> why the change to create NetLog on the heap?  looks like a leak

Done.

https://codereview.chromium.org/1817553002/diff/60001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:103:
EXPECT_TRUE(context->host_resolver()->GetDnsConfigAsValue());
On 2016/06/10 14:19:37, pauljensen wrote:
> Please add a test that the rules were installed in the host resolver

Done.

[pauljensen, 2016-06-30 15:26:37]

Can we replace all the CronetTestUtil.registerHostResolverProc() logic with this
now?

https://codereview.chromium.org/1817553002/diff/80001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/1817553002/diff/80001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:106:
std::unique_ptr<net::AddressList> addresses(new net::AddressList);
net::AddressList addresses;

[mgersh, 2016-06-30 22:53:03]

On 2016/06/30 15:26:37, pauljensen wrote:
> Can we replace all the CronetTestUtil.registerHostResolverProc() logic with
this
> now?

I don't really see the benefit--adding an experimental options JSON string seems
uglier than what's there now, for not significantly less code, although I guess
it would be done entirely in Java which could be nice.

If you do still think it's nicer, I'd prefer to file a bug and do it in a
follow-up CL, since it would require non-bulk changes to a bunch of tests (they
all keep their CronetEngine.Builder in different places) and I'd like to get
this submitted.

> 
>
https://codereview.chromium.org/1817553002/diff/80001/components/cronet/url_r...
> File components/cronet/url_request_context_config_unittest.cc (right):
> 
>
https://codereview.chromium.org/1817553002/diff/80001/components/cronet/url_r...
> components/cronet/url_request_context_config_unittest.cc:106:
> std::unique_ptr<net::AddressList> addresses(new net::AddressList);
> net::AddressList addresses;

[mgersh, 2016-06-30 22:54:54]

https://codereview.chromium.org/1817553002/diff/80001/components/cronet/url_r...
File components/cronet/url_request_context_config_unittest.cc (right):

https://codereview.chromium.org/1817553002/diff/80001/components/cronet/url_r...
components/cronet/url_request_context_config_unittest.cc:106:
std::unique_ptr<net::AddressList> addresses(new net::AddressList);
On 2016/06/30 15:26:37, pauljensen wrote:
> net::AddressList addresses;

Done.

[pauljensen, 2016-07-01 11:47:11]

On 2016/06/30 22:53:03, mgersh wrote:
> On 2016/06/30 15:26:37, pauljensen wrote:
> > Can we replace all the CronetTestUtil.registerHostResolverProc() logic with
> this
> > now?
> 
> I don't really see the benefit--adding an experimental options JSON string
seems
> uglier than what's there now, for not significantly less code, although I
guess
> it would be done entirely in Java which could be nice.

I disagree, CronetTestUtil.registerHostResolverProc() is very hacky:
1. downcasts CronetEngine
2. uses ugly backdoor getUrlRequestContextAdapter()
3. uses a ConditionVariable to ensure synchronization between two threads
With your new method, all this code is simply redundant.

> If you do still think it's nicer, I'd prefer to file a bug and do it in a
> follow-up CL, since it would require non-bulk changes to a bunch of tests
(they
> all keep their CronetEngine.Builder in different places) and I'd like to get
> this submitted.

I don't think it will take too long.  I'd prefer it was done in this CL but if
you want to do it in a follow-up CL, that's fine but I want to know that it'll
happen very soon.  I hate having duplicate redundant code, esp when one is very
hacky.  The motivating bug here has been open for 3 months, so I'm not sure we
need to rush to land this when it incurs technical debt.

[mgersh, 2016-07-22 15:32:55]

I uploaded a new patch set here a while ago but maybe it didn't send email since
I didn't send any comments? I don't really understand how this thing works.
Anyway, I deleted registerHostResolverProc().

[pauljensen, 2016-07-22 16:11:50]

On 2016/07/22 15:32:55, mgersh wrote:
> I uploaded a new patch set here a while ago but maybe it didn't send email
since
> I didn't send any comments? I don't really understand how this thing works.
> Anyway, I deleted registerHostResolverProc().

Ya, it's rather confusing.  Uploading patch sets and other activities don't send
email (unlike gerrit where uploading a patch set does send email).  To send
email in rietveld either use "Publish+Mail comments" or use "Reply" with "Send
mail to reviewers" checked.  I'll take another look at the CL soon.

[pauljensen, 2016-07-26 18:18:52]

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/NativeTestServer.java
(left):

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/NativeTestServer.java:56:
return nativeGetSdchURL();
Can we go back to this old code so we don't have to duplicate the host in two
places?

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/QuicTestServer.java
(left):

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/QuicTestServer.java:60:
return nativeGetServerHost();
ditto

[mgersh, 2016-07-27 21:14:58]

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/NativeTestServer.java
(left):

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/NativeTestServer.java:56:
return nativeGetSdchURL();
On 2016/07/26 18:18:51, pauljensen wrote:
> Can we go back to this old code so we don't have to duplicate the host in two
> places?

I don't understand the complaint. I moved "fake.sdch.domain" (and
"test.example.com") from cronet_test_util.cc to CronetTestUtil.java, which
doesn't add any duplication.

[pauljensen, 2016-08-17 12:54:26]

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
File components/cronet/android/test/src/org/chromium/net/NativeTestServer.java
(left):

https://codereview.chromium.org/1817553002/diff/120001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/NativeTestServer.java:56:
return nativeGetSdchURL();
On 2016/07/27 21:14:58, mgersh wrote:
> On 2016/07/26 18:18:51, pauljensen wrote:
> > Can we go back to this old code so we don't have to duplicate the host in
two
> > places?
> 
> I don't understand the complaint. I moved "fake.sdch.domain" (and
> "test.example.com") from cronet_test_util.cc to CronetTestUtil.java, which
> doesn't add any duplication.

GetSdchURL() is defined in native_test_server.cc.  You're adding a duplicate
copy of the URL  here.

[pauljensen, 2016-08-22 15:08:46]

lgtm

It's a little weird to me that the QUIC and SDCH server host names are stored in
CronetTestUtil instead of QuicTestServer and NativeTestServer, but I guess this
makes sense because those servers actually respond to any host name and so the
only place the host name counts is in the resolver rules.

I guess I wouldn't be opposed to moving those host name String constants back
into QuicTestServer and NativeTestServer but it's fine the way it is now.

[mgersh, 2016-08-22 15:42:24]

The CQ bit was checked by mgersh@chromium.org

[commit-bot: I haz the power, 2016-08-22 15:43:01]

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-08-22 16:34:10]

Description was changed from

==========
Add host resolver rules experimental flag for Cronet

This option allows Cronet embedders to do end-to-end testing which
requires pointing requests to a particular test server.

BUG=594601
==========

to

==========
Add host resolver rules experimental flag for Cronet

This option allows Cronet embedders to do end-to-end testing which
requires pointing requests to a particular test server.

BUG=594601
==========

[commit-bot: I haz the power, 2016-08-22 16:34:12]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-22 16:35:29]

Description was changed from

==========
Add host resolver rules experimental flag for Cronet

This option allows Cronet embedders to do end-to-end testing which
requires pointing requests to a particular test server.

BUG=594601
==========

to

==========
Add host resolver rules experimental flag for Cronet

This option allows Cronet embedders to do end-to-end testing which
requires pointing requests to a particular test server.

BUG=594601

Committed: https://crrev.com/af2c12c7e4968c60f39b33e8244429146447f3a5
Cr-Commit-Position: refs/heads/master@{#413454}
==========

[commit-bot: I haz the power, 2016-08-22 16:35:33]

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