Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23181010:
Fast-fail WPAD detection. (Closed)
|
Created:
7 years, 4 months ago by Elly Fong-Jones Modified:
7 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFast-fail WPAD detection.
Right now, if there's no WPAD host on the local network, we have to wait up to
two seconds for ProxyScriptDecider to time out, during which all requests are
stalled waiting to find out what proxy to use. This is a pretty crappy
experience when you have proxy autodetection turned on and no WPAD host.
Therefore, add a new state to ProxyScriptDecider's state machine: "QuickCheck".
When we're in this state, we are doing a DNS query only for WPAD, with a timeout
of 1 second; if this query doesn't succeed, we abort the entire WPAD process. We
also upload an UMA report saying how long the WPAD resolution took to succeed.
In future, once we have a good idea how long WPAD requests usually actually
take, we'll turn this timeout down, probably to something extremely low (from
experiments, IE appears to use a timeout of 50ms).
TEST=unit,trybot,manual
BUG=chromium:161826
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224030
Patch Set 1 #
Total comments: 5
Patch Set 2 : Refactor #
Total comments: 4
Patch Set 3 : Move STATE_QUICK_CHECK after STATE_WAIT #
Total comments: 13
Patch Set 4 : Fix nits #
Total comments: 22
Patch Set 5 : Undo API change #
Total comments: 8
Patch Set 6 : Add unit tests #
Total comments: 20
Patch Set 7 : Unit test fixes #
Total comments: 2
Patch Set 8 : Only invoke native resolver. #
Total comments: 10
Patch Set 9 : Remove SetHostResolverForTesting #
Total comments: 6
Patch Set 10 : Add HostResolverImpl test for SYSTEM_ONLY #
Total comments: 1
Patch Set 11 : info_native -> info_bypass #Patch Set 12 : Rebase to LKGR #Patch Set 13 : Rebase to LKGR #
Total comments: 1
Patch Set 14 : Fix windows build warning #Patch Set 15 : Disable QuickCheck with explicit PAC URL #Patch Set 16 : Fix comment in MockHostResolver #Patch Set 17 : Re-fix MockHostResolver #
Messages
Total messages: 47 (0 generated)
Please take a look at my approach for this. I haven't yet written unit tests, so the CL is not complete, but I'd like early feedback.
Looks good. A few suggestions, and you'll need to lint it for code style. https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decide... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decide... net/proxy/proxy_script_decider.cc:230: // already in cache and invalid, bail now We don't actually cache negative results in HostResolverImpl. A synchronous failure can only happen due to incorrect request (not a well-formed domain name or IP address) or HostResolverImpl's internal request queue overflowing. So, I'd probably CHECK here. https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decide... net/proxy/proxy_script_decider.cc:244: next_state_ = STATE_FAILED; You'll probably want to UMA this as well. Putting it in the same histogram would work. This way you'd be able to completely replace OnQuickCheckTimerFired with OnQuickCheckComplete(ERR_NAME_NOT_RESOLVED). https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decider.h File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decide... net/proxy/proxy_script_decider.h:191: base::TimeDelta quick_check_delay_; Do you anticipate changing this value? If not, declare const int kQuickCheckDelaySeconds in an anonymous namespace (in .cc) and construct TimeDelta where you need it. https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decide... net/proxy/proxy_script_decider.h:193: base::Time quick_check_started_; Instead of storing it here, bind it into the Callback passed to the |quick_check_timer_|.
https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decider.h File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/1/net/proxy/proxy_script_decide... net/proxy/proxy_script_decider.h:193: base::Time quick_check_started_; On 2013/08/21 19:11:21, szym wrote: > Instead of storing it here, bind it into the Callback passed to the > |quick_check_timer_|. Both the timer and HostResolver::Request. CompletionCallback callback = base::Bind(&...::OnQuickCheckComplete, base::Unretained(this), base::Time::Now()); Resolve(..., callback); timer_.Start(FROM_HERE, delay, base::Bind(callback, ERR_NAME_NOT_RESOLVED)); ...::OnQuickCheckComplete(base::Time start_time, int result) { // compute TimeDelta, UMA result, OnIOComplete(result) } For this to work timer_ needs to be base::Timer(false, false).
https://codereview.chromium.org/23181010/diff/8001/net/proxy/proxy_script_dec... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/8001/net/proxy/proxy_script_dec... net/proxy/proxy_script_decider.cc:218: If you do CompletionCallback callback = base::Bind(&ProxyScript, ...Now()); then... https://codereview.chromium.org/23181010/diff/8001/net/proxy/proxy_script_dec... net/proxy/proxy_script_decider.cc:247: base::Time::Now(), ... you can just pass: base::Bind(callback, ERR_NAME_NOT_RESOLVED)
https://codereview.chromium.org/23181010/diff/8001/net/proxy/proxy_script_dec... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/8001/net/proxy/proxy_script_dec... net/proxy/proxy_script_decider.cc:176: break; To be compatible with the DoLoop state machine you should: case STATE_QUICK_CHECK_COMPLETE: rv = DoQuickCheckComplete(rv) https://codereview.chromium.org/23181010/diff/8001/net/proxy/proxy_script_dec... net/proxy/proxy_script_decider.cc:222: &ProxyScriptDecider::OnQuickCheckComplete, To be compatible with the DoLoop state machine you should just set next_state_ = STATE_QUICK_CHECK_COMPLETE and pass OnIOComplete here. This means that your original approach of storing the quick_check_start_time_ was more DoLoop-compatible.
https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:217: int ProxyScriptDecider::DoQuickCheck() { nit: Move both after DoWait() (to match my nit regarding the header) https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:224: int rv = host_resolver_.Resolve(reqinfo, HIGHEST, // we block startup... nit: suggest putting this comment above this line. Also per code style: "Use full sentences." https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:233: next_state_ = STATE_FETCH_PAC_SCRIPT; Perhaps this should be GetStartState()... ... but something tells me that you should after QUICK_CHECK we should always go to FETCH_PAC_SCRIPT, and after WAIT we should go to either QUICK_CHECK or VERIFY_PAC_SCRIPT, which means you need to update the other transitions in lines 252 and 276. Frankly, I'm not quite sure if WAIT should be preceding VERIFY_PAC. I'll need to read Eric's old comments. https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:109: STATE_QUICK_CHECK, nit: these should be after STATE_WAIT https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:127: int DoQuickCheck(); nit: these should be after DoWait() https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:187: AddressList wpad_addrs_; nit: wpad_addresses_ https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:190: base::Time quick_check_started_; nit: ...start_time_
https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:217: int ProxyScriptDecider::DoQuickCheck() { On 2013/08/23 20:45:57, szym wrote: > nit: Move both after DoWait() (to match my nit regarding the header) Done. https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:224: int rv = host_resolver_.Resolve(reqinfo, HIGHEST, // we block startup... On 2013/08/23 20:45:57, szym wrote: > nit: suggest putting this comment above this line. > > Also per code style: "Use full sentences." Done. https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:109: STATE_QUICK_CHECK, On 2013/08/23 20:45:57, szym wrote: > nit: these should be after STATE_WAIT Done. https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:127: int DoQuickCheck(); On 2013/08/23 20:45:57, szym wrote: > nit: these should be after DoWait() Done. https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:187: AddressList wpad_addrs_; On 2013/08/23 20:45:57, szym wrote: > nit: wpad_addresses_ Done. https://codereview.chromium.org/23181010/diff/13001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:190: base::Time quick_check_started_; On 2013/08/23 20:45:57, szym wrote: > nit: ...start_time_ Done.
https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:252: // we can't get an error response - the name is known to be valid, and we nit: capitalize first letter in a sentence https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:254: CHECK(rv == OK || rv == ERR_IO_PENDING); Change this to a DCHECK. Start the timer only if rv == ERR_IO_PENDING return rv at the end. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:257: // already in cache or something and valid, we're golden No need for the comment. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:274: UMA_HISTOGRAM_TIMES("Net.WpadQuickCheck", delta); I suggest you distinguish successes from failures. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:277: next_state_ = result == OK ? GetStartState() : STATE_NONE; if (result == OK) next_state_ = GetStartState(); (STATE_NONE is the default next_state_) https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:117: STATE_FAILED, You don't need STATE_FAILED. Remove.
https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:258: next_state_ = STATE_FETCH_PAC_SCRIPT; Should be GetStartState()
Are you planning on writing unit tests? (Hint: you should be...) https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:52: static const int kQuickCheckDelayMs = 1000; Probably better to move this into the anonymous namespace and remove the static declaration (ditto for KWpadUrl). https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:90: proxy_script_fetcher->GetRequestContext()->host_resolver()) { Nit: I think I'd prefer passing in the HostResolver* as an argument instead of fetching via the proxy_script_fetcher. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:90: proxy_script_fetcher->GetRequestContext()->host_resolver()) { I'm a bit confused. I thought the reason we are using the SingleRequestHostResolver (rather than URLRequestContext's host resolver) is to try to force using getaddrinfo when resolving wpad. I don't see anything here which forces that. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:274: UMA_HISTOGRAM_TIMES("Net.WpadQuickCheck", delta); On 2013/08/26 17:24:31, szym wrote: > I suggest you distinguish successes from failures. +1
https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:90: proxy_script_fetcher->GetRequestContext()->host_resolver()) { On 2013/08/26 18:19:50, cbentzel wrote: > Nit: I think I'd prefer passing in the HostResolver* as an argument instead of > fetching via the proxy_script_fetcher. The rationale here is that QUICK_CHECK simply establishes a short timeout on the HostResolver part of proxy_script_fetcher. To do that, it should be using the same HostResolver instance. This way there is only one DNS query and it ends up in one HostCache. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:90: proxy_script_fetcher->GetRequestContext()->host_resolver()) { On 2013/08/26 18:19:50, cbentzel wrote: > I'm a bit confused. I thought the reason we are using the > SingleRequestHostResolver (rather than URLRequestContext's host resolver) is to > try to force using getaddrinfo when resolving wpad. I don't see anything here > which forces that. I am not sure this matters. If it does matter, we should be using a custom version of HostResolver (as in an earlier, since then deleted, patch) and on success, stuff the result in the HostCache of proxy_script_fetcher's HostResolver.
On 2013/08/26 18:31:33, szym wrote: > https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... > File net/proxy/proxy_script_decider.cc (right): > > https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... > net/proxy/proxy_script_decider.cc:90: > proxy_script_fetcher->GetRequestContext()->host_resolver()) { > On 2013/08/26 18:19:50, cbentzel wrote: > > Nit: I think I'd prefer passing in the HostResolver* as an argument instead of > > fetching via the proxy_script_fetcher. > > The rationale here is that QUICK_CHECK simply establishes a short timeout on the > HostResolver part of proxy_script_fetcher. To do that, it should be using the > same HostResolver instance. This way there is only one DNS query and it ends up > in one HostCache. > > https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... > net/proxy/proxy_script_decider.cc:90: > proxy_script_fetcher->GetRequestContext()->host_resolver()) { > On 2013/08/26 18:19:50, cbentzel wrote: > > I'm a bit confused. I thought the reason we are using the > > SingleRequestHostResolver (rather than URLRequestContext's host resolver) is > to > > try to force using getaddrinfo when resolving wpad. I don't see anything here > > which forces that. > > I am not sure this matters. If it does matter, we should be using a custom > version of HostResolver (as in an earlier, since then deleted, patch) and on > success, stuff the result in the HostCache of proxy_script_fetcher's > HostResolver. My primary concern is if we do an A/AAAA query and take a while to fallback to getaddrinfo, that it will be well after we have short-circuited WPAD discovery - especially if most resolutions take place over LLMNR/NetBIOS.
(1) Should not slow down the custom PAC URL with resolution of "wpad". ProxyScriptDecider handles both custom PAC URLs and WPAD. Resolving the name "wpad" is unnecessary for custom PAC URLs and hence should not block the state machine. (2) Should the DNS failfast option be done for custom PAC URLs as well? Probably not, however that improve a performance issue for Googler's whose laptops hardcode it on all networks. (3) The consequences of this sort of change are rather subtle and could break certain users setups. Can you write up a document (independent of the bug) which explains the policy, and justification? I am sure we will get resulting bug reports, so it would be good to have something we can point to those users. (4) We can tackle this as a follow-up, however it seems to me like rather than aborting the "wpad" DNS request (which is likely going to keep running anyway), we should consider the result when it completes. If we fail fast, but then it completes 2 seconds later, shouldn't we switch over to to the discovered script?
On 2013/08/28 19:35:23, eroman wrote: > (1) Should not slow down the custom PAC URL with resolution of "wpad". > > ProxyScriptDecider handles both custom PAC URLs and WPAD. Resolving the name > "wpad" is unnecessary for custom PAC URLs and hence should not block the state > machine. > > (2) Should the DNS failfast option be done for custom PAC URLs as well? Probably > not, however that improve a performance issue for Googler's whose laptops > hardcode it on all networks. > > (3) The consequences of this sort of change are rather subtle and could break > certain users setups. Can you write up a document (independent of the bug) which > explains the policy, and justification? I am sure we will get resulting bug > reports, so it would be good to have something we can point to those users. > > (4) We can tackle this as a follow-up, however it seems to me like rather than > aborting the "wpad" DNS request (which is likely going to keep running anyway), > we should consider the result when it completes. If we fail fast, but then it > completes 2 seconds later, shouldn't we switch over to to the discovered script? For (3) and (4) we were effectively looking at how IE9+10 on Win7 handles slow wpad discovery and basing the behavior on that. The general assumption is that being consistent with IE's policy will work well for the majority of networks that care about WPAD. IE does not take into account the script if the DNS response comes after the initial short window. For (1) and (2) - thanks for pointing out the bugs!
https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:52: static const int kQuickCheckDelayMs = 1000; On 2013/08/26 18:19:50, cbentzel wrote: > Probably better to move this into the anonymous namespace and remove the static > declaration (ditto for KWpadUrl). Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:90: proxy_script_fetcher->GetRequestContext()->host_resolver()) { On 2013/08/26 18:31:34, szym wrote: > On 2013/08/26 18:19:50, cbentzel wrote: > > I'm a bit confused. I thought the reason we are using the > > SingleRequestHostResolver (rather than URLRequestContext's host resolver) is > to > > try to force using getaddrinfo when resolving wpad. I don't see anything here > > which forces that. > > I am not sure this matters. If it does matter, we should be using a custom > version of HostResolver (as in an earlier, since then deleted, patch) and on > success, stuff the result in the HostCache of proxy_script_fetcher's > HostResolver. Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:252: // we can't get an error response - the name is known to be valid, and we On 2013/08/26 17:24:31, szym wrote: > nit: capitalize first letter in a sentence Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:254: CHECK(rv == OK || rv == ERR_IO_PENDING); On 2013/08/26 17:24:31, szym wrote: > Change this to a DCHECK. > > Start the timer only if rv == ERR_IO_PENDING > > return rv at the end. Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:257: // already in cache or something and valid, we're golden On 2013/08/26 17:24:31, szym wrote: > No need for the comment. Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:258: next_state_ = STATE_FETCH_PAC_SCRIPT; On 2013/08/26 17:25:39, szym wrote: > Should be GetStartState() Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:274: UMA_HISTOGRAM_TIMES("Net.WpadQuickCheck", delta); On 2013/08/26 18:19:50, cbentzel wrote: > On 2013/08/26 17:24:31, szym wrote: > > I suggest you distinguish successes from failures. > > +1 Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:277: next_state_ = result == OK ? GetStartState() : STATE_NONE; On 2013/08/26 17:24:31, szym wrote: > if (result == OK) > next_state_ = GetStartState(); > > (STATE_NONE is the default next_state_) Done. https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/17001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:117: STATE_FAILED, On 2013/08/26 17:24:31, szym wrote: > You don't need STATE_FAILED. Remove. Done.
looks good, any tests? https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:91: && proxy_script_fetcher->GetRequestContext() && on the previous line, and don't use extra spaces after ( https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:252: if (host_resolver_.get() == NULL) { Move it to top of function. https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:253: // if we have no resolver, skip QuickCheck altogether. nit: If https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:280: - quick_check_start_time_; - on previous line, but it fits on one line anyway
https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:91: && proxy_script_fetcher->GetRequestContext() On 2013/09/09 23:58:51, szym wrote: > && on the previous line, and don't use extra spaces after ( Done. https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:252: if (host_resolver_.get() == NULL) { On 2013/09/09 23:58:51, szym wrote: > Move it to top of function. Done. https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:253: // if we have no resolver, skip QuickCheck altogether. On 2013/09/09 23:58:51, szym wrote: > nit: If Done. https://codereview.chromium.org/23181010/diff/29001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:280: - quick_check_start_time_; On 2013/09/09 23:58:51, szym wrote: > - on previous line, but it fits on one line anyway Done.
https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:93: proxy_script_fetcher->GetRequestContext()->host_resolver())); Ok, so just triple-checking here. This will make the query using the DnsClient if enabled. If "wpad" is served via NetBIOS or LLMNR then this request will have to fail with DNS twice. That said, I don't suspect DNS queries for "wpad" are particularly slow (unless there is a deep suffix search list and domain name devolution enabled). To be sure, we could add a measurement for how long do DNS queries for "wpad" take. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider_unittest.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:115: virtual void SetRequestContext(URLRequestContext* context) { nit: move this out of here, so that the "ProxyScriptFetcher implementation" is a single block. I suggest moving it right after the ctor. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:262: virtual void SetUp() { OVERRIDE https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:269: virtual int StartDecider() { why virtual? https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:278: private: I'd suggest making this protected and using directly instead of the three getters above. The goal is to have less boiler plate. If you don't want to expose everything, have a separate private section. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:313: base::MessageLoop::current()->RunUntilIdle(); You should callback_.WaitForResult rather than RunUntilIdle. BTW MessageLoop::RunUntilIdle is deprecated and new code should be using base::RunLoop().RunUntilIdle() https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:324: // let the timer run I don't understand the comment. There's barely any time spent here letting anything happen. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:330: You need a test which exercises the quick check timeout. I.e., don't call ResolveAllPending, wait for callback with failure.
https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:93: proxy_script_fetcher->GetRequestContext()->host_resolver())); On 2013/09/11 14:06:22, szym wrote: > Ok, so just triple-checking here. > > This will make the query using the DnsClient if enabled. If "wpad" is served via > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > That said, I don't suspect DNS queries for "wpad" are particularly slow (unless > there is a deep suffix search list and domain name devolution enabled). > > To be sure, we could add a measurement for how long do DNS queries for "wpad" > take. Which component is doing the LLMNR and NetBIOS queries? https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider_unittest.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:115: virtual void SetRequestContext(URLRequestContext* context) { On 2013/09/11 14:06:22, szym wrote: > nit: move this out of here, so that the "ProxyScriptFetcher implementation" is a > single block. I suggest moving it right after the ctor. Done. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:262: virtual void SetUp() { On 2013/09/11 14:06:22, szym wrote: > OVERRIDE Done. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:269: virtual int StartDecider() { On 2013/09/11 14:06:22, szym wrote: > why virtual? Habit. Removed. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:278: private: On 2013/09/11 14:06:22, szym wrote: > I'd suggest making this protected and using directly instead of the three > getters above. The goal is to have less boiler plate. > > If you don't want to expose everything, have a separate private section. Done. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:313: base::MessageLoop::current()->RunUntilIdle(); On 2013/09/11 14:06:22, szym wrote: > You should callback_.WaitForResult rather than RunUntilIdle. > > BTW MessageLoop::RunUntilIdle is deprecated and new code should be using > base::RunLoop().RunUntilIdle() Oh man, I never knew about WaitForResult! This also lets me write the failure test I wanted to do - I tried it before with RunUntilIdle(), but it didn't work. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:324: // let the timer run On 2013/09/11 14:06:22, szym wrote: > I don't understand the comment. There's barely any time spent here letting > anything happen. Done. https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:330: On 2013/09/11 14:06:22, szym wrote: > You need a test which exercises the quick check timeout. > > I.e., don't call ResolveAllPending, wait for callback with failure. Done.
just a few minor issues https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:93: proxy_script_fetcher->GetRequestContext()->host_resolver())); On 2013/09/11 20:20:37, Elly Jones wrote: > On 2013/09/11 14:06:22, szym wrote: > > Ok, so just triple-checking here. > > > > This will make the query using the DnsClient if enabled. If "wpad" is served > via > > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > > > That said, I don't suspect DNS queries for "wpad" are particularly slow > (unless > > there is a deep suffix search list and domain name devolution enabled). > > > > To be sure, we could add a measurement for how long do DNS queries for "wpad" > > take. > > Which component is doing the LLMNR and NetBIOS queries? If DnsClient is enabled, the call flow is: HostResolver::Resolve DnsClient...DnsTransaction...Start // fails getaddrinfo DNSAPI::QueryDns // fails LLMNR/NetBIOS // maybe succeeds https://codereview.chromium.org/23181010/diff/40001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider_unittest.cc (right): https://codereview.chromium.org/23181010/diff/40001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:291: // Fails if a synchronous DNS lookup success for wpad causes QuickCheck to fail. The common practice, at least in net/ and certainly within this file is that the comment explains the asserted component behavior rather than what the test does, i.e., this should be // QuickCheck passes if DNS lookup for wpad succeeds synchronously. https://codereview.chromium.org/23181010/diff/40001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider_unittest.cc:337: callback_.WaitForResult(); WaitForResult returns the neterror passed to the CompletionCallback, so this should be wrapped in EXPECT_EQ(ERR_NAME_NOT_RESOLVED, ...)
https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:93: proxy_script_fetcher->GetRequestContext()->host_resolver())); On 2013/09/11 21:29:21, szym wrote: > On 2013/09/11 20:20:37, Elly Jones wrote: > > On 2013/09/11 14:06:22, szym wrote: > > > Ok, so just triple-checking here. > > > > > > This will make the query using the DnsClient if enabled. If "wpad" is served > > via > > > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > > > > > That said, I don't suspect DNS queries for "wpad" are particularly slow > > (unless > > > there is a deep suffix search list and domain name devolution enabled). > > > > > > To be sure, we could add a measurement for how long do DNS queries for > "wpad" > > > take. > > > > Which component is doing the LLMNR and NetBIOS queries? > > If DnsClient is enabled, the call flow is: > HostResolver::Resolve > DnsClient...DnsTransaction...Start // fails > getaddrinfo > DNSAPI::QueryDns // fails > LLMNR/NetBIOS // maybe succeeds I thought we had decided to do a SingleRequestHostResolver which would only do the getaddrinfo based resolution for WPAD. I think this is necessary particularly if we roll out the built-in DNS resolver widely on Windows.
https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:93: proxy_script_fetcher->GetRequestContext()->host_resolver())); On 2013/09/12 13:44:30, cbentzel wrote: > On 2013/09/11 21:29:21, szym wrote: > > On 2013/09/11 20:20:37, Elly Jones wrote: > > > On 2013/09/11 14:06:22, szym wrote: > > > > Ok, so just triple-checking here. > > > > > > > > This will make the query using the DnsClient if enabled. If "wpad" is > served > > > via > > > > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > > > > > > > That said, I don't suspect DNS queries for "wpad" are particularly slow > > > (unless > > > > there is a deep suffix search list and domain name devolution enabled). > > > > > > > > To be sure, we could add a measurement for how long do DNS queries for > > "wpad" > > > > take. > > > > > > Which component is doing the LLMNR and NetBIOS queries? > > > > If DnsClient is enabled, the call flow is: > > HostResolver::Resolve > > DnsClient...DnsTransaction...Start // fails > > getaddrinfo > > DNSAPI::QueryDns // fails > > LLMNR/NetBIOS // maybe succeeds > > I thought we had decided to do a SingleRequestHostResolver which would only do > the getaddrinfo based resolution for WPAD. I think this is necessary > particularly if we roll out the built-in DNS resolver widely on Windows. I don't know how often wpad is served via LLMNR/NetBIOS, but this channel is always tried last (after DNS). So disabling DnsClient for this HostResolver::Resolve request only helps if the DNS query for wpad takes a long time. This could happen if the latency to the DNS server is significant. Do we suspect that the DNS queries take a long time? If so, we should certainly create a custom HostResolver and subsequently stuff the response in the global HostResolver's cache.
https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:93: proxy_script_fetcher->GetRequestContext()->host_resolver())); On 2013/09/12 13:44:30, cbentzel wrote: > On 2013/09/11 21:29:21, szym wrote: > > On 2013/09/11 20:20:37, Elly Jones wrote: > > > On 2013/09/11 14:06:22, szym wrote: > > > > Ok, so just triple-checking here. > > > > > > > > This will make the query using the DnsClient if enabled. If "wpad" is > served > > > via > > > > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > > > > > > > That said, I don't suspect DNS queries for "wpad" are particularly slow > > > (unless > > > > there is a deep suffix search list and domain name devolution enabled). > > > > > > > > To be sure, we could add a measurement for how long do DNS queries for > > "wpad" > > > > take. > > > > > > Which component is doing the LLMNR and NetBIOS queries? > > > > If DnsClient is enabled, the call flow is: > > HostResolver::Resolve > > DnsClient...DnsTransaction...Start // fails > > getaddrinfo > > DNSAPI::QueryDns // fails > > LLMNR/NetBIOS // maybe succeeds > > I thought we had decided to do a SingleRequestHostResolver which would only do > the getaddrinfo based resolution for WPAD. I think this is necessary > particularly if we roll out the built-in DNS resolver widely on Windows. Stuffing the cache is a little bit more work. // in ctor host_resolver_(HostResolver::CreateDefaultResolver(net_log)) single_request_resolver_(host_resolver_.get()) // in DoQuickCheck host_resolver_->SetDnsClientEnabled(false); // in DoQuickCheckComplete if (result == OK) { HostCache* cache = proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); if (cache) { cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), HostCache::Entry(wpad_addresses_)); } }
On 2013/09/12 18:01:51, szym wrote: > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > File net/proxy/proxy_script_decider.cc (right): > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > net/proxy/proxy_script_decider.cc:93: > proxy_script_fetcher->GetRequestContext()->host_resolver())); > On 2013/09/12 13:44:30, cbentzel wrote: > > On 2013/09/11 21:29:21, szym wrote: > > > On 2013/09/11 20:20:37, Elly Jones wrote: > > > > On 2013/09/11 14:06:22, szym wrote: > > > > > Ok, so just triple-checking here. > > > > > > > > > > This will make the query using the DnsClient if enabled. If "wpad" is > > served > > > > via > > > > > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > > > > > > > > > That said, I don't suspect DNS queries for "wpad" are particularly slow > > > > (unless > > > > > there is a deep suffix search list and domain name devolution enabled). > > > > > > > > > > To be sure, we could add a measurement for how long do DNS queries for > > > "wpad" > > > > > take. > > > > > > > > Which component is doing the LLMNR and NetBIOS queries? > > > > > > If DnsClient is enabled, the call flow is: > > > HostResolver::Resolve > > > DnsClient...DnsTransaction...Start // fails > > > getaddrinfo > > > DNSAPI::QueryDns // fails > > > LLMNR/NetBIOS // maybe succeeds > > > > I thought we had decided to do a SingleRequestHostResolver which would only do > > the getaddrinfo based resolution for WPAD. I think this is necessary > > particularly if we roll out the built-in DNS resolver widely on Windows. > > Stuffing the cache is a little bit more work. > > // in ctor > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) > single_request_resolver_(host_resolver_.get()) > > // in DoQuickCheck > host_resolver_->SetDnsClientEnabled(false); > > // in DoQuickCheckComplete > if (result == OK) { > HostCache* cache = > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); > if (cache) { > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), > HostCache::Entry(wpad_addresses_)); > } > } I don't think we need to worry about stuffing host cache. WPAD queries are done very infrequently and given that we only do the queries at network transition time we'd end up wiping out the results anyway.
On 2013/09/12 21:02:12, cbentzel wrote: > On 2013/09/12 18:01:51, szym wrote: > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > File net/proxy/proxy_script_decider.cc (right): > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > net/proxy/proxy_script_decider.cc:93: > > proxy_script_fetcher->GetRequestContext()->host_resolver())); > > On 2013/09/12 13:44:30, cbentzel wrote: > > > On 2013/09/11 21:29:21, szym wrote: > > > > On 2013/09/11 20:20:37, Elly Jones wrote: > > > > > On 2013/09/11 14:06:22, szym wrote: > > > > > > Ok, so just triple-checking here. > > > > > > > > > > > > This will make the query using the DnsClient if enabled. If "wpad" is > > > served > > > > > via > > > > > > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > > > > > > > > > > > That said, I don't suspect DNS queries for "wpad" are particularly > slow > > > > > (unless > > > > > > there is a deep suffix search list and domain name devolution > enabled). > > > > > > > > > > > > To be sure, we could add a measurement for how long do DNS queries for > > > > "wpad" > > > > > > take. > > > > > > > > > > Which component is doing the LLMNR and NetBIOS queries? > > > > > > > > If DnsClient is enabled, the call flow is: > > > > HostResolver::Resolve > > > > DnsClient...DnsTransaction...Start // fails > > > > getaddrinfo > > > > DNSAPI::QueryDns // fails > > > > LLMNR/NetBIOS // maybe succeeds > > > > > > I thought we had decided to do a SingleRequestHostResolver which would only > do > > > the getaddrinfo based resolution for WPAD. I think this is necessary > > > particularly if we roll out the built-in DNS resolver widely on Windows. > > > > Stuffing the cache is a little bit more work. > > > > // in ctor > > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) > > single_request_resolver_(host_resolver_.get()) > > > > // in DoQuickCheck > > host_resolver_->SetDnsClientEnabled(false); > > > > // in DoQuickCheckComplete > > if (result == OK) { > > HostCache* cache = > > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); > > if (cache) { > > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), > > HostCache::Entry(wpad_addresses_)); > > } > > } > > I don't think we need to worry about stuffing host cache. WPAD queries are done > very infrequently and given that we only do the queries at network transition > time we'd end up wiping out the results anyway. Ah - then I guess we can easily create a HostResolver which bypasses DnsClient. When we looked at how this was done from Chrome on Windows, almost all the queries were done using NetBIOS/LLMNR only. Not sure if the windows host cache had a no-response for the DNS A queries.
On 2013/09/12 21:03:40, cbentzel wrote: > On 2013/09/12 21:02:12, cbentzel wrote: > > On 2013/09/12 18:01:51, szym wrote: > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > > File net/proxy/proxy_script_decider.cc (right): > > > > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > > net/proxy/proxy_script_decider.cc:93: > > > proxy_script_fetcher->GetRequestContext()->host_resolver())); > > > On 2013/09/12 13:44:30, cbentzel wrote: > > > > On 2013/09/11 21:29:21, szym wrote: > > > > > On 2013/09/11 20:20:37, Elly Jones wrote: > > > > > > On 2013/09/11 14:06:22, szym wrote: > > > > > > > Ok, so just triple-checking here. > > > > > > > > > > > > > > This will make the query using the DnsClient if enabled. If "wpad" > is > > > > served > > > > > > via > > > > > > > NetBIOS or LLMNR then this request will have to fail with DNS twice. > > > > > > > > > > > > > > That said, I don't suspect DNS queries for "wpad" are particularly > > slow > > > > > > (unless > > > > > > > there is a deep suffix search list and domain name devolution > > enabled). > > > > > > > > > > > > > > To be sure, we could add a measurement for how long do DNS queries > for > > > > > "wpad" > > > > > > > take. > > > > > > > > > > > > Which component is doing the LLMNR and NetBIOS queries? > > > > > > > > > > If DnsClient is enabled, the call flow is: > > > > > HostResolver::Resolve > > > > > DnsClient...DnsTransaction...Start // fails > > > > > getaddrinfo > > > > > DNSAPI::QueryDns // fails > > > > > LLMNR/NetBIOS // maybe succeeds > > > > > > > > I thought we had decided to do a SingleRequestHostResolver which would > only > > do > > > > the getaddrinfo based resolution for WPAD. I think this is necessary > > > > particularly if we roll out the built-in DNS resolver widely on Windows. > > > > > > Stuffing the cache is a little bit more work. > > > > > > // in ctor > > > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) > > > single_request_resolver_(host_resolver_.get()) > > > > > > // in DoQuickCheck > > > host_resolver_->SetDnsClientEnabled(false); > > > > > > // in DoQuickCheckComplete > > > if (result == OK) { > > > HostCache* cache = > > > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); > > > if (cache) { > > > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), > > > HostCache::Entry(wpad_addresses_)); > > > } > > > } > > > > I don't think we need to worry about stuffing host cache. WPAD queries are > done > > very infrequently and given that we only do the queries at network transition > > time we'd end up wiping out the results anyway. > > Ah - then I guess we can easily create a HostResolver which bypasses DnsClient. > > When we looked at how this was done from Chrome on Windows, almost all the > queries were done using NetBIOS/LLMNR only. Not sure if the windows host cache > had a no-response for the DNS A queries. ellyjones+szym: It seems like we could just have the ProxyScriptDecider call CreateDefaultHostResolver and then SetDnsClientEnabled on it, and use that rather than the URLRequestContext's HostResolver. We would want to copy over netlog, but do you see any concerns beyond that?
On 2013/09/13 09:28:45, cbentzel wrote: > On 2013/09/12 21:03:40, cbentzel wrote: > > On 2013/09/12 21:02:12, cbentzel wrote: > > > On 2013/09/12 18:01:51, szym wrote: > > > > > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > > > File net/proxy/proxy_script_decider.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > > > net/proxy/proxy_script_decider.cc:93: > > > > proxy_script_fetcher->GetRequestContext()->host_resolver())); > > > > On 2013/09/12 13:44:30, cbentzel wrote: > > > > > On 2013/09/11 21:29:21, szym wrote: > > > > > > On 2013/09/11 20:20:37, Elly Jones wrote: > > > > > > > On 2013/09/11 14:06:22, szym wrote: > > > > > > > > Ok, so just triple-checking here. > > > > > > > > > > > > > > > > This will make the query using the DnsClient if enabled. If "wpad" > > is > > > > > served > > > > > > > via > > > > > > > > NetBIOS or LLMNR then this request will have to fail with DNS > twice. > > > > > > > > > > > > > > > > That said, I don't suspect DNS queries for "wpad" are particularly > > > slow > > > > > > > (unless > > > > > > > > there is a deep suffix search list and domain name devolution > > > enabled). > > > > > > > > > > > > > > > > To be sure, we could add a measurement for how long do DNS queries > > for > > > > > > "wpad" > > > > > > > > take. > > > > > > > > > > > > > > Which component is doing the LLMNR and NetBIOS queries? > > > > > > > > > > > > If DnsClient is enabled, the call flow is: > > > > > > HostResolver::Resolve > > > > > > DnsClient...DnsTransaction...Start // fails > > > > > > getaddrinfo > > > > > > DNSAPI::QueryDns // fails > > > > > > LLMNR/NetBIOS // maybe succeeds > > > > > > > > > > I thought we had decided to do a SingleRequestHostResolver which would > > only > > > do > > > > > the getaddrinfo based resolution for WPAD. I think this is necessary > > > > > particularly if we roll out the built-in DNS resolver widely on Windows. > > > > > > > > Stuffing the cache is a little bit more work. > > > > > > > > // in ctor > > > > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) > > > > single_request_resolver_(host_resolver_.get()) > > > > > > > > // in DoQuickCheck > > > > host_resolver_->SetDnsClientEnabled(false); > > > > > > > > // in DoQuickCheckComplete > > > > if (result == OK) { > > > > HostCache* cache = > > > > > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); > > > > if (cache) { > > > > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), > > > > HostCache::Entry(wpad_addresses_)); > > > > } > > > > } > > > > > > I don't think we need to worry about stuffing host cache. WPAD queries are > > done > > > very infrequently and given that we only do the queries at network > transition > > > time we'd end up wiping out the results anyway. > > > > Ah - then I guess we can easily create a HostResolver which bypasses > DnsClient. > > > > When we looked at how this was done from Chrome on Windows, almost all the > > queries were done using NetBIOS/LLMNR only. Not sure if the windows host cache > > had a no-response for the DNS A queries. > > ellyjones+szym: It seems like we could just have the ProxyScriptDecider call > CreateDefaultHostResolver and then SetDnsClientEnabled on it, and use that > rather than the URLRequestContext's HostResolver. We would want to copy over > netlog, but do you see any concerns beyond that? Is there a reason to prefer the default host resolver?
On 2013/09/13 14:12:03, Elly Jones wrote: > On 2013/09/13 09:28:45, cbentzel wrote: > > On 2013/09/12 21:03:40, cbentzel wrote: > > > On 2013/09/12 21:02:12, cbentzel wrote: > > > > On 2013/09/12 18:01:51, szym wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > > > > File net/proxy/proxy_script_decider.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > > > > > net/proxy/proxy_script_decider.cc:93: > > > > > proxy_script_fetcher->GetRequestContext()->host_resolver())); > > > > > On 2013/09/12 13:44:30, cbentzel wrote: > > > > > > On 2013/09/11 21:29:21, szym wrote: > > > > > > > On 2013/09/11 20:20:37, Elly Jones wrote: > > > > > > > > On 2013/09/11 14:06:22, szym wrote: > > > > > > > > > Ok, so just triple-checking here. > > > > > > > > > > > > > > > > > > This will make the query using the DnsClient if enabled. If > "wpad" > > > is > > > > > > served > > > > > > > > via > > > > > > > > > NetBIOS or LLMNR then this request will have to fail with DNS > > twice. > > > > > > > > > > > > > > > > > > That said, I don't suspect DNS queries for "wpad" are > particularly > > > > slow > > > > > > > > (unless > > > > > > > > > there is a deep suffix search list and domain name devolution > > > > enabled). > > > > > > > > > > > > > > > > > > To be sure, we could add a measurement for how long do DNS > queries > > > for > > > > > > > "wpad" > > > > > > > > > take. > > > > > > > > > > > > > > > > Which component is doing the LLMNR and NetBIOS queries? > > > > > > > > > > > > > > If DnsClient is enabled, the call flow is: > > > > > > > HostResolver::Resolve > > > > > > > DnsClient...DnsTransaction...Start // fails > > > > > > > getaddrinfo > > > > > > > DNSAPI::QueryDns // fails > > > > > > > LLMNR/NetBIOS // maybe succeeds > > > > > > > > > > > > I thought we had decided to do a SingleRequestHostResolver which would > > > only > > > > do > > > > > > the getaddrinfo based resolution for WPAD. I think this is necessary > > > > > > particularly if we roll out the built-in DNS resolver widely on > Windows. > > > > > > > > > > Stuffing the cache is a little bit more work. > > > > > > > > > > // in ctor > > > > > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) > > > > > single_request_resolver_(host_resolver_.get()) > > > > > > > > > > // in DoQuickCheck > > > > > host_resolver_->SetDnsClientEnabled(false); > > > > > > > > > > // in DoQuickCheckComplete > > > > > if (result == OK) { > > > > > HostCache* cache = > > > > > > > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); > > > > > if (cache) { > > > > > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), > > > > > HostCache::Entry(wpad_addresses_)); > > > > > } > > > > > } > > > > > > > > I don't think we need to worry about stuffing host cache. WPAD queries are > > > done > > > > very infrequently and given that we only do the queries at network > > transition > > > > time we'd end up wiping out the results anyway. > > > > > > Ah - then I guess we can easily create a HostResolver which bypasses > > DnsClient. > > > > > > When we looked at how this was done from Chrome on Windows, almost all the > > > queries were done using NetBIOS/LLMNR only. Not sure if the windows host > cache > > > had a no-response for the DNS A queries. > > > > ellyjones+szym: It seems like we could just have the ProxyScriptDecider call > > CreateDefaultHostResolver and then SetDnsClientEnabled on it, and use that > > rather than the URLRequestContext's HostResolver. We would want to copy over > > netlog, but do you see any concerns beyond that? > > Is there a reason to prefer the default host resolver? They are both "default" in the sense that they are created with CreateDefaultHostResolver. Chris is suggesting to always use an internal instance, rather than internal one for QuickCheck and the one in the URLRequestContext for the actual PAC fetch. This would save us the need to stuff the HostCache.
Uh, I was actually looking to have a separate one for QuickCheck that always calls getaddrinfo. I don't think there's a real need to stuff the HostCache at all since the only time we do WPAD queries are after network changes, and those network changes will wipe out the HostCache. On Fri, Sep 13, 2013 at 10:17 AM, <szym@chromium.org> wrote: > On 2013/09/13 14:12:03, Elly Jones wrote: >> >> On 2013/09/13 09:28:45, cbentzel wrote: >> > On 2013/09/12 21:03:40, cbentzel wrote: >> > > On 2013/09/12 21:02:12, cbentzel wrote: >> > > > On 2013/09/12 18:01:51, szym wrote: >> > > > > >> > > > >> > > >> > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... >> >> > > > > File net/proxy/proxy_script_decider.cc (right): >> > > > > >> > > > > >> > > > >> > > >> > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... >> >> > > > > net/proxy/proxy_script_decider.cc:93: >> > > > > proxy_script_fetcher->GetRequestContext()->host_resolver())); >> > > > > On 2013/09/12 13:44:30, cbentzel wrote: >> > > > > > On 2013/09/11 21:29:21, szym wrote: >> > > > > > > On 2013/09/11 20:20:37, Elly Jones wrote: >> > > > > > > > On 2013/09/11 14:06:22, szym wrote: >> > > > > > > > > Ok, so just triple-checking here. >> > > > > > > > > >> > > > > > > > > This will make the query using the DnsClient if enabled. >> > > > > > > > > If >> "wpad" >> > > is >> > > > > > served >> > > > > > > > via >> > > > > > > > > NetBIOS or LLMNR then this request will have to fail with >> > > > > > > > > DNS >> > twice. >> > > > > > > > > >> > > > > > > > > That said, I don't suspect DNS queries for "wpad" are >> particularly >> > > > slow >> > > > > > > > (unless >> > > > > > > > > there is a deep suffix search list and domain name >> > > > > > > > > devolution >> > > > enabled). >> > > > > > > > > >> > > > > > > > > To be sure, we could add a measurement for how long do DNS >> queries >> > > for >> > > > > > > "wpad" >> > > > > > > > > take. >> > > > > > > > >> > > > > > > > Which component is doing the LLMNR and NetBIOS queries? >> > > > > > > >> > > > > > > If DnsClient is enabled, the call flow is: >> > > > > > > HostResolver::Resolve >> > > > > > > DnsClient...DnsTransaction...Start // fails >> > > > > > > getaddrinfo >> > > > > > > DNSAPI::QueryDns // fails >> > > > > > > LLMNR/NetBIOS // maybe succeeds >> > > > > > >> > > > > > I thought we had decided to do a SingleRequestHostResolver which > > would >> >> > > only >> > > > do >> > > > > > the getaddrinfo based resolution for WPAD. I think this is >> > > > > > necessary >> > > > > > particularly if we roll out the built-in DNS resolver widely on >> Windows. >> > > > > >> > > > > Stuffing the cache is a little bit more work. >> > > > > >> > > > > // in ctor >> > > > > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) >> > > > > single_request_resolver_(host_resolver_.get()) >> > > > > >> > > > > // in DoQuickCheck >> > > > > host_resolver_->SetDnsClientEnabled(false); >> > > > > >> > > > > // in DoQuickCheckComplete >> > > > > if (result == OK) { >> > > > > HostCache* cache = >> > > > > >> > >> > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); >> > > > > if (cache) { >> > > > > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), >> > > > > HostCache::Entry(wpad_addresses_)); >> > > > > } >> > > > > } >> > > > >> > > > I don't think we need to worry about stuffing host cache. WPAD >> > > > queries > > are >> >> > > done >> > > > very infrequently and given that we only do the queries at network >> > transition >> > > > time we'd end up wiping out the results anyway. >> > > >> > > Ah - then I guess we can easily create a HostResolver which bypasses >> > DnsClient. >> > > >> > > When we looked at how this was done from Chrome on Windows, almost all >> > > the >> > > queries were done using NetBIOS/LLMNR only. Not sure if the windows >> > > host >> cache >> > > had a no-response for the DNS A queries. >> > >> > ellyjones+szym: It seems like we could just have the ProxyScriptDecider >> > call >> > CreateDefaultHostResolver and then SetDnsClientEnabled on it, and use >> > that >> > rather than the URLRequestContext's HostResolver. We would want to copy >> > over >> > netlog, but do you see any concerns beyond that? > > >> Is there a reason to prefer the default host resolver? > > > They are both "default" in the sense that they are created with > CreateDefaultHostResolver. Chris is suggesting to always use an internal > instance, rather than internal one for QuickCheck and the one in the > URLRequestContext for the actual PAC fetch. This would save us the need to > stuff > the HostCache. > > https://codereview.chromium.org/23181010/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I was thinking that If we don't stuff the cache we will end up making the PAC fetch longer by the time spent in quickcheck because the fetcher will need to call getaddrinfo again. Then I realized that getaddrinfo on windows has its own cache so there is no need to do this. On Sep 13, 2013 1:10 PM, "Chris Bentzel" <cbentzel@chromium.org> wrote: > Uh, I was actually looking to have a separate one for QuickCheck that > always calls getaddrinfo. I don't think there's a real need to stuff > the HostCache at all since the only time we do WPAD queries are after > network changes, and those network changes will wipe out the > HostCache. > > On Fri, Sep 13, 2013 at 10:17 AM, <szym@chromium.org> wrote: > > On 2013/09/13 14:12:03, Elly Jones wrote: > >> > >> On 2013/09/13 09:28:45, cbentzel wrote: > >> > On 2013/09/12 21:03:40, cbentzel wrote: > >> > > On 2013/09/12 21:02:12, cbentzel wrote: > >> > > > On 2013/09/12 18:01:51, szym wrote: > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > >> > >> > > > > File net/proxy/proxy_script_decider.cc (right): > >> > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... > >> > >> > > > > net/proxy/proxy_script_decider.cc:93: > >> > > > > proxy_script_fetcher->GetRequestContext()->host_resolver())); > >> > > > > On 2013/09/12 13:44:30, cbentzel wrote: > >> > > > > > On 2013/09/11 21:29:21, szym wrote: > >> > > > > > > On 2013/09/11 20:20:37, Elly Jones wrote: > >> > > > > > > > On 2013/09/11 14:06:22, szym wrote: > >> > > > > > > > > Ok, so just triple-checking here. > >> > > > > > > > > > >> > > > > > > > > This will make the query using the DnsClient if enabled. > >> > > > > > > > > If > >> "wpad" > >> > > is > >> > > > > > served > >> > > > > > > > via > >> > > > > > > > > NetBIOS or LLMNR then this request will have to fail > with > >> > > > > > > > > DNS > >> > twice. > >> > > > > > > > > > >> > > > > > > > > That said, I don't suspect DNS queries for "wpad" are > >> particularly > >> > > > slow > >> > > > > > > > (unless > >> > > > > > > > > there is a deep suffix search list and domain name > >> > > > > > > > > devolution > >> > > > enabled). > >> > > > > > > > > > >> > > > > > > > > To be sure, we could add a measurement for how long do > DNS > >> queries > >> > > for > >> > > > > > > "wpad" > >> > > > > > > > > take. > >> > > > > > > > > >> > > > > > > > Which component is doing the LLMNR and NetBIOS queries? > >> > > > > > > > >> > > > > > > If DnsClient is enabled, the call flow is: > >> > > > > > > HostResolver::Resolve > >> > > > > > > DnsClient...DnsTransaction...Start // fails > >> > > > > > > getaddrinfo > >> > > > > > > DNSAPI::QueryDns // fails > >> > > > > > > LLMNR/NetBIOS // maybe succeeds > >> > > > > > > >> > > > > > I thought we had decided to do a SingleRequestHostResolver > which > > > > would > >> > >> > > only > >> > > > do > >> > > > > > the getaddrinfo based resolution for WPAD. I think this is > >> > > > > > necessary > >> > > > > > particularly if we roll out the built-in DNS resolver widely > on > >> Windows. > >> > > > > > >> > > > > Stuffing the cache is a little bit more work. > >> > > > > > >> > > > > // in ctor > >> > > > > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) > >> > > > > single_request_resolver_(host_resolver_.get()) > >> > > > > > >> > > > > // in DoQuickCheck > >> > > > > host_resolver_->SetDnsClientEnabled(false); > >> > > > > > >> > > > > // in DoQuickCheckComplete > >> > > > > if (result == OK) { > >> > > > > HostCache* cache = > >> > > > > > >> > > >> > > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); > >> > > > > if (cache) { > >> > > > > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), > >> > > > > HostCache::Entry(wpad_addresses_)); > >> > > > > } > >> > > > > } > >> > > > > >> > > > I don't think we need to worry about stuffing host cache. WPAD > >> > > > queries > > > > are > >> > >> > > done > >> > > > very infrequently and given that we only do the queries at network > >> > transition > >> > > > time we'd end up wiping out the results anyway. > >> > > > >> > > Ah - then I guess we can easily create a HostResolver which bypasses > >> > DnsClient. > >> > > > >> > > When we looked at how this was done from Chrome on Windows, almost > all > >> > > the > >> > > queries were done using NetBIOS/LLMNR only. Not sure if the windows > >> > > host > >> cache > >> > > had a no-response for the DNS A queries. > >> > > >> > ellyjones+szym: It seems like we could just have the > ProxyScriptDecider > >> > call > >> > CreateDefaultHostResolver and then SetDnsClientEnabled on it, and use > >> > that > >> > rather than the URLRequestContext's HostResolver. We would want to > copy > >> > over > >> > netlog, but do you see any concerns beyond that? > > > > > >> Is there a reason to prefer the default host resolver? > > > > > > They are both "default" in the sense that they are created with > > CreateDefaultHostResolver. Chris is suggesting to always use an internal > > instance, rather than internal one for QuickCheck and the one in the > > URLRequestContext for the actual PAC fetch. This would save us the need > to > > stuff > > the HostCache. > > > > https://codereview.chromium.org/23181010/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Cool. My primary concerns are a) Making sure that we don't falsely declare "There are no WPAD servers" by being too aggressive with our timeout. b) Still making the timeout as small as possible so that most of the users who have WPAD on but no WPAD servers will not be penalized particularly at startup. I don't care as much about adding a tiny bit more fetch latency if there are WPAD servers. On Fri, Sep 13, 2013 at 4:51 PM, Szymon Jakubczak <szym@chromium.org> wrote: > I was thinking that If we don't stuff the cache we will end up making the > PAC fetch longer by the time spent in quickcheck because the fetcher will > need to call getaddrinfo again. Then I realized that getaddrinfo on windows > has its own cache so there is no need to do this. > > On Sep 13, 2013 1:10 PM, "Chris Bentzel" <cbentzel@chromium.org> wrote: >> >> Uh, I was actually looking to have a separate one for QuickCheck that >> always calls getaddrinfo. I don't think there's a real need to stuff >> the HostCache at all since the only time we do WPAD queries are after >> network changes, and those network changes will wipe out the >> HostCache. >> >> On Fri, Sep 13, 2013 at 10:17 AM, <szym@chromium.org> wrote: >> > On 2013/09/13 14:12:03, Elly Jones wrote: >> >> >> >> On 2013/09/13 09:28:45, cbentzel wrote: >> >> > On 2013/09/12 21:03:40, cbentzel wrote: >> >> > > On 2013/09/12 21:02:12, cbentzel wrote: >> >> > > > On 2013/09/12 18:01:51, szym wrote: >> >> > > > > >> >> > > > >> >> > > >> >> > >> > >> > >> > >> > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... >> >> >> >> > > > > File net/proxy/proxy_script_decider.cc (right): >> >> > > > > >> >> > > > > >> >> > > > >> >> > > >> >> > >> > >> > >> > >> > https://codereview.chromium.org/23181010/diff/34001/net/proxy/proxy_script_de... >> >> >> >> > > > > net/proxy/proxy_script_decider.cc:93: >> >> > > > > proxy_script_fetcher->GetRequestContext()->host_resolver())); >> >> > > > > On 2013/09/12 13:44:30, cbentzel wrote: >> >> > > > > > On 2013/09/11 21:29:21, szym wrote: >> >> > > > > > > On 2013/09/11 20:20:37, Elly Jones wrote: >> >> > > > > > > > On 2013/09/11 14:06:22, szym wrote: >> >> > > > > > > > > Ok, so just triple-checking here. >> >> > > > > > > > > >> >> > > > > > > > > This will make the query using the DnsClient if >> >> > > > > > > > > enabled. >> >> > > > > > > > > If >> >> "wpad" >> >> > > is >> >> > > > > > served >> >> > > > > > > > via >> >> > > > > > > > > NetBIOS or LLMNR then this request will have to fail >> >> > > > > > > > > with >> >> > > > > > > > > DNS >> >> > twice. >> >> > > > > > > > > >> >> > > > > > > > > That said, I don't suspect DNS queries for "wpad" are >> >> particularly >> >> > > > slow >> >> > > > > > > > (unless >> >> > > > > > > > > there is a deep suffix search list and domain name >> >> > > > > > > > > devolution >> >> > > > enabled). >> >> > > > > > > > > >> >> > > > > > > > > To be sure, we could add a measurement for how long do >> >> > > > > > > > > DNS >> >> queries >> >> > > for >> >> > > > > > > "wpad" >> >> > > > > > > > > take. >> >> > > > > > > > >> >> > > > > > > > Which component is doing the LLMNR and NetBIOS queries? >> >> > > > > > > >> >> > > > > > > If DnsClient is enabled, the call flow is: >> >> > > > > > > HostResolver::Resolve >> >> > > > > > > DnsClient...DnsTransaction...Start // fails >> >> > > > > > > getaddrinfo >> >> > > > > > > DNSAPI::QueryDns // fails >> >> > > > > > > LLMNR/NetBIOS // maybe succeeds >> >> > > > > > >> >> > > > > > I thought we had decided to do a SingleRequestHostResolver >> >> > > > > > which >> > >> > would >> >> >> >> > > only >> >> > > > do >> >> > > > > > the getaddrinfo based resolution for WPAD. I think this is >> >> > > > > > necessary >> >> > > > > > particularly if we roll out the built-in DNS resolver widely >> >> > > > > > on >> >> Windows. >> >> > > > > >> >> > > > > Stuffing the cache is a little bit more work. >> >> > > > > >> >> > > > > // in ctor >> >> > > > > host_resolver_(HostResolver::CreateDefaultResolver(net_log)) >> >> > > > > single_request_resolver_(host_resolver_.get()) >> >> > > > > >> >> > > > > // in DoQuickCheck >> >> > > > > host_resolver_->SetDnsClientEnabled(false); >> >> > > > > >> >> > > > > // in DoQuickCheckComplete >> >> > > > > if (result == OK) { >> >> > > > > HostCache* cache = >> >> > > > > >> >> > >> >> > >> >> > proxy_script_fetcher_->GetRequestContext()->host_resolver()->GetHostCache(); >> >> > > > > if (cache) { >> >> > > > > cache->Set(HostCache::Key("wpad", ADDRESS_FAMILY_..., 0), >> >> > > > > HostCache::Entry(wpad_addresses_)); >> >> > > > > } >> >> > > > > } >> >> > > > >> >> > > > I don't think we need to worry about stuffing host cache. WPAD >> >> > > > queries >> > >> > are >> >> >> >> > > done >> >> > > > very infrequently and given that we only do the queries at >> >> > > > network >> >> > transition >> >> > > > time we'd end up wiping out the results anyway. >> >> > > >> >> > > Ah - then I guess we can easily create a HostResolver which >> >> > > bypasses >> >> > DnsClient. >> >> > > >> >> > > When we looked at how this was done from Chrome on Windows, almost >> >> > > all >> >> > > the >> >> > > queries were done using NetBIOS/LLMNR only. Not sure if the windows >> >> > > host >> >> cache >> >> > > had a no-response for the DNS A queries. >> >> > >> >> > ellyjones+szym: It seems like we could just have the >> >> > ProxyScriptDecider >> >> > call >> >> > CreateDefaultHostResolver and then SetDnsClientEnabled on it, and >> >> > use >> >> > that >> >> > rather than the URLRequestContext's HostResolver. We would want to >> >> > copy >> >> > over >> >> > netlog, but do you see any concerns beyond that? >> > >> > >> >> Is there a reason to prefer the default host resolver? >> > >> > >> > They are both "default" in the sense that they are created with >> > CreateDefaultHostResolver. Chris is suggesting to always use an internal >> > instance, rather than internal one for QuickCheck and the one in the >> > URLRequestContext for the actual PAC fetch. This would save us the need >> > to >> > stuff >> > the HostCache. >> > >> > https://codereview.chromium.org/23181010/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/23181010/diff/53001/net/base/address_family.h File net/base/address_family.h (right): https://codereview.chromium.org/23181010/diff/53001/net/base/address_family.h... net/base/address_family.h:28: HOST_RESOLVER_NATIVE_ONLY = 1 << 3 Suggest "system" instead of "native". https://codereview.chromium.org/23181010/diff/53001/net/dns/host_resolver_imp... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/23181010/diff/53001/net/dns/host_resolver_imp... net/dns/host_resolver_impl.cc:1156: const RequestInfo& info) No need for this. The flags are available in |key.host_resolver_flags|. https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:90: if (proxy_script_fetcher && proxy_script_fetcher->GetRequestContext() && suggest one clause per line https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:91: proxy_script_fetcher->GetRequestContext()->host_resolver()) Needs {} https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:82: void SetHostResolverForTesting(HostResolver* resolver); Needs comment. Is this going to be used for quickcheck or for everything? Why isn't the HostResolver from the fetcher enough?
https://codereview.chromium.org/23181010/diff/53001/net/base/address_family.h File net/base/address_family.h (right): https://codereview.chromium.org/23181010/diff/53001/net/base/address_family.h... net/base/address_family.h:28: HOST_RESOLVER_NATIVE_ONLY = 1 << 3 On 2013/09/16 21:50:32, szym wrote: > Suggest "system" instead of "native". Done. https://codereview.chromium.org/23181010/diff/53001/net/dns/host_resolver_imp... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/23181010/diff/53001/net/dns/host_resolver_imp... net/dns/host_resolver_impl.cc:1156: const RequestInfo& info) On 2013/09/16 21:50:32, szym wrote: > No need for this. The flags are available in |key.host_resolver_flags|. Done. https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:90: if (proxy_script_fetcher && proxy_script_fetcher->GetRequestContext() && On 2013/09/16 21:50:32, szym wrote: > suggest one clause per line Done. https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:91: proxy_script_fetcher->GetRequestContext()->host_resolver()) On 2013/09/16 21:50:32, szym wrote: > Needs {} Done. https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.h (right): https://codereview.chromium.org/23181010/diff/53001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.h:82: void SetHostResolverForTesting(HostResolver* resolver); On 2013/09/16 21:50:32, szym wrote: > Needs comment. Is this going to be used for quickcheck or for everything? Why > isn't the HostResolver from the fetcher enough? Deleted.
LGTM with minor remarks. Don't forget to update TEST= in CL description. Also, I suggest you add a test to HostResolverImplDnsTest confirming that HOST_RESOLVER_SYSTEM_ONLY works as intended. https://codereview.chromium.org/23181010/diff/62001/net/base/address_family.h File net/base/address_family.h (right): https://codereview.chromium.org/23181010/diff/62001/net/base/address_family.h... net/base/address_family.h:27: // The resolver should only invoke getaddrinfo, not DnsClient nit: '.' at end https://codereview.chromium.org/23181010/diff/62001/net/dns/host_resolver_imp... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/23181010/diff/62001/net/dns/host_resolver_imp... net/dns/host_resolver_impl.cc:1364: bool native_only = key_.host_resolver_flags & HOST_RESOLVER_SYSTEM_ONLY; |system_only| https://codereview.chromium.org/23181010/diff/62001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/62001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:261: reqinfo.set_host_resolver_flags(flags); This is unnecessary. The default flags are 0. reqinfo.set_host_resolver_flags(HOST_RESOLVER_SYSTEM_ONLY); ..and do it right after reqinfo is constructed in line 254.
https://codereview.chromium.org/23181010/diff/62001/net/base/address_family.h File net/base/address_family.h (right): https://codereview.chromium.org/23181010/diff/62001/net/base/address_family.h... net/base/address_family.h:27: // The resolver should only invoke getaddrinfo, not DnsClient On 2013/09/16 22:15:54, szym wrote: > nit: '.' at end Done. https://codereview.chromium.org/23181010/diff/62001/net/dns/host_resolver_imp... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/23181010/diff/62001/net/dns/host_resolver_imp... net/dns/host_resolver_impl.cc:1364: bool native_only = key_.host_resolver_flags & HOST_RESOLVER_SYSTEM_ONLY; On 2013/09/16 22:15:54, szym wrote: > |system_only| Done. https://codereview.chromium.org/23181010/diff/62001/net/proxy/proxy_script_de... File net/proxy/proxy_script_decider.cc (right): https://codereview.chromium.org/23181010/diff/62001/net/proxy/proxy_script_de... net/proxy/proxy_script_decider.cc:261: reqinfo.set_host_resolver_flags(flags); On 2013/09/16 22:15:54, szym wrote: > This is unnecessary. The default flags are 0. > > reqinfo.set_host_resolver_flags(HOST_RESOLVER_SYSTEM_ONLY); > > ..and do it right after reqinfo is constructed in line 254. Done.
lgtm https://codereview.chromium.org/23181010/diff/70001/net/dns/host_resolver_imp... File net/dns/host_resolver_impl_unittest.cc (right): https://codereview.chromium.org/23181010/diff/70001/net/dns/host_resolver_imp... net/dns/host_resolver_impl_unittest.cc:1527: HostResolver::RequestInfo info_native(HostPortPair("ok", 80)); suggest info_bypass
Now trying to get my windows LKGR to build so I can do a final test :).
Don't forget to update TEST= to reflect the newly added tests. https://codereview.chromium.org/23181010/diff/51001/net/dns/mock_host_resolve... File net/dns/mock_host_resolver.cc (right): https://codereview.chromium.org/23181010/diff/51001/net/dns/mock_host_resolve... net/dns/mock_host_resolver.cc:354: HostResolverFlags flags = host_resolver_flags & ~HOST_RESOLVER_SYSTEM_ONLY; The comment above this statement does not match. Suggest you move it up before comment and add another comment: // Ignore HOST_RESOLVER_SYSTEM_ONLY flag.
Finally, remove Signed-off from CL description.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/23181010/34002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
patch 15 lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/23181010/97001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/23181010/85001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ellyjones@chromium.org/23181010/47001
Message was sent while issue was closed.
Change committed as 224030 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
