[Safety Tips] Better handle same-document navigations.
This CL prevents same-document navigations such as history.pushState and fragment navigations from resulting in a closed Safety Tip.
(cherry picked from commit 8521a09f519d40a3f47dab4bfe133b01543c7435)
Fixed: 1137661
Change-Id: Id0125055147d50c936c26a8763839e1f47244f8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2468189
Reviewed-by: Mustafa Emre Acer <[email protected]>
Commit-Queue: Joe DeBlasio <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#816832}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2476560
Reviewed-by: Joe DeBlasio <[email protected]>
Cr-Commit-Position: refs/branch-heads/4280@{#419}
Cr-Branched-From: ea420fb963f9658c9969b6513c56b8f47efa1a2a-refs/heads/master@{#812852}
diff --git a/chrome/browser/reputation/reputation_web_contents_observer.cc b/chrome/browser/reputation/reputation_web_contents_observer.cc
index 3b55c933..877987a 100644
--- a/chrome/browser/reputation/reputation_web_contents_observer.cc
+++ b/chrome/browser/reputation/reputation_web_contents_observer.cc
@@ -198,12 +198,20 @@
void ReputationWebContentsObserver::DidFinishNavigation(
content::NavigationHandle* navigation_handle) {
if (!navigation_handle->IsInMainFrame() ||
- navigation_handle->IsSameDocument() ||
!navigation_handle->HasCommitted() || navigation_handle->IsErrorPage()) {
MaybeCallReputationCheckCallback(false);
return;
}
+ // Same doc navigations keep the same status as their predecessor. Update last
+ // navigation entry so that GetSafetyTipInfoForVisibleNavigation() works.
+ if (navigation_handle->IsSameDocument()) {
+ last_safety_tip_navigation_entry_id_ =
+ web_contents()->GetController().GetLastCommittedEntry()->GetUniqueID();
+ MaybeCallReputationCheckCallback(false);
+ return;
+ }
+
last_navigation_safety_tip_info_ = {security_state::SafetyTipStatus::kUnknown,
GURL()};
last_safety_tip_navigation_entry_id_ = 0;
diff --git a/chrome/browser/reputation/safety_tip_ui.h b/chrome/browser/reputation/safety_tip_ui.h
index 065efc5..633d29e 100644
--- a/chrome/browser/reputation/safety_tip_ui.h
+++ b/chrome/browser/reputation/safety_tip_ui.h
@@ -18,7 +18,8 @@
// Represents the different user interactions with a Safety Tip dialog.
//
// These values are persisted to logs. Entries should not be renumbered and
-// numeric values should never be reused. Keep in sync with SafetyTipStatus.
+// numeric values should never be reused. Keep in sync with
+// SafetyTipInteraction in enums.xml.
enum class SafetyTipInteraction {
// The user dismissed the safety tip. Every time the user dismisses the
// dialog, a histogram will be recorded once with this value, and again with a
diff --git a/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.cc b/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.cc
index 9e9ff5e..fdfe468 100644
--- a/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.cc
+++ b/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.cc
@@ -18,6 +18,7 @@
#include "chrome/grit/theme_resources.h"
#include "components/security_state/core/security_state.h"
#include "components/strings/grit/components_strings.h"
+#include "content/public/browser/navigation_handle.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/resource/resource_bundle.h"
#include "ui/gfx/color_utils.h"
@@ -251,6 +252,17 @@
OpenHelpCenterFromSafetyTip(web_contents());
}
+void SafetyTipPageInfoBubbleView::DidStartNavigation(
+ content::NavigationHandle* handle) {
+ if (handle->IsInMainFrame() && !handle->IsSameDocument()) {
+ GetWidget()->CloseWithReason(views::Widget::ClosedReason::kUnspecified);
+ }
+}
+
+void SafetyTipPageInfoBubbleView::DidChangeVisibleSecurityState() {
+ // Do nothing. (Base class closes the bubble.)
+}
+
void ShowSafetyTipDialog(
content::WebContents* web_contents,
security_state::SafetyTipStatus safety_tip_status,
diff --git a/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.h b/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.h
index c75fbc9b..aeef439 100644
--- a/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.h
+++ b/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.h
@@ -54,10 +54,15 @@
private:
friend class SafetyTipPageInfoBubbleViewBrowserTest;
+ void ExecuteLeaveCommand();
void OpenHelpCenter();
views::Button* GetLeaveButtonForTesting() { return leave_button_; }
+ // WebContentsObserver:
+ void DidStartNavigation(content::NavigationHandle* handle) override;
+ void DidChangeVisibleSecurityState() override;
+
const security_state::SafetyTipStatus safety_tip_status_;
// The URL of the page the Safety Tip suggests you intended to go to, when
diff --git a/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view_browsertest.cc b/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view_browsertest.cc
index 9463248..e3bdef0 100644
--- a/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view_browsertest.cc
+++ b/chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view_browsertest.cc
@@ -532,6 +532,26 @@
GURL()));
}
+// Ensure same-document navigations don't close the Safety Tip.
+// Regression test for crbug.com/1137661
+IN_PROC_BROWSER_TEST_P(SafetyTipPageInfoBubbleViewBrowserTest,
+ StillShowAfterSameDocNav) {
+ auto kNavigatedUrl = GetURL("site1.com");
+ SetSafetyTipBadRepPatterns({"site1.com/"});
+
+ // Generate a Safety Tip.
+ NavigateToURL(browser(), kNavigatedUrl, WindowOpenDisposition::CURRENT_TAB);
+ EXPECT_TRUE(IsUIShowingOrSuspiciousSitesDisabled());
+
+ // Now generate a same-document navigation and verify the tip is still there.
+ NavigateToURL(browser(), GURL(kNavigatedUrl.spec() + "#fragment"),
+ WindowOpenDisposition::CURRENT_TAB);
+ EXPECT_TRUE(IsUIShowingOrSuspiciousSitesDisabled());
+
+ ASSERT_NO_FATAL_FAILURE(CheckPageInfoShowsSafetyTipInfo(
+ browser(), security_state::SafetyTipStatus::kBadReputation, GURL()));
+}
+
// Ensure explicitly-allowed sites don't get blocked when the site is otherwise
// blocked server-side.
IN_PROC_BROWSER_TEST_P(SafetyTipPageInfoBubbleViewBrowserTest,
