Fix memory management of spellcheck callbacks on Mac

Mojo doesn't allow an unrun callback to be removed before the
message pipe is closed. However, Mac spellchecker manages
RequestTextCheckCallbacks as (indirectly) owned by a completion
barrier in SpellingRequest, which lives independently with the
message pipe. Hence, in some shutdown orders, the callback can
be cleared while the message pipe is still open.

This patch fixes the issue by making SpellCheckHost own
all pending SpellingRequests, and SpellingRequest own
RequestTextCheckCallback.

Note: this patch serves as a quick fix to the regressed test
cases. A follow-up patch will clean up the code.

Bug: 814845
Change-Id: I22a08cdce2744b50fe23c0ff48c10d5207fa5f8b
Reviewed-on: https://chromium-review.googlesource.com/946848
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541624}

chrome/browser/spellchecker/spell_check_host_chrome_impl.cc

@@ -16,11 +16,15 @@
 #include "content/public/browser/browser_thread.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 
+#if !defined(OS_MACOSX)
+// Mac needs different constructor and destructor for Mac-only members.
+
 SpellCheckHostChromeImpl::SpellCheckHostChromeImpl(
     const service_manager::Identity& renderer_identity)
     : renderer_identity_(renderer_identity), weak_factory_(this) {}
 
 SpellCheckHostChromeImpl::~SpellCheckHostChromeImpl() = default;
+#endif
 
 // static
 void SpellCheckHostChromeImpl::Create(

chrome/browser/spellchecker/spell_check_host_chrome_impl.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_SPELLCHECKER_SPELL_CHECK_HOST_CHROME_IMPL_H_
 #define CHROME_BROWSER_SPELLCHECKER_SPELL_CHECK_HOST_CHROME_IMPL_H_
 
+#include "base/containers/unique_ptr_adapters.h"
 #include "build/build_config.h"
 #include "components/spellcheck/browser/spell_check_host_impl.h"
 #include "components/spellcheck/browser/spelling_service_client.h"
@@ -12,6 +13,7 @@
 
 class SpellcheckCustomDictionary;
 class SpellcheckService;
+class SpellingRequest;
 
 struct SpellCheckResult;
 
@@ -65,6 +67,9 @@ class SpellCheckHostChromeImpl : public SpellCheckHostImpl {
                         int route_id,
                         RequestTextCheckCallback callback) override;
 
+  // Clears a finished request from |requests_|. Exposed to SpellingRequest.
+  void OnRequestFinished(SpellingRequest* request);
+
   // Exposed to tests only.
   static void CombineResultsForTesting(
       std::vector<SpellCheckResult>* remote_results,
@@ -73,6 +78,10 @@ class SpellCheckHostChromeImpl : public SpellCheckHostImpl {
   int ToDocumentTag(int route_id);
   void RetireDocumentTag(int route_id);
   std::map<int, int> tag_map_;
+
+  // All pending requests.
+  std::set<std::unique_ptr<SpellingRequest>, base::UniquePtrComparator>
+      requests_;
 #endif  // defined(OS_MACOSX)
 
   // Returns the SpellcheckService of our |render_process_id_|. The return

chrome/browser/spellchecker/spell_check_host_chrome_impl_mac.cc

@@ -52,16 +52,19 @@ void CombineResults(std::vector<SpellCheckResult>* remote_results,
 
 }  // namespace
 
+// SpellingRequest is owned by SpellCheckHostChromeImpl.
 class SpellingRequest {
  public:
   using RequestTextCheckCallback =
       spellcheck::mojom::SpellCheckHost::RequestTextCheckCallback;
+  using DestructionCallback = base::OnceCallback<void(SpellingRequest*)>;
 
   SpellingRequest(SpellingServiceClient* client,
                   const base::string16& text,
                   const service_manager::Identity& renderer_identity,
                   int document_tag,
-                  RequestTextCheckCallback callback);
+                  RequestTextCheckCallback callback,
+                  DestructionCallback destruction_callback);
 
  private:
   // Request server-side checking for |text_|.
@@ -73,14 +76,19 @@ class SpellingRequest {
   // Check if all pending requests are done, send reply to render process if so.
   void OnCheckCompleted();
 
-  // Called when server-side checking is complete.
+  // Called when server-side checking is complete. Must be called on UI thread.
   void OnRemoteCheckCompleted(bool success,
                               const base::string16& text,
                               const std::vector<SpellCheckResult>& results);
 
-  // Called when local checking is complete.
+  // Called when local checking is complete. Must be called on UI thread.
   void OnLocalCheckCompleted(const std::vector<SpellCheckResult>& results);
 
+  // Forwards the results back to UI thread when local checking completes.
+  static void OnLocalCheckCompletedOnAnyThread(
+      base::WeakPtr<SpellingRequest> request,
+      const std::vector<SpellCheckResult>& results);
+
   std::vector<SpellCheckResult> local_results_;
   std::vector<SpellCheckResult> remote_results_;
 
@@ -92,6 +100,10 @@ class SpellingRequest {
   const service_manager::Identity renderer_identity_;
   int document_tag_;
   RequestTextCheckCallback callback_;
+
+  DestructionCallback destruction_callback_;
+
+  base::WeakPtrFactory<SpellingRequest> weak_factory_;
 };
 
 SpellingRequest::SpellingRequest(
@@ -99,19 +111,21 @@ SpellingRequest::SpellingRequest(
     const base::string16& text,
     const service_manager::Identity& renderer_identity,
     int document_tag,
-    RequestTextCheckCallback callback)
+    RequestTextCheckCallback callback,
+    DestructionCallback destruction_callback)
     : remote_success_(false),
       text_(text),
       renderer_identity_(renderer_identity),
       document_tag_(document_tag),
-      callback_(std::move(callback)) {
+      callback_(std::move(callback)),
+      destruction_callback_(std::move(destruction_callback)),
+      weak_factory_(this) {
   DCHECK(!text_.empty());
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Send the remote query out. The barrier owns |this|, ensuring it is deleted
-  // after completion.
-  completion_barrier_ = BarrierClosure(
-      2, base::BindOnce(&SpellingRequest::OnCheckCompleted, base::Owned(this)));
+  completion_barrier_ =
+      BarrierClosure(2, base::BindOnce(&SpellingRequest::OnCheckCompleted,
+                                       weak_factory_.GetWeakPtr()));
   RequestRemoteCheck(client);
   RequestLocalCheck();
 }
@@ -123,21 +137,24 @@ void SpellingRequest::RequestRemoteCheck(SpellingServiceClient* client) {
   if (host)
     context = host->GetBrowserContext();
 
+  // |this| may be gone at callback invocation if the owner has been removed.
   client->RequestTextCheck(
       context, SpellingServiceClient::SPELLCHECK, text_,
       base::BindOnce(&SpellingRequest::OnRemoteCheckCompleted,
-                     base::Unretained(this)));
+                     weak_factory_.GetWeakPtr()));
 }
 
 void SpellingRequest::RequestLocalCheck() {
+  // |this| may be gone at callback invocation if the owner has been removed.
   spellcheck_platform::RequestTextCheck(
       document_tag_, text_,
-      base::BindOnce(&SpellingRequest::OnLocalCheckCompleted,
-                     base::Unretained(this)));
+      base::BindOnce(&SpellingRequest::OnLocalCheckCompletedOnAnyThread,
+                     weak_factory_.GetWeakPtr()));
 }
 
 void SpellingRequest::OnCheckCompleted() {
-  // Final completion can happen on any thread - don't DCHECK thread.
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   std::vector<SpellCheckResult>* check_results = &local_results_;
   if (remote_success_) {
     std::sort(remote_results_.begin(), remote_results_.end(), CompareLocation);
@@ -146,13 +163,11 @@ void SpellingRequest::OnCheckCompleted() {
     check_results = &remote_results_;
   }
 
-  // |callback_| must be run on UI thread.
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(std::move(callback_), std::move(*check_results)));
+  std::move(callback_).Run(*check_results);
+
+  std::move(destruction_callback_).Run(this);
 
-  // Object is self-managed - at this point, its life span is over.
-  // No need to delete, since the OnCheckCompleted callback owns |this|.
+  // |destruction_callback_| removes |this|. No more operations allowed.
 }
 
 void SpellingRequest::OnRemoteCheckCompleted(
@@ -165,13 +180,30 @@ void SpellingRequest::OnRemoteCheckCompleted(
   completion_barrier_.Run();
 }
 
-void SpellingRequest::OnLocalCheckCompleted(
+// static
+void SpellingRequest::OnLocalCheckCompletedOnAnyThread(
+    base::WeakPtr<SpellingRequest> request,
     const std::vector<SpellCheckResult>& results) {
   // Local checking can happen on any thread - don't DCHECK thread.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&SpellingRequest::OnLocalCheckCompleted, request,
+                     results));
+}
+
+void SpellingRequest::OnLocalCheckCompleted(
+    const std::vector<SpellCheckResult>& results) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   local_results_ = results;
   completion_barrier_.Run();
 }
 
+SpellCheckHostChromeImpl::SpellCheckHostChromeImpl(
+    const service_manager::Identity& renderer_identity)
+    : renderer_identity_(renderer_identity), weak_factory_(this) {}
+
+SpellCheckHostChromeImpl::~SpellCheckHostChromeImpl() = default;
+
 // static
 void SpellCheckHostChromeImpl::CombineResultsForTesting(
     std::vector<SpellCheckResult>* remote_results,
@@ -208,9 +240,18 @@ void SpellCheckHostChromeImpl::RequestTextCheck(
   // happen on UI thread.
   GetSpellcheckService();
 
-  // SpellingRequest self-destructs.
-  new SpellingRequest(&client_, text, renderer_identity_,
-                      ToDocumentTag(route_id), std::move(callback));
+  // |SpellingRequest| self-destructs on completion.
+  // OK to store unretained |this| in a |SpellingRequest| owned by |this|.
+  requests_.insert(std::make_unique<SpellingRequest>(
+      &client_, text, renderer_identity_, ToDocumentTag(route_id),
+      std::move(callback),
+      base::BindOnce(&SpellCheckHostChromeImpl::OnRequestFinished,
+                     base::Unretained(this))));
+}
+
+void SpellCheckHostChromeImpl::OnRequestFinished(SpellingRequest* request) {
+  auto iterator = requests_.find(request);
+  requests_.erase(iterator);
 }
 
 int SpellCheckHostChromeImpl::ToDocumentTag(int route_id) {