HostContentSettingsMap is RefCountedThreadSafe but its destruction isn't thread-safe |
||
Issue descriptionI tripped this in https://codereview.chromium.org/2576243003/ which merely tweaked things enough to unintentionally highlight this existing race. HostContentSettingsMap can't be deleted from any thread because it vends WeakPtrs. It's owned through a RefCountedThreadSafe scheme via RefcountedKeyedService. The construction task runner should be bound to the RefcountedKeyedService for RefcountedKeyedServiceTraits::Destruct() to properly flush the deletion to the owner thread. Also RefcountedKeyedServiceTraits::Destruct() blindly checks base::ThreadTaskRunnerHandle::Get() which DCHECKs when !base::ThreadTaskRunnerHandle::IsSet() (which can happen under the same circumstances) so it should check for that first. And lastly RefCountedKeyService binds to a SingleThreadTaskRunner but it would be fine with a SequencedTaskRunner (I'll do that too as it lines up with our attempt to go away from unnecessary thread-affinity). [ RUN ] ExtensionApiTest.IncognitoNoScript [17552:3116:1216/110642.503:WARNING:embedded_test_server.cc(219)] Request not handled. Returning 404: /favicon.ico [17552:10564:1216/110642.607:FATAL:sequenced_worker_pool.cc(294)] Check failed: is_processing_task_. Backtrace: base::debug::StackTrace::StackTrace [0x67300463+19] (D:\src\chrome\src\base\debug\stack_trace_win.cc:214) logging::LogMessage::~LogMessage [0x67329C31+49] (D:\src\chrome\src\base\logging.cc:537) base::SequencedWorkerPool::GetSequenceTokenForCurrentThread [0x673C1590+224] (D:\src\chrome\src\base\threading\sequenced_worker_pool.cc:1399) base::SequenceCheckerImpl::EnsureSequenceTokenAssigned [0x67381BD6+70] (D:\src\chrome\src\base\sequence_checker_impl.cc:51) base::SequenceCheckerImpl::CalledOnValidSequence [0x67381CAB+43] (D:\src\chrome\src\base\sequence_checker_impl.cc:22) base::internal::WeakReferenceOwner::Invalidate [0x673397FF+47] (D:\src\chrome\src\base\memory\weak_ptr.cc:65) base::internal::WeakReferenceOwner::~WeakReferenceOwner [0x6733978B+11] (D:\src\chrome\src\base\memory\weak_ptr.cc:53) HostContentSettingsMap::~HostContentSettingsMap [0x02C69149+123] (D:\src\chrome\src\components\content_settings\core\browser\host_content_settings_map.cc:830) HostContentSettingsMap::`vector deleting destructor' [0x02C67D14+16] (D:\src\chrome\src\components\content_settings\core\browser\host_content_settings_map.h:0) impl::RefcountedKeyedServiceTraits::Destruct [0x56DB9385+129] (D:\src\chrome\src\components\keyed_service\core\refcounted_keyed_service.cc:18) scoped_refptr<HostContentSettingsMap>::~scoped_refptr [0x020A856D+35] (D:\src\chrome\src\base\memory\ref_counted.h:310) content_settings::CookieSettings::~CookieSettings [0x02C69FAC+38] (D:\src\chrome\src\components\content_settings\core\browser\cookie_settings.cc:203) content_settings::CookieSettings::~CookieSettings [0x02C69E91+11] (D:\src\chrome\src\components\content_settings\core\browser\cookie_settings.cc:202) impl::RefcountedKeyedServiceTraits::Destruct [0x56DB9385+129] (D:\src\chrome\src\components\keyed_service\core\refcounted_keyed_service.cc:18) scoped_refptr<content_settings::CookieSettings>::~scoped_refptr [0x0104B3DD+35] (D:\src\chrome\src\base\memory\ref_counted.h:310) ExtensionSpecialStoragePolicy::~ExtensionSpecialStoragePolicy [0x0301BA0C+24] (D:\src\chrome\src\chrome\browser\extensions\extension_special_storage_policy.cc:87) ExtensionSpecialStoragePolicy::~ExtensionSpecialStoragePolicy [0x0301A73F+11] (D:\src\chrome\src\chrome\browser\extensions\extension_special_storage_policy.cc:87) base::RefCountedThreadSafe<storage::SpecialStoragePolicy, base::DefaultRefCountedThreadSafeTraits<storage::SpecialStoragePolicy> >::Release [0x57F019EE+30] (D:\src\chrome\src\base\memory\ref_counted.h:184) content::DOMStorageContextImpl::~DOMStorageContextImpl [0x5918FF0E+346] (D:\src\chrome\src\content\browser\dom_storage\dom_storage_context_impl.cc:117) content::DOMStorageContextImpl::~DOMStorageContextImpl [0x5918CD35+11] (D:\src\chrome\src\content\browser\dom_storage\dom_storage_context_impl.cc:102) scoped_refptr<content::DOMStorageContextImpl>::~scoped_refptr [0x5918ED6A+38] (D:\src\chrome\src\base\memory\ref_counted.h:310) base::internal::BindState<void (content::DOMStorageContextImpl::*)() __attribute__((thiscall)), scoped_refptr<content::DOMStorageContextImpl> >::Destroy [0x5918ED36+18] (D:\src\chrome\src\base\bind_internal.h:485) base::internal::CallbackBase<base::internal::CopyMode::MoveOnly>::~CallbackBase [0x672E4F0C+28] (D:\src\chrome\src\base\callback_internal.cc:79) base::SequencedWorkerPool::Inner::ThreadLoop [0x673BE8B7+3383] (D:\src\chrome\src\base\threading\sequenced_worker_pool.cc:1080) base::SequencedWorkerPool::Worker::Run [0x673BDAAE+446] (D:\src\chrome\src\base\threading\sequenced_worker_pool.cc:603) base::SimpleThread::ThreadMain [0x673C75AF+223] (D:\src\chrome\src\base\threading\simple_thread.cc:68) base::`anonymous namespace'::ThreadFunc [0x673BBFB8+168] (D:\src\chrome\src\base\threading\platform_thread_win.cc:86) BaseThreadInitThunk [0x774D38F4+36] RtlUnicodeStringToInteger [0x77B75DE3+595] RtlUnicodeStringToInteger [0x77B75DAE+542]
,
Dec 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/28fb80beda6b171647d037057543f77e5b78e170 commit 28fb80beda6b171647d037057543f77e5b78e170 Author: gab <gab@chromium.org> Date: Mon Dec 19 20:09:28 2016 Force HostContentSettingsMap to be destroyed on its owning thread. It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI). The rest of the changes can then be inferred from the stack trace on the bug: 1) RefcountedKeyedServiceTraits::Destruct() must check for ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get(). ** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH comparison. 2) Nothing about RefcountedKeyedService's use of its |task_runner| is thread-affine => using SequencedTaskRunner instead is inline with our desire to make all top-level API use sequence-affinity instead of thread-affinity for thread-safety. 3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor requires the SequencedWorkerPool's task info to be set in that scope => brought changes from same requirements in another CL @ https://codereview.chromium.org/2491613004/#ps160001 Note: intentionally not adding tests as this change highlights that this deletion is racy in theory (deleting without owning the sequence means potentially deleting things from the same SequenceToken in parallel... any good test would fail TSAN and I also don't want to address this issue given it's long standing and SWP is being replaced by TaskScheduler shortly). 4) Switch to ScopedMockTimeMessageLoopTaskRunner in NotificationPermissionContextTest => otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap is sent for deletion on the wrong task runner and the task is never flushed. (ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before replacing it, both ways). Precursor requirement to https://codereview.chromium.org/2576243003/ which for some reason makes this destruction race come to life. BUG= 674946 , 665588 , 622400 TBR=mukai (NotificationPermissionContextTest side-effects) Review-Url: https://codereview.chromium.org/2581213002 Cr-Commit-Position: refs/heads/master@{#439534} [modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/base/threading/sequenced_worker_pool.cc [modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/chrome/browser/notifications/notification_permission_context_unittest.cc [modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/content_settings/core/browser/cookie_settings_unittest.cc [modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/content_settings/core/browser/host_content_settings_map.cc [modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/keyed_service/core/refcounted_keyed_service.cc [modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/keyed_service/core/refcounted_keyed_service.h
,
Dec 19 2016
|
||
►
Sign in to add a comment |
||
Comment 1 by gab@chromium.org
, Dec 16 2016