Lifetime of Compose's BubbleContentsWrapperT matches its BubbleDialogView.
Previously the ChromeComposeClient would own (indirectly) the
BubbleContentsWrapperT via ChromeComposeDialogController. Normally this
would not be a problem as the dialog closes right after the web
contents, but it was possible for the webui to reference the destroyed
WebContents in certain scenarios.
Bug: 1499835
Change-Id: Id8c7ea3caad2dc9d4eb20cb97804b0cbadd0f230
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5021240
Reviewed-by: Jeffrey Cohen <jeffreycohen@chromium.org>
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1224489}
diff --git a/chrome/browser/compose/compose_dialog_browsertest.cc b/chrome/browser/compose/compose_dialog_browsertest.cc
new file mode 100644
index 0000000..4487159
--- /dev/null
+++ b/chrome/browser/compose/compose_dialog_browsertest.cc
@@ -0,0 +1,66 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/compose/compose_session.h"
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_enums.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/compose/core/browser/compose_features.h"
+#include "components/optimization_guide/core/optimization_guide_features.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+#include "ui/gfx/geometry/point_conversions.h"
+
+namespace compose {
+
+// IDC_CONTEXT_COMPOSE
+
+class ComposeSessionBrowserTest : public InProcessBrowserTest {
+ public:
+ void SetUp() override {
+ feature_list()->InitWithFeatures(
+ {compose::features::kEnableCompose,
+ optimization_guide::features::kOptimizationGuideModelExecution},
+ {});
+ InProcessBrowserTest::SetUp();
+ }
+
+ base::test::ScopedFeatureList* feature_list() { return &feature_list_; }
+
+ protected:
+ base::test::ScopedFeatureList feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(ComposeSessionBrowserTest, LifetimeOfBubbleWrapper) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+ auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
+ ASSERT_TRUE(ui_test_utils::NavigateToURL(
+ browser(), embedded_test_server()->GetURL("/compose/test2.html")));
+ ASSERT_NE(nullptr, ChromeComposeClient::FromWebContents(web_contents));
+ auto* client = ChromeComposeClient::FromWebContents(web_contents);
+ client->GetComposeEnabling().SetEnabledForTesting();
+
+ // get point of element
+ gfx::PointF textarea_center =
+ content::GetCenterCoordinatesOfElementWithId(web_contents, "elem1");
+ autofill::FormFieldData field_data;
+ field_data.bounds = gfx::RectF((textarea_center), gfx::SizeF(1, 1));
+
+ client->ShowComposeDialog(
+ autofill::AutofillComposeDelegate::UiEntryPoint::kAutofillPopup,
+ field_data, std::nullopt, base::NullCallback());
+
+ // close window right away
+ browser()->tab_strip_model()->CloseWebContentsAt(0,
+ TabCloseTypes::CLOSE_NONE);
+}
+
+} // namespace compose
diff --git a/chrome/browser/ui/BUILD.gn b/chrome/browser/ui/BUILD.gn
index 4d3d438..39d75152 100644
--- a/chrome/browser/ui/BUILD.gn
+++ b/chrome/browser/ui/BUILD.gn
@@ -5017,8 +5017,6 @@
"views/commerce/price_tracking_view.h",
"views/commerce/shopping_collection_iph_view.cc",
"views/commerce/shopping_collection_iph_view.h",
- "views/compose/compose_dialog_view.cc",
- "views/compose/compose_dialog_view.h",
"views/confirm_bubble_views.cc",
"views/confirm_bubble_views.h",
"views/constrained_web_dialog_delegate_views.cc",
@@ -6282,6 +6280,8 @@
sources += [
"views/compose/chrome_compose_dialog_controller.cc",
"views/compose/chrome_compose_dialog_controller.h",
+ "views/compose/compose_dialog_view.cc",
+ "views/compose/compose_dialog_view.h",
]
deps += [ "//components/compose/core/browser" ]
diff --git a/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.cc b/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.cc
index 5c1a64c..3066cc6 100644
--- a/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.cc
+++ b/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.cc
@@ -52,31 +52,29 @@
Profile* profile =
Profile::FromBrowserContext(web_contents_->GetBrowserContext());
- bubble_wrapper_ = std::make_unique<BubbleContentsWrapperT<ComposeUI>>(
+ auto bubble_wrapper = std::make_unique<BubbleContentsWrapperT<ComposeUI>>(
GURL(kComposeURL), profile, IDS_COMPOSE_TITLE);
- bubble_wrapper_->ReloadWebContents();
+ bubble_wrapper->ReloadWebContents();
// This WebUI needs to know the calling BrowserContents so that the compose
// request/result can be properly associated with the triggering form.
- bubble_wrapper_->GetWebUIController()->set_triggering_web_contents(
+ bubble_wrapper->GetWebUIController()->set_triggering_web_contents(
web_contents_.get());
- auto bubble_view = std::make_unique<WebUIBubbleDialogView>(
- anchor_view, bubble_wrapper_.get(),
- gfx::ToRoundedRect(element_bounds_in_screen));
-
- // Allows the bubble bounds to escape the browser window.
- bubble_view->set_has_parent(false);
-
- bubble_ = bubble_view->GetWeakPtr();
- views::BubbleDialogDelegateView::CreateBubble(std::move(bubble_view));
-
- bubble_->set_adjust_if_offscreen(true);
+ auto compose_dialog_view = std::make_unique<ComposeDialogView>(
+ anchor_view, std::move(bubble_wrapper),
+ gfx::ToRoundedRect(element_bounds_in_screen),
+ views::BubbleBorder::Arrow::NONE);
+ bubble_ = compose_dialog_view->GetWeakPtr();
+ views::BubbleDialogDelegateView::CreateBubble(std::move(compose_dialog_view));
}
BubbleContentsWrapperT<ComposeUI>*
ChromeComposeDialogController::GetBubbleWrapper() const {
- return bubble_wrapper_.get();
+ if (bubble_) {
+ return bubble_->bubble_wrapper();
+ }
+ return nullptr;
}
void ChromeComposeDialogController::ShowUI() {
@@ -87,8 +85,10 @@
// TODO(b/300939629): Flesh out implementation and cover other closing paths.
void ChromeComposeDialogController::Close() {
- bubble_wrapper_->CloseUI();
- bubble_wrapper_.reset();
+ auto* wrapper = GetBubbleWrapper();
+ if (wrapper) {
+ wrapper->CloseUI();
+ }
}
ChromeComposeDialogController::ChromeComposeDialogController(
diff --git a/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.h b/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.h
index 2e149a1..4839c2d7 100644
--- a/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.h
+++ b/chrome/browser/ui/views/compose/chrome_compose_dialog_controller.h
@@ -42,8 +42,7 @@
private:
friend class ChromeComposeDialogControllerTest;
- std::unique_ptr<BubbleContentsWrapperT<ComposeUI>> bubble_wrapper_;
- base::WeakPtr<WebUIBubbleDialogView> bubble_;
+ base::WeakPtr<ComposeDialogView> bubble_;
base::WeakPtr<content::WebContents> web_contents_;
base::WeakPtrFactory<ChromeComposeDialogController> weak_ptr_factory_{this};
diff --git a/chrome/browser/ui/views/compose/compose_dialog_view.cc b/chrome/browser/ui/views/compose/compose_dialog_view.cc
index ad5a7c6..bf70beac 100644
--- a/chrome/browser/ui/views/compose/compose_dialog_view.cc
+++ b/chrome/browser/ui/views/compose/compose_dialog_view.cc
@@ -8,33 +8,25 @@
#include "ui/gfx/geometry/skia_conversions.h"
#include "ui/views/bubble/bubble_border.h"
-namespace compose {
-
-// Default size from Figma spec. The size of the view will follow the requested
-// size of the WebUI, once these are connected.
-constexpr gfx::Size kInputDialogSize(448, 216);
-
ComposeDialogView::~ComposeDialogView() = default;
-ComposeDialogView::ComposeDialogView(View* anchor_view,
- views::BubbleBorder::Arrow anchor_position,
- const gfx::Rect bounds,
- content::WebContents* web_contents)
- : BubbleDialogDelegateView(anchor_view, anchor_position) {
- // For testing, a test Window widget is used. Otherwise, no |anchor_view| is
- // given and the parent Window must be manually set.
- if (!anchor_view) {
- set_parent_window(web_contents->GetNativeView());
- SetAnchorRect(bounds);
- }
+ComposeDialogView::ComposeDialogView(
+ View* anchor_view,
+ std::unique_ptr<BubbleContentsWrapperT<ComposeUI>> bubble_wrapper,
+ const gfx::Rect& anchor_bounds,
+ views::BubbleBorder::Arrow anchor_position)
+ : WebUIBubbleDialogView(anchor_view,
+ bubble_wrapper.get(),
+ anchor_bounds,
+ anchor_position),
+ bubble_wrapper_(std::move(bubble_wrapper)) {
+ set_has_parent(false);
+ set_adjust_if_offscreen(true);
+}
- SetButtons(ui::DIALOG_BUTTON_NONE);
- SetPreferredSize(kInputDialogSize);
-
- // Empty contents, to be populated by WebUI.
+base::WeakPtr<ComposeDialogView> ComposeDialogView::GetWeakPtr() {
+ return weak_ptr_factory_.GetWeakPtr();
}
BEGIN_METADATA(ComposeDialogView, views::View)
END_METADATA
-
-} // namespace compose
diff --git a/chrome/browser/ui/views/compose/compose_dialog_view.h b/chrome/browser/ui/views/compose/compose_dialog_view.h
index 196e9d8d..2db755e 100644
--- a/chrome/browser/ui/views/compose/compose_dialog_view.h
+++ b/chrome/browser/ui/views/compose/compose_dialog_view.h
@@ -5,26 +5,34 @@
#ifndef CHROME_BROWSER_UI_VIEWS_COMPOSE_COMPOSE_DIALOG_VIEW_H_
#define CHROME_BROWSER_UI_VIEWS_COMPOSE_COMPOSE_DIALOG_VIEW_H_
+#include "chrome/browser/ui/views/bubble/webui_bubble_dialog_view.h"
+#include "chrome/browser/ui/webui/compose/compose_ui.h"
#include "content/public/browser/web_contents.h"
#include "ui/base/metadata/metadata_header_macros.h"
-#include "ui/views/bubble/bubble_dialog_delegate_view.h"
#include "ui/views/view.h"
-namespace compose {
-
// A view for the contents area of the Compose dialog.
-class ComposeDialogView : public views::BubbleDialogDelegateView {
+class ComposeDialogView : public WebUIBubbleDialogView {
public:
METADATA_HEADER(ComposeDialogView);
- explicit ComposeDialogView(View* anchor_view,
- views::BubbleBorder::Arrow anchor_position,
- const gfx::Rect bounds,
- content::WebContents* web_contents);
+ explicit ComposeDialogView(
+ View* anchor_view,
+ std::unique_ptr<BubbleContentsWrapperT<ComposeUI>> bubble_wrapper,
+ const gfx::Rect& anchor_bounds,
+ views::BubbleBorder::Arrow anchor_position);
~ComposeDialogView() override;
-};
-} // namespace compose
+ BubbleContentsWrapperT<ComposeUI>* bubble_wrapper() {
+ return bubble_wrapper_.get();
+ }
+
+ base::WeakPtr<ComposeDialogView> GetWeakPtr();
+
+ private:
+ std::unique_ptr<BubbleContentsWrapperT<ComposeUI>> bubble_wrapper_;
+ base::WeakPtrFactory<ComposeDialogView> weak_ptr_factory_{this};
+};
#endif // CHROME_BROWSER_UI_VIEWS_COMPOSE_COMPOSE_DIALOG_VIEW_H_
diff --git a/chrome/browser/ui/webui/compose/compose_ui.cc b/chrome/browser/ui/webui/compose/compose_ui.cc
index 62d5615..bf4f226 100644
--- a/chrome/browser/ui/webui/compose/compose_ui.cc
+++ b/chrome/browser/ui/webui/compose/compose_ui.cc
@@ -90,9 +90,12 @@
content::WebContents* web_contents = triggering_web_contents_
? triggering_web_contents_.get()
: web_ui()->GetWebContents();
- ChromeComposeClient::FromWebContents(web_contents)
- ->BindComposeDialog(std::move(close_handler), std::move(handler),
- std::move(dialog));
+ ChromeComposeClient* client =
+ ChromeComposeClient::FromWebContents(web_contents);
+ if (client) {
+ client->BindComposeDialog(std::move(close_handler), std::move(handler),
+ std::move(dialog));
+ }
}
WEB_UI_CONTROLLER_TYPE_IMPL(ComposeUI)
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index b0325462..6310e2d 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -2844,7 +2844,12 @@
]
if (enable_compose) {
- sources += [ "../browser/compose/compose_inner_text_browsertest.cc" ]
+ sources += [
+ "../browser/compose/compose_dialog_browsertest.cc",
+ "../browser/compose/compose_inner_text_browsertest.cc",
+ ]
+
+ deps += [ "//components/compose/core/browser:features" ]
}
if (enable_supervised_users) {
diff --git a/chrome/test/data/compose/test2.html b/chrome/test/data/compose/test2.html
new file mode 100644
index 0000000..699e41d2
--- /dev/null
+++ b/chrome/test/data/compose/test2.html
@@ -0,0 +1,11 @@
+<html>
+ <head>
+ <meta name="viewport" content="width=device-width,minimum-scale=1"></meta>
+ </head>
+ <body>
+ <textarea id="elem1" rows="4" style="font-family: monospace; width: 50ch">
+The
+textarea tag defines a multi-line text input control.
+</textarea>
+ </body>
+</html>