Windows Spellcheck: Fix for delayed init on clicking editable content

This change fixes an issue with the delayed initialization of the
spellcheck service (a feature that in Chromium is currently behind the
WinDelaySpellcheckServiceInit feature flag). While the first click in
the editable content indeed initializes the spellcheck service by
loading dictionaries, it does not guarantee a successful spell check
request. Spelling may not be checked until further typing or focusing
in a different editable element. If the text is long enough
(multilined?), then a single click works.

The bug is caused by a race condition in the mojo calls from the
browser to the renderer process. There is no guarantee that the
callback to the renderer process when InitializeDictionaries completes
will be invoked in the renderer process after the call to
SpellCheck::Initialize that results from the browser method
SpellcheckService::InitForRenderer. If the first call wins the race,
no spellchecking will occur because the spellcheck provider thinks
there are no dictionaries available and does not check spelling.

Mojo does not guarantee ordering in separate message pipes, which
the SpellChecker and SpellCheckHost interfaces comprise. One fix that
I considered was to make these associated interfaces that share a
single channel between the browser and renderer processes. But I
believe this risks breaking other platforms. So the fix I have
here instead ensures that when the InitializeDictionaries callback is
received in the renderer, SpellCheck::Initialize is called with the
list of dictionaries provided by the browser process.

Test coverage is provided by a browser test based on the original test
that caught this bug (ChromeSitePerProcessTest.OOPIFSpellCheckTest):
ChromeSitePerProcessSpellCheckTestDelayInit.OOPIFSpellCheckTest. The
HTML source for this browser test contains just a snippet of editable
content, so putting focus in it to invoke spellchecking and initialize
the spellcheck service on demand exposed the issue.

Bug: 1103847
Change-Id: I809e4aef3d51808d222012ddd0c0b46e3d57098a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2335028Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Guillaume Jenkins <gujen@google.com>
Commit-Queue: Bruce Long <brlong@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#796789}

chrome/browser/spellchecker/spell_check_host_chrome_impl.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/spellchecker/spellcheck_service.h"
 #include "components/spellcheck/browser/spellcheck_host_metrics.h"
 #include "components/spellcheck/browser/spellcheck_platform.h"
+#include "components/spellcheck/common/spellcheck_features.h"
 #include "components/spellcheck/spellcheck_buildflags.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -208,24 +209,63 @@ void SpellCheckHostChromeImpl::GetPerLanguageSuggestions(
   spellcheck_platform::GetPerLanguageSuggestions(
       spellcheck->platform_spell_checker(), word, std::move(callback));
 }
-#endif  // defined(OS_WIN)
 
 void SpellCheckHostChromeImpl::InitializeDictionaries(
     InitializeDictionariesCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Initialize the spellcheck service if needed. Initialization must
-  // happen on UI thread.
-  SpellcheckService* spellcheck = GetSpellcheckService();
+  if (base::FeatureList::IsEnabled(
+          spellcheck::kWinDelaySpellcheckServiceInit)) {
+    // Initialize the spellcheck service if needed. Initialization must
+    // happen on UI thread.
+    SpellcheckService* spellcheck = GetSpellcheckService();
 
-  if (!spellcheck) {  // Teardown.
-    std::move(callback).Run();
+    if (!spellcheck) {  // Teardown.
+      std::move(callback).Run(/*dictionaries=*/{}, /*custom_words=*/{},
+                              /*enable=*/false);
+      return;
+    }
+
+    dictionaries_loaded_callback_ = std::move(callback);
+
+    spellcheck->InitializeDictionaries(
+        base::BindOnce(&SpellCheckHostChromeImpl::OnDictionariesInitialized,
+                       weak_factory_.GetWeakPtr()));
     return;
   }
 
-  spellcheck->InitializeDictionaries(std::move(callback));
+  NOTREACHED();
+  std::move(callback).Run(/*dictionaries=*/{}, /*custom_words=*/{},
+                          /*enable=*/false);
 }
 
+void SpellCheckHostChromeImpl::OnDictionariesInitialized() {
+  DCHECK(dictionaries_loaded_callback_);
+  SpellcheckService* spellcheck = GetSpellcheckService();
+
+  const bool enable = spellcheck->IsSpellcheckEnabled();
+
+  std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries;
+  std::vector<std::string> custom_words;
+  if (enable) {
+    for (const auto& hunspell_dictionary :
+         spellcheck->GetHunspellDictionaries()) {
+      dictionaries.push_back(spellcheck::mojom::SpellCheckBDictLanguage::New(
+          hunspell_dictionary->GetDictionaryFile().Duplicate(),
+          hunspell_dictionary->GetLanguage()));
+    }
+
+    SpellcheckCustomDictionary* custom_dictionary =
+        spellcheck->GetCustomDictionary();
+    custom_words.assign(custom_dictionary->GetWords().begin(),
+                        custom_dictionary->GetWords().end());
+  }
+
+  std::move(dictionaries_loaded_callback_)
+      .Run(std::move(dictionaries), custom_words, enable);
+}
+#endif  // defined(OS_WIN)
+
 void SpellCheckHostChromeImpl::OnRequestFinished(SpellingRequest* request) {
   auto iterator = requests_.find(request);
   requests_.erase(iterator);

chrome/browser/spellchecker/spell_check_host_chrome_impl.h

@@ -83,9 +83,9 @@ class SpellCheckHostChromeImpl : public SpellCheckHostImpl {
   void GetPerLanguageSuggestions(
       const base::string16& word,
       GetPerLanguageSuggestionsCallback callback) override;
-#endif  // defined(OS_WIN)
 
   void InitializeDictionaries(InitializeDictionariesCallback callback) override;
+#endif  // defined(OS_WIN)
 
   // Clears a finished request from |requests_|. Exposed to SpellingRequest.
   void OnRequestFinished(SpellingRequest* request);
@@ -95,6 +95,14 @@ class SpellCheckHostChromeImpl : public SpellCheckHostImpl {
       std::vector<SpellCheckResult>* remote_results,
       const std::vector<SpellCheckResult>& local_results);
 
+#if defined(OS_WIN)
+  void OnDictionariesInitialized();
+
+  // Callback passed as argument to InitializeDictionaries, and invoked when
+  // the dictionaries are loaded for the first time.
+  InitializeDictionariesCallback dictionaries_loaded_callback_;
+#endif  // defined(OS_WIN)
+
   // All pending requests.
   std::set<std::unique_ptr<SpellingRequest>, base::UniquePtrComparator>
       requests_;

chrome/browser/spellchecker/spell_check_host_chrome_impl_win_browsertest.cc

@@ -188,7 +188,10 @@ class SpellCheckHostChromeImplWinBrowserTestDelayInit
     RunUntilResultReceived();
   }
 
-  void InitializeDictionariesCallback() {
+  void InitializeDictionariesCallback(
+      std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries,
+      const std::vector<std::string>& custom_words,
+      bool enable) {
     received_result_ = true;
     if (quit_)
       std::move(quit_).Run();

chrome/browser/spellchecker/spellcheck_service.cc

@@ -378,33 +378,17 @@ void SpellcheckService::InitForRenderer(content::RenderProcessHost* host) {
   if (SpellcheckServiceFactory::GetForContext(context) != this)
     return;
 
-  const PrefService* prefs = user_prefs::UserPrefs::Get(context);
-  std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries;
-
-  bool enable_if_uninitialized = false;
-#if defined(OS_WIN)
-  if (spellcheck::UseBrowserSpellChecker() &&
-      base::FeatureList::IsEnabled(
-          spellcheck::kWinDelaySpellcheckServiceInit)) {
-    // If initialization of the spellcheck service is on-demand, the
-    // renderer-side SpellCheck object needs to start out as enabled in order
-    // for a click on editable content to initialize the spellcheck service.
-    if (!dictionaries_loaded())
-      enable_if_uninitialized = true;
-  }
-#endif  // defined(OS_WIN)
-
-  for (const auto& hunspell_dictionary : hunspell_dictionaries_) {
-    dictionaries.push_back(spellcheck::mojom::SpellCheckBDictLanguage::New(
-        hunspell_dictionary->GetDictionaryFile().Duplicate(),
-        hunspell_dictionary->GetLanguage()));
-  }
-
-  bool enable = prefs->GetBoolean(spellcheck::prefs::kSpellCheckEnable) &&
-                (!dictionaries.empty() || enable_if_uninitialized);
+  const bool enable = IsSpellcheckEnabled();
 
+  std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries;
   std::vector<std::string> custom_words;
   if (enable) {
+    for (const auto& hunspell_dictionary : hunspell_dictionaries_) {
+      dictionaries.push_back(spellcheck::mojom::SpellCheckBDictLanguage::New(
+          hunspell_dictionary->GetDictionaryFile().Duplicate(),
+          hunspell_dictionary->GetLanguage()));
+    }
+
     custom_words.assign(custom_dictionary_->GetWords().begin(),
                         custom_dictionary_->GetWords().end());
   }
@@ -503,6 +487,26 @@ SpellcheckService::GetHunspellDictionaries() {
   return hunspell_dictionaries_;
 }
 
+bool SpellcheckService::IsSpellcheckEnabled() const {
+  const PrefService* prefs = user_prefs::UserPrefs::Get(context_);
+
+  bool enable_if_uninitialized = false;
+#if defined(OS_WIN)
+  if (spellcheck::UseBrowserSpellChecker() &&
+      base::FeatureList::IsEnabled(
+          spellcheck::kWinDelaySpellcheckServiceInit)) {
+    // If initialization of the spellcheck service is on-demand, the
+    // renderer-side SpellCheck object needs to start out as enabled in order
+    // for a click on editable content to initialize the spellcheck service.
+    if (!dictionaries_loaded())
+      enable_if_uninitialized = true;
+  }
+#endif  // defined(OS_WIN)
+
+  return prefs->GetBoolean(spellcheck::prefs::kSpellCheckEnable) &&
+         (!hunspell_dictionaries_.empty() || enable_if_uninitialized);
+}
+
 bool SpellcheckService::LoadExternalDictionary(std::string language,
                                                std::string locale,
                                                std::string path,

chrome/browser/spellchecker/spellcheck_service.h

@@ -137,6 +137,10 @@ class SpellcheckService : public KeyedService,
   const std::vector<std::unique_ptr<SpellcheckHunspellDictionary>>&
   GetHunspellDictionaries();
 
+  // Returns whether spellchecking is enabled in preferences and if there are
+  // dictionaries available.
+  bool IsSpellcheckEnabled() const;
+
   // Load a dictionary from a given path. Format specifies how the dictionary
   // is stored. Return value is true if successful.
   bool LoadExternalDictionary(std::string language,

components/spellcheck/browser/spell_check_host_impl.cc

@@ -77,15 +77,15 @@ void SpellCheckHostImpl::GetPerLanguageSuggestions(
   // This API requires Chrome-only features.
   std::move(callback).Run(std::vector<std::vector<base::string16>>());
 }
-#endif  // defined(OS_WIN)
 
 void SpellCheckHostImpl::InitializeDictionaries(
     InitializeDictionariesCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   NOTREACHED();
-  std::move(callback).Run();
+  std::move(callback).Run(/*dictionaries=*/{}, /*custom_words=*/{},
+                          /*enable=*/false);
 }
-
+#endif  // defined(OS_WIN)
 #endif  //  BUILDFLAG(USE_BROWSER_SPELLCHECKER) &&
         //  !BUILDFLAG(ENABLE_SPELLING_SERVICE)
 

components/spellcheck/browser/spell_check_host_impl.h

@@ -55,10 +55,9 @@ class SpellCheckHostImpl : public spellcheck::mojom::SpellCheckHost {
   void GetPerLanguageSuggestions(
       const base::string16& word,
       GetPerLanguageSuggestionsCallback callback) override;
-#endif  // defined(OS_WIN)
 
   void InitializeDictionaries(InitializeDictionariesCallback callback) override;
-
+#endif  // defined(OS_WIN)
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER) &&
         // !BUILDFLAG(ENABLE_SPELLING_SERVICE)
 

components/spellcheck/common/spellcheck.mojom

@@ -82,8 +82,11 @@ interface SpellCheckHost {
       (array<array<mojo_base.mojom.String16>> suggestions);
 
   // Completes initialization of the spellcheck service by loading dictionaries.
-  [EnableIf=USE_BROWSER_SPELLCHECKER]
-  InitializeDictionaries() => ();
+  [EnableIf=is_win]
+  InitializeDictionaries() =>
+      (array<SpellCheckBDictLanguage> dictionaries,
+       array<string> custom_words,
+       bool enable);
 };
 
 enum Decoration {

components/spellcheck/renderer/spellcheck.h

@@ -172,6 +172,15 @@ class SpellCheck : public base::SupportsWeakPtr<SpellCheck>,
   // Overridden by tests in spellcheck_provider_test.cc (FakeSpellCheck class).
   virtual size_t EnabledLanguageCount();
 
+  // spellcheck::mojom::SpellChecker:
+  // Initialize the SpellCheck object with data provided by the browser process.
+  // Method is public since called directly in
+  // SpellCheckProvider::OnRespondInitializeDictionaries.
+  void Initialize(
+      std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries,
+      const std::vector<std::string>& custom_words,
+      bool enable) override;
+
  private:
    friend class SpellCheckTest;
    friend class FakeSpellCheck;
@@ -180,10 +189,6 @@ class SpellCheck : public base::SupportsWeakPtr<SpellCheck>,
        RequestSpellCheckMultipleTimesWithoutInitialization);
 
    // spellcheck::mojom::SpellChecker:
-   void Initialize(
-       std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries,
-       const std::vector<std::string>& custom_words,
-       bool enable) override;
    void CustomDictionaryChanged(
        const std::vector<std::string>& words_added,
        const std::vector<std::string>& words_removed) override;

components/spellcheck/renderer/spellcheck_provider.cc

@@ -143,8 +143,15 @@ void SpellCheckProvider::RequestTextChecking(
       // Initialize the spellcheck service on demand (this spellcheck request
       // could be the result of the first click in editable content), then
       // complete the text check request when the dictionaries are loaded.
+      // The delayed spell check service initialization sequence, starting from
+      // when the user clicks in editable content, is as follows:
+      // - SpellcheckProvider::RequestTextChecking (Renderer, this method)
+      // - SpellCheckHostChromeImpl::InitializeDictionaries (Browser)
+      // - SpellcheckService::InitializeDictionaries (Browser)
+      // - SpellCheckHostChromeImpl::OnDictionariesInitialized (Browser)
+      // - SpellcheckProvider::OnRespondInitializeDictionaries (Renderer)
       GetSpellCheckHost().InitializeDictionaries(
-          base::BindOnce(&SpellCheckProvider::RequestTextCheckingFromBrowser,
+          base::BindOnce(&SpellCheckProvider::OnRespondInitializeDictionaries,
                          weak_factory_.GetWeakPtr(), text));
       return;
     }
@@ -169,7 +176,6 @@ void SpellCheckProvider::RequestTextCheckingFromBrowser(
     const base::string16& text) {
   DCHECK(spellcheck::UseBrowserSpellChecker());
 #if defined(OS_WIN)
-  dictionaries_loaded_ = true;
 
   // Determine whether a hybrid check is needed.
   bool use_hunspell = spellcheck_->EnabledLanguageCount() > 0;
@@ -209,6 +215,27 @@ void SpellCheckProvider::RequestTextCheckingFromBrowser(
       base::BindOnce(&SpellCheckProvider::OnRespondTextCheck,
                      weak_factory_.GetWeakPtr(), last_identifier_, text));
 }
+
+#if defined(OS_WIN)
+void SpellCheckProvider::OnRespondInitializeDictionaries(
+    const base::string16& text,
+    std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries,
+    const std::vector<std::string>& custom_words,
+    bool enable) {
+  DCHECK(!dictionaries_loaded_);
+  dictionaries_loaded_ = true;
+
+  // Because the SpellChecker and SpellCheckHost mojo interfaces use different
+  // channels, there is no guarantee that the SpellChecker::Initialize response
+  // will be received before the SpellCheckHost::InitializeDictionaries
+  // callback. If the order is reversed, no spellcheck will be performed since
+  // the renderer side thinks there are no dictionaries available. Ensure that
+  // the SpellChecker is initialized before performing a spellcheck.
+  spellcheck_->Initialize(std::move(dictionaries), custom_words, enable);
+
+  RequestTextCheckingFromBrowser(text);
+}
+#endif  // defined(OS_WIN)
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
 void SpellCheckProvider::FocusedElementChanged(

components/spellcheck/renderer/spellcheck_provider.h

@@ -137,6 +137,13 @@ class SpellCheckProvider : public content::RenderFrameObserver,
   void RequestTextCheckingFromBrowser(const base::string16& text);
 
 #if defined(OS_WIN)
+  // Callback for when spellcheck service has been initialized on demand.
+  void OnRespondInitializeDictionaries(
+      const base::string16& text,
+      std::vector<spellcheck::mojom::SpellCheckBDictLanguagePtr> dictionaries,
+      const std::vector<std::string>& custom_words,
+      bool enable);
+
   // Flag indicating that the spellcheck service has been initialized and
   // the dictionaries have been loaded initially. Used to avoid an unnecessary
   // mojo call to determine this in every text check request.

components/spellcheck/renderer/spellcheck_provider_test.cc

@@ -191,20 +191,19 @@ void TestingSpellCheckProvider::GetPerLanguageSuggestions(
     GetPerLanguageSuggestionsCallback callback) {
   NOTREACHED();
 }
-#endif  // defined(OS_WIN)
 
 void TestingSpellCheckProvider::InitializeDictionaries(
     InitializeDictionariesCallback callback) {
-#if defined(OS_WIN)
   if (base::FeatureList::IsEnabled(
           spellcheck::kWinDelaySpellcheckServiceInit)) {
-    std::move(callback).Run();
+    std::move(callback).Run(/*dictionaries=*/{}, /*custom_words=*/{},
+                            /*enable=*/false);
     return;
   }
-#endif  // defined(OS_WIN)
 
   NOTREACHED();
 }
+#endif  // defined(OS_WIN)
 #endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
 #if defined(OS_ANDROID)

components/spellcheck/renderer/spellcheck_provider_test.h

@@ -149,11 +149,9 @@ class TestingSpellCheckProvider : public SpellCheckProvider,
   void GetPerLanguageSuggestions(
       const base::string16& word,
       GetPerLanguageSuggestionsCallback callback) override;
-#endif  // defined(OS_WIN)
-
   void InitializeDictionaries(InitializeDictionariesCallback callback) override;
-
-#endif
+#endif  // defined(OS_WIN)
+#endif  // BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 
 #if defined(OS_ANDROID)
   void DisconnectSessionBridge() override;