From 990284c41ea025390a200b87e95b46a0ee42afc5 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 21 May 2020 15:03:43 +0000 Subject: [PATCH] Make sure hunspell file is not destroyed in UI thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this code: base::PostTaskAndReplyWithResult( task_runner_.get(), FROM_HERE, base::BindOnce(&InitializeDictionaryLocation, language_), base::BindOnce(&InitializeDictionaryLocationComplete, weak_ptr_factory_.GetWeakPtr())); When |SpellcheckHunspellDictionary| is destroyed before |InitializeDictionaryLocation| is finished, the |InitializeDictionaryLocationComplete| method would not be called, and the returned |DictionaryFile| will be destroyed on UI thread, with its |base::File| member. It would then result in a DCHECK like this: [904:0518/140618.257:FATAL:thread_restrictions.cc(78)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as blocking was called from a scope that disallows blocking! If this task is running inside the ThreadPool, it needs to have MayBlock() in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBlocking (see its documentation for best practices). g_blocking_disallowed currently set to true by base::debug::CollectStackTrace [0x00007FF6805435C2+18] (o:\base\debug\stack_trace_win.cc:284) base::debug::StackTrace::StackTrace [0x00007FF68049BCC2+18] (o:\base\debug\stack_trace.cc:203) logging::LogMessage::~LogMessage [0x00007FF6804AF447+215] (o:\base\logging.cc:605) logging::LogMessage::~LogMessage [0x00007FF6804B0190+16] (o:\base\logging.cc:598) base::internal::AssertBlockingAllowed [0x00007FF680514B33+163] (o:\base\threading\thread_restrictions.cc:85) base::ScopedBlockingCall::ScopedBlockingCall [0x00007FF6805103EB+155] (o:\base\threading\scoped_blocking_call.cc:40) base::File::Close [0x00007FF680549E4B+139] (o:\base\files\file_win.cc:42) SpellcheckHunspellDictionary::DictionaryFile::~DictionaryFile [0x00007FF680071DDE+194] (o:\chrome\browser\spellchecker\spellcheck_hunspell_dictionary.cc:102) base::OnceCallback::Run [0x00007FF6800749BF+75] (o:\base\callback.h:100) To avoid have |base::File| destroyed on UI thread, we should make sure the file is closed in the destructor of |DictionaryFile|. Bug: 1085227 Change-Id: I1dba3b36661ec7968b22cf6d7835e41a81a313cc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2206199 Reviewed-by: Rouslan Solomakhin Reviewed-by: Sami Kyöstilä Reviewed-by: Guillaume Jenkins Commit-Queue: Rouslan Solomakhin Cr-Commit-Position: refs/heads/master@{#771037} --- AUTHORS | 1 + .../spellcheck_hunspell_dictionary.cc | 34 +++++++++++-------- .../spellcheck_hunspell_dictionary.h | 10 ++++-- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/AUTHORS b/AUTHORS index 75307b5679ecff..c3187a59bfebdc 100644 --- a/AUTHORS +++ b/AUTHORS @@ -184,6 +184,7 @@ Chanho Park Chansik Yun Chaobin Zhang Charles Vaughn +Cheng Zhao Choongwoo Han Chris Greene Chris Harrelson diff --git a/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc b/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc index c2c45f5bb2e764..ca546f2253b5b4 100644 --- a/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc +++ b/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.cc @@ -90,21 +90,28 @@ bool SaveDictionaryData(std::unique_ptr data, } // namespace -SpellcheckHunspellDictionary::DictionaryFile::DictionaryFile() { -} +SpellcheckHunspellDictionary::DictionaryFile::DictionaryFile( + base::TaskRunner* task_runner) : task_runner_(task_runner) {} SpellcheckHunspellDictionary::DictionaryFile::~DictionaryFile() { + if (file.IsValid()) { + task_runner_->PostTask(FROM_HERE, + base::BindOnce(&CloseDictionary, std::move(file))); + } } SpellcheckHunspellDictionary::DictionaryFile::DictionaryFile( DictionaryFile&& other) - : path(other.path), file(std::move(other.file)) {} + : path(other.path), + file(std::move(other.file)), + task_runner_(std::move(other.task_runner_)) {} SpellcheckHunspellDictionary::DictionaryFile& SpellcheckHunspellDictionary::DictionaryFile::operator=( DictionaryFile&& other) { path = other.path; file = std::move(other.file); + task_runner_ = std::move(other.task_runner_); return *this; } @@ -120,15 +127,10 @@ SpellcheckHunspellDictionary::SpellcheckHunspellDictionary( use_browser_spellchecker_(false), browser_context_(browser_context), spellcheck_service_(spellcheck_service), - download_status_(DOWNLOAD_NONE) {} + download_status_(DOWNLOAD_NONE), + dictionary_file_(task_runner_.get()) {} SpellcheckHunspellDictionary::~SpellcheckHunspellDictionary() { - if (dictionary_file_.file.IsValid()) { - task_runner_->PostTask( - FROM_HERE, - base::BindOnce(&CloseDictionary, std::move(dictionary_file_.file))); - } - #if BUILDFLAG(USE_BROWSER_SPELLCHECKER) // Disable the language from platform spellchecker. if (spellcheck::UseBrowserSpellChecker() && HasPlatformSupport()) { @@ -334,7 +336,8 @@ void SpellcheckHunspellDictionary::DownloadDictionary(GURL url) { #if !defined(OS_ANDROID) // static SpellcheckHunspellDictionary::DictionaryFile -SpellcheckHunspellDictionary::OpenDictionaryFile(const base::FilePath& path) { +SpellcheckHunspellDictionary::OpenDictionaryFile(base::TaskRunner* task_runner, + const base::FilePath& path) { base::ScopedBlockingCall scoped_blocking_call(FROM_HERE, base::BlockingType::MAY_BLOCK); @@ -345,7 +348,7 @@ SpellcheckHunspellDictionary::OpenDictionaryFile(const base::FilePath& path) { // For systemwide installations on Windows, the default directory may not // have permissions for download. In that case, the alternate directory for // download is chrome::DIR_USER_DATA. - DictionaryFile dictionary; + DictionaryFile dictionary(task_runner); #if defined(OS_WIN) // Check if the dictionary exists in the fallback location. If so, use it @@ -387,7 +390,7 @@ SpellcheckHunspellDictionary::OpenDictionaryFile(const base::FilePath& path) { // static SpellcheckHunspellDictionary::DictionaryFile SpellcheckHunspellDictionary::InitializeDictionaryLocation( - const std::string& language) { + base::TaskRunner* task_runner, const std::string& language) { base::ScopedBlockingCall scoped_blocking_call(FROM_HERE, base::BlockingType::MAY_BLOCK); @@ -402,7 +405,7 @@ SpellcheckHunspellDictionary::InitializeDictionaryLocation( base::FilePath dict_path = spellcheck::GetVersionedFileName(language, dict_dir); - return OpenDictionaryFile(dict_path); + return OpenDictionaryFile(task_runner, dict_path); } void SpellcheckHunspellDictionary::InitializeDictionaryLocationComplete( @@ -492,7 +495,8 @@ void SpellcheckHunspellDictionary::PlatformSupportsLanguageComplete( #if !defined(OS_ANDROID) && BUILDFLAG(USE_RENDERER_SPELLCHECKER) base::PostTaskAndReplyWithResult( task_runner_.get(), FROM_HERE, - base::BindOnce(&InitializeDictionaryLocation, language_), + base::BindOnce(&InitializeDictionaryLocation, + base::RetainedRef(task_runner_.get()), language_), base::BindOnce( &SpellcheckHunspellDictionary::InitializeDictionaryLocationComplete, weak_ptr_factory_.GetWeakPtr())); diff --git a/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.h b/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.h index 5c4c5cc17c0749..a8acd00dc44981 100644 --- a/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.h +++ b/chrome/browser/spellchecker/spellcheck_hunspell_dictionary.h @@ -100,7 +100,7 @@ class SpellcheckHunspellDictionary // blocking sequence. struct DictionaryFile { public: - DictionaryFile(); + explicit DictionaryFile(base::TaskRunner* task_runner); ~DictionaryFile(); DictionaryFile(DictionaryFile&& other); @@ -113,6 +113,9 @@ class SpellcheckHunspellDictionary base::File file; private: + // Task runner where the file is created. + scoped_refptr task_runner_; + DISALLOW_COPY_AND_ASSIGN(DictionaryFile); }; @@ -127,11 +130,12 @@ class SpellcheckHunspellDictionary #if !defined(OS_ANDROID) // Figures out the location for the dictionary, verifies its contents, and // opens it. - static DictionaryFile OpenDictionaryFile(const base::FilePath& path); + static DictionaryFile OpenDictionaryFile(base::TaskRunner* task_runner, + const base::FilePath& path); // Gets the default location for the dictionary file. static DictionaryFile InitializeDictionaryLocation( - const std::string& language); + base::TaskRunner* task_runner, const std::string& language); // The reply point for PostTaskAndReplyWithResult, called after the dictionary // file has been initialized.