[Tailored security] Store ref to window when sending scoped message

The TailoredSecurityConsentedModalAndroid class enqueues a window scoped message, but was keeping a reference to a WebContents in a callback. There were cases where the WebContents can be destroyed before the message is cleared from the message queue. When this would happen, Chrome would crash if the user was later shown the message and the user would click settings (which would run the callback). This change addresses the problem by keeping a reference to the window that the message is scoped to and adding APIs that support being passed a window. The lifetime of the window will exceed the lifetime of the message, so the callback should now be able to run without crashing. Change-Id: I81f1174edd08c537054fbd29134d4c0d7a108613 Bug: 1445443 Low-Coverage-Reason: consented_message_android.cc is covered, but android native tests aren't reported by the low-coverage-check tool. Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4617345 Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Commit-Queue: Javier Castro <jacastro@chromium.org> Reviewed-by: Xinghui Lu <xinghuilu@chromium.org> Cr-Commit-Position: refs/heads/main@{#1159197}
chromium · Jun 17, 2023 · 04793ab · 04793ab
1 parent 0fc8e90
commit 04793ab
Show file tree

Hide file tree

Showing 8 changed files with 26 additions and 21 deletions.
diff --git a/...roid/java/src/org/chromium/chrome/browser/safe_browsing/SafeBrowsingSettingsLauncher.java b/...roid/java/src/org/chromium/chrome/browser/safe_browsing/SafeBrowsingSettingsLauncher.java
@@ -10,19 +10,15 @@
 import org.chromium.chrome.browser.safe_browsing.settings.SafeBrowsingSettingsFragment;
 import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
 import org.chromium.components.browser_ui.settings.SettingsLauncher;
-import org.chromium.content_public.browser.WebContents;
 import org.chromium.ui.base.WindowAndroid;
 
-/**
- * Bridge between Java and native SafeBrowsing code to launch the Safe Browsing settings page.
- */
+/** Bridge between Java and native SafeBrowsing code to launch the Safe Browsing settings page. */
 public class SafeBrowsingSettingsLauncher {
     private SafeBrowsingSettingsLauncher() {}
 
     @CalledByNative
     private static void showSafeBrowsingSettings(
-            WebContents webContents, @SettingsAccessPoint int accessPoint) {
-        WindowAndroid window = webContents.getTopLevelNativeWindow();
+            WindowAndroid window, @SettingsAccessPoint int accessPoint) {
         if (window == null) return;
         Context currentContext = window.getContext().get();
         SettingsLauncher settingsLauncher = new SettingsLauncherImpl();

diff --git a/chrome/browser/interstitials/chrome_settings_page_helper.cc b/chrome/browser/interstitials/chrome_settings_page_helper.cc
@@ -33,7 +33,8 @@ void ChromeSettingsPageHelper::OpenEnhancedProtectionSettings(
     content::WebContents* web_contents) const {
 #if BUILDFLAG(IS_ANDROID)
   safe_browsing::ShowSafeBrowsingSettings(
-      web_contents, safe_browsing::SettingsAccessPoint::kSecurityInterstitial);
+      web_contents->GetTopLevelNativeWindow(),
+      safe_browsing::SettingsAccessPoint::kSecurityInterstitial);
 #else
   // In rare circumstances, this happens outside of a Browser, better ignore
   // than crash.

diff --git a/chrome/browser/safe_browsing/android/safe_browsing_settings_launcher_android.cc b/chrome/browser/safe_browsing/android/safe_browsing_settings_launcher_android.cc
@@ -6,14 +6,14 @@
 
 #include "base/android/jni_android.h"
 #include "chrome/android/chrome_jni_headers/SafeBrowsingSettingsLauncher_jni.h"
-#include "content/public/browser/web_contents.h"
+#include "ui/android/window_android.h"
 
 namespace safe_browsing {
 
-void ShowSafeBrowsingSettings(content::WebContents* web_contents,
+void ShowSafeBrowsingSettings(ui::WindowAndroid* window,
                               SettingsAccessPoint access_point) {
   Java_SafeBrowsingSettingsLauncher_showSafeBrowsingSettings(
-      base::android::AttachCurrentThread(), web_contents->GetJavaWebContents(),
+      base::android::AttachCurrentThread(), window->GetJavaObject(),
       static_cast<int>(access_point));
 }
 

diff --git a/chrome/browser/safe_browsing/android/safe_browsing_settings_launcher_android.h b/chrome/browser/safe_browsing/android/safe_browsing_settings_launcher_android.h
@@ -6,15 +6,12 @@
 #define CHROME_BROWSER_SAFE_BROWSING_ANDROID_SAFE_BROWSING_SETTINGS_LAUNCHER_ANDROID_H_
 
 #include "components/safe_browsing/core/common/safe_browsing_settings_metrics.h"
-
-namespace content {
-class WebContents;
-}
+#include "ui/android/window_android.h"
 
 namespace safe_browsing {
 
 // Opens the Safe Browsing settings page on Android.
-void ShowSafeBrowsingSettings(content::WebContents* web_contents,
+void ShowSafeBrowsingSettings(ui::WindowAndroid* window,
                               SettingsAccessPoint access_point);
 
 }  // namespace safe_browsing

diff --git a/chrome/browser/safe_browsing/tailored_security/consented_message_android.cc b/chrome/browser/safe_browsing/tailored_security/consented_message_android.cc
@@ -38,7 +38,7 @@ TailoredSecurityConsentedModalAndroid::TailoredSecurityConsentedModalAndroid(
     content::WebContents* web_contents,
     bool enable,
     base::OnceClosure dismiss_callback)
-    : web_contents_(web_contents),
+    : window_android_(web_contents->GetTopLevelNativeWindow()),
       dismiss_callback_(std::move(dismiss_callback)),
       is_enable_message_(enable) {
   message_ = std::make_unique<messages::MessageWrapper>(
@@ -100,8 +100,7 @@ TailoredSecurityConsentedModalAndroid::TailoredSecurityConsentedModalAndroid(
       base::Unretained(this)));
 
   messages::MessageDispatcherBridge::Get()->EnqueueWindowScopedMessage(
-      message_.get(), web_contents_->GetTopLevelNativeWindow(),
-      messages::MessagePriority::kNormal);
+      message_.get(), window_android_, messages::MessagePriority::kNormal);
   LogOutcome(TailoredSecurityOutcome::kShown, is_enable_message_);
 }
 
@@ -119,7 +118,7 @@ void TailoredSecurityConsentedModalAndroid::DismissMessageInternal(
 }
 
 void TailoredSecurityConsentedModalAndroid::HandleSettingsClicked() {
-  ShowSafeBrowsingSettings(web_contents_,
+  ShowSafeBrowsingSettings(window_android_,
                            SettingsAccessPoint::kTailoredSecurity);
   LogOutcome(TailoredSecurityOutcome::kSettings, is_enable_message_);
   DismissMessageInternal(messages::DismissReason::SECONDARY_ACTION);

diff --git a/chrome/browser/safe_browsing/tailored_security/consented_message_android.h b/chrome/browser/safe_browsing/tailored_security/consented_message_android.h
@@ -10,6 +10,7 @@
 #include "base/memory/raw_ptr.h"
 #include "components/messages/android/message_enums.h"
 #include "components/messages/android/message_wrapper.h"
+#include "ui/android/window_android.h"
 
 namespace content {
 class WebContents;
@@ -34,7 +35,7 @@ class TailoredSecurityConsentedModalAndroid {
   void HandleMessageDismissed(messages::DismissReason dismiss_reason);
 
   std::unique_ptr<messages::MessageWrapper> message_;
-  raw_ptr<content::WebContents> web_contents_ = nullptr;
+  raw_ptr<ui::WindowAndroid> window_android_ = nullptr;
   base::OnceClosure dismiss_callback_;
 
   // Whether the message is shown for Tailored Security being enabled or

diff --git a/chrome/browser/safe_browsing/tailored_security/consented_message_android_unittest.cc b/chrome/browser/safe_browsing/tailored_security/consented_message_android_unittest.cc
@@ -12,6 +12,7 @@
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/web_contents_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/android/window_android.h"
 
 namespace safe_browsing {
 class TailoredSecurityConsentedModalAndroidTest : public testing::Test {
@@ -69,6 +70,11 @@ TEST_F(TailoredSecurityConsentedModalAndroidTest,
 
 TEST_F(TailoredSecurityConsentedModalAndroidTest,
        DisabledDialogHandleSettingsClickedLogsUserAction) {
+  // Create a scoped window so that WebContents::GetTopLevelNativeWindow does
+  // not return null.
+  std::unique_ptr<ui::WindowAndroid::ScopedWindowAndroidForTesting> window =
+      ui::WindowAndroid::CreateForTesting();
+  window.get()->get()->AddChild(web_contents_.get()->GetNativeView());
   TailoredSecurityConsentedModalAndroid consented_modal(
       web_contents_.get(), /*enabled=*/false, base::DoNothing());
   DoSettingsClicked(&consented_modal);
@@ -113,6 +119,11 @@ TEST_F(TailoredSecurityConsentedModalAndroidTest,
 
 TEST_F(TailoredSecurityConsentedModalAndroidTest,
        EnabledDialogHandleSettingsClickedLogsUserAction) {
+  // Create a scoped window so that WebContents::GetTopLevelNativeWindow does
+  // not return null.
+  std::unique_ptr<ui::WindowAndroid::ScopedWindowAndroidForTesting> window =
+      ui::WindowAndroid::CreateForTesting();
+  window.get()->get()->AddChild(web_contents_.get()->GetNativeView());
   TailoredSecurityConsentedModalAndroid consented_modal(
       web_contents_.get(), /*enabled=*/true, base::DoNothing());
   DoSettingsClicked(&consented_modal);

diff --git a/chrome/browser/safe_browsing/tailored_security/unconsented_message_android.cc b/chrome/browser/safe_browsing/tailored_security/unconsented_message_android.cc
@@ -189,7 +189,7 @@ void TailoredSecurityUnconsentedMessageAndroid::HandleMessageAccepted() {
     LogMessageOutcome(TailoredSecurityOutcome::kSettings, is_in_flow_);
   }
 
-  ShowSafeBrowsingSettings(web_contents_,
+  ShowSafeBrowsingSettings(web_contents_->GetTopLevelNativeWindow(),
                            SettingsAccessPoint::kTailoredSecurity);
 }