DCHECK when killing Chrome in background |
||||
Issue description
Killing Chrome in background, I get the following DCHECK in ProfileSyncService::Shutdown:
DCHECK(!observers_.might_have_observers());
Inspecting observers_, there are still two items registered in the list:
servers_
(base::ObserverList<syncer::SyncServiceObserver, false, true>) $5 = {
base::SupportsWeakPtr<base::ObserverList<syncer::SyncServiceObserver, false, true> > = {
weak_reference_owner_ = {
flag_ = {
ptr_ = 0x00006100000bef60
}
}
}
observers_ = size=2 {
[0] = 0x000060c000242a38
[1] = 0x0000620000243960
}
live_iterator_count_ = 0
policy_ = ALL
}
(lldb) p (syncer::SyncServiceObserver *)0x000060c000242a38
(DesktopPromotionSyncObserver *) $7 = 0x000060c000242a38
(lldb) p (syncer::SyncServiceObserver *)0x0000620000243960
(SyncObserverBridge *) $8 = 0x0000620000243960
SyncObserverBridge is a C++ wrapper that allows to forward the events to an Objective-C observer. Usually it is owned by an Objective-C object, so I suspect this is the case here and the Objective-C object is somehow leaked (my guess would be that it is retained by some block).
DesktopPromotionSyncObserver is owned by DesktopPromotionSyncService. This is a KeyedService that register its observer when it is created and unregister it when it is destroyed. This is incorrect, it should unregister the observer in the Shutdown method.
,
Jun 19 2018
Adding some logging revealed that the SyncObserverBridge is used by a downstream class, so will use a RBS bug to track solving it.
,
Jun 19 2018
,
Jun 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ddd2927c777cbc385efa52154ba7c4ba5b0d1b91 commit ddd2927c777cbc385efa52154ba7c4ba5b0d1b91 Author: Sylvain Defresne <sdefresne@chromium.org> Date: Tue Jun 19 23:44:31 2018 Unregister SyncObserver on shutdown ProfileSyncService DCHECK in Shutdown that all observer have been unregistered, so refactor DesktopPromotionSyncService to unregister its observer in Shutdown. The refactor consist in removal of the helper observer class DesktopPromotionSyncObserver that is folded in the service as it makes it easier to track whether the observer is still registered (the alternative is to introduce a delegate class that is implemented by the service and invoked by the observer when it needs to unregister). As the class had no behaviour at this point, merging it with the observer is simpler. Also fix the fact that desktop_metrics_logger_initiated_ was not initialized in DesktopPromotionSyncObserver constructor, so the check in OnStateChange was undefined behaviour (most likely the result would have been that the variable was read to be non-false). Mark some local variable as const and avoid reading prefs that are not used. Bug: 854118 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I8d5d2bcb09da0430d3a08d79058bdecd047f3f28 Reviewed-on: https://chromium-review.googlesource.com/1105978 Commit-Queue: Justin Cohen <justincohen@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Cr-Commit-Position: refs/heads/master@{#568647} [modify] https://crrev.com/ddd2927c777cbc385efa52154ba7c4ba5b0d1b91/ios/chrome/browser/desktop_promotion/BUILD.gn [delete] https://crrev.com/6a5dd83659799940e0ea7cbf6c4e5fec2b442bff/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.cc [delete] https://crrev.com/6a5dd83659799940e0ea7cbf6c4e5fec2b442bff/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_observer.h [modify] https://crrev.com/ddd2927c777cbc385efa52154ba7c4ba5b0d1b91/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service.cc [modify] https://crrev.com/ddd2927c777cbc385efa52154ba7c4ba5b0d1b91/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service.h [modify] https://crrev.com/ddd2927c777cbc385efa52154ba7c4ba5b0d1b91/ios/chrome/browser/desktop_promotion/desktop_promotion_sync_service_factory.cc
,
Jun 20 2018
,
Jun 20 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by sdefresne@chromium.org
, Jun 19 2018