Handle compose CloseUI with close button

Before this CL, the CloseUI() call would be called on the ComposeSession that needs to be deleted.

This CL creates a new mojo interface that is implemented by ComposeClient, so that it can handle methods that close the session,
from the object that owns the session (and not from within the session).

Bug: b/303919475
Change-Id: If2253d4062017865449a9c45963c09970efc4d59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4950480
Commit-Queue: edchin <edchin@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212579}

chrome/browser/compose/chrome_compose_client.cc

@@ -43,7 +43,8 @@ const char kComposeURL[] = "chrome://compose/";
 
 ChromeComposeClient::ChromeComposeClient(content::WebContents* web_contents)
     : content::WebContentsUserData<ChromeComposeClient>(*web_contents),
-      manager_(this) {
+      manager_(this),
+      close_page_receiver_(this) {
   profile_ = Profile::FromBrowserContext(GetWebContents().GetBrowserContext());
   opt_guide_ = OptimizationGuideKeyedServiceFactory::GetForProfile(profile_);
 
@@ -62,8 +63,13 @@ ChromeComposeClient::ChromeComposeClient(content::WebContents* web_contents)
 ChromeComposeClient::~ChromeComposeClient() = default;
 
 void ChromeComposeClient::BindComposeDialog(
+    mojo::PendingReceiver<compose::mojom::ComposeDialogClosePageHandler>
+        close_handler,
     mojo::PendingReceiver<compose::mojom::ComposeDialogPageHandler> handler,
     mojo::PendingRemote<compose::mojom::ComposeDialog> dialog) {
+  close_page_receiver_.reset();
+  close_page_receiver_.Bind(std::move(close_handler));
+
   url::Origin origin =
       GetWebContents().GetPrimaryMainFrame()->GetLastCommittedOrigin();
   if (origin == url::Origin::Create(GURL(kComposeURL))) {
@@ -72,8 +78,7 @@ void ChromeComposeClient::BindComposeDialog(
     debug_session_->Bind(std::move(handler), std::move(dialog));
     return;
   }
-
-  sessions_.at(last_compose_field_id_)
+  sessions_.at(last_compose_field_id_.value())
       ->Bind(std::move(handler), std::move(dialog));
 }
 
@@ -96,24 +101,45 @@ void ChromeComposeClient::ShowComposeDialog(
   }
 }
 
+void ChromeComposeClient::CloseUI(compose::mojom::CloseReason reason) {
+  switch (reason) {
+    case compose::mojom::CloseReason::kCloseButton:
+      RemoveActiveSession();
+      break;
+  }
+  // TODO(b/302748101) Call CloseDialog() on ComposeDialogController.
+}
+
 void ChromeComposeClient::SaveFieldAndCreateComposeStateIfEmpty(
     const autofill::FieldGlobalId& field_id,
     ComposeCallback callback) {
-  last_compose_field_id_ = field_id;
-  auto it = sessions_.find(last_compose_field_id_);
+  last_compose_field_id_ =
+      std::make_optional<autofill::FieldGlobalId>(field_id);
+  auto it = sessions_.find(last_compose_field_id_.value());
   if (it != sessions_.end()) {
     // Update existing session
     auto& existing_session = *it->second;
     existing_session.SetComposeResultCallback(std::move(callback));
     return;
   }
-
   sessions_.emplace(
-      last_compose_field_id_,
+      last_compose_field_id_.value(),
       std::make_unique<ComposeSession>(&GetWebContents(), GetModelExecutor(),
                                        std::move(callback)));
 }
 
+void ChromeComposeClient::RemoveActiveSession() {
+  if (debug_session_) {
+    debug_session_.reset();
+    return;
+  }
+  auto it = sessions_.find(last_compose_field_id_.value());
+  CHECK(it != sessions_.end())
+      << "Attempted to remove compose session that doesn't exist.";
+  sessions_.erase(last_compose_field_id_.value());
+  last_compose_field_id_.reset();
+}
+
 compose::ComposeManager& ChromeComposeClient::GetManager() {
   return manager_;
 }

chrome/browser/compose/chrome_compose_client.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_COMPOSE_CHROME_COMPOSE_CLIENT_H_
 
 #include <memory>
+#include <optional>
 #include <string>
 
 #include "base/containers/flat_map.h"
@@ -33,7 +34,8 @@ class WebContents;
 // An implementation of `ComposeClient` for Desktop and Android.
 class ChromeComposeClient
     : public compose::ComposeClient,
-      public content::WebContentsUserData<ChromeComposeClient> {
+      public content::WebContentsUserData<ChromeComposeClient>,
+      public compose::mojom::ComposeDialogClosePageHandler {
  public:
   ChromeComposeClient(const ChromeComposeClient&) = delete;
   ChromeComposeClient& operator=(const ChromeComposeClient&) = delete;
@@ -48,7 +50,14 @@ class ChromeComposeClient
           popup_screen_location,
       ComposeCallback callback) override;
 
+  // ComposeDialogClosePageHandler
+  // Closes the compose dialog. `reason` describes the user action that
+  // triggered the close.
+  void CloseUI(compose::mojom::CloseReason reason) override;
+
   void BindComposeDialog(
+      mojo::PendingReceiver<compose::mojom::ComposeDialogClosePageHandler>
+          close_handler,
       mojo::PendingReceiver<compose::mojom::ComposeDialogPageHandler> handler,
       mojo::PendingRemote<compose::mojom::ComposeDialog> dialog);
 
@@ -77,6 +86,10 @@ class ChromeComposeClient
       const autofill::FieldGlobalId& field_id,
       ComposeCallback callback);
 
+  // Removes `last_compose_field_id_` from `sessions_` and resets
+  // `last_compose_field_id_`.
+  void RemoveActiveSession();
+
   compose::ComposeManagerImpl manager_;
 
   std::unique_ptr<compose::ComposeDialogController> compose_dialog_controller_;
@@ -88,12 +101,20 @@ class ChromeComposeClient
       model_executor_for_test_;
 
   // The unique renderer ID of the last field the user selected compose on.
-  autofill::FieldGlobalId last_compose_field_id_;
+  std::optional<autofill::FieldGlobalId> last_compose_field_id_;
 
   // Saved states for each compose field.
   base::flat_map<autofill::FieldGlobalId, std::unique_ptr<ComposeSession>>
       sessions_;
 
+  // A mojom receiver that is bound to `this` in `BindComposeDialog()`. A pipe
+  // may disconnect but this receiver will still be bound, until reset in the
+  // next bind call. With mojo, there is no need to immediately reset the
+  // binding when the pipe disconnects. Any callbacks in receiver methods can be
+  // safely called even when the pipe is disconnected.
+  mojo::Receiver<compose::mojom::ComposeDialogClosePageHandler>
+      close_page_receiver_;
+
   // Used to test Compose in a tab at |chrome://compose|.
   std::unique_ptr<ComposeSession> debug_session_;
 

chrome/browser/compose/chrome_compose_client_unittest.cc

@@ -119,6 +119,9 @@ class ChromeComposeClientTest : public BrowserWithTestWindowTest {
 
   void BindMojo() {
     // Setup Dialog Page Handler.
+    mojo::PendingReceiver<compose::mojom::ComposeDialogClosePageHandler>
+        close_page_handler_pending_receiver =
+            close_page_handler_.BindNewPipeAndPassReceiver();
     mojo::PendingReceiver<compose::mojom::ComposeDialogPageHandler>
         page_handler_pending_receiver =
             page_handler_.BindNewPipeAndPassReceiver();
@@ -132,7 +135,8 @@ class ChromeComposeClientTest : public BrowserWithTestWindowTest {
             callback_router_->BindNewPipeAndPassRemote();
 
     // Bind mojo to client.
-    client_->BindComposeDialog(std::move(page_handler_pending_receiver),
+    client_->BindComposeDialog(std::move(close_page_handler_pending_receiver),
+                               std::move(page_handler_pending_receiver),
                                std::move(callback_router_pending_remote));
   }
 
@@ -141,6 +145,11 @@ class ChromeComposeClientTest : public BrowserWithTestWindowTest {
   MockOptimizationGuideDecider& opt_guide() { return opt_guide_; }
   MockComposeDialog& compose_dialog() { return compose_dialog_; }
 
+  mojo::Remote<compose::mojom::ComposeDialogClosePageHandler>&
+  close_page_handler() {
+    return close_page_handler_;
+  }
+
   mojo::Remote<compose::mojom::ComposeDialogPageHandler>& page_handler() {
     return page_handler_;
   }
@@ -194,9 +203,12 @@ class ChromeComposeClientTest : public BrowserWithTestWindowTest {
   MockModelExecutor model_executor_;
   MockOptimizationGuideDecider opt_guide_;
   MockComposeDialog compose_dialog_;
+  autofill::FormFieldData field_data_;
 
   std::unique_ptr<mojo::Receiver<compose::mojom::ComposeDialog>>
       callback_router_;
+  mojo::Remote<compose::mojom::ComposeDialogClosePageHandler>
+      close_page_handler_;
   mojo::Remote<compose::mojom::ComposeDialogPageHandler> page_handler_;
 };
 
@@ -560,6 +572,22 @@ TEST_F(ChromeComposeClientTest, NoStateWorksAtChromeCompose) {
   EXPECT_EQ("Cucumbers", result->result);
 }
 
+// Tests that closing after showing the dialog does not crash the browser.
+TEST_F(ChromeComposeClientTest, TestCloseUI) {
+  ShowDialogAndBindMojo();
+  close_page_handler()->CloseUI(compose::mojom::CloseReason::kCloseButton);
+}
+
+// Tests that closing the session at chrome://compose does not crash the
+// browser, even though there is no dialog shown at that URL.
+TEST_F(ChromeComposeClientTest, TestCloseUIAtChromeCompose) {
+  NavigateAndCommitActiveTab(GURL("chrome://compose"));
+  // We skip the dialog showing here, as there is no dialog required at this
+  // URL.
+  BindMojo();
+  close_page_handler()->CloseUI(compose::mojom::CloseReason::kCloseButton);
+}
+
 #if defined(GTEST_HAS_DEATH_TEST)
 // Tests that the Compose client crashes the browser if a webcontents
 // tries to bind mojo without opening the dialog at a non Compose URL.
@@ -568,4 +596,44 @@ TEST_F(ChromeComposeClientTest, NoStateCrashesAtOtherUrls) {
   // We skip the dialog showing here, to validate that non special URLs check.
   EXPECT_DEATH(BindMojo(), "");
 }
+
+// Tests that the Compose client crashes the browser if a webcontents
+// sends any message when the dialog has not been shown.
+TEST_F(ChromeComposeClientTest, TestCannotSendMessagesToNotShownDialog) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  EXPECT_DEATH(page_handler()->SaveWebUIState(""), "");
+}
+
+// Tests that the Compose client crashes the browser if a webcontents
+// tries to close the dialog when the dialog has not been shown.
+TEST_F(ChromeComposeClientTest, TestCannotCloseNotShownDialog) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  EXPECT_DEATH(
+      close_page_handler()->CloseUI(compose::mojom::CloseReason::kCloseButton),
+      "");
+}
+
+// Tests that the Compose client crashes the browser if a webcontents
+// tries to close the dialog when the dialog has not been shown.
+TEST_F(ChromeComposeClientTest, TestCannotSendMessagesAfterClosingDialog) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  ShowDialogAndBindMojo();
+  close_page_handler()->CloseUI(compose::mojom::CloseReason::kCloseButton);
+  // Any message after closing the session will crash.
+  EXPECT_DEATH(page_handler()->SaveWebUIState(""), "");
+}
+
+// Tests that the Compose client crashes the browser if a webcontents
+// sends any more messages after closing the dialog at chrome://contents.
+TEST_F(ChromeComposeClientTest,
+       TestCannotSendMessagesAfterClosingDialogAtChromeCompose) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  NavigateAndCommitActiveTab(GURL("chrome://compose"));
+  // We skip the dialog showing here, as there is no dialog required at this
+  // URL.
+  BindMojo();
+  close_page_handler()->CloseUI(compose::mojom::CloseReason::kCloseButton);
+  // Any message after closing the session will crash.
+  EXPECT_DEATH(page_handler()->SaveWebUIState(""), "");
+}
 #endif  // GTEST_HAS_DEATH_TEST

chrome/browser/resources/compose/compose_api_proxy.ts

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import {ComposeDialogCallbackRouter, ComposeDialogPageHandlerFactory, ComposeDialogPageHandlerInterface, ComposeDialogPageHandlerRemote, StyleModifiers} from './compose.mojom-webui.js';
+import {CloseReason, ComposeDialogCallbackRouter, ComposeDialogClosePageHandlerRemote, ComposeDialogPageHandlerFactory, ComposeDialogPageHandlerRemote, StyleModifiers} from './compose.mojom-webui.js';
 
 /** @interface */
 export interface ComposeApiProxy {
@@ -14,14 +14,14 @@ export interface ComposeApiProxy {
 export class ComposeApiProxyImpl implements ComposeApiProxy {
   static instance: ComposeApiProxy|null = null;
 
-  handler: ComposeDialogPageHandlerInterface =
-      new ComposeDialogPageHandlerRemote();
   composeDialogPageHandler = new ComposeDialogPageHandlerRemote();
+  composeDialogClosePageHandler = new ComposeDialogClosePageHandlerRemote();
   router = new ComposeDialogCallbackRouter();
 
   constructor() {
     const factoryRemote = ComposeDialogPageHandlerFactory.getRemote();
     factoryRemote.createComposeDialogPageHandler(
+        this.composeDialogClosePageHandler.$.bindNewPipeAndPassReceiver(),
         this.composeDialogPageHandler.$.bindNewPipeAndPassReceiver(),
         this.router.$.bindNewPipeAndPassRemote());
   }
@@ -49,4 +49,9 @@ export class ComposeApiProxyImpl implements ComposeApiProxy {
   acceptComposeResult() {
     this.composeDialogPageHandler.acceptComposeResult();
   }
+
+  /** @override */
+  closeUi(reason: CloseReason) {
+    this.composeDialogClosePageHandler.closeUI(reason);
+  }
 }

chrome/browser/ui/webui/compose/compose_ui.cc

@@ -49,6 +49,8 @@ void ComposeUI::BindInterface(
 }
 
 void ComposeUI::CreateComposeDialogPageHandler(
+    mojo::PendingReceiver<compose::mojom::ComposeDialogClosePageHandler>
+        close_handler,
     mojo::PendingReceiver<compose::mojom::ComposeDialogPageHandler> handler,
     mojo::PendingRemote<compose::mojom::ComposeDialog> dialog) {
   DCHECK(dialog.is_valid());
@@ -57,7 +59,8 @@ void ComposeUI::CreateComposeDialogPageHandler(
                                            ? triggering_web_contents_.get()
                                            : web_ui()->GetWebContents();
   ChromeComposeClient::FromWebContents(web_contents)
-      ->BindComposeDialog(std::move(handler), std::move(dialog));
+      ->BindComposeDialog(std::move(close_handler), std::move(handler),
+                          std::move(dialog));
 }
 
 WEB_UI_CONTROLLER_TYPE_IMPL(ComposeUI)

chrome/browser/ui/webui/compose/compose_ui.h

@@ -43,6 +43,8 @@ class ComposeUI : public ui::MojoBubbleWebUIController,
 
  private:
   void CreateComposeDialogPageHandler(
+      mojo::PendingReceiver<compose::mojom::ComposeDialogClosePageHandler>
+          close_handler,
       mojo::PendingReceiver<compose::mojom::ComposeDialogPageHandler> handler,
       mojo::PendingRemote<compose::mojom::ComposeDialog> dialog) override;
   mojo::Receiver<compose::mojom::ComposeDialogPageHandlerFactory>

chrome/common/compose/compose.mojom

@@ -9,6 +9,7 @@ module compose.mojom;
 interface ComposeDialogPageHandlerFactory {
   // Called from dialog compose JS to set up native handler.
   CreateComposeDialogPageHandler(
+      pending_receiver<ComposeDialogClosePageHandler> close_handler,
       pending_receiver<ComposeDialogPageHandler> handler,
       pending_remote<ComposeDialog> dialog);
 };
@@ -72,6 +73,11 @@ struct OpenMetadata {
   ComposeState compose_state;
 };
 
+// The trigger for dismissing the compose dialog.
+enum CloseReason {
+  // The X button on the upper-right of the dialog.
+  kCloseButton,
+};
 
 // Interface for calls from Compose dialog JS into the Browser process.
 interface ComposeDialogPageHandler {
@@ -96,6 +102,13 @@ interface ComposeDialogPageHandler {
   RequestInitialState() => (OpenMetadata initial_state);
 };
 
+// Interface for calls from Compose dialog JS into the Browser process.
+interface ComposeDialogClosePageHandler {
+  // Asks the receiver to close the compose dialog. `reason` describes
+  // the user action that triggered the close.
+  CloseUI(CloseReason reason);
+};
+
 // Interface for calls from the Browser process into Compose dialog JS.
 interface ComposeDialog {
   // Called when a compose request is fulfilled, either by the model execution