Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 2749823002: Restore KeyedServiceFactory diagnostics for context use-after-destroy. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by sense (YandexTeam)
Modified:
4 months, 3 weeks ago
CC:
chromium-reviews, extensions-reviews_chromium.org, dominickn+watch_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, aboxhall+watch_chromium.org, tfarina, johnme+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, Peter Beverloo, je_julie, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chromium-apps-reviews_chromium.org, gcasto+watchlist_chromium.org, harkness+watch_chromium.org, ios-reviews_chromium.org, jlebel
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Restore KeyedServiceFactory diagnostics for context use-after-destroy. Existing AssertContextWasntDestroyed logic doesn't work if GetBrowserContextToUse() is overriden by a user factory. This CL moves the check to the non overridable for the user GetContextToUse() method. Also it enables the check to trigger DumpWithoutCrashing() if DCHECKs are disabled. BUG=701321 R=vasilii@chromium.org, benwells@chromium.org, mvanouwerkerk@chromium.org, phajdan.jr@chromium.org Review-Url: https://codereview.chromium.org/2749823002 Cr-Commit-Position: refs/heads/master@{#459358} Committed: https://chromium.googlesource.com/chromium/src/+/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758

Patch Set 1 #

Patch Set 2 : Add test fixes, more service shutdown fixes. #

Total comments: 7

Patch Set 3 : Add shared shutdown logic in SiteEngagementService tests. #

Total comments: 8

Patch Set 4 : Refactor SiteEngagementService tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+484 lines, -508 lines) Patch
M apps/app_load_service.h View 1 chunk +3 lines, -0 lines 0 comments Download
M apps/app_load_service.cc View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/engagement/site_engagement_service.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/engagement/site_engagement_service.cc View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/engagement/site_engagement_service_unittest.cc View 1 2 3 33 chunks +363 lines, -399 lines 0 comments Download
M chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/api/downloads/downloads_api_unittest.cc View 1 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/input_ime/input_ime_api.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/api/input_ime/input_ime_api.cc View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/signed_in_devices/signed_in_devices_manager.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/api/signed_in_devices/signed_in_devices_manager.cc View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/extensions/extension_gcm_app_handler.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/extension_gcm_app_handler.cc View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/extensions/extension_gcm_app_handler_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/password_manager/password_manager_internals_service_unittest.cc View 1 chunk +1 line, -13 lines 0 comments Download
M chrome/browser/push_messaging/push_messaging_service_impl.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/test/base/testing_profile.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M components/keyed_service/content/browser_context_dependency_manager.h View 1 chunk +9 lines, -10 lines 0 comments Download
M components/keyed_service/content/browser_context_dependency_manager.cc View 1 chunk +3 lines, -5 lines 0 comments Download
M components/keyed_service/content/browser_context_keyed_base_factory.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M components/keyed_service/content/browser_context_keyed_service_factory.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M components/keyed_service/content/refcounted_browser_context_keyed_service_factory.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M components/keyed_service/core/dependency_manager.h View 3 chunks +9 lines, -13 lines 0 comments Download
M components/keyed_service/core/dependency_manager.cc View 4 chunks +10 lines, -9 lines 0 comments Download
M components/keyed_service/core/keyed_service_base_factory.h View 1 chunk +5 lines, -7 lines 0 comments Download
M components/keyed_service/core/keyed_service_base_factory.cc View 1 chunk +5 lines, -6 lines 0 comments Download
M components/keyed_service/core/keyed_service_factory.cc View 1 chunk +1 line, -3 lines 0 comments Download
M components/keyed_service/core/refcounted_keyed_service_factory.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M components/keyed_service/ios/browser_state_dependency_manager.h View 1 chunk +6 lines, -8 lines 0 comments Download
M components/keyed_service/ios/browser_state_dependency_manager.cc View 1 chunk +3 lines, -5 lines 0 comments Download
M components/keyed_service/ios/browser_state_keyed_service_factory.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M components/keyed_service/ios/refcounted_browser_state_keyed_service_factory.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api.h View 1 chunk +1 line, -0 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api.cc View 1 chunk +3 lines, -1 line 0 comments Download
M extensions/browser/process_manager.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M extensions/browser/process_manager.cc View 1 1 chunk +4 lines, -1 line 0 comments Download
M extensions/shell/browser/shell_content_browser_client.cc View 1 2 chunks +5 lines, -0 lines 0 comments Download
Trybot results:  chromium_presubmit   ios-simulator   linux_chromium_asan_rel_ng   ios-device-xcode-clang   win_chromium_rel_ng   linux_chromium_chromeos_ozone_rel_ng   win_chromium_x64_rel_ng   ios-simulator-xcode-clang   linux_chromium_tsan_rel_ng   linux_chromium_chromeos_rel_ng   ios-device   linux_chromium_compile_dbg_ng   win_clang   mac_chromium_compile_dbg_ng   linux_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_rel_ng   linux_android_rel_ng   chromium_presubmit   cast_shell_android   android_compile_dbg   android_clang_dbg_recipe   chromeos_amd64-generic_chromium_compile_only_ng   android_cronet   android_n5x_swarming_rel   chromeos_daisy_chromium_compile_only_ng   android_arm64_dbg_recipe   cast_shell_linux   win_clang   chromeos_daisy_chromium_compile_only_ng   win_chromium_x64_rel_ng   linux_chromium_compile_dbg_ng   cast_shell_linux   mac_chromium_rel_ng   linux_chromium_asan_rel_ng   linux_chromium_tsan_rel_ng   ios-simulator   win_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   android_cronet   linux_chromium_chromeos_ozone_rel_ng   linux_android_rel_ng   android_clang_dbg_recipe   chromeos_amd64-generic_chromium_compile_only_ng   android_n5x_swarming_rel   ios-device   ios-device-xcode-clang   android_arm64_dbg_recipe   win_chromium_rel_ng   android_compile_dbg   cast_shell_android   chromium_presubmit   linux_chromium_rel_ng   ios-simulator-xcode-clang   mac_chromium_compile_dbg_ng 
Commit queue not available (can’t edit this change).

Messages

Total messages: 56 (34 generated)
sense (YandexTeam)
5 months ago (2017-03-14 12:28:34 UTC) #3
vasilii
Ideally you write what you need these 5 reviewers to do.
5 months ago (2017-03-14 13:15:21 UTC) #6
sense (YandexTeam)
First of all I would like to ask phajdan.jr@ and sdefresne@ to take a look ...
5 months ago (2017-03-15 08:36:15 UTC) #14
Michael van Ouwerkerk
s/mvanouwerkerk/peter/
5 months ago (2017-03-15 10:39:57 UTC) #16
vasilii
https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/password_manager/password_manager_internals_service_unittest.cc File chrome/browser/password_manager/password_manager_internals_service_unittest.cc (left): https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/password_manager/password_manager_internals_service_unittest.cc#oldcode33 chrome/browser/password_manager/password_manager_internals_service_unittest.cc:33: std::unique_ptr<TestingProfile> profile(builder.Build()); On 2017/03/15 08:36:15, sense (YandexTeam) wrote: > ...
5 months ago (2017-03-15 10:59:25 UTC) #19
sense (YandexTeam)
https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/password_manager/password_manager_internals_service_unittest.cc File chrome/browser/password_manager/password_manager_internals_service_unittest.cc (left): https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/password_manager/password_manager_internals_service_unittest.cc#oldcode33 chrome/browser/password_manager/password_manager_internals_service_unittest.cc:33: std::unique_ptr<TestingProfile> profile(builder.Build()); On 2017/03/15 10:59:24, vasilii wrote: > On ...
5 months ago (2017-03-15 11:10:58 UTC) #20
vasilii
lgtm chrome/browser/password_manager/password_manager_internals_service_unittest.cc
5 months ago (2017-03-15 11:45:11 UTC) #21
sdefresne
components/keyed_service lgtm Thank you for restoring those checks and for adding them at runtime.
5 months ago (2017-03-15 13:55:53 UTC) #22
Peter Beverloo
push_messaging lgtm
5 months ago (2017-03-15 18:09:18 UTC) #23
sense (YandexTeam)
Kindly ping. phajdan.jr@ components/keyed_service benwells@ apps; chrome/browser/engagement; chrome/browser/extensions; extensions dmazzoni@ chrome/browser/extensions/api/braille_display_private benjhayden@ chrome/browser/extensions/api/downloads shuchen@ chrome/browser/extensions/api/input_ime ...
5 months ago (2017-03-17 03:48:27 UTC) #24
benwells
https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc#newcode486 chrome/browser/engagement/site_engagement_service_unittest.cc:486: service->Shutdown(); This is unfortunate. Why do all the tests ...
4 months, 4 weeks ago (2017-03-20 03:15:27 UTC) #25
sense (YandexTeam)
https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc#newcode486 chrome/browser/engagement/site_engagement_service_unittest.cc:486: service->Shutdown(); On 2017/03/20 03:15:27, benwells wrote: > This is ...
4 months, 4 weeks ago (2017-03-20 04:01:57 UTC) #26
benwells
https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc#newcode486 chrome/browser/engagement/site_engagement_service_unittest.cc:486: service->Shutdown(); On 2017/03/20 04:01:56, sense (YandexTeam) wrote: > On ...
4 months, 4 weeks ago (2017-03-20 04:14:50 UTC) #27
dmazzoni
lgtm for braille_display_private
4 months, 4 weeks ago (2017-03-20 18:13:01 UTC) #32
sense (YandexTeam)
On 2017/03/20 04:14:50, benwells wrote: > https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc > File chrome/browser/engagement/site_engagement_service_unittest.cc (right): > > https://codereview.chromium.org/2749823002/diff/20001/chrome/browser/engagement/site_engagement_service_unittest.cc#newcode486 > ...
4 months, 4 weeks ago (2017-03-21 04:04:19 UTC) #37
Paweł Hajdan Jr.
Thanks. I'd like to understand some details a bit better. https://codereview.chromium.org/2749823002/diff/40001/components/keyed_service/content/browser_context_keyed_base_factory.cc File components/keyed_service/content/browser_context_keyed_base_factory.cc (left): https://codereview.chromium.org/2749823002/diff/40001/components/keyed_service/content/browser_context_keyed_base_factory.cc#oldcode24 ...
4 months, 4 weeks ago (2017-03-21 13:03:25 UTC) #38
benwells
https://codereview.chromium.org/2749823002/diff/40001/chrome/browser/engagement/site_engagement_service_unittest.cc File chrome/browser/engagement/site_engagement_service_unittest.cc (right): https://codereview.chromium.org/2749823002/diff/40001/chrome/browser/engagement/site_engagement_service_unittest.cc#newcode216 chrome/browser/engagement/site_engagement_service_unittest.cc:216: void CreateServiceWithTestClock(SiteEngagementService** service, Thanks for factoring out this too. ...
4 months, 3 weeks ago (2017-03-21 23:44:51 UTC) #39
sense (YandexTeam)
Here is an explanation about modified assertions and why they didn't work before. https://codereview.chromium.org/2749823002/diff/40001/chrome/browser/engagement/site_engagement_service_unittest.cc File ...
4 months, 3 weeks ago (2017-03-22 07:38:22 UTC) #40
Paweł Hajdan Jr.
LGTM
4 months, 3 weeks ago (2017-03-23 12:49:29 UTC) #45
benwells
lgtm
4 months, 3 weeks ago (2017-03-24 02:02:04 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2749823002/60001
4 months, 3 weeks ago (2017-03-24 04:02:31 UTC) #53
commit-bot: I haz the power
4 months, 3 weeks ago (2017-03-24 04:08:00 UTC) #56
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/383ce0fea1b6fd7c0bfa3d981d7c...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b