New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 701321 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocking:
issue 701326



Sign in to add a comment

Revisit KeyedServiceFactory diagnostics for context use-after-destroy

Project Member Reported by se...@yandex-team.ru, Mar 14 2017

Issue description

Currently KeyedServiceFactory and friends have a debug assertion for context use-after-destroy (AssertContextWasntDestroyed). But there is a problem - this assertion is located in the virtual method GetBrowserContextToUse() which is almost always overriden by user factories. Because of that, the assertion is almost always ignored! This can create unpleasant situations with invalid service destructions which can lead to memory corruption and more.

You can see the current implementation here:
https://chromium.googlesource.com/chromium/src.git/+/59.0.3041.2/components/keyed_service/content/browser_context_keyed_base_factory.cc#24
https://chromium.googlesource.com/chromium/src.git/+/59.0.3041.2/components/keyed_service/content/browser_context_keyed_service_factory.cc#54

Adding to that, this assertion works only in debug mode and doesn't have an effect in release builds. We had some crashes which were traced down to the invalid service shutdown, and we decided to add DumpWithoutCrashes() for this situation in production.
I've prepared CL with all required improvements and fixes, will upload it soon.

--

And there is one more thing: the virtual method GetBrowserContextToUse() also contains an important DCHECK(CalledOnValidThread()) which is also almost always ignored. When I enabled it for all services, I found few problems in NaCl and Sync service implementations. I will create another issue for that and link it in the CL.

NaCl:
[22036:14544:0314/060338.808:FATAL:keyed_service_base_factory.cc(87)] Check failed: CalledOnValidThread(). 
Backtrace:
	base::debug::StackTrace::StackTrace [0x70473E40+32]
	base::debug::StackTrace::StackTrace [0x704732AD+13]
	logging::LogMessage::~LogMessage [0x7049DB11+49]
	KeyedServiceBaseFactory::AssertContextWasntDestroyed [0x6A4A62CC+90]
	BrowserContextKeyedServiceFactory::GetContextToUse [0x69DA3698+14]
	KeyedServiceFactory::GetServiceForContext [0x6A4A6858+196]
	extensions::ExtensionSystemFactory::GetForBrowserContext [0x02FB3615+23]
	extensions::ExtensionSystem::Get [0x01C9D707+27]
	NaClBrowserDelegateImpl::GetExtensionInfoMap [0x027A07D3+103]
	NaClBrowserDelegateImpl::MapUrlToLocalFilePath [0x027A0740+36]
	nacl::NaClHostMessageFilter::LaunchNaClContinuationOnIOThread [0x037EE56A+414]

Sync service:
(1) [11052:14732:0314/060126.726:FATAL:keyed_service_base_factory.cc(87)] Check failed: CalledOnValidThread(). 
Backtrace:
	base::debug::StackTrace::StackTrace [0x70963E40+32]
	base::debug::StackTrace::StackTrace [0x709632AD+13]
	logging::LogMessage::~LogMessage [0x7098DB11+49]
	KeyedServiceBaseFactory::AssertContextWasntDestroyed [0x6C3762CC+90]
	BrowserContextKeyedServiceFactory::GetContextToUse [0x6BDA3698+14]
	KeyedServiceFactory::GetServiceForContext [0x6C376858+196]
	extensions::StorageFrontend::Get [0x00DF60D8+28]
	extensions::settings_sync_util::GetSyncableService [0x01D361A5+193]
	browser_sync::ChromeSyncClient::GetSyncableServiceForType [0x0148324A+130]
	syncer::SharedChangeProcessor::Connect [0x02AA0023+311]
	syncer::SharedChangeProcessor::StartAssociation [0x02A9F80A+206]

(2) [10612:11124:0314/060126.867:FATAL:keyed_service_base_factory.cc(87)] Check failed: CalledOnValidThread(). 
Backtrace:
	base::debug::StackTrace::StackTrace [0x70963E40+32]
	base::debug::StackTrace::StackTrace [0x709632AD+13]
	logging::LogMessage::~LogMessage [0x7098DB11+49]
	KeyedServiceBaseFactory::AssertContextWasntDestroyed [0x6C3762CC+90]
	BrowserContextKeyedServiceFactory::GetContextToUse [0x6BDA3698+14]
	KeyedServiceFactory::GetServiceForContext [0x6C376858+196]
	HistoryServiceFactory::GetForProfile [0x014A160A+110]
	browser_sync::ChromeSyncClient::GetSyncableServiceForType [0x014832C1+249]
	syncer::SharedChangeProcessor::Connect [0x02AA0023+311]
	syncer::SharedChangeProcessor::StartAssociation [0x02A9F80A+206]
 
Blocking: 701326

Comment 2 by droger@chromium.org, Mar 14 2017

Cc: sdefresne@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758

commit 383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758
Author: sense <sense@yandex-team.ru>
Date: Fri Mar 24 04:06:43 2017

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}

[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/apps/app_load_service.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/apps/app_load_service.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/engagement/site_engagement_service.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/engagement/site_engagement_service.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/engagement/site_engagement_service_unittest.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/api/input_ime/input_ime_api.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/api/input_ime/input_ime_api.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_manager.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/api/signed_in_devices/signed_in_devices_manager.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/extension_gcm_app_handler.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/extension_gcm_app_handler.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/password_manager/password_manager_internals_service_unittest.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/browser/push_messaging/push_messaging_service_impl.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/chrome/test/base/testing_profile.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/content/browser_context_dependency_manager.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/content/browser_context_dependency_manager.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/content/browser_context_keyed_base_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/content/browser_context_keyed_service_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/content/refcounted_browser_context_keyed_service_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/core/dependency_manager.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/core/dependency_manager.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/core/keyed_service_base_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/core/keyed_service_base_factory.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/core/keyed_service_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/core/refcounted_keyed_service_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/ios/browser_state_dependency_manager.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/ios/browser_state_dependency_manager.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/ios/browser_state_keyed_service_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/components/keyed_service/ios/refcounted_browser_state_keyed_service_factory.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/extensions/browser/api/web_request/web_request_api.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/extensions/browser/process_manager.cc
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/extensions/browser/process_manager.h
[modify] https://crrev.com/383ce0fea1b6fd7c0bfa3d981d7c71bcd5bcd758/extensions/shell/browser/shell_content_browser_client.cc

Status: Fixed (was: Started)
Required checks was moved to the appropriate methods. The next step is to fix
thread problems described in https://bugs.chromium.org/p/chromium/issues/detail?id=701326.

Sign in to add a comment