Cleaning up the net predictor results caching |
||||
Issue descriptionThe chrome_browser_net::Predictor has a caching layer that keeps results of all speculative DNS lookups and never discards them. This cause memory leaks reported in bug 752954 . Given that another cache for dns requests already exists down the stack, the predictor results caching layer may be remove that'd lead to the less and simpler code. Predictor::results_ member can be completely removed (thus UrlInfo class can be removed) or kept simplified for the deduplication of pending DNS requests.
,
Aug 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3e8ebaa0937fc6910a77ef2399c37d4a8719a8cc commit 3e8ebaa0937fc6910a77ef2399c37d4a8719a8cc Author: Alexandr Ilin <alexilin@chromium.org> Date: Wed Aug 23 16:21:19 2017 net: Set a UrlInfo's final state before delete The net::predictor deletes UrlInfo before setting the final state (FOUND or NO_SUCH_NAME). That leads to the fact that the DNS.PrefetchResolution histogram is never reported. This CL fixes this problem by setting the UrlInfo's final state before deleting it. Bug: 757458 Change-Id: I948345fbc443e2a81a7404817edd8f043a0708df Reviewed-on: https://chromium-review.googlesource.com/628522 Commit-Queue: Alexandr Ilin <alexilin@chromium.org> Reviewed-by: Helen Li <xunjieli@chromium.org> Cr-Commit-Position: refs/heads/master@{#496703} [modify] https://crrev.com/3e8ebaa0937fc6910a77ef2399c37d4a8719a8cc/chrome/browser/net/predictor.cc [modify] https://crrev.com/3e8ebaa0937fc6910a77ef2399c37d4a8719a8cc/chrome/browser/net/url_info.cc
,
Aug 30 2017
I am consistently hitting a DCHECK in url_info.cc [FATAL:url_info.cc(137)] Check failed: ASSIGNED == state_. Stack looks like this: chrome_browser_net::UrlInfo::RemoveFromQueue() chrome_browser_net::Predictor::CongestionControlPerformed(chrome_browser_net::UrlInfo*) chrome_browser_net::Predictor::StartSomeQueuedResolutions() Looks like others are hitting this issue as well (cf 758204). Could this be related to these changes?
,
Aug 30 2017
Bumping priority to P1. alexilin@ can you look into these failures?
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3eb9c381ad0e5868df09682d09d7291a4a0117b5 commit 3eb9c381ad0e5868df09682d09d7291a4a0117b5 Author: Charlie Harrison <csharrison@chromium.org> Date: Wed Aug 30 22:12:08 2017 Revert "net: Set a UrlInfo's final state before delete" This reverts commit 3e8ebaa0937fc6910a77ef2399c37d4a8719a8cc. Reason for revert: Potentially the cause of issue 757458 and 760656 Original change's description: > net: Set a UrlInfo's final state before delete > > The net::predictor deletes UrlInfo before setting the final state (FOUND > or NO_SUCH_NAME). That leads to the fact that the DNS.PrefetchResolution > histogram is never reported. > > This CL fixes this problem by setting the UrlInfo's final state before > deleting it. > > Bug: 757458 > Change-Id: I948345fbc443e2a81a7404817edd8f043a0708df > Reviewed-on: https://chromium-review.googlesource.com/628522 > Commit-Queue: Alexandr Ilin <alexilin@chromium.org> > Reviewed-by: Helen Li <xunjieli@chromium.org> > Cr-Commit-Position: refs/heads/master@{#496703} TBR=xunjieli@chromium.org,alexilin@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 757458 Change-Id: Ie4d6908785fcceedc1c762734dbd0e621d0852f4 Reviewed-on: https://chromium-review.googlesource.com/644266 Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Commit-Queue: Helen Li <xunjieli@chromium.org> Cr-Commit-Position: refs/heads/master@{#498636} [modify] https://crrev.com/3eb9c381ad0e5868df09682d09d7291a4a0117b5/chrome/browser/net/predictor.cc [modify] https://crrev.com/3eb9c381ad0e5868df09682d09d7291a4a0117b5/chrome/browser/net/url_info.cc
,
Aug 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/191bc9d35eccd1e57861a85e3720c1f6883decac commit 191bc9d35eccd1e57861a85e3720c1f6883decac Author: Alexandr Ilin <alexilin@chromium.org> Date: Thu Aug 31 18:29:20 2017 Reland "net: Set a UrlInfo's final state before delete" This reverts commit 3eb9c381ad0e5868df09682d09d7291a4a0117b5. Reason for reland: It's unlikely the cause for 760656 and the fix for DCHECK crashes in 757458c#3 and 758204 is in the CL. The DCHECK crash was actually introduced in http://crrev.com/c/612380. The crash appears when the resolution work queue has a backlog and this case wasn't properly tested. The CL fixes the issue of DCHECKs (incorrect transition in the state graph) and adds the browsertest for the work queue congestion control. Bug: 757458 , 758204 Change-Id: I11c867a94f52bb860e3146d3cc26e5ee270b83f6 Reviewed-on: https://chromium-review.googlesource.com/646248 Reviewed-by: Charlie Harrison <csharrison@chromium.org> Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Alexandr Ilin <alexilin@chromium.org> Cr-Commit-Position: refs/heads/master@{#498926} [modify] https://crrev.com/191bc9d35eccd1e57861a85e3720c1f6883decac/chrome/browser/net/predictor.cc [modify] https://crrev.com/191bc9d35eccd1e57861a85e3720c1f6883decac/chrome/browser/net/predictor.h [modify] https://crrev.com/191bc9d35eccd1e57861a85e3720c1f6883decac/chrome/browser/net/predictor_browsertest.cc [modify] https://crrev.com/191bc9d35eccd1e57861a85e3720c1f6883decac/chrome/browser/net/url_info.cc
,
Sep 1 2017
The issue described in c#3 should be fixed now. Returning priority to P3.
,
Sep 7
Predictor is deprecated |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Aug 22 2017