FeatureEngagementTracker should not use model when initialization failed |
||||||||
Issue descriptionChrome Version: M60 OS: (e.g. Win7, OSX 10.9.5, etc...) What steps will reproduce the problem? - Make initialization fail of LevelDB databases. - Call NotifyEvent What is the expected result? DB should not be invoked, and nothing should be stored. What happens instead? All events are stored forever in memory. This only affects events db.
,
Jun 24 2017
CL in review to add a test for and fix this: https://chromium-review.googlesource.com/c/546959/
,
Jun 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dacfd003d4b463152a155266d17c56504cfc6f12 commit dacfd003d4b463152a155266d17c56504cfc6f12 Author: Tommy Nyquist <nyquist@chromium.org> Date: Sat Jun 24 07:20:56 2017 Do not queue up events forever when initialization fails The FeatureEngagementTracker can end up queueing events up forever in memory. It will stay there until the browser is killed. This can happen in two ways: For events that are notified before initialization is completed, they will stay in memory. In addition, events that are notified after init has completed (but failed), they will also be kept in memory. This CL adds a test that identifies both these scenarios, and also adds the code to fix the issue. BUG= 736550 Change-Id: I201c97839a9246cbbebf9b866d0131ea42643823 Reviewed-on: https://chromium-review.googlesource.com/546959 Reviewed-by: Xing Liu <xingliu@chromium.org> Commit-Queue: Tommy Nyquist <nyquist@chromium.org> Cr-Commit-Position: refs/heads/master@{#482147} [modify] https://crrev.com/dacfd003d4b463152a155266d17c56504cfc6f12/components/feature_engagement_tracker/internal/init_aware_model.cc [modify] https://crrev.com/dacfd003d4b463152a155266d17c56504cfc6f12/components/feature_engagement_tracker/internal/init_aware_model.h [modify] https://crrev.com/dacfd003d4b463152a155266d17c56504cfc6f12/components/feature_engagement_tracker/internal/init_aware_model_unittest.cc
,
Jun 26 2017
Marking as RB for beta. Was deployed to Canary this morning MTV time. Planning on requesting merge when 24 hours has passed.
,
Jun 27 2017
Requesting merge of https://chromium.googlesource.com/chromium/src.git/+/dacfd003d4b463152a155266d17c56504cfc6f12 # Full automated unit test coverage Unit test to catch any regressions has been added, and unit test coverage expanded in other scenarios to ensure no other scenarios can exhibit the same behavior. # Deployed in Canary for at least 24 hours Code has been shipped in canary 61.0.3141.0 @ 2017-06-26 14:07:21 UTC. # Safe Merge The change to prod code is minor and should have no side effects other than fixing the issue it was supposed to fix.
,
Jun 27 2017
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 27 2017
Approved for M60 branch 3112.
,
Jun 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0237f1b0c6e47ecd5b42ae723b641a1149e4806 commit f0237f1b0c6e47ecd5b42ae723b641a1149e4806 Author: Alex Mineer <amineer@chromium.org> Date: Tue Jun 27 17:12:08 2017 Do not queue up events forever when initialization fails The FeatureEngagementTracker can end up queueing events up forever in memory. It will stay there until the browser is killed. This can happen in two ways: For events that are notified before initialization is completed, they will stay in memory. In addition, events that are notified after init has completed (but failed), they will also be kept in memory. This CL adds a test that identifies both these scenarios, and also adds the code to fix the issue. BUG= 736550 (cherry picked from commit dacfd003d4b463152a155266d17c56504cfc6f12) Change-Id: I201c97839a9246cbbebf9b866d0131ea42643823 Reviewed-on: https://chromium-review.googlesource.com/546959 Reviewed-by: Xing Liu <xingliu@chromium.org> Commit-Queue: Tommy Nyquist <nyquist@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#482147} Cr-Commit-Position: refs/branch-heads/3112@{#480} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/f0237f1b0c6e47ecd5b42ae723b641a1149e4806/components/feature_engagement_tracker/internal/init_aware_model.cc [modify] https://crrev.com/f0237f1b0c6e47ecd5b42ae723b641a1149e4806/components/feature_engagement_tracker/internal/init_aware_model.h [modify] https://crrev.com/f0237f1b0c6e47ecd5b42ae723b641a1149e4806/components/feature_engagement_tracker/internal/init_aware_model_unittest.cc
,
Jun 27 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by nyquist@chromium.org
, Jun 24 2017