Issue metadata
Sign in to add a comment
|
DCHECK in chromeos::AccessibilityManager::UpdateChromeOSAccessibilityHistograms |
||||||||||||||||||||||||
Issue descriptionWhen built with DCHECKs enabled content::BrowserAccessibilityStateImpl::UpdateHistograms triggers a failure in chromeos::AccessibilityManager::UpdateChromeOSAccessibilityHistograms because it calls PrefService::GetBoolean() from the wrong thread: [35510:35547:0522/144556.800950:FATAL:pref_service.cc(117)] Check failed: CalledOnValidThread(). #0 0x7f68999dd64c base::debug::StackTrace::StackTrace() #1 0x7f6899a01ba1 logging::LogMessage::~LogMessage() #2 0x7f689832a283 PrefService::GetBoolean() #3 0x55b7d9c8472c chromeos::AccessibilityManager::UpdateChromeOSAccessibilityHistograms() #4 0x7f6896fdb947 content::BrowserAccessibilityStateImpl::UpdateHistograms() #5 0x7f6896cc728c _ZN4base8internal7InvokerINS0_9BindStateIMN7content17WebFileSystemImpl23WaitableCallbackResultsEFvvEJ13scoped_refptrIS5_EEEEFvvEE3RunEPNS0_13BindStateBaseE #6 0x7f68999c8b19 _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv #7 0x7f68999ddfe4 base::debug::TaskAnnotator::RunTask() #8 0x7f6899a6e6b7 base::internal::TaskTracker::PerformRunTask() #9 0x7f6899a6ed47 base::internal::TaskTrackerPosix::PerformRunTask() #10 0x7f6899a6dea7 base::internal::TaskTracker::RunTask() #11 0x7f6899a6866e base::internal::SchedulerWorker::Thread::ThreadMain() #12 0x7f6899a78aef base::(anonymous namespace)::ThreadFunc() #13 0x7f6899b55184 start_thread #14 0x7f688ddf9bed clone I believe the switch to using the Task Scheduler in r473281 was the cause.
,
May 23 2017
Looks like ChromeOS grabs prefs as part of updating its histograms. That's the real issue here. Since ChromeOS is POSIX, this ran on the UI thread and no one was the wiser. Since the change moved the code to the task scheduler, the ChromeOS code is no longer running on the UI thread and is not permitted to access prefs. That data will either need to be computed ahead of time and passed through or obtained via a PostTaskAndReply to the UI thread to perform the histograms update.
,
May 23 2017
Exactly, access to prefs must happen on the UI thread. Per: #if defined(OS_WIN) // On Windows, UpdateHistograms calls some system functions with unknown // runtime, so call it on the file thread to ensure there's no jank. // Everything in that method must be safe to call on another thread. BrowserThread::ID update_histogram_thread = BrowserThread::FILE; #else // On all other platforms, UpdateHistograms should be called on the main // thread. BrowserThread::ID update_histogram_thread = BrowserThread::UI; #endif The easiest solution is probably to only base::PostTaskWithTraits on Windows. Otherwise it will require untangling the fact that Windows does file I/O while other platforms access prefs from the same "cross platform" code (that's bad cross platform code design but we can't redesign all of Chrome as part of this migration either...).
,
May 23 2017
Yep, I'll rewrite my CL and I'll keep the platform check this time.
,
May 23 2017
,
Jul 7 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by sebmarchand@chromium.org
, May 22 2017