Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871323006:
Make Network Predictor skip empty urls (Closed)
|
Created:
3 years, 7 months ago by xunjieli Modified:
3 years, 7 months ago Reviewers:
Charlie Harrison CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake Network Predictor skip empty urls
Predictor::CanonicalizeUrl() can return empty urls. This causes
HttpStreamFactoryImpl::PreconnectStream() to preconnect empty urls, which
is bad because HttpStreamFactoryImpl layer and lower (e.g. proxy resolution)
assume that the urls are valid.
This CL makes Predictor to skip empty urls.
BUG=721981
Review-Url: https://codereview.chromium.org/2871323006
Cr-Commit-Position: refs/heads/master@{#471837}
Committed: https://chromium.googlesource.com/chromium/src/+/cb7c661c1e2dbeb007a02832f9305ebc7199b845
Patch Set 1 : self #
Total comments: 2
Patch Set 2 : add test #Patch Set 3 : forgot one #Patch Set 4 : self #
Total comments: 5
Patch Set 5 : address comment #
Dependent Patchsets: Messages
Total messages: 28 (19 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Make Network Predictor not pass empty urls BUG=721981 ========== to ========== Make Network Predictor skip empty urls Predictor::CanonicalizeUrl() can return empty urls. This causes HttpStreamFactoryImpl::PreconnectStream() to preconnect empty urls, which is bad because HttpStreamFactoryImpl layer and lower (e.g. proxy resolution) assume that the urls are valid. This CL makes Predictor to skip empty urls. BUG=721981 ==========
xunjieli@chromium.org changed reviewers: + csharrison@chromium.org
Charlie, PTAL. Thanks.
Description was changed from ========== Make Network Predictor skip empty urls Predictor::CanonicalizeUrl() can return empty urls. This causes HttpStreamFactoryImpl::PreconnectStream() to preconnect empty urls, which is bad because HttpStreamFactoryImpl layer and lower (e.g. proxy resolution) assume that the urls are valid. This CL makes Predictor to skip empty urls. BUG=721981 ========== to ========== Make Network Predictor skip empty urls Predictor::CanonicalizeUrl() can return empty urls. This causes HttpStreamFactoryImpl::PreconnectStream() to preconnect empty urls, which is bad because HttpStreamFactoryImpl layer and lower (e.g. proxy resolution) assume that the urls are valid. This CL makes Predictor to skip empty urls. BUG=721981 ==========
LGTM % comment. A test would be nice (predictor_browsertests is where they live) but I'm not sure how easy to write it will be. This code is in pretty bad shape. I had a refactoring to improve the canonicalization scheme a while back but never got around to finishing. https://codereview.chromium.org/2871323006/diff/20001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2871323006/diff/20001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:174: GURL canonicalized_url = CanonicalizeUrl(url); Here and below can you make sure we canoncalize the URL only if preconnect/preresolve is enabled? The URL canonicalization we do is kinda expensive.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Charlie, could you take another look? I added a test which will fail without the change (Observer::OnPreconnectUrl() will be notified with an empty url). I reverted the Predictor::AnticipateOmniboxUrl() change because PreconnectUrl() is only invoked when url's host is not a new host. I don't know what's going on there, so I think I will leave it as it is. I have added a DCHECK both in PreconnectUrl() and in HttpStreamFactoryImpl::PreconnectStreams(). https://codereview.chromium.org/2871323006/diff/20001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2871323006/diff/20001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:174: GURL canonicalized_url = CanonicalizeUrl(url); On 2017/05/15 00:01:07, Charlie Harrison wrote: > Here and below can you make sure we canoncalize the URL only if > preconnect/preresolve is enabled? The URL canonicalization we do is kinda > expensive. Done.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:249: GURL canonicalized_url = CanonicalizeUrl(url); nit: const GURL https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:323: preconnect_url_attempts_.insert(original_url); Does OnDnsLookupFinished trigger for these bad URLs? If so it would be preferred to use unsuccessful_dns_lookups / CheckForWaitingLoop so you don't need RunUntilIdle().
Thanks for the review. https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor.cc:249: GURL canonicalized_url = CanonicalizeUrl(url); On 2017/05/15 17:01:25, Charlie Harrison wrote: > nit: const GURL Done. https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:323: preconnect_url_attempts_.insert(original_url); On 2017/05/15 17:01:25, Charlie Harrison wrote: > Does OnDnsLookupFinished trigger for these bad URLs? If so it would be preferred > to use unsuccessful_dns_lookups / CheckForWaitingLoop so you don't need > RunUntilIdle(). Unfortunately no. OnDnsLookupFinished() is only used for presolve.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2871323006/#ps120001 (title: "address comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/2871323006/diff/100001/chrome/browser/net/pre... chrome/browser/net/predictor_browsertest.cc:323: preconnect_url_attempts_.insert(original_url); On 2017/05/15 17:06:51, xunjieli wrote: > On 2017/05/15 17:01:25, Charlie Harrison wrote: > > Does OnDnsLookupFinished trigger for these bad URLs? If so it would be > preferred > > to use unsuccessful_dns_lookups / CheckForWaitingLoop so you don't need > > RunUntilIdle(). > > Unfortunately no. OnDnsLookupFinished() is only used for presolve. s/presolve/preresolve :)
LGTM it would be great to avoid RunUntilIdle but I think it would be a bit difficult. Thanks for writing the test
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494868018416100,
"parent_rev": "67372f3c7eae35937bd6603450e17fd2a62e6cc8", "commit_rev":
"cb7c661c1e2dbeb007a02832f9305ebc7199b845"}
Message was sent while issue was closed.
Description was changed from ========== Make Network Predictor skip empty urls Predictor::CanonicalizeUrl() can return empty urls. This causes HttpStreamFactoryImpl::PreconnectStream() to preconnect empty urls, which is bad because HttpStreamFactoryImpl layer and lower (e.g. proxy resolution) assume that the urls are valid. This CL makes Predictor to skip empty urls. BUG=721981 ========== to ========== Make Network Predictor skip empty urls Predictor::CanonicalizeUrl() can return empty urls. This causes HttpStreamFactoryImpl::PreconnectStream() to preconnect empty urls, which is bad because HttpStreamFactoryImpl layer and lower (e.g. proxy resolution) assume that the urls are valid. This CL makes Predictor to skip empty urls. BUG=721981 Review-Url: https://codereview.chromium.org/2871323006 Cr-Commit-Position: refs/heads/master@{#471837} Committed: https://chromium.googlesource.com/chromium/src/+/cb7c661c1e2dbeb007a02832f930... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/cb7c661c1e2dbeb007a02832f930... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
