From 207e93c7f91a9aa33644034f6b94cd8ed128811d Mon Sep 17 00:00:00 2001 From: Charlie Reis Date: Wed, 4 Oct 2023 23:48:49 +0000 Subject: [PATCH] Remove dangling raw_ptr for WebContentsImpl::current_fullscreen_frame_. It may be possible for the referenced RenderFrameHost to be deleted without clearing this member, so replace it with an ID instead. It turns out this member was only needed for comparisons anyway, and looking up the corresponding RenderFrameHost is never needed. Bug: 1291138 Change-Id: I39edef6d9b3b8f42a712d7ae8a982ab48922e595 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908280 Commit-Queue: Charlie Reis Reviewed-by: Arthur Sonzogni Cr-Commit-Position: refs/heads/main@{#1205560} --- .../display_cutout/display_cutout_host_impl.h | 2 +- .../browser/web_contents/web_contents_impl.cc | 9 ++++--- .../browser/web_contents/web_contents_impl.h | 9 ++++--- .../web_contents_impl_browsertest.cc | 27 ++++++++++++------- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/content/browser/display_cutout/display_cutout_host_impl.h b/content/browser/display_cutout/display_cutout_host_impl.h index 6a35fde7a9a039..55e8ead91a83bc 100644 --- a/content/browser/display_cutout/display_cutout_host_impl.h +++ b/content/browser/display_cutout/display_cutout_host_impl.h @@ -66,7 +66,7 @@ class DisplayCutoutHostImpl : public blink::mojom::DisplayCutoutHost { // Stores the current |RenderFrameHost| that has the applied safe area insets // and is controlling the viewport fit value. This value is different than - // `WebContentsImpl::current_fullscreen_frame_` because it also considers + // `WebContentsImpl::current_fullscreen_frame_id_` because it also considers // browser side driven fullscreen mode, not just renderer side requested // frames. base::WeakPtr current_rfh_; diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index 1bd950d689c7a2..37dde1b4b883ce 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc @@ -3782,7 +3782,7 @@ void WebContentsImpl::ExitFullscreenMode(bool will_cause_resize) { } } - current_fullscreen_frame_ = nullptr; + current_fullscreen_frame_id_ = GlobalRenderFrameHostId(); observers_.NotifyObservers( &WebContentsObserver::DidToggleFullscreenModeForTab, IsFullscreen(), @@ -3898,7 +3898,7 @@ void WebContentsImpl::FullscreenFrameSetUpdated() { OPTIONAL_TRACE_EVENT0("content", "WebContentsImpl::FullscreenFrameSetUpdated"); if (fullscreen_frames_.empty()) { - current_fullscreen_frame_ = nullptr; + current_fullscreen_frame_id_ = GlobalRenderFrameHostId(); return; } @@ -3910,9 +3910,10 @@ void WebContentsImpl::FullscreenFrameSetUpdated() { // If we have already notified observers about this frame then we should not // fire the observers again. - if (new_fullscreen_frame == current_fullscreen_frame_) + if (new_fullscreen_frame->GetGlobalId() == current_fullscreen_frame_id_) { return; - current_fullscreen_frame_ = new_fullscreen_frame; + } + current_fullscreen_frame_id_ = new_fullscreen_frame->GetGlobalId(); observers_.NotifyObservers(&WebContentsObserver::DidAcquireFullscreen, new_fullscreen_frame); diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h index c99c31b550bb9c..c5448daf2e2659 100644 --- a/content/browser/web_contents/web_contents_impl.h +++ b/content/browser/web_contents/web_contents_impl.h @@ -1824,7 +1824,7 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents, void ClearTargetURL(); // Called each time |fullscreen_frames_| is updated. Find the new - // |current_fullscreen_frame_| and notify observers whenever it changes. + // |current_fullscreen_frame_id_| and notify observers whenever it changes. void FullscreenFrameSetUpdated(); // ui::NativeThemeObserver: @@ -2307,9 +2307,10 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents, // See https://fullscreen.spec.whatwg.org. std::set fullscreen_frames_; - // Store the frame that is currently fullscreen, nullptr if there is none. - raw_ptr current_fullscreen_frame_ = - nullptr; + // Store an ID for the frame that is currently fullscreen, or an invalid ID if + // there is none. + GlobalRenderFrameHostId current_fullscreen_frame_id_ = + GlobalRenderFrameHostId(); // Whether location bar should be focused by default. This is computed in // DidStartNavigation/DidFinishNavigation and only set for an initial diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc index 2c5ade01686212..a15e0cc3565615 100644 --- a/content/browser/web_contents/web_contents_impl_browsertest.cc +++ b/content/browser/web_contents/web_contents_impl_browsertest.cc @@ -52,6 +52,7 @@ #include "content/public/browser/back_forward_cache.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/file_select_listener.h" +#include "content/public/browser/global_routing_id.h" #include "content/public/browser/host_zoom_map.h" #include "content/public/browser/invalidate_type.h" #include "content/public/browser/javascript_dialog_manager.h" @@ -159,7 +160,7 @@ class WebContentsImplBrowserTest : public ContentBrowserTest { bool IsInFullscreen() { WebContentsImpl* web_contents = static_cast(shell()->web_contents()); - return web_contents->current_fullscreen_frame_; + return !!web_contents->current_fullscreen_frame_id_; } protected: @@ -3827,7 +3828,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, NotifyFullscreenAcquired) { fullscreen_frames.insert(main_frame); EXPECT_EQ(fullscreen_frames, web_contents->fullscreen_frames_); - EXPECT_EQ(main_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(main_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); // Make the child frame fullscreen. { @@ -3839,7 +3841,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, NotifyFullscreenAcquired) { fullscreen_frames.insert(child_frame); EXPECT_EQ(fullscreen_frames, web_contents->fullscreen_frames_); - EXPECT_EQ(child_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(child_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); // Exit fullscreen on the child frame. // This will not work with --site-per-process until crbug.com/617369 @@ -3853,7 +3856,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, NotifyFullscreenAcquired) { fullscreen_frames.erase(child_frame); EXPECT_EQ(fullscreen_frames, web_contents->fullscreen_frames_); - EXPECT_EQ(main_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(main_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); } } @@ -3951,7 +3955,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, nodes.insert(main_frame); EXPECT_EQ(nodes, web_contents->fullscreen_frames_); - EXPECT_EQ(main_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(main_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); // Make the child frame fullscreen. { @@ -3963,7 +3968,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, nodes.insert(child_frame); EXPECT_EQ(nodes, web_contents->fullscreen_frames_); - EXPECT_EQ(child_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(child_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); // Perform a cross origin navigation on the main frame. EXPECT_TRUE( @@ -4000,7 +4006,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, fullscreen_frames.insert(main_frame); EXPECT_EQ(fullscreen_frames, web_contents->fullscreen_frames_); - EXPECT_EQ(main_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(main_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); // Make the child frame fullscreen. { @@ -4012,7 +4019,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, fullscreen_frames.insert(child_frame); EXPECT_EQ(fullscreen_frames, web_contents->fullscreen_frames_); - EXPECT_EQ(child_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(child_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); // Exit fullscreen on the child frame. { @@ -4023,7 +4031,8 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, fullscreen_frames.erase(child_frame); EXPECT_EQ(fullscreen_frames, web_contents->fullscreen_frames_); - EXPECT_EQ(main_frame, web_contents->current_fullscreen_frame_); + EXPECT_EQ(main_frame->GetGlobalId(), + web_contents->current_fullscreen_frame_id_); } IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, PropagateFullscreenOptions) {