From 2b8f8ff3ac00229d7f97260d5c51bc69e6a1643f Mon Sep 17 00:00:00 2001 From: Arthur Edelstein Date: Thu, 19 Jan 2023 12:13:36 -0600 Subject: [PATCH 1/3] Fix 27492 (broken facebook popup when screen farbling enabled) (uplift to 1.48.x) (#16571) --- .../brave_screen_farbling_browsertest.cc | 114 ++++++++++++++---- .../renderer/core/frame/local_dom_window.cc | 24 ++++ .../renderer/core/frame/local_dom_window.h | 10 ++ .../renderer/core/page/chrome_client_impl.cc | 14 ++- 4 files changed, 135 insertions(+), 27 deletions(-) diff --git a/browser/farbling/brave_screen_farbling_browsertest.cc b/browser/farbling/brave_screen_farbling_browsertest.cc index 02a4ec3a410a..b639f9242004 100644 --- a/browser/farbling/brave_screen_farbling_browsertest.cc +++ b/browser/farbling/brave_screen_farbling_browsertest.cc @@ -26,6 +26,8 @@ #include "net/test/embedded_test_server/embedded_test_server.h" #include "net/test/embedded_test_server/http_request.h" #include "third_party/blink/public/common/features.h" +#include "ui/views/widget/widget.h" +#include "ui/views/widget/widget_observer.h" using brave_shields::ControlType; @@ -33,10 +35,54 @@ namespace { const gfx::Rect kTestWindowBounds[] = { gfx::Rect(200, 100, 300, 200), gfx::Rect(50, 50, 200, 200), - gfx::Rect(50, 50, 555, 444), gfx::Rect(0, 0, 200, 200)}; + gfx::Rect(50, 50, 475, 460), gfx::Rect(0, 0, 200, 200)}; } // namespace +// A helper class to wait for widget bounds changes beyond given thresholds. +class WidgetBoundsChangeWaiter final : public views::WidgetObserver { + public: + WidgetBoundsChangeWaiter(views::Widget* widget, int threshold) + : widget_(widget), + threshold_(threshold), + initial_bounds_(widget->GetWindowBoundsInScreen()) { + widget_->AddObserver(this); + } + + WidgetBoundsChangeWaiter(const WidgetBoundsChangeWaiter&) = delete; + WidgetBoundsChangeWaiter& operator=(const WidgetBoundsChangeWaiter&) = delete; + ~WidgetBoundsChangeWaiter() final { widget_->RemoveObserver(this); } + + // views::WidgetObserver: + void OnWidgetBoundsChanged(views::Widget* widget, + const gfx::Rect& rect) final { + if (BoundsChangeMeetsThreshold(rect)) { + widget_->RemoveObserver(this); + run_loop_.Quit(); + } + } + + // Wait for changes to occur, or return immediately if they already have. + void Wait() { + if (!BoundsChangeMeetsThreshold(widget_->GetWindowBoundsInScreen())) { + run_loop_.Run(); + } + } + + private: + bool BoundsChangeMeetsThreshold(const gfx::Rect& rect) const { + return std::abs(rect.x() - initial_bounds_.x()) >= threshold_ && + std::abs(rect.y() - initial_bounds_.y()) >= threshold_ && + std::abs(rect.width() - initial_bounds_.width()) >= threshold_ && + std::abs(rect.height() - initial_bounds_.height()) >= threshold_; + } + + const raw_ptr widget_; + const int threshold_; + const gfx::Rect initial_bounds_; + base::RunLoop run_loop_; +}; + class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { public: void SetUpOnMainThread() override { @@ -57,8 +103,7 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { ASSERT_TRUE(embedded_test_server()->Start()); - top_level_page_url_ = embedded_test_server()->GetURL("a.com", "/"); - farbling_url_ = embedded_test_server()->GetURL("a.com", "/iframe.html"); + parent_url_ = embedded_test_server()->GetURL("a.com", "/iframe.html"); } void TearDown() override { @@ -81,7 +126,7 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { void SetFingerprintingSetting(bool allow) { brave_shields::SetFingerprintingControlType( ContentSettings(), allow ? ControlType::ALLOW : ControlType::DEFAULT, - top_level_page_url_); + parent_url()); } content::WebContents* Contents() const { @@ -110,8 +155,6 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { virtual bool IsFlagDisabled() const = 0; - const GURL& FarblingUrl() { return farbling_url_; } - gfx::Rect SetBounds(const gfx::Rect& bounds) { browser()->window()->SetBounds(bounds); return browser()->window()->GetBounds(); @@ -132,16 +175,20 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { EXPECT_GE(8, EvalJs(host, "window.outerWidth - parent.innerWidth")); EXPECT_GE(8, EvalJs(host, "window.outerHeight - parent.innerHeight")); + EXPECT_GE(8, EvalJs(host, + "window.screen.availWidth - Math.max(450, " + "parent.innerWidth)")); + EXPECT_GE(8, EvalJs(host, + "window.screen.availHeight - Math.max(450, " + "parent.innerHeight)")); EXPECT_GE( 8, - EvalJs(host, "window.screen.availWidth - parent.innerWidth")); - EXPECT_GE( - 8, - EvalJs(host, "window.screen.availHeight - parent.innerHeight")); - EXPECT_GE(8, - EvalJs(host, "window.screen.width - parent.innerWidth")); - EXPECT_GE( - 8, EvalJs(host, "window.screen.height - parent.innerHeight")); + EvalJs( + host, + "window.screen.width - Math.max(450, parent.innerWidth)")); + EXPECT_GE(8, EvalJs(host, + "window.screen.height - Math.max(450, " + "parent.innerHeight)")); } else { EXPECT_LE(0, EvalJs(host, "window.outerWidth - parent.innerWidth")); EXPECT_LT(8, @@ -176,7 +223,7 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { SetFingerprintingSetting(allow_fingerprinting); for (int i = 0; i < static_cast(std::size(kTestWindowBounds)); ++i) { SetBounds(kTestWindowBounds[i]); - NavigateToURLUntilLoadStop(FarblingUrl()); + NavigateToURLUntilLoadStop(parent_url()); for (bool test_iframe : {false, true}) { content::RenderFrameHost* host = test_iframe ? Parent() : IFrame(); if (!allow_fingerprinting && !IsFlagDisabled()) { @@ -208,7 +255,7 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { SetBounds(kTestWindowBounds[j]); for (bool allow_fingerprinting : {false, true}) { SetFingerprintingSetting(allow_fingerprinting); - NavigateToURLUntilLoadStop(FarblingUrl()); + NavigateToURLUntilLoadStop(parent_url()); for (bool test_iframe : {false, true}) { content::RenderFrameHost* host = test_iframe ? Parent() : IFrame(); EXPECT_EQ( @@ -233,10 +280,10 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { } while (parent_bounds.width() > 600 || parent_bounds.height() > 600); for (bool allow_fingerprinting : {false, true}) { SetFingerprintingSetting(allow_fingerprinting); - NavigateToURLUntilLoadStop(FarblingUrl()); + NavigateToURLUntilLoadStop(parent_url()); for (bool test_iframe : {false, true}) { const char* script = - "open('http://d.test/', '', `" + "open('/simple.html', '', `" "left=10," "top=10," "width=${outerWidth + 200}," @@ -249,24 +296,43 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { if (!allow_fingerprinting && !IsFlagDisabled()) { EXPECT_GE(child_bounds.x(), parent_bounds.x()); EXPECT_GE(child_bounds.y(), parent_bounds.y()); - EXPECT_GE(10 + parent_bounds.width(), child_bounds.width()); - EXPECT_GE(10 + parent_bounds.height(), child_bounds.height()); + int maxWidth = 10 + std::max(450, parent_bounds.width()); + int maxHeight = 10 + std::max(450, parent_bounds.width()); + EXPECT_GE(maxWidth, child_bounds.width()); + EXPECT_GE(maxHeight, child_bounds.height()); } else { EXPECT_LE(child_bounds.x(), std::max(80, 10 + parent_bounds.x())); EXPECT_LE(child_bounds.y(), std::max(80, 10 + parent_bounds.y())); EXPECT_LE(parent_bounds.width(), child_bounds.width()); EXPECT_LE(parent_bounds.height(), child_bounds.height()); } + if (!test_iframe) { + auto bounds_before = popup->window()->GetBounds(); + auto* widget = views::Widget::GetWidgetForNativeWindow( + popup->window()->GetNativeWindow()); + auto waiter = WidgetBoundsChangeWaiter(widget, 10); + ASSERT_TRUE( + ExecJs(popup_contents, "moveTo(screenX + 11, screenY + 12)")); + ASSERT_TRUE(ExecJs(popup_contents, + "resizeTo(outerWidth + 13, outerHeight + 14)")); + waiter.Wait(); + auto bounds_after = popup->window()->GetBounds(); + EXPECT_EQ(11, bounds_after.x() - bounds_before.x()); + EXPECT_EQ(12, bounds_after.y() - bounds_before.y()); + EXPECT_EQ(13, bounds_after.width() - bounds_before.width()); + EXPECT_EQ(14, bounds_after.height() - bounds_before.height()); + } } } } + GURL& parent_url() { return parent_url_; } + protected: base::test::ScopedFeatureList feature_list_; private: - GURL top_level_page_url_; - GURL farbling_url_; + GURL parent_url_; std::unique_ptr content_client_; std::unique_ptr browser_content_client_; }; @@ -295,12 +361,12 @@ class BraveScreenFarblingBrowserTest_DisableFlag IN_PROC_BROWSER_TEST_F(BraveScreenFarblingBrowserTest_EnableFlag, FarbleScreenSize_EnableFlag) { - FarbleScreenSize(FarblingUrl(), true); + FarbleScreenSize(parent_url(), true); } IN_PROC_BROWSER_TEST_F(BraveScreenFarblingBrowserTest_DisableFlag, FarbleScreenSize_DisableFlag) { - FarbleScreenSize(FarblingUrl(), true); + FarbleScreenSize(parent_url(), true); } IN_PROC_BROWSER_TEST_F(BraveScreenFarblingBrowserTest_EnableFlag, diff --git a/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.cc b/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.cc index 47cac6c871c4..052f55219918 100644 --- a/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.cc +++ b/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.cc @@ -14,6 +14,8 @@ #define outerWidth outerWidth_ChromiumImpl #define screenX screenX_ChromiumImpl #define screenY screenY_ChromiumImpl +#define resizeTo resizeTo_ChromiumImpl +#define moveTo moveTo_ChromiumImpl #include "src/third_party/blink/renderer/core/frame/local_dom_window.cc" @@ -21,6 +23,8 @@ #undef outerWidth #undef screenX #undef screenY +#undef resizeTo +#undef moveTo namespace blink { @@ -90,4 +94,24 @@ int LocalDOMWindow::screenY() const { : screenY_ChromiumImpl(); } +void LocalDOMWindow::resizeTo(int width, int height) const { + ExecutionContext* context = GetExecutionContext(); + if (BlockScreenFingerprinting(context)) { + resizeTo_ChromiumImpl(width + outerWidth_ChromiumImpl() - outerWidth(), + height + outerHeight_ChromiumImpl() - outerHeight()); + } else { + resizeTo_ChromiumImpl(width, height); + } +} + +void LocalDOMWindow::moveTo(int x, int y) const { + ExecutionContext* context = GetExecutionContext(); + if (BlockScreenFingerprinting(context)) { + moveTo_ChromiumImpl(x + screenX_ChromiumImpl() - screenX(), + y + screenY_ChromiumImpl() - screenY()); + } else { + moveTo_ChromiumImpl(x, y); + } +} + } // namespace blink diff --git a/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.h b/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.h index d4b8f3208a48..c7f287e075ef 100644 --- a/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.h +++ b/chromium_src/third_party/blink/renderer/core/frame/local_dom_window.h @@ -36,6 +36,14 @@ screenY_ChromiumImpl() const; \ int screenTop +#define resizeTo \ + resizeTo_ChromiumImpl(int width, int height) const; \ + void resizeTo + +#define moveTo \ + moveTo_ChromiumImpl(int x, int y) const; \ + void moveTo + #include "src/third_party/blink/renderer/core/frame/local_dom_window.h" #undef SetStorageKey @@ -43,5 +51,7 @@ #undef outerWidth #undef screenLeft #undef screenTop +#undef resizeTo +#undef moveTo #endif // BRAVE_CHROMIUM_SRC_THIRD_PARTY_BLINK_RENDERER_CORE_FRAME_LOCAL_DOM_WINDOW_H_ diff --git a/chromium_src/third_party/blink/renderer/core/page/chrome_client_impl.cc b/chromium_src/third_party/blink/renderer/core/page/chrome_client_impl.cc index 725b16cf6ede..c011179009d5 100644 --- a/chromium_src/third_party/blink/renderer/core/page/chrome_client_impl.cc +++ b/chromium_src/third_party/blink/renderer/core/page/chrome_client_impl.cc @@ -3,6 +3,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ +#include + #include "src/third_party/blink/renderer/core/page/chrome_client_impl.cc" #include "brave/third_party/blink/renderer/core/farbling/brave_session_cache.h" @@ -27,9 +29,15 @@ const display::ScreenInfos& ChromeClientImpl::BraveGetScreenInfos( if (!brave::BlockScreenFingerprinting(context)) { return GetScreenInfos(frame); } - gfx::Rect farbled_screen_rect(dom_window->screenX(), dom_window->screenY(), - dom_window->outerWidth(), - dom_window->outerHeight()); + // Don't tell window screen is smaller than 450x450. + int min_width = + FarbleInteger(context, brave::FarbleKey::kWindowInnerWidth, 450, 0, 8); + int min_height = + FarbleInteger(context, brave::FarbleKey::kWindowInnerHeight, 450, 0, 8); + gfx::Rect farbled_screen_rect( + dom_window->screenX(), dom_window->screenY(), + std::max(min_width, dom_window->outerWidth()), + std::max(min_height, dom_window->outerHeight())); screen_info.rect = farbled_screen_rect; screen_info.available_rect = farbled_screen_rect; screen_info.is_extended = false; From 81f5202841c26faa75c9d3d75c64850a02b892af Mon Sep 17 00:00:00 2001 From: Arthur Edelstein Date: Wed, 22 Feb 2023 01:31:21 -0500 Subject: [PATCH 2/3] fix re-enable full FarbleScreenPopupPosition test (uplift to 1.48.x) (#17277) * fix re-enable full FarbleScreenPopupPosition test * make window smaller before offseting it --- .../brave_screen_farbling_browsertest.cc | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/browser/farbling/brave_screen_farbling_browsertest.cc b/browser/farbling/brave_screen_farbling_browsertest.cc index b639f9242004..53f4fc3afa8d 100644 --- a/browser/farbling/brave_screen_farbling_browsertest.cc +++ b/browser/farbling/brave_screen_farbling_browsertest.cc @@ -71,10 +71,10 @@ class WidgetBoundsChangeWaiter final : public views::WidgetObserver { private: bool BoundsChangeMeetsThreshold(const gfx::Rect& rect) const { - return std::abs(rect.x() - initial_bounds_.x()) >= threshold_ && - std::abs(rect.y() - initial_bounds_.y()) >= threshold_ && - std::abs(rect.width() - initial_bounds_.width()) >= threshold_ && - std::abs(rect.height() - initial_bounds_.height()) >= threshold_; + return (std::abs(rect.x() - initial_bounds_.x()) >= threshold_ && + std::abs(rect.y() - initial_bounds_.y()) >= threshold_) || + (std::abs(rect.width() - initial_bounds_.width()) >= threshold_ && + std::abs(rect.height() - initial_bounds_.height()) >= threshold_); } const raw_ptr widget_; @@ -307,20 +307,26 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { EXPECT_LE(parent_bounds.height(), child_bounds.height()); } if (!test_iframe) { - auto bounds_before = popup->window()->GetBounds(); auto* widget = views::Widget::GetWidgetForNativeWindow( popup->window()->GetNativeWindow()); + + auto bounds_before = popup->window()->GetBounds(); + auto waiter2 = WidgetBoundsChangeWaiter(widget, 10); + ASSERT_TRUE(ExecJs(popup_contents, + "resizeTo(outerWidth - 13, outerHeight - 14)")); + waiter2.Wait(); + auto bounds_after = popup->window()->GetBounds(); + EXPECT_EQ(-13, bounds_after.width() - bounds_before.width()); + EXPECT_EQ(-14, bounds_after.height() - bounds_before.height()); + + bounds_before = popup->window()->GetBounds(); auto waiter = WidgetBoundsChangeWaiter(widget, 10); ASSERT_TRUE( ExecJs(popup_contents, "moveTo(screenX + 11, screenY + 12)")); - ASSERT_TRUE(ExecJs(popup_contents, - "resizeTo(outerWidth + 13, outerHeight + 14)")); waiter.Wait(); - auto bounds_after = popup->window()->GetBounds(); + bounds_after = popup->window()->GetBounds(); EXPECT_EQ(11, bounds_after.x() - bounds_before.x()); EXPECT_EQ(12, bounds_after.y() - bounds_before.y()); - EXPECT_EQ(13, bounds_after.width() - bounds_before.width()); - EXPECT_EQ(14, bounds_after.height() - bounds_before.height()); } } } From 16941f47c4cf41936f87e20b4f58b259948169e2 Mon Sep 17 00:00:00 2001 From: Arthur Edelstein Date: Thu, 23 Feb 2023 12:46:12 -0500 Subject: [PATCH 3/3] fix broken screen farbling regression test (uplift to 1.48.x) (#17341) --- .../brave_screen_farbling_browsertest.cc | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/browser/farbling/brave_screen_farbling_browsertest.cc b/browser/farbling/brave_screen_farbling_browsertest.cc index 53f4fc3afa8d..939fe11257f8 100644 --- a/browser/farbling/brave_screen_farbling_browsertest.cc +++ b/browser/farbling/brave_screen_farbling_browsertest.cc @@ -157,6 +157,7 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { gfx::Rect SetBounds(const gfx::Rect& bounds) { browser()->window()->SetBounds(bounds); + content::RunAllPendingInMessageLoop(); return browser()->window()->GetBounds(); } @@ -274,20 +275,17 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { void FarbleScreenPopupPosition(int j) { gfx::Rect parent_bounds; - // Make sure parent_bounds dimensions aren't unexpectedly large. - do { - parent_bounds = SetBounds(kTestWindowBounds[j]); - } while (parent_bounds.width() > 600 || parent_bounds.height() > 600); + parent_bounds = SetBounds(kTestWindowBounds[j]); for (bool allow_fingerprinting : {false, true}) { SetFingerprintingSetting(allow_fingerprinting); NavigateToURLUntilLoadStop(parent_url()); for (bool test_iframe : {false, true}) { const char* script = "open('/simple.html', '', `" - "left=10," - "top=10," - "width=${outerWidth + 200}," - "height=${outerHeight + 200}" + "left=30," + "top=30," + "width=${outerWidth + 20}," + "height=${outerHeight + 20}" "`);"; Browser* popup = OpenPopup(script, test_iframe); auto* popup_contents = popup->tab_strip_model()->GetActiveWebContents(); @@ -311,19 +309,19 @@ class BraveScreenFarblingBrowserTest : public InProcessBrowserTest { popup->window()->GetNativeWindow()); auto bounds_before = popup->window()->GetBounds(); - auto waiter2 = WidgetBoundsChangeWaiter(widget, 10); + auto waiter1 = WidgetBoundsChangeWaiter(widget, 10); ASSERT_TRUE(ExecJs(popup_contents, "resizeTo(outerWidth - 13, outerHeight - 14)")); - waiter2.Wait(); + waiter1.Wait(); auto bounds_after = popup->window()->GetBounds(); EXPECT_EQ(-13, bounds_after.width() - bounds_before.width()); EXPECT_EQ(-14, bounds_after.height() - bounds_before.height()); bounds_before = popup->window()->GetBounds(); - auto waiter = WidgetBoundsChangeWaiter(widget, 10); + auto waiter2 = WidgetBoundsChangeWaiter(widget, 10); ASSERT_TRUE( ExecJs(popup_contents, "moveTo(screenX + 11, screenY + 12)")); - waiter.Wait(); + waiter2.Wait(); bounds_after = popup->window()->GetBounds(); EXPECT_EQ(11, bounds_after.x() - bounds_before.x()); EXPECT_EQ(12, bounds_after.y() - bounds_before.y());