SPM: Fix VK crash on leaving tablet mode

The crash is caused by a race of WS and KeyboardControllerObserver
mojo interface. When we close keyboard hosting window, we send
OnKeyboardOccludedBoundsChanged and OnKeyboardUIDestroyed via
KeyboardControllerObserver then close the hosting window. But
the browser process could receive the hosting window close via
WS first hence the crash.

The CL wires the unembed code path that happens on hosting window
close with ChromeKeyboardControllerClient::OnKeyboardUIDestroyed.
After OnKeyboardUIDestroyed, the calls on KeyboardControllerObserver
interface will be no-op and the crash would not happen.

Bug: 931910
Change-Id: I2b777a56dca3c26e1f734b732d488165936a291a
Reviewed-on: https://chromium-review.googlesource.com/c/1482948
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634758}

chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.cc

@@ -4,7 +4,7 @@
 
 #include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h"
 
-#include <memory>
+#include <utility>
 
 #include "ash/public/interfaces/constants.mojom.h"
 #include "base/bind.h"
@@ -379,8 +379,13 @@ void ChromeKeyboardControllerClient::OnLoadKeyboardContentsRequested() {
   DVLOG(1) << "OnLoadKeyboardContentsRequested: Create: " << keyboard_url;
   keyboard_contents_ = std::make_unique<ChromeKeyboardWebContents>(
       GetProfile(), keyboard_url,
+      /*load_callback=*/
       base::BindOnce(&ChromeKeyboardControllerClient::OnKeyboardContentsLoaded,
-                     weak_ptr_factory_.GetWeakPtr()));
+                     weak_ptr_factory_.GetWeakPtr()),
+      /*unembed_callback=*/
+      base::BindRepeating(
+          &ChromeKeyboardControllerClient::OnKeyboardUIDestroyed,
+          weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ChromeKeyboardControllerClient::OnKeyboardUIDestroyed() {

chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h

@@ -6,6 +6,8 @@
 #define CHROME_BROWSER_UI_ASH_KEYBOARD_CHROME_KEYBOARD_CONTROLLER_CLIENT_H_
 
 #include <memory>
+#include <set>
+#include <vector>
 
 #include "ash/public/interfaces/keyboard_controller.mojom.h"
 #include "base/callback_forward.h"
@@ -103,7 +105,7 @@ class ChromeKeyboardControllerClient
   // Returns whether |flag| has been set.
   bool IsEnableFlagSet(const keyboard::mojom::KeyboardEnableFlag& flag);
 
-  // Calls forwarded to ash.mojom.KeyboardController..
+  // Calls forwarded to ash.mojom.KeyboardController.
   void ReloadKeyboardIfNeeded();
   void RebuildKeyboardIfEnabled();
   void ShowKeyboard();

chrome/browser/ui/ash/keyboard/chrome_keyboard_ui.cc

@@ -9,6 +9,7 @@
 
 #include "ash/shell.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/no_destructor.h"
 #include "chrome/browser/ui/ash/keyboard/chrome_keyboard_bounds_observer.h"
@@ -55,7 +56,8 @@ aura::Window* ChromeKeyboardUI::LoadKeyboardWindow(LoadCallback callback) {
             ChromeKeyboardControllerClient::Get()->NotifyKeyboardLoaded();
             std::move(callback).Run();
           },
-          std::move(callback)));
+          std::move(callback)),
+      base::NullCallback());
 
   aura::Window* keyboard_window =
       keyboard_contents_->web_contents()->GetNativeView();

chrome/browser/ui/ash/keyboard/chrome_keyboard_web_contents.cc

@@ -4,7 +4,11 @@
 
 #include "chrome/browser/ui/ash/keyboard/chrome_keyboard_web_contents.h"
 
+#include <string>
+#include <utility>
+
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/feature_list.h"
 #include "chrome/browser/extensions/chrome_extension_web_contents_observer.h"
 #include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
@@ -133,8 +137,10 @@ class ChromeKeyboardContentsDelegate : public content::WebContentsDelegate,
 ChromeKeyboardWebContents::ChromeKeyboardWebContents(
     content::BrowserContext* context,
     const GURL& url,
-    LoadCallback load_callback)
-    : load_callback_(std::move(load_callback)) {
+    LoadCallback load_callback,
+    UnembedCallback unembed_callback)
+    : load_callback_(std::move(load_callback)),
+      unembed_callback_(std::move(unembed_callback)) {
   VLOG(1) << "ChromeKeyboardWebContents: " << url;
   DCHECK(context);
   content::WebContents::CreateParams web_contents_params(
@@ -232,6 +238,7 @@ void ChromeKeyboardWebContents::DidStopLoading() {
     return;
   remote_view_provider_ = std::make_unique<views::RemoteViewProvider>(
       web_contents_->GetNativeView());
+  remote_view_provider_->SetCallbacks(base::NullCallback(), unembed_callback_);
   remote_view_provider_->GetEmbedToken(
       base::BindOnce(&ChromeKeyboardWebContents::OnGotEmbedToken,
                      weak_ptr_factory_.GetWeakPtr()));

chrome/browser/ui/ash/keyboard/chrome_keyboard_web_contents.h

@@ -44,12 +44,17 @@ class ChromeKeyboardWebContents : public content::WebContentsObserver,
  public:
   using LoadCallback = base::OnceCallback<void(const base::UnguessableToken&,
                                                const gfx::Size& size)>;
+  using UnembedCallback = base::RepeatingClosure;
 
-  // Immediately starts loading |url| in a WebContents. |callback| is called
-  // when the WebContents finishes loading.
+  // Immediately starts loading |url| in a WebContents. |load_callback| is
+  // called when the WebContents finishes loading. |unembed_callback| is only
+  // used when the content is embedded using Window Service and is called when
+  // it gets unembedded (e.g. the hosting window is closed). Note that
+  // |unembed_callback| might end up deleting this.
   ChromeKeyboardWebContents(content::BrowserContext* context,
                             const GURL& url,
-                            LoadCallback load_callback);
+                            LoadCallback load_callback,
+                            UnembedCallback unembed_callback);
   ~ChromeKeyboardWebContents() override;
 
   // Updates the keyboard URL if |url| does not match the existing url.
@@ -88,10 +93,13 @@ class ChromeKeyboardWebContents : public content::WebContentsObserver,
   std::unique_ptr<content::WebContents> web_contents_;
   std::unique_ptr<ChromeKeyboardBoundsObserver> window_bounds_observer_;
 
-  // Called from DidStopLoading(). If the WindowService is running, passes a
+  // Called from DidStopLoading(). If the Window Service is running, passes a
   // token for embedding the contents, otherwise passes an empty token.
   LoadCallback load_callback_;
 
+  // Called when content is unembedded from Window Service.
+  UnembedCallback unembed_callback_;
+
   base::UnguessableToken token_;
   gfx::Size contents_size_;
 

chrome/browser/ui/ash/keyboard/chrome_keyboard_web_contents_unittest.cc

@@ -5,7 +5,9 @@
 #include "chrome/browser/ui/ash/keyboard/chrome_keyboard_web_contents.h"
 
 #include <memory>
+#include <utility>
 
+#include "base/bind_helpers.h"
 #include "base/run_loop.h"
 #include "chrome/browser/ui/ash/keyboard/chrome_keyboard_controller_client.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
@@ -38,7 +40,7 @@ class ChromeKeyboardWebContentsTest : public ChromeRenderViewHostTestHarness {
   void CreateWebContents(const GURL& url,
                          ChromeKeyboardWebContents::LoadCallback callback) {
     chrome_keyboard_web_contents_ = std::make_unique<ChromeKeyboardWebContents>(
-        profile(), url, std::move(callback));
+        profile(), url, std::move(callback), base::NullCallback());
   }
 
  protected: