Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
545633002:
Don't preresolve DNS if a fixed proxy configuration is in place. (Closed)
DescriptionDon't preresolve DNS if a fixed proxy configuration is in place.
This change does not affect the behavior when using a system-configured
proxy or a PAC file.
BUG=123716
Committed: https://crrev.com/1515a12c29da0477a461204b3e67c8031d68a1be
Cr-Commit-Position: refs/heads/master@{#300356}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Use null callback to avoid queueing a PAC request #Patch Set 3 : Undo no-longer-needed virtual marking #
Total comments: 6
Patch Set 4 : Move synchronous proxy check into a separate function and remove virtual marking #
Total comments: 2
Patch Set 5 : Remote superfluous net:: prefix #
Messages
Total messages: 22 (2 generated)
bemasc@chromium.org changed reviewers: + eroman@chromium.org, mmenke@chromium.org
PTAL
Eric: Mind taking this one? Almost up to 20 codereviews this week.
On 2014/09/05 14:54:55, mmenke wrote: > Eric: Mind taking this one? Almost up to 20 codereviews this week. Ping.
https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor.cc File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1042: bool Predictor::WouldDefinitelyProxyURL(const GURL& url) { This title is misleading. It is still possible for the URL to go through direct when this return true. And conversely, it is still possible for the URL to go through a proxy when this returns false. https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1051: if (response_code != net::LOAD_NORMAL) { This code is incorrect: (1) Use net::OK not net::LOAD_NORMAL. They both happen to be zero, but are from different enum sets. (2) There are three types of responses to consider: == 0 <0 == ERROR_IO_PENDING (3) It is only correct to call CancelPacRequest() in the case where the return code above was ERROR_IO_PENDING https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1054: proxy_service_->CancelPacRequest(pac_request); In the case where a PAC script _is_ configured, it would be better to not start and then cancel a request. Since starting it may post a task to a worker thread, which is not subsequently actually able to be cancelled. How about adding synchronous-only support to ProxyService::ResolveProxy(). Perhaps triggered by passing in a null callback.
Thanks for the comments Eric! I could use a little more detail before continuing with this. Please give me your take on the questions here. https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor.cc File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1042: bool Predictor::WouldDefinitelyProxyURL(const GURL& url) { On 2014/09/10 01:26:04, eroman wrote: > This title is misleading. It is still possible for the URL to go through direct > when this return true. OK. Do you think I should change the name ("WouldTryProxy"?), or try to tighten the requirements? For example, I suppose I could add a method like "ProxyInfo::contains_direct()", if you're concerned about correctly handling the possibility of fallback to "direct". > And conversely, it is still possible for the URL to go > through a proxy when this returns false. That's fine. (It's what I was trying to convey with the name.) https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1054: proxy_service_->CancelPacRequest(pac_request); On 2014/09/10 01:26:04, eroman wrote: > In the case where a PAC script _is_ configured, it would be better to not start > and then cancel a request. Since starting it may post a task to a worker thread, > which is not subsequently actually able to be cancelled. > > How about adding synchronous-only support to ProxyService::ResolveProxy(). > Perhaps triggered by passing in a null callback. OK. There are 10 calls to ResolveProxy in the code, plus 80 in the unit tests. Should I convert them all to pass pointers to the callback? Or would you prefer to turn ResolveProxy into a wrapper function, or an overload, so that they don't have to change?
On 2014/09/10 15:19:02, bemasc wrote: > Thanks for the comments Eric! I could use a little more detail before > continuing with this. Please give me your take on the questions here. > > https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor.cc > File chrome/browser/net/predictor.cc (right): > > https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... > chrome/browser/net/predictor.cc:1042: bool > Predictor::WouldDefinitelyProxyURL(const GURL& url) { > On 2014/09/10 01:26:04, eroman wrote: > > This title is misleading. It is still possible for the URL to go through > direct > > when this return true. > > OK. Do you think I should change the name ("WouldTryProxy"?), or try to tighten > the requirements? For example, I suppose I could add a method like > "ProxyInfo::contains_direct()", if you're concerned about correctly handling the > possibility of fallback to "direct". > > > And conversely, it is still possible for the URL to go > > through a proxy when this returns false. > > That's fine. (It's what I was trying to convey with the name.) > > https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... > chrome/browser/net/predictor.cc:1054: > proxy_service_->CancelPacRequest(pac_request); > On 2014/09/10 01:26:04, eroman wrote: > > In the case where a PAC script _is_ configured, it would be better to not > start > > and then cancel a request. Since starting it may post a task to a worker > thread, > > which is not subsequently actually able to be cancelled. > > > > How about adding synchronous-only support to ProxyService::ResolveProxy(). > > Perhaps triggered by passing in a null callback. > > OK. There are 10 calls to ResolveProxy in the code, plus 80 in the unit tests. > Should I convert them all to pass pointers to the callback? Or would you prefer > to turn ResolveProxy into a wrapper function, or an overload, so that they don't > have to change? Ping.
On 2014/09/10 22:43:06, bemasc wrote: > On 2014/09/10 15:19:02, bemasc wrote: > > Thanks for the comments Eric! I could use a little more detail before > > continuing with this. Please give me your take on the questions here. > > > > > https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor.cc > > File chrome/browser/net/predictor.cc (right): > > > > > https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... > > chrome/browser/net/predictor.cc:1042: bool > > Predictor::WouldDefinitelyProxyURL(const GURL& url) { > > On 2014/09/10 01:26:04, eroman wrote: > > > This title is misleading. It is still possible for the URL to go through > > direct > > > when this return true. > > > > OK. Do you think I should change the name ("WouldTryProxy"?), or try to > tighten > > the requirements? For example, I suppose I could add a method like > > "ProxyInfo::contains_direct()", if you're concerned about correctly handling > the > > possibility of fallback to "direct". > > > > > And conversely, it is still possible for the URL to go > > > through a proxy when this returns false. > > > > That's fine. (It's what I was trying to convey with the name.) > > > > > https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... > > chrome/browser/net/predictor.cc:1054: > > proxy_service_->CancelPacRequest(pac_request); > > On 2014/09/10 01:26:04, eroman wrote: > > > In the case where a PAC script _is_ configured, it would be better to not > > start > > > and then cancel a request. Since starting it may post a task to a worker > > thread, > > > which is not subsequently actually able to be cancelled. > > > > > > How about adding synchronous-only support to ProxyService::ResolveProxy(). > > > Perhaps triggered by passing in a null callback. > > > > OK. There are 10 calls to ResolveProxy in the code, plus 80 in the unit > tests. > > Should I convert them all to pass pointers to the callback? Or would you > prefer > > to turn ResolveProxy into a wrapper function, or an overload, so that they > don't > > have to change? > > Ping. Ping. eroman@, please help me understand what you have in mind here.
https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor.cc File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1042: bool Predictor::WouldDefinitelyProxyURL(const GURL& url) { On 2014/09/10 15:19:02, bemasc wrote: > On 2014/09/10 01:26:04, eroman wrote: > > This title is misleading. It is still possible for the URL to go through > direct > > when this return true. > > OK. Do you think I should change the name ("WouldTryProxy"?), or try to tighten > the requirements? For example, I suppose I could add a method like > "ProxyInfo::contains_direct()", if you're concerned about correctly handling the > possibility of fallback to "direct". > > > And conversely, it is still possible for the URL to go > > through a proxy when this returns false. > > That's fine. (It's what I was trying to convey with the name.) How about: "WouldLikelyProxyURL()" That name is more in line with the guarantees we can promise. https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1054: proxy_service_->CancelPacRequest(pac_request); On 2014/09/10 15:19:02, bemasc wrote: > On 2014/09/10 01:26:04, eroman wrote: > > In the case where a PAC script _is_ configured, it would be better to not > start > > and then cancel a request. Since starting it may post a task to a worker > thread, > > which is not subsequently actually able to be cancelled. > > > > How about adding synchronous-only support to ProxyService::ResolveProxy(). > > Perhaps triggered by passing in a null callback. > > OK. There are 10 calls to ResolveProxy in the code, plus 80 in the unit tests. > Should I convert them all to pass pointers to the callback? Or would you prefer > to turn ResolveProxy into a wrapper function, or an overload, so that they don't > have to change? My thinking was that ResolveProxy() keep the same signature as today, however you pass in a "null" CompletionCallback. ResolveProxy() would test it with: callback.is_null() (Note that this is different from a NULL pointer, but achieves the same end).
PTAL https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor.cc File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1042: bool Predictor::WouldDefinitelyProxyURL(const GURL& url) { On 2014/09/18 17:51:28, eroman wrote: > On 2014/09/10 15:19:02, bemasc wrote: > > On 2014/09/10 01:26:04, eroman wrote: > > > This title is misleading. It is still possible for the URL to go through > > direct > > > when this return true. > > > > OK. Do you think I should change the name ("WouldTryProxy"?), or try to > tighten > > the requirements? For example, I suppose I could add a method like > > "ProxyInfo::contains_direct()", if you're concerned about correctly handling > the > > possibility of fallback to "direct". > > > > > And conversely, it is still possible for the URL to go > > > through a proxy when this returns false. > > > > That's fine. (It's what I was trying to convey with the name.) > > How about: > > "WouldLikelyProxyURL()" > > That name is more in line with the guarantees we can promise. Done. https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1051: if (response_code != net::LOAD_NORMAL) { On 2014/09/10 01:26:04, eroman wrote: > This code is incorrect: > (1) Use net::OK not net::LOAD_NORMAL. They both happen to be zero, but are > from different enum sets. > > (2) There are three types of responses to consider: > == 0 > <0 > == ERROR_IO_PENDING > > (3) It is only correct to call CancelPacRequest() in the case where the > return code above was ERROR_IO_PENDING Done. https://codereview.chromium.org/545633002/diff/1/chrome/browser/net/predictor... chrome/browser/net/predictor.cc:1054: proxy_service_->CancelPacRequest(pac_request); On 2014/09/18 17:51:28, eroman wrote: > On 2014/09/10 15:19:02, bemasc wrote: > > On 2014/09/10 01:26:04, eroman wrote: > > > In the case where a PAC script _is_ configured, it would be better to not > > start > > > and then cancel a request. Since starting it may post a task to a worker > > thread, > > > which is not subsequently actually able to be cancelled. > > > > > > How about adding synchronous-only support to ProxyService::ResolveProxy(). > > > Perhaps triggered by passing in a null callback. > > > > OK. There are 10 calls to ResolveProxy in the code, plus 80 in the unit > tests. > > Should I convert them all to pass pointers to the callback? Or would you > prefer > > to turn ResolveProxy into a wrapper function, or an overload, so that they > don't > > have to change? > > My thinking was that ResolveProxy() keep the same signature as today, however > you pass in a "null" CompletionCallback. > > ResolveProxy() would test it with: > callback.is_null() > > (Note that this is different from a NULL pointer, but achieves the same end). Done.
PTAL. Ready for review.
https://codereview.chromium.org/545633002/diff/40001/chrome/browser/net/predi... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/545633002/diff/40001/chrome/browser/net/predi... chrome/browser/net/predictor.cc:1073: if ((proxy_advisor_ && proxy_advisor_->WouldProxyURL(url)) || What is the role of proxy_advisor? How does this differe? https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.... net/proxy/proxy_service.cc:1008: return ERR_IO_PENDING; This calling contract is confusing, since the request is not really pending in this case (it will never be started). You can extract this as a helper function, and then have two entry points: (1) The existing: int ResolveProxy(...) (2) A new: bool TryResolveProxySynchronously(...) In the case of ResolveProxy() passing a null callback should remain an error, whereas TryResolveProxySynchronously won't even take a callback. https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.... net/proxy/proxy_service.h:125: // This method is virtual for unit testing. Can your tests can instead use: ProxyService::CreateFixed() ? I don't want this to have to be virtual just for a test purpose.
Done. PTAL. https://codereview.chromium.org/545633002/diff/40001/chrome/browser/net/predi... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/545633002/diff/40001/chrome/browser/net/predi... chrome/browser/net/predictor.cc:1073: if ((proxy_advisor_ && proxy_advisor_->WouldProxyURL(url)) || On 2014/09/23 20:50:41, eroman wrote: > What is the role of proxy_advisor? How does this differe? proxy_advisor is poorly named, IMHO. It is only non-null when using the Flywheel page-rewriting proxy, and its "WouldProxyURL" function only applies to that kind of proxy. For example, it returns false for any HTTPS URL, because Flywheel can't rewrite HTTPS pages. https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.... net/proxy/proxy_service.cc:1008: return ERR_IO_PENDING; On 2014/09/23 20:50:42, eroman wrote: > This calling contract is confusing, since the request is not really pending in > this case (it will never be started). > > You can extract this as a helper function, and then have two entry points: > (1) The existing: int ResolveProxy(...) > (2) A new: bool TryResolveProxySynchronously(...) > > In the case of ResolveProxy() passing a null callback should remain an error, > whereas TryResolveProxySynchronously won't even take a callback. Done. https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/545633002/diff/40001/net/proxy/proxy_service.... net/proxy/proxy_service.h:125: // This method is virtual for unit testing. On 2014/09/23 20:50:42, eroman wrote: > Can your tests can instead use: ProxyService::CreateFixed() ? > > I don't want this to have to be virtual just for a test purpose. Done.
mmenke@, it looks like eroman@ is on vacation. I'm not sure when he'll be back. Could you take a look in the meantime?
On 2014/09/30 15:25:46, bemasc wrote: > mmenke@, it looks like eroman@ is on vacation. I'm not sure when he'll be back. > Could you take a look in the meantime? Probably not - still swamped, unfortunately.
I'm back!
lgtm https://codereview.chromium.org/545633002/diff/60001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/545633002/diff/60001/net/proxy/proxy_service.... net/proxy/proxy_service.cc:1064: net_log) == net::OK; nit: no need for "net::" since this is inside the net namespace.
https://codereview.chromium.org/545633002/diff/60001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/545633002/diff/60001/net/proxy/proxy_service.... net/proxy/proxy_service.cc:1064: net_log) == net::OK; On 2014/10/14 21:16:33, eroman wrote: > nit: no need for "net::" since this is inside the net namespace. Done.
The CQ bit was checked by bemasc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545633002/90001
Message was sent while issue was closed.
Committed patchset #5 (id:90001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1515a12c29da0477a461204b3e67c8031d68a1be Cr-Commit-Position: refs/heads/master@{#300356} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
