diff --git a/content/browser/renderer_host/render_frame_host_impl_unittest.cc b/content/browser/renderer_host/render_frame_host_impl_unittest.cc index dc1075cef5a32..3cfb19f4fa957 100644 --- a/content/browser/renderer_host/render_frame_host_impl_unittest.cc +++ b/content/browser/renderer_host/render_frame_host_impl_unittest.cc @@ -27,6 +27,7 @@ #include "content/test/test_render_frame_host.h" #include "content/test/test_render_view_host.h" #include "content/test/test_web_contents.h" +#include "content/test/web_contents_observer_consistency_checker.h" #include "net/base/features.h" #include "net/base/isolation_info.h" #include "net/base/network_isolation_partition.h" @@ -236,6 +237,75 @@ TEST_F(RenderFrameHostImplTest, ExitFullscreenDestruction) { main_test_rfh()->ExitFullscreen(); } +// On macOS, WebContentsDelegate::ExitFullscreenModeForTab can spin a nested +// AppKit run loop ([NSWindow toggleFullScreen:]) which may process a queued +// WebContents-destruction task. When that happens during a navigation commit, +// the WeakPtr early-return in Navigator::DidNavigate destroys the by-value +// std::unique_ptr parameter *after* the WebContentsImpl +// (and the embedded root FrameTreeNode) has already been freed. +// ~NavigationRequest then dereferences its RAW_PTR_EXCLUSION +// `frame_tree_node_` pointer into the freed allocation. +TEST_F(RenderFrameHostImplTest, ExitFullscreenDestructionDuringCommit) { + class DestructionDelegate : public WebContentsDelegate { + public: + explicit DestructionDelegate(base::OnceClosure destruction_closure) + : destruction_closure_(std::move(destruction_closure)) {} + void ExitFullscreenModeForTab(WebContents* web_contents) override { + // Simulates a nested run loop processing a queued tab-close task. + if (destruction_closure_) { + std::move(destruction_closure_).Run(); + } + } + bool IsFullscreenForTabOrPending(const WebContents*) override { + return is_fullscreen_; + } + void set_fullscreen(bool v) { is_fullscreen_ = v; } + + private: + bool is_fullscreen_ = false; + base::OnceClosure destruction_closure_; + }; + + // Commit an initial page so the next navigation is a real cross-document + // commit through Navigator::DidNavigate. + NavigateAndCommit(GURL("https://a.test/")); + + DestructionDelegate delegate(base::BindOnce( + &RenderFrameHostImplTest::DeleteContents, base::Unretained(this))); + contents()->SetDelegate(&delegate); + + // Bring a cross-document main-frame navigation to READY_TO_COMMIT so the + // NavigationRequest has been moved into the committing RFH's + // navigation_requests_ map. + std::unique_ptr sim = + NavigationSimulator::CreateBrowserInitiated(GURL("https://b.test/"), + contents()); + sim->ReadyToCommit(); + + // Mark the tab fullscreen so DidNavigateAnyFramePreCommit calls + // ExitFullscreen() during the commit. + delegate.set_fullscreen(true); + + // The test-only consistency checker CHECKs that no navigation is ongoing + // when WebContentsDestroyed() fires. That invariant does not hold in + // production for this scenario (the NavigationRequest survives on the + // Navigator::DidNavigate stack frame), so detach it to expose the + // production behaviour. + WebContentsObserverConsistencyChecker::Disable(contents()); + + // Commit: RFHI::DidCommitNavigation extracts the NavigationRequest and + // moves it (by value) into Navigator::DidNavigate, which calls + // WebContentsImpl::DidNavigateAnyFramePreCommit -> ExitFullscreen -> + // delegate->ExitFullscreenModeForTab, which deletes the WebContents + // (including the embedded root FrameTreeNode and Navigator). The three + // WeakPtr guards then early-return out of Navigator::DidNavigate, + // destroying the unique_ptr. ~NavigationRequest reads + // frame_tree_node_->navigation_request() and later + // GetDelegate()->DidFinishNavigation(this), both of which dereference + // freed memory. + sim->Commit(); +} + // Ensures that IsolationInfo's SiteForCookies is empty and // that it correctly generates a StorageKey with a kCrossSite // AncestorChainBit when frames are nested in an A->B->A diff --git a/content/test/web_contents_observer_consistency_checker.cc b/content/test/web_contents_observer_consistency_checker.cc index b0e9ab72c341b..eb70608a797b1 100644 --- a/content/test/web_contents_observer_consistency_checker.cc +++ b/content/test/web_contents_observer_consistency_checker.cc @@ -58,6 +58,23 @@ void WebContentsObserverConsistencyChecker::Enable(WebContents* web_contents) { new WebContentsObserverConsistencyChecker(web_contents))); } +// static +void WebContentsObserverConsistencyChecker::Disable(WebContents* web_contents) { + auto* checker = static_cast( + web_contents->GetUserData(&kWebContentsObserverConsistencyCheckerKey)); + if (!checker) { + return; + } + // Detach as observer and clear state so the destructor's invariants pass. + // Tests calling Disable() are intentionally exercising scenarios that + // violate the usual observer ordering (e.g. WebContents destruction inside a + // nested run loop while a navigation is mid-commit). + checker->Observe(nullptr); + checker->web_contents_destroyed_ = true; + checker->ready_to_commit_hosts_.clear(); + web_contents->RemoveUserData(&kWebContentsObserverConsistencyCheckerKey); +} + void WebContentsObserverConsistencyChecker::RenderFrameCreated( RenderFrameHost* render_frame_host) { CHECK(!web_contents_destroyed_); diff --git a/content/test/web_contents_observer_consistency_checker.h b/content/test/web_contents_observer_consistency_checker.h index 57d974f00bbea..9caf20c89dd60 100644 --- a/content/test/web_contents_observer_consistency_checker.h +++ b/content/test/web_contents_observer_consistency_checker.h @@ -46,6 +46,11 @@ class WebContentsObserverConsistencyChecker // ContentBrowserConsistencyChecker should call this for you. static void Enable(WebContents* web_contents); + // Removes the checker from |web_contents|. Only for tests that exercise + // production-only code paths whose invariants this checker would otherwise + // mask (e.g. WebContents destruction inside a nested run loop). + static void Disable(WebContents* web_contents); + // WebContentsObserver implementation. void RenderFrameCreated(RenderFrameHost* render_frame_host) override; void RenderFrameDeleted(RenderFrameHost* render_frame_host) override;