From a76001f8c7085d89a129896d786970e7cf7eca26 Mon Sep 17 00:00:00 2001 From: Ivan Efremov Date: Tue, 8 Jun 2021 20:38:55 +0700 Subject: [PATCH] 16256: Properly get tab origin for brave shields. Fixes https://github.com/brave/brave-browser/issues/16265 --- .../ad_block_service_browsertest.cc | 35 +++++++++++++++ browser/net/url_context.cc | 8 ++-- .../brave_shields_web_contents_observer.cc | 43 ------------------- .../brave_shields_web_contents_observer.h | 10 ----- 4 files changed, 40 insertions(+), 56 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index e5ecfd286ebf..3d45519e0bce 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/browser/brave_shields/ad_block_service_browsertest.cc @@ -533,6 +533,41 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, SubFrame) { EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 2ULL); } +// Checks nothing is blocked if shields are off. +IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, SubFrameShieldsOff) { + ASSERT_TRUE(InstallDefaultAdBlockExtension()); + + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); + GURL url = embedded_test_server()->GetURL("a.com", "/iframe_blocking.html"); + + brave_shields::SetBraveShieldsEnabled(content_settings(), false, url); + + ui_test_utils::NavigateToURL(browser(), url); + content::WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + + EXPECT_EQ(true, EvalJs(contents->GetAllFrames()[1], + "setExpectations(0, 0, 1, 0);" + "xhr('adbanner.js?1')")); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); + + // Check also an explicit request for a script since it is a common real-world + // scenario. + EXPECT_EQ(true, EvalJs(contents->GetAllFrames()[1], + R"( + new Promise(function (resolve, reject) { + var s = document.createElement('script'); + s.onload = () => resolve(true); + s.onerror = reject; + s.src = 'adbanner.js?2'; + document.head.appendChild(s); + }) + )")); + content::RunAllTasksUntilIdle(); + EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL); + brave_shields::ResetBraveShieldsEnabled(content_settings(), url); +} + // Requests made by a service worker should be blocked as well. IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, ServiceWorkerRequest) { UpdateAdBlockInstanceWithRules("adbanner.js"); diff --git a/browser/net/url_context.cc b/browser/net/url_context.cc index c9b0cf00b06d..a8e1beca8aec 100644 --- a/browser/net/url_context.cc +++ b/browser/net/url_context.cc @@ -105,9 +105,11 @@ std::shared_ptr BraveRequestInfo::MakeCTX( // |AddChannelRequest| provides only old-fashioned |site_for_cookies|. // (See |BraveProxyingWebSocket|). if (ctx->tab_origin.is_empty()) { - ctx->tab_origin = brave_shields::BraveShieldsWebContentsObserver:: - GetTabURLFromRenderFrameInfo(ctx->frame_tree_node_id) - .GetOrigin(); + content::WebContents* contents = + content::WebContents::FromFrameTreeNodeId(ctx->frame_tree_node_id); + if (contents) { + ctx->tab_origin = contents->GetLastCommittedURL().GetOrigin(); + } } if (old_ctx) { diff --git a/components/brave_shields/browser/brave_shields_web_contents_observer.cc b/components/brave_shields/browser/brave_shields_web_contents_observer.cc index 8c7be5371341..2fc76ef78963 100644 --- a/components/brave_shields/browser/brave_shields_web_contents_observer.cc +++ b/components/brave_shields/browser/brave_shields_web_contents_observer.cc @@ -5,11 +5,9 @@ #include "brave/components/brave_shields/browser/brave_shields_web_contents_observer.h" -#include #include #include #include -#include #include "base/strings/utf_string_conversions.h" #include "brave/common/pref_names.h" @@ -47,7 +45,6 @@ using extensions::Event; using extensions::EventRouter; #endif -using content::Referrer; using content::RenderFrameHost; using content::WebContents; @@ -84,11 +81,6 @@ void UpdateContentSettingsToRendererFrames(content::WebContents* web_contents) { namespace brave_shields { -base::Lock BraveShieldsWebContentsObserver::frame_data_map_lock_; - -std::map - BraveShieldsWebContentsObserver::frame_tree_node_id_to_tab_url_; - BraveShieldsWebContentsObserver::~BraveShieldsWebContentsObserver() { } @@ -107,19 +99,9 @@ void BraveShieldsWebContentsObserver::RenderFrameCreated( WebContents* web_contents = WebContents::FromRenderFrameHost(rfh); if (web_contents) { UpdateContentSettingsToRendererFrames(web_contents); - - base::AutoLock lock(frame_data_map_lock_); - frame_tree_node_id_to_tab_url_[rfh->GetFrameTreeNodeId()] = - web_contents->GetURL(); } } -void BraveShieldsWebContentsObserver::RenderFrameDeleted( - RenderFrameHost* rfh) { - base::AutoLock lock(frame_data_map_lock_); - frame_tree_node_id_to_tab_url_.erase(rfh->GetFrameTreeNodeId()); -} - void BraveShieldsWebContentsObserver::RenderFrameHostChanged( RenderFrameHost* old_host, RenderFrameHost* new_host) { if (old_host) { @@ -130,31 +112,6 @@ void BraveShieldsWebContentsObserver::RenderFrameHostChanged( } } -void BraveShieldsWebContentsObserver::DidFinishNavigation( - content::NavigationHandle* navigation_handle) { - RenderFrameHost* main_frame = web_contents()->GetMainFrame(); - if (!web_contents() || !main_frame) { - return; - } - int tree_node_id = main_frame->GetFrameTreeNodeId(); - - base::AutoLock lock(frame_data_map_lock_); - frame_tree_node_id_to_tab_url_[tree_node_id] = web_contents()->GetURL(); -} - -// static -GURL BraveShieldsWebContentsObserver::GetTabURLFromRenderFrameInfo( - int render_frame_tree_node_id) { - base::AutoLock lock(frame_data_map_lock_); - if (-1 != render_frame_tree_node_id) { - auto iter = frame_tree_node_id_to_tab_url_.find(render_frame_tree_node_id); - if (iter != frame_tree_node_id_to_tab_url_.end()) { - return iter->second; - } - } - return GURL(); -} - bool BraveShieldsWebContentsObserver::IsBlockedSubresource( const std::string& subresource) { return blocked_url_paths_.find(subresource) != blocked_url_paths_.end(); diff --git a/components/brave_shields/browser/brave_shields_web_contents_observer.h b/components/brave_shields/browser/brave_shields_web_contents_observer.h index 8a757586b01d..3ed563f7126a 100644 --- a/components/brave_shields/browser/brave_shields_web_contents_observer.h +++ b/components/brave_shields/browser/brave_shields_web_contents_observer.h @@ -12,7 +12,6 @@ #include #include "base/macros.h" -#include "base/synchronization/lock.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/browser/web_contents_user_data.h" @@ -47,13 +46,10 @@ class BraveShieldsWebContentsObserver : public content::WebContentsObserver, protected: // content::WebContentsObserver overrides. void RenderFrameCreated(content::RenderFrameHost* host) override; - void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override; void RenderFrameHostChanged(content::RenderFrameHost* old_host, content::RenderFrameHost* new_host) override; void ReadyToCommitNavigation( content::NavigationHandle* navigation_handle) override; - void DidFinishNavigation( - content::NavigationHandle* navigation_handle) override; // Invoked if an IPC message is coming from a specific RenderFrameHost. bool OnMessageReceived(const IPC::Message& message, @@ -65,12 +61,6 @@ class BraveShieldsWebContentsObserver : public content::WebContentsObserver, content::RenderFrameHost* render_frame_host, const std::u16string& details); - // TODO(iefremov): Refactor this away or at least put into base::NoDestructor. - // Protects global maps below from being concurrently written on the UI thread - // and read on the IO thread. - static base::Lock frame_data_map_lock_; - static std::map frame_tree_node_id_to_tab_url_; - private: friend class content::WebContentsUserData; std::vector allowed_script_origins_;