Destructors of objects in sequence-local storage should run on the owning sequence |
|||||||
Issue descriptionCurrently, destructors of objects in sequence-local storage ( bug 695727 ) run in the context of ~SequenceLocalStorageMap, which runs in the context of ~Sequence, which could be running on another Sequence or even on no active Sequence at all. This can cause e.g. DCHECKs when objects stored in SequenceLocalStorageMap expect to be destroyed on the same sequence they've been created on. An example issue is with the scoped_refptr holding SyncHandleRegistry[1]. [1] https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/lib/sync_handle_registry.cc?rcl=ed60367fd5aee162692049220741b6506cb75027&l=19 This issue is reproducible with a browsertest with --login-manager and --force-login-manager-in-tests which does nothing in its test body and then crashes with this stack trace on exit: [16448:16486:0731/073705.268991:FATAL:ref_counted.h(83)] Check failed: CalledOnValidSequence(). #0 0x7fe38d00ba6d base::debug::StackTrace::StackTrace() #1 0x7fe38cd1e3dc base::debug::StackTrace::StackTrace() #2 0x7fe38cd87f9d logging::LogMessage::~LogMessage() #3 0x7fe387b9f6be base::subtle::RefCountedBase::Release() #4 0x7fe387ba293f base::RefCounted<>::Release() #5 0x7fe387ba2915 scoped_refptr<>::Release() #6 0x7fe387ba25da scoped_refptr<>::~scoped_refptr() #7 0x7fe387ba786b _ZZN4base24SequenceLocalStorageSlotI13scoped_refptrIN4mojo18SyncHandleRegistryEENSt3__114default_deleteIS4_EEE3SetES4_ENKUlPvE_clES9_ #8 0x7fe387ba7825 _ZZN4base24SequenceLocalStorageSlotI13scoped_refptrIN4mojo18SyncHandleRegistryEENSt3__114default_deleteIS4_EEE3SetES4_ENUlPvE_8__invokeES9_ #9 0x7fe38cf43ddb base::internal::SequenceLocalStorageMap::ValueDestructorPair::~ValueDestructorPair() #10 0x7fe38cf442c9 std::__1::pair<>::~pair() #11 0x7fe38cf441ee std::__1::__vector_base<>::~__vector_base() #12 0x7fe38cf440c5 std::__1::vector<>::~vector() #13 0x7fe38cf440a5 base::internal::flat_tree<>::Impl::~Impl() #14 0x7fe38cf44085 base::internal::flat_tree<>::~flat_tree() #15 0x7fe38cf43e65 base::flat_map<>::~flat_map() #16 0x7fe38cf438f5 base::internal::SequenceLocalStorageMap::~SequenceLocalStorageMap() #17 0x7fe38cf286c5 base::internal::Sequence::~Sequence() #18 0x7fe38cf0e1b7 base::RefCountedThreadSafe<>::DeleteInternal<>() #19 0x7fe38cf0e185 base::DefaultRefCountedThreadSafeTraits<>::Destruct( #20 0x7fe38cf0e168 base::RefCountedThreadSafe<>::Release() #21 0x7fe38cf0e125 scoped_refptr<>::Release() #22 0x7fe38cf0deca scoped_refptr<>::~scoped_refptr() #23 0x7fe38cf15927 base::internal::(anonymous namespace)::SchedulerWorkerDelegate::OnMainExit() #24 0x7fe38cf1aa98 base::internal::SchedulerWorker::RunWorker() #25 0x7fe38cf1a1ac base::internal::SchedulerWorker::RunSharedWorker() #26 0x7fe38cf1a0ad base::internal::SchedulerWorker::ThreadMain() #27 0x7fe38d03d16d base::(anonymous namespace)::ThreadFunc() #28 0x7fe368c3b494 start_thread #29 0x7fe367648a8f clone Minimal repro: class TestWeirdCrashBrowserTest : public InProcessBrowserTest { void SetUpCommandLine(base::CommandLine* command_line) override { InProcessBrowserTest::SetUpCommandLine(command_line); command_line->AppendSwitch(chromeos::switches::kLoginManager); command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests); } }; IN_PROC_BROWSER_TEST_F(TestWeirdCrashBrowserTest, Test) { } gn args: target_os="chromeos" is_component_build = true is_debug = true Workaround for the browser test: A workaround seems to be to wait until the login UI is available: void SetUpOnMainThread() override { InProcessBrowserTest::SetUpOnMainThread(); // Wait for the login manager UI to be available before continuing. content::WindowedNotificationObserver( chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE, content::NotificationService::AllSources()) .Wait(); } But this does not fix the root cause.
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d930a146087845db374ebcbe39a1a95411031217 commit d930a146087845db374ebcbe39a1a95411031217 Author: Pavol Marko <pmarko@chromium.org> Date: Tue Jul 31 12:13:27 2018 Workaround to fix crash in UserAffiliationBrowserTest in debug mode Ensure to wait until the login manager UI is loaded/available before continuing with the actual test. When UserAffiliationBrowserTest was passing "--login-manager" and "--force-login-manager-in-test" and the test function did not do any substantial work, the test crashed on shutdown. browser_tests --gtest_filter=*UserAffiliationBrowserTest* Bug: 869272 Test: compile browser tests with is_debug=true and run \ Change-Id: I44c35e5df6850d908a694a64b2bd648b008ec5a6 Reviewed-on: https://chromium-review.googlesource.com/1155592 Reviewed-by: Lutz Justen <ljusten@chromium.org> Commit-Queue: Pavol Marko <pmarko@chromium.org> Cr-Commit-Position: refs/heads/master@{#579378} [modify] https://crrev.com/d930a146087845db374ebcbe39a1a95411031217/chrome/browser/chromeos/policy/user_affiliation_browsertest.cc
,
Jul 31
,
Jul 31
,
Jul 31
,
Jul 31
Issue 844572 has been merged into this issue.
,
Jul 31
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/521a221d210d903618f7168b4c41c380673c0980 commit 521a221d210d903618f7168b4c41c380673c0980 Author: Pavol Marko <pmarko@chromium.org> Date: Wed Aug 01 16:42:37 2018 Set SequenceToken for SequenceLocalStorageMap destructor Ensure that the SequenceLocalStorageMap and consequently objects still stored in the sequence-local storage are destroyed while the owning Sequence's SequenceToken is set for the current thread. This makes sure that objects stored in the sequence-local storage which expect to be destroyed on the same Sequence they've been created on do not (D)CHECK in their destrutor. Bug: 869272 Test: base_unittests && browser_tests --gtest_filter=*UserAffiliation* Change-Id: I8c76b274a746a35c4ad038e7d8f04ea212ef1514 Reviewed-on: https://chromium-review.googlesource.com/1156510 Commit-Queue: Pavol Marko <pmarko@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#579834} [modify] https://crrev.com/521a221d210d903618f7168b4c41c380673c0980/base/sequence_token.cc [modify] https://crrev.com/521a221d210d903618f7168b4c41c380673c0980/base/sequence_token.h [modify] https://crrev.com/521a221d210d903618f7168b4c41c380673c0980/base/sequence_token_unittest.cc [modify] https://crrev.com/521a221d210d903618f7168b4c41c380673c0980/base/task_scheduler/sequence.cc [modify] https://crrev.com/521a221d210d903618f7168b4c41c380673c0980/base/task_scheduler/sequence.h [modify] https://crrev.com/521a221d210d903618f7168b4c41c380673c0980/base/threading/sequence_local_storage_map.cc [modify] https://crrev.com/521a221d210d903618f7168b4c41c380673c0980/base/threading/sequence_local_storage_map.h
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6061320172b76334d7e3f9ab068f7c57d13c0633 commit 6061320172b76334d7e3f9ab068f7c57d13c0633 Author: Pavol Marko <pmarko@chromium.org> Date: Thu Aug 02 04:07:13 2018 Revert "Set SequenceToken for SequenceLocalStorageMap destructor" This reverts commit 521a221d210d903618f7168b4c41c380673c0980. Reason for revert: Reported to cause crashes - https://crbug.com/870147 Original change's description: > Set SequenceToken for SequenceLocalStorageMap destructor > > Ensure that the SequenceLocalStorageMap and consequently objects still > stored in the sequence-local storage are destroyed while the owning > Sequence's SequenceToken is set for the current thread. > This makes sure that objects stored in the sequence-local storage which > expect to be destroyed on the same Sequence they've been created on do > not (D)CHECK in their destrutor. > > Bug: 869272 > Test: base_unittests && browser_tests --gtest_filter=*UserAffiliation* > Change-Id: I8c76b274a746a35c4ad038e7d8f04ea212ef1514 > Reviewed-on: https://chromium-review.googlesource.com/1156510 > Commit-Queue: Pavol Marko <pmarko@chromium.org> > Reviewed-by: Gabriel Charette <gab@chromium.org> > Cr-Commit-Position: refs/heads/master@{#579834} TBR=gab@chromium.org,pmarko@chromium.org Change-Id: Iece11abba0b755a05bbafcd89046a4921354703c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 869272 Reviewed-on: https://chromium-review.googlesource.com/1159941 Reviewed-by: Pavol Marko <pmarko@chromium.org> Commit-Queue: Pavol Marko <pmarko@chromium.org> Cr-Commit-Position: refs/heads/master@{#580055} [modify] https://crrev.com/6061320172b76334d7e3f9ab068f7c57d13c0633/base/sequence_token.cc [modify] https://crrev.com/6061320172b76334d7e3f9ab068f7c57d13c0633/base/sequence_token.h [modify] https://crrev.com/6061320172b76334d7e3f9ab068f7c57d13c0633/base/sequence_token_unittest.cc [modify] https://crrev.com/6061320172b76334d7e3f9ab068f7c57d13c0633/base/task_scheduler/sequence.cc [modify] https://crrev.com/6061320172b76334d7e3f9ab068f7c57d13c0633/base/task_scheduler/sequence.h [modify] https://crrev.com/6061320172b76334d7e3f9ab068f7c57d13c0633/base/threading/sequence_local_storage_map.cc [modify] https://crrev.com/6061320172b76334d7e3f9ab068f7c57d13c0633/base/threading/sequence_local_storage_map.h
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb3a625e65dc819a56aa1ac7c8b7758a557d2170 commit cb3a625e65dc819a56aa1ac7c8b7758a557d2170 Author: Pavol Marko <pmarko@chromium.org> Date: Thu Aug 02 06:08:14 2018 Revert "Set SequenceToken for SequenceLocalStorageMap destructor" This reverts commit 521a221d210d903618f7168b4c41c380673c0980. Reason for revert: Reported to cause crashes - https://crbug.com/870147 Original change's description: > Set SequenceToken for SequenceLocalStorageMap destructor > > Ensure that the SequenceLocalStorageMap and consequently objects still > stored in the sequence-local storage are destroyed while the owning > Sequence's SequenceToken is set for the current thread. > This makes sure that objects stored in the sequence-local storage which > expect to be destroyed on the same Sequence they've been created on do > not (D)CHECK in their destrutor. > > Bug: 869272 > Test: base_unittests && browser_tests --gtest_filter=*UserAffiliation* > Change-Id: I8c76b274a746a35c4ad038e7d8f04ea212ef1514 > Reviewed-on: https://chromium-review.googlesource.com/1156510 > Commit-Queue: Pavol Marko <pmarko@chromium.org> > Reviewed-by: Gabriel Charette <gab@chromium.org> > Cr-Commit-Position: refs/heads/master@{#579834} TBR=gab@chromium.org,pmarko@chromium.org Change-Id: Iece11abba0b755a05bbafcd89046a4921354703c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 869272 Reviewed-on: https://chromium-review.googlesource.com/1159941 Reviewed-by: Pavol Marko <pmarko@chromium.org> Commit-Queue: Pavol Marko <pmarko@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#580055}(cherry picked from commit 6061320172b76334d7e3f9ab068f7c57d13c0633) Reviewed-on: https://chromium-review.googlesource.com/1159991 Reviewed-by: Abdul Syed <abdulsyed@google.com> Cr-Commit-Position: refs/branch-heads/3510@{#3} Cr-Branched-From: a7e2a40c1c3d42b8b0b9df2c543615d9553d9cb5-refs/heads/master@{#579964} [modify] https://crrev.com/cb3a625e65dc819a56aa1ac7c8b7758a557d2170/base/sequence_token.cc [modify] https://crrev.com/cb3a625e65dc819a56aa1ac7c8b7758a557d2170/base/sequence_token.h [modify] https://crrev.com/cb3a625e65dc819a56aa1ac7c8b7758a557d2170/base/sequence_token_unittest.cc [modify] https://crrev.com/cb3a625e65dc819a56aa1ac7c8b7758a557d2170/base/task_scheduler/sequence.cc [modify] https://crrev.com/cb3a625e65dc819a56aa1ac7c8b7758a557d2170/base/task_scheduler/sequence.h [modify] https://crrev.com/cb3a625e65dc819a56aa1ac7c8b7758a557d2170/base/threading/sequence_local_storage_map.cc [modify] https://crrev.com/cb3a625e65dc819a56aa1ac7c8b7758a557d2170/base/threading/sequence_local_storage_map.h
,
Aug 2
My attempt at a fix, CL https://chromium-review.googlesource.com/1156510 has caused crashes on Windows (bug 870147). The CL has been reverted. An example crash is: CRASHED [EXCEPTION_ACCESS_VIOLATION_READ @ 0x000001a9 ] 0x00007ff941eabdc0 (chrome.dll -thread_local_storage.cc:371 ) base::ThreadLocalStorage::Slot::Get() 0x00007ff944040020 (chrome.dll -sequence_token.cc:98 ) base::ScopedSetNestedSequenceTokenForDestructorForCurrentThread::ScopedSetNestedSequenceTokenForDestructorForCurrentThread(base::SequenceToken const &) 0x00007ff941f41ac4 (chrome.dll -sequence.cc:83 ) base::internal::Sequence::~Sequence() 0x00007ff9425c8e6c (chrome.dll -scheduler_worker_pool.cc:120 ) base::internal::SchedulerSequencedTaskRunner::~SchedulerSequencedTaskRunner 0x00007ff9448af263 (chrome.dll -onexit.cpp:204 ) <lambda_f03950bc5685219e0bcd2087efbe011e>::operator() 0x00007ff9448af36c (chrome.dll -internal_shared.h:204 ) __crt_seh_guarded_call<int>::operator()<<lambda_7777bce6b2f8c936911f934f8298dc43>,<lambda_f03950bc5685219e0bcd2087efbe011e> & __ptr64,<lambda_3883c3dff614d5e0c5f61bb1ac94921c> > 0x00007ff9448aefe4 (chrome.dll -onexit.cpp:160 ) execute_onexit_table ... 0x00007ff99d240897 (ntdll.dll + 0x00040897 ) RtlExitUserProcess 0x00007ff99d0cb199 (KERNEL32.DLL + 0x0001b199 ) ExitProcessImplementation 0x00007ff6d79fbedc (chrome.exe -exit.cpp:143 ) exit_or_terminate_process 0x00007ff6d79fbfa6 (chrome.exe -exit.cpp:280 ) common_exit ... So it seems that when shutting down, a SchedulerSequencedTaskRunner destructor runs. Its |sequence_| holds the last reference to the owned Sequence object, so it is destroyed too. With this CL, we try to temporarily change the SequenceToken (in thread-local storage) so destructors of objects in sequence-local storage see the Sequence's SequenceToken. However, ThreadLocalStorage::Slot::Get crashes at |tls_data[slot_].version| trying to read address 0x000001a9. The offset of version should be 8, so the &(tls_data[slot_]) is 0x1a1. As sizeof(tls_data[0]) should be 16=0x10, I'd expect that slot_=0x1a and tls_data = 1, which maps to kDestroyed[1]. This would mean that the Thread-local storage has been destroyed before the SchedulerSequencedTaskRunner and the associated Sequence is being destroyed. I guess if both things happen on thread exit, the order is undefined, so this would not be surprising. Thoughts: (1) Is it ok for a SchedulerSequencedTaskRunner::~SchedulerSequencedTaskRunner to be running this late at shutdown/should it have been released earlier/should it have been leaky and never run? (2) One workaround may be moving SequenceLocalStorageMap to the heap after all, and only destroying it in ~Sequence (with the correct SequenceToken set) if ThreadLocalStorage::HasBeenDestroyed is returning false. Otherwise, release() the unique_ptr and leak it. [1] https://cs.chromium.org/chromium/src/base/threading/thread_local_storage.cc?rcl=25037c651a9b5ddc192ca89a9432a7ef460bf795&l=109 |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by pmarko@chromium.org
, Jul 31