Finch seed freshness regressed 6x due to ResourceRequestAllowedNotifier to NetworkConnectionTracker migration |
||||
Issue descriptionFinch seed freshness regressed 6x due to ResourceRequestAllowedNotifier to NetworkConnectionTracker migration. See the following alert and discussion: https://groups.google.com/a/google.com/forum/#!topic/uma-alerts/Ty_eGN4EYeA Marking P1 as regression and RBS since Finch seed updates are critical. Please fix or revert the change ASAP. Thanks!
,
Aug 30
,
Aug 30
[Auto-generated comment by a script] We noticed that this issue is targeted for M-70; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-70 label, otherwise remove Merge-TBD label. Thanks.
,
Aug 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bf72b12cff1b1a34d05e85dbacc71635550698b commit 5bf72b12cff1b1a34d05e85dbacc71635550698b Author: Clark DuVall <cduvall@chromium.org> Date: Fri Aug 31 19:53:30 2018 Reland "Reland "Reland "Migrate ResourceRequestAllowedNotifier to NetworkConnectionTracker""" This reverts commit 22b92b162796a4d67497685889b836faa9fba994. Reason for revert: Fixed bug where listeners would not be notified when waiting for initial connection status. This change fixes the bug and adds a regression test. In addition, this puts the change behind the ResourceRequestAllowedMigration feature, as suggested by asvitkine@. We can test this on Canary to make sure the Variations.SeedFreshness metric is not affected. Note that the Variations.ResourceRequestsAllowed will have different behavior with this change, as now instead of just sending RESOURCE_REQUESTS_ALLOWED when the network is connected, it will send RESOURCE_REQUESTS_NOT_ALLOWED_NETWORK_STATE_NOT_INITIALIZED and then RESOURCE_REQUESTS_ALLOWED_NOTIFIED. This hopefully should not affect Variations.SeedFreshness. See diffs starting at Patchset 1 for changes from last reland. Some of the old code needed to be added back to support both versions behind the feature flag. Original change's description: > Revert "Reland "Reland "Migrate ResourceRequestAllowedNotifier to NetworkConnectionTracker""" > > This reverts commit 34141782bc8b1e459167cbdafa9a18ef0c6294e2. > > Reason for revert: Caused Finch seed freshness regression, see http://crbug.com/879252 and > http://crbug.com/879115 > > Original change's description: > > Reland "Reland "Migrate ResourceRequestAllowedNotifier to NetworkConnectionTracker"" > > > > This is a reland of e00fddec92e49b1b57f5de2f1134697fd6b1c703 > > > > Needed to initialize variables in ChromeBrowserMainBrowserTest for asan/msan. > > Verified this works with msan build. > > > > Original change's description: > > > Reland "Migrate ResourceRequestAllowedNotifier to NetworkConnectionTracker" > > > > > > This is a reland of a9ed46b74b92dbd1e7ccf11deabfae7b0aed737c > > > > > > ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange > > > was flaky in the original change. Added logic in the test to wait for the > > > connection type change. > > > > > > Only diffs from original are in chrome/browser/chrome_browser_main_browsertest.cc > > > > > > Original change's description: > > > > Migrate ResourceRequestAllowedNotifier to NetworkConnectionTracker > > > > > > > > A getter is used for NetworkConnectionTracker because some services that > > > > use ResourceRequestAllowedNotifier are initialized early in browser > > > > startup (e.g. VariationsService), and only perform the initialization > > > > of ResourceRequestAllowedNotifier later on the UI thread. The getter > > > > allows us to run get the connection tracker at that point so we don't get > > > > DCHECKs about being on the UI thread when running > > > > content::GetNetworkConnectionTracker(). > > > > > > > > This also moves the NetworkConnectionTracker in ios/ from BrowserState to > > > > ApplicationContext, which is available everywhere. It also matches non-IOS > > > > usage more closely, since we have it as a global there. > > > > > > > > Bug: 868021 > > > > Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet > > > > Change-Id: I130c6b47feb90f0f7f0776ccc65666414a1ae802 > > > > Reviewed-on: https://chromium-review.googlesource.com/1180360 > > > > Reviewed-by: Eugene But <eugenebut@chromium.org> > > > > Reviewed-by: Robert Sesek <rsesek@chromium.org> > > > > Reviewed-by: John Abd-El-Malek <jam@chromium.org> > > > > Reviewed-by: Robbie McElrath <rmcelrath@chromium.org> > > > > Commit-Queue: Clark DuVall <cduvall@chromium.org> > > > > Cr-Commit-Position: refs/heads/master@{#584849} > > > > > > TBR=eugenebut@chromium.org,rsesek@chromium.org,rmcelrath@chromium.org,jam@chromium.org > > > > > > Bug: 868021 > > > Change-Id: I5941b72474657159f0d4a1e6667fd77a3c475887 > > > Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet > > > Reviewed-on: https://chromium-review.googlesource.com/1185602 > > > Reviewed-by: Clark DuVall <cduvall@chromium.org> > > > Reviewed-by: Robbie McElrath <rmcelrath@chromium.org> > > > Commit-Queue: Clark DuVall <cduvall@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#585387} > > > > TBR=eugenebut@chromium.org,rsesek@chromium.org,rmcelrath@chromium.org,jam@chromium.org > > > > Bug: 868021 , 876861 > > Change-Id: I46fccf072d0b3080603e97c73ff055ac7c45e723 > > Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet > > Reviewed-on: https://chromium-review.googlesource.com/1187081 > > Commit-Queue: Clark DuVall <cduvall@chromium.org> > > Reviewed-by: Clark DuVall <cduvall@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#585565} > > TBR=jam@chromium.org,eugenebut@chromium.org,rsesek@chromium.org,rmcelrath@chromium.org,cduvall@chromium.org > > # Not skipping CQ checks because original CL landed > 1 day ago. > > Bug: 868021 , 876861 , 879252 , 879115 > Change-Id: If0417f53386a94de0ec3b64c283df123c673ec03 > Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs > Reviewed-on: https://chromium-review.googlesource.com/1197325 > Commit-Queue: Clark DuVall <cduvall@chromium.org> > Reviewed-by: Clark DuVall <cduvall@chromium.org> > Cr-Commit-Position: refs/heads/master@{#587694} TBR=eugenebut@chromium.org Bug: 868021 , 876861 , 879252 , 879115 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs Change-Id: I2190d82f8de1188763b6f1bb1de2b8762d23aa07 Reviewed-on: https://chromium-review.googlesource.com/1198102 Commit-Queue: Clark DuVall <cduvall@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Robbie McElrath <rmcelrath@chromium.org> Cr-Commit-Position: refs/heads/master@{#588133} [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/chrome/browser/chrome_browser_main_browsertest.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/chrome/browser/plugins/plugins_resource_service.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/chrome/browser/translate/translate_manager_render_view_host_unittest.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/chrome/browser/translate/translate_service.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/chrome/browser/translate/translate_service.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/chrome/browser/translate/translate_service_unittest.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/variations/service/variations_service.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/variations/service/variations_service.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/variations/service/variations_service_unittest.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/BUILD.gn [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/resource_request_allowed_notifier.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/resource_request_allowed_notifier.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/resource_request_allowed_notifier_test_util.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/resource_request_allowed_notifier_test_util.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/resource_request_allowed_notifier_unittest.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/web_resource_service.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/web_resource_service.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/components/web_resource/web_resource_service_unittest.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/BUILD.gn [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/DEPS [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/application_context.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/application_context_impl.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/application_context_impl.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/google/BUILD.gn [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/google/google_url_tracker_factory.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/metrics/ios_chrome_metrics_services_manager_client.mm [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/browser/translate/translate_service_ios.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/test/testing_application_context.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/chrome/test/testing_application_context.mm [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web/DEPS [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web/browser_state.mm [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web/public/browser_state.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web_view/BUILD.gn [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web_view/internal/DEPS [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web_view/internal/app/application_context.cc [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web_view/internal/app/application_context.h [modify] https://crrev.com/5bf72b12cff1b1a34d05e85dbacc71635550698b/ios/web_view/internal/translate/web_view_translate_service.cc
,
Sep 7
Does this need merge to M70? Seems like it landed after branch.
,
Sep 7
The revert in comment #1 is in M70 I believe, it should be fine that the reland didn't make it.
,
Sep 7
Removing merge labels. |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Aug 30