Skip to content

GitLab

T
tangled
Commit 4076b3ed authored by Carlos Caballero's avatar Carlos Caballero Committed by Commit Bot
Browse files
Options

Correctly attribute popups to WebContents 

Popups triggered by a different WebContents that then one that actually
opens it were being misattributed to the former.

This is a partial revert of https://crrev.com/c/2277737 where we thought
that source_frame was the frame that opened the popup which is not the
case.

Added a test to prevent future regressions.

Bug: 1128495
Change-Id: I51511716857b0074dad90feee709440c7b5f8795
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2418324
Commit-Queue: Carlos Caballero <carlscab@google.com>
Reviewed-by: default avatarJochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808839}
parent 04b71323
Hide whitespace changes
Inline Side-by-side
Showing with 49 additions and 20 deletions
chrome/browser/ui/blocked_content/popup_blocker_browsertest.cc
View file @ 4076b3ed
...@@ -38,6 +38,7 @@ ...@@ -38,6 +38,7 @@
#include "chrome/test/base/ui_test_utils.h" #include "chrome/test/base/ui_test_utils.h"
#include "components/blocked_content/list_item_position.h" #include "components/blocked_content/list_item_position.h"
#include "components/blocked_content/popup_blocker_tab_helper.h" #include "components/blocked_content/popup_blocker_tab_helper.h"
#include "components/content_settings/browser/page_specific_content_settings.h"
#include "components/content_settings/core/browser/host_content_settings_map.h" #include "components/content_settings/core/browser/host_content_settings_map.h"
#include "components/embedder_support/switches.h" #include "components/embedder_support/switches.h"
#include "components/javascript_dialogs/app_modal_dialog_controller.h" #include "components/javascript_dialogs/app_modal_dialog_controller.h"
...@@ -876,4 +877,41 @@ IN_PROC_BROWSER_TEST_F(PopupBlockerBrowserTest, PopupsDisableBackForwardCache) { ...@@ -876,4 +877,41 @@ IN_PROC_BROWSER_TEST_F(PopupBlockerBrowserTest, PopupsDisableBackForwardCache) {
"PopupBlockerTabHelper")); "PopupBlockerTabHelper"));
} }
// Make sure the poput is attributed to the right WebContents when it is
// triggered from a different WebContents. Regression test for
// https://crbug.com/1128495
IN_PROC_BROWSER_TEST_F(PopupBlockerBrowserTest,
PopupTriggeredFromDifferentWebContents) {
content::BackForwardCacheDisabledTester tester;
const GURL url(
embedded_test_server()->GetURL("/popup_blocker/popup-in-href.html"));
ui_test_utils::NavigateToURL(browser(), url);
ASSERT_EQ(browser()->tab_strip_model()->count(), 1);
content::WebContents* tab_1 =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::TabAddedWaiter tab_Added_waiter(browser());
SimulateMouseClickOrTapElementWithId(tab_1, "link");
tab_Added_waiter.Wait();
content::WebContents* tab_2 =
browser()->tab_strip_model()->GetActiveWebContents();
ASSERT_NE(tab_1, tab_2);
// We need to make sure the js in the new tab that comes from the href runs
// before we perform the checks further down. Since we have no control over
// that script we just run some more (that we do control) and wait for it to
// finish.
EXPECT_TRUE(content::ExecuteScriptWithoutUserGesture(tab_2, ""));
EXPECT_FALSE(content_settings::PageSpecificContentSettings::GetForFrame(
tab_1->GetMainFrame())
->IsContentBlocked(ContentSettingsType::POPUPS));
EXPECT_TRUE(content_settings::PageSpecificContentSettings::GetForFrame(
tab_2->GetMainFrame())
->IsContentBlocked(ContentSettingsType::POPUPS));
}
} // namespace } // namespace
chrome/test/data/popup_blocker/popup-in-href.html 0 → 100644
View file @ 4076b3ed
<!doctype html>
<html>
<body>
<a id="link" href="javascript:window.open('https://example.com');" target="_blank">open</a>.
</body>
</html>
components/blocked_content/android/popup_blocked_infobar_delegate_unittest.cc
View file @ 4076b3ed
...@@ -109,7 +109,6 @@ TEST_F(PopupBlockedInfoBarDelegateTest, ReplacesInfobarOnSecondPopup) { ...@@ -109,7 +109,6 @@ TEST_F(PopupBlockedInfoBarDelegateTest, ReplacesInfobarOnSecondPopup) {
TEST_F(PopupBlockedInfoBarDelegateTest, ShowsBlockedPopups) { TEST_F(PopupBlockedInfoBarDelegateTest, ShowsBlockedPopups) {
TestPopupNavigationDelegate::ResultHolder result; TestPopupNavigationDelegate::ResultHolder result;
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kPopupUrl), &result), std::make_unique<TestPopupNavigationDelegate>(GURL(kPopupUrl), &result),
blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture); blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture);
bool on_accept_called = false; bool on_accept_called = false;
......
components/blocked_content/popup_blocker.cc
View file @ 4076b3ed
...@@ -125,8 +125,8 @@ std::unique_ptr<PopupNavigationDelegate> MaybeBlockPopup( ...@@ -125,8 +125,8 @@ std::unique_ptr<PopupNavigationDelegate> MaybeBlockPopup(
// first. // first.
content::RenderFrameHost* source_frame = content::RenderFrameHost* source_frame =
GetSourceFrameForPopup(delegate.get(), open_url_params, web_contents); GetSourceFrameForPopup(delegate.get(), open_url_params, web_contents);
popup_blocker->AddBlockedPopup(source_frame, std::move(delegate), popup_blocker->AddBlockedPopup(std::move(delegate), window_features,
window_features, block_type); block_type);
auto* trigger = safe_browsing::AdPopupTrigger::FromWebContents(web_contents); auto* trigger = safe_browsing::AdPopupTrigger::FromWebContents(web_contents);
if (trigger) { if (trigger) {
trigger->PopupWasBlocked(source_frame); trigger->PopupWasBlocked(source_frame);
......
components/blocked_content/popup_blocker_tab_helper.cc
View file @ 4076b3ed
...@@ -82,7 +82,6 @@ void PopupBlockerTabHelper::HidePopupNotification() { ...@@ -82,7 +82,6 @@ void PopupBlockerTabHelper::HidePopupNotification() {
} }
void PopupBlockerTabHelper::AddBlockedPopup( void PopupBlockerTabHelper::AddBlockedPopup(
content::RenderFrameHost* source_frame,
std::unique_ptr<PopupNavigationDelegate> delegate, std::unique_ptr<PopupNavigationDelegate> delegate,
const blink::mojom::WindowFeatures& window_features, const blink::mojom::WindowFeatures& window_features,
PopupBlockType block_type) { PopupBlockType block_type) {
...@@ -95,10 +94,9 @@ void PopupBlockerTabHelper::AddBlockedPopup( ...@@ -95,10 +94,9 @@ void PopupBlockerTabHelper::AddBlockedPopup(
blocked_popups_[id] = std::make_unique<BlockedRequest>( blocked_popups_[id] = std::make_unique<BlockedRequest>(
std::move(delegate), window_features, block_type); std::move(delegate), window_features, block_type);
// PageSpecificContentSettings is per page so it does not matter that we use
// the source_frame here and the main frame in HidePopupNotification
auto* content_settings = auto* content_settings =
content_settings::PageSpecificContentSettings::GetForFrame(source_frame); content_settings::PageSpecificContentSettings::GetForFrame(
web_contents()->GetMainFrame());
if (content_settings) { if (content_settings) {
content_settings->OnContentBlocked(ContentSettingsType::POPUPS); content_settings->OnContentBlocked(ContentSettingsType::POPUPS);
} }
......
components/blocked_content/popup_blocker_tab_helper.h
View file @ 4076b3ed
...@@ -19,10 +19,6 @@ ...@@ -19,10 +19,6 @@
#include "ui/base/window_open_disposition.h" #include "ui/base/window_open_disposition.h"
#include "url/gurl.h" #include "url/gurl.h"
namespace content {
class RenderFrameHost;
}
namespace blocked_content { namespace blocked_content {
class PopupNavigationDelegate; class PopupNavigationDelegate;
...@@ -69,8 +65,7 @@ class PopupBlockerTabHelper ...@@ -69,8 +65,7 @@ class PopupBlockerTabHelper
void ShowBlockedPopup(int32_t popup_id, WindowOpenDisposition disposition); void ShowBlockedPopup(int32_t popup_id, WindowOpenDisposition disposition);
// Adds a new blocked popup to the UI. // Adds a new blocked popup to the UI.
void AddBlockedPopup(content::RenderFrameHost* source_frame, void AddBlockedPopup(std::unique_ptr<PopupNavigationDelegate> delegate,
std::unique_ptr<PopupNavigationDelegate> delegate,
const blink::mojom::WindowFeatures& window_features, const blink::mojom::WindowFeatures& window_features,
PopupBlockType block_type); PopupBlockType block_type);
......
components/blocked_content/popup_blocker_tab_helper_unittest.cc
View file @ 4076b3ed
...@@ -84,7 +84,6 @@ TEST_F(PopupBlockerTabHelperTest, BlocksAndShowsPopup) { ...@@ -84,7 +84,6 @@ TEST_F(PopupBlockerTabHelperTest, BlocksAndShowsPopup) {
blink::mojom::WindowFeatures window_features; blink::mojom::WindowFeatures window_features;
window_features.has_x = true; window_features.has_x = true;
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result), std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result),
window_features, PopupBlockType::kNoGesture); window_features, PopupBlockType::kNoGesture);
EXPECT_EQ(result.total_popups_blocked_on_page, 1); EXPECT_EQ(result.total_popups_blocked_on_page, 1);
...@@ -103,7 +102,6 @@ TEST_F(PopupBlockerTabHelperTest, MultiplePopups) { ...@@ -103,7 +102,6 @@ TEST_F(PopupBlockerTabHelperTest, MultiplePopups) {
BlockedUrlListObserver observer(helper()); BlockedUrlListObserver observer(helper());
TestPopupNavigationDelegate::ResultHolder result1; TestPopupNavigationDelegate::ResultHolder result1;
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result1), std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result1),
blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture); blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture);
EXPECT_EQ(result1.total_popups_blocked_on_page, 1); EXPECT_EQ(result1.total_popups_blocked_on_page, 1);
...@@ -113,7 +111,6 @@ TEST_F(PopupBlockerTabHelperTest, MultiplePopups) { ...@@ -113,7 +111,6 @@ TEST_F(PopupBlockerTabHelperTest, MultiplePopups) {
TestPopupNavigationDelegate::ResultHolder result2; TestPopupNavigationDelegate::ResultHolder result2;
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl2), &result2), std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl2), &result2),
blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture); blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture);
EXPECT_EQ(result2.total_popups_blocked_on_page, 2); EXPECT_EQ(result2.total_popups_blocked_on_page, 2);
...@@ -137,7 +134,6 @@ TEST_F(PopupBlockerTabHelperTest, MultiplePopups) { ...@@ -137,7 +134,6 @@ TEST_F(PopupBlockerTabHelperTest, MultiplePopups) {
TEST_F(PopupBlockerTabHelperTest, DoesNotShowPopupWithInvalidID) { TEST_F(PopupBlockerTabHelperTest, DoesNotShowPopupWithInvalidID) {
TestPopupNavigationDelegate::ResultHolder result; TestPopupNavigationDelegate::ResultHolder result;
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result), std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result),
blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture); blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture);
EXPECT_EQ(helper()->GetBlockedPopupsCount(), 1u); EXPECT_EQ(helper()->GetBlockedPopupsCount(), 1u);
...@@ -160,13 +156,11 @@ TEST_F(PopupBlockerTabHelperTest, SetsContentSettingsPopupState) { ...@@ -160,13 +156,11 @@ TEST_F(PopupBlockerTabHelperTest, SetsContentSettingsPopupState) {
TestPopupNavigationDelegate::ResultHolder result; TestPopupNavigationDelegate::ResultHolder result;
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result), std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result),
blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture); blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture);
EXPECT_TRUE(content_settings->IsContentBlocked(ContentSettingsType::POPUPS)); EXPECT_TRUE(content_settings->IsContentBlocked(ContentSettingsType::POPUPS));
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl2), &result), std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl2), &result),
blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture); blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture);
EXPECT_TRUE(content_settings->IsContentBlocked(ContentSettingsType::POPUPS)); EXPECT_TRUE(content_settings->IsContentBlocked(ContentSettingsType::POPUPS));
...@@ -181,7 +175,6 @@ TEST_F(PopupBlockerTabHelperTest, SetsContentSettingsPopupState) { ...@@ -181,7 +175,6 @@ TEST_F(PopupBlockerTabHelperTest, SetsContentSettingsPopupState) {
TEST_F(PopupBlockerTabHelperTest, ClearsContentSettingsPopupStateOnNavigation) { TEST_F(PopupBlockerTabHelperTest, ClearsContentSettingsPopupStateOnNavigation) {
TestPopupNavigationDelegate::ResultHolder result; TestPopupNavigationDelegate::ResultHolder result;
helper()->AddBlockedPopup( helper()->AddBlockedPopup(
web_contents()->GetMainFrame(),
std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result), std::make_unique<TestPopupNavigationDelegate>(GURL(kUrl1), &result),
blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture); blink::mojom::WindowFeatures(), PopupBlockType::kNoGesture);
EXPECT_TRUE(content_settings::PageSpecificContentSettings::GetForFrame( EXPECT_TRUE(content_settings::PageSpecificContentSettings::GetForFrame(
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment