Skip to content

Commit def42ea

Browse files
khushalsagarmibrunin
authored andcommitted
[Backport] Dependency for CVE-2024-11116
Manual backport of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/5381283: view-transition: Add paint holding for same-origin cross-RFH navigations For a same-origin navigation within the same RFH, there is no change in the SurfaceID embedded by the parent frame. This means when the navigation commits, redraws by the embedder display painted content from the previous Document until the new Document is ready to render. However for same-origin navigations which are cross-RFH, i.e. the default path for RenderDocument, the SurfaceID embedded by the parent frame is changed to point to the new RFH's FrameSinkId. This means there is a flash of no content until the new Document paints. Avoid the above by using the old Document's SurfaceID as a fallback with a timeout. The approach is similar to the fallback Surface logic in the browser process for main frame navigations. R=rakina@chromium.org, vmpstr@chromium.org Bug: 330920526 Change-Id: Iec7d0a3bc5b8d5d9191f935a8ea20c1f08f2cefd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5381283 Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Auto-Submit: Khushal Sagar <khushalsagar@chromium.org> Commit-Queue: Khushal Sagar <khushalsagar@chromium.org> Cr-Commit-Position: refs/heads/main@{#1292531} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/604271 Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io> Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/607612
1 parent 61ef2cc commit def42ea

19 files changed

+188
-51
lines changed

chromium/content/browser/renderer_host/cross_process_frame_connector.cc

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,11 @@ CrossProcessFrameConnector::~CrossProcessFrameConnector() {
6060
}
6161

6262
// Notify the view of this object being destroyed, if the view still exists.
63-
SetView(nullptr);
63+
SetView(nullptr, /*allow_paint_holding=*/false);
6464
}
6565

66-
void CrossProcessFrameConnector::SetView(RenderWidgetHostViewChildFrame* view) {
66+
void CrossProcessFrameConnector::SetView(RenderWidgetHostViewChildFrame* view,
67+
bool allow_paint_holding) {
6768
// Detach ourselves from the previous |view_|.
6869
if (view_) {
6970
RenderWidgetHostViewBase* root_view = GetRootRenderWidgetHostView();
@@ -110,7 +111,7 @@ void CrossProcessFrameConnector::SetView(RenderWidgetHostViewChildFrame* view) {
110111
if (frame_proxy_in_parent_renderer_ &&
111112
frame_proxy_in_parent_renderer_->is_render_frame_proxy_live()) {
112113
frame_proxy_in_parent_renderer_->GetAssociatedRemoteFrame()
113-
->SetFrameSinkId(view_->GetFrameSinkId());
114+
->SetFrameSinkId(view_->GetFrameSinkId(), allow_paint_holding);
114115
}
115116
}
116117
}

chromium/content/browser/renderer_host/cross_process_frame_connector.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ class CONTENT_EXPORT CrossProcessFrameConnector {
100100
// above.
101101
RenderWidgetHostViewChildFrame* get_view_for_testing() { return view_; }
102102

103-
void SetView(RenderWidgetHostViewChildFrame* view);
103+
void SetView(RenderWidgetHostViewChildFrame* view, bool allow_paint_holding);
104104

105105
// Returns the parent RenderWidgetHostView or nullptr if it doesn't have one.
106106
virtual RenderWidgetHostViewBase* GetParentRenderWidgetHostView();

chromium/content/browser/renderer_host/navigator.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,10 @@ void Navigator::DidNavigate(
516516
// Store this information before DidNavigateFrame() potentially swaps RFHs.
517517
url::Origin old_frame_origin = old_frame_host->GetLastCommittedOrigin();
518518

519+
// Only allow paint holding for same-origin navigations.
520+
const bool allow_subframe_paint_holding =
521+
old_frame_origin.IsSameOriginWith(params.origin);
522+
519523
// DidNavigateFrame() must be called before replicating the new origin and
520524
// other properties to proxies. This is because it destroys the subframes of
521525
// the frame we're navigating from, which might trigger those subframes to
@@ -526,7 +530,8 @@ void Navigator::DidNavigate(
526530
was_within_same_document,
527531
navigation_request->browsing_context_group_swap()
528532
.ShouldClearProxiesOnCommit(),
529-
navigation_request->commit_params().frame_policy);
533+
navigation_request->commit_params().frame_policy,
534+
allow_subframe_paint_holding);
530535

531536
// The main frame, same site, and cross-site navigation checks for user
532537
// activation mirror the checks in DocumentLoader::CommitNavigation() (note:

chromium/content/browser/renderer_host/render_frame_host_impl.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8546,7 +8546,8 @@ void RenderFrameHostImpl::AdoptPortal(
85468546
->render_manager()
85478547
->GetRenderWidgetHostView()
85488548
->GetFrameSinkId();
8549-
proxy_host->GetAssociatedRemoteFrame()->SetFrameSinkId(frame_sink_id);
8549+
// generally disallow paint holding for security reasons
8550+
proxy_host->GetAssociatedRemoteFrame()->SetFrameSinkId(frame_sink_id, /*allow_paint_holding*/ false);
85508551

85518552
std::move(callback).Run(
85528553
proxy_host->frame_tree_node()->current_replication_state().Clone(),

chromium/content/browser/renderer_host/render_frame_host_manager.cc

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -731,10 +731,11 @@ void RenderFrameHostManager::DidNavigateFrame(
731731
bool was_caused_by_user_gesture,
732732
bool is_same_document_navigation,
733733
bool clear_proxies_on_commit,
734-
const blink::FramePolicy& frame_policy) {
734+
const blink::FramePolicy& frame_policy,
735+
bool allow_subframe_paint_holding) {
735736
CommitPendingIfNecessary(render_frame_host, was_caused_by_user_gesture,
736-
is_same_document_navigation,
737-
clear_proxies_on_commit);
737+
is_same_document_navigation, clear_proxies_on_commit,
738+
allow_subframe_paint_holding);
738739

739740
// Make sure any dynamic changes to this frame's sandbox flags and permissions
740741
// policy that were made prior to navigation take effect. This should only
@@ -770,7 +771,8 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
770771
RenderFrameHostImpl* render_frame_host,
771772
bool was_caused_by_user_gesture,
772773
bool is_same_document_navigation,
773-
bool clear_proxies_on_commit) {
774+
bool clear_proxies_on_commit,
775+
bool allow_subframe_paint_holding) {
774776
if (!speculative_render_frame_host_) {
775777
// There's no speculative RenderFrameHost so it must be that the current
776778
// RenderFrameHost completed a navigation.
@@ -784,7 +786,8 @@ void RenderFrameHostManager::CommitPendingIfNecessary(
784786
if (render_frame_host == speculative_render_frame_host_.get()) {
785787
// A cross-RenderFrameHost navigation completed, so show the new renderer.
786788
CommitPending(std::move(speculative_render_frame_host_),
787-
std::move(stored_page_to_restore_), clear_proxies_on_commit);
789+
std::move(stored_page_to_restore_), clear_proxies_on_commit,
790+
allow_subframe_paint_holding);
788791

789792
if (GetNavigationQueueingFeatureLevel() >=
790793
NavigationQueueingFeatureLevel::kAvoidRedundantCancellations) {
@@ -1467,7 +1470,8 @@ void RenderFrameHostManager::PerformEarlyRenderFrameHostSwapIfNeeded(
14671470

14681471
CommitPending(
14691472
std::move(speculative_render_frame_host_), nullptr,
1470-
request->browsing_context_group_swap().ShouldClearProxiesOnCommit());
1473+
request->browsing_context_group_swap().ShouldClearProxiesOnCommit(),
1474+
/* allow_subframe_paint_holding */ false);
14711475
request->SetAssociatedRFHType(
14721476
NavigationRequest::AssociatedRenderFrameHostType::CURRENT);
14731477

@@ -4028,7 +4032,8 @@ void RenderFrameHostManager::SetRWHViewForInnerFrameTree(
40284032
RenderWidgetHostViewChildFrame* child_rwhv) {
40294033
DCHECK(IsMainFrameForInnerDelegate());
40304034
DCHECK(GetProxyToOuterDelegate());
4031-
GetProxyToOuterDelegate()->SetChildRWHView(child_rwhv, nullptr);
4035+
GetProxyToOuterDelegate()->SetChildRWHView(child_rwhv, nullptr,
4036+
/*allow_paint_holding=*/false);
40324037
}
40334038

40344039
bool RenderFrameHostManager::InitRenderView(
@@ -4340,7 +4345,8 @@ RenderFrameHostManager::GetFrameTokenForSiteInstanceGroup(
43404345
void RenderFrameHostManager::CommitPending(
43414346
std::unique_ptr<RenderFrameHostImpl> pending_rfh,
43424347
std::unique_ptr<StoredPage> pending_stored_page,
4343-
bool clear_proxies_on_commit) {
4348+
bool clear_proxies_on_commit,
4349+
bool allow_subframe_paint_holding) {
43444350
TRACE_EVENT1("navigation", "RenderFrameHostManager::CommitPending",
43454351
"FrameTreeNode id", frame_tree_node_->frame_tree_node_id());
43464352
CHECK(pending_rfh);
@@ -4730,7 +4736,7 @@ void RenderFrameHostManager::CommitPending(
47304736
if (proxy_to_parent_or_outer_delegate) {
47314737
proxy_to_parent_or_outer_delegate->SetChildRWHView(
47324738
static_cast<RenderWidgetHostViewChildFrame*>(new_view),
4733-
old_size ? &*old_size : nullptr);
4739+
old_size ? &*old_size : nullptr, allow_subframe_paint_holding);
47344740
}
47354741

47364742
if (render_frame_host_->is_local_root()) {
@@ -5136,8 +5142,10 @@ void RenderFrameHostManager::CreateNewFrameForInnerDelegateAttachIfNecessary() {
51365142
// Swap in the speculative frame. It will later be replaced when
51375143
// WebContents::AttachToOuterWebContentsFrame is called.
51385144
speculative_render_frame_host_->SwapIn();
5145+
51395146
CommitPending(std::move(speculative_render_frame_host_), nullptr,
5140-
false /* clear_proxies_on_commit */);
5147+
false /* clear_proxies_on_commit */,
5148+
/* allow_subframe_paint_holding */ false);
51415149
NotifyPrepareForInnerDelegateAttachComplete(true /* success */);
51425150
}
51435151

chromium/content/browser/renderer_host/render_frame_host_manager.h

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,8 @@ class CONTENT_EXPORT RenderFrameHostManager {
322322
bool was_caused_by_user_gesture,
323323
bool is_same_document_navigation,
324324
bool clear_proxies_on_commit,
325-
const blink::FramePolicy& frame_policy);
325+
const blink::FramePolicy& frame_policy,
326+
bool allow_subframe_paint_holding);
326327

327328
// Called when this frame's opener is changed to the frame specified by
328329
// |opener_frame_token| in |source_site_instance_group|'s process. This
@@ -971,15 +972,19 @@ class CONTENT_EXPORT RenderFrameHostManager {
971972
// |clear_proxies_on_commit| Indicates if the proxies and opener must be
972973
// removed during the commit. This can happen following some BrowsingInstance
973974
// swaps, such as those for COOP.
975+
// |allow_subframe_paint_holding| Indicates that paint holding is allowed if
976+
// this is a subframe navigation.
974977
void CommitPending(std::unique_ptr<RenderFrameHostImpl> pending_rfh,
975978
std::unique_ptr<StoredPage> pending_stored_page,
976-
bool clear_proxies_on_commit);
979+
bool clear_proxies_on_commit,
980+
bool allow_subframe_paint_holding);
977981

978982
// Helper to call CommitPending() in all necessary cases.
979983
void CommitPendingIfNecessary(RenderFrameHostImpl* render_frame_host,
980984
bool was_caused_by_user_gesture,
981985
bool is_same_document_navigation,
982-
bool clear_proxies_on_commit);
986+
bool clear_proxies_on_commit,
987+
bool allow_subframe_paint_holding);
983988

984989
// Runs the unload handler in the old RenderFrameHost, after the new
985990
// RenderFrameHost has committed. |old_render_frame_host| will either be

chromium/content/browser/renderer_host/render_frame_proxy_host.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -192,10 +192,10 @@ RenderFrameProxyHost::~RenderFrameProxyHost() {
192192
TRACE_EVENT_END("navigation", perfetto::Track::FromPointer(this));
193193
}
194194

195-
void RenderFrameProxyHost::SetChildRWHView(
196-
RenderWidgetHostViewChildFrame* view,
197-
const gfx::Size* initial_frame_size) {
198-
cross_process_frame_connector_->SetView(view);
195+
void RenderFrameProxyHost::SetChildRWHView(RenderWidgetHostViewChildFrame* view,
196+
const gfx::Size* initial_frame_size,
197+
bool allow_paint_holding) {
198+
cross_process_frame_connector_->SetView(view, allow_paint_holding);
199199
if (initial_frame_size)
200200
cross_process_frame_connector_->SetLocalFrameSize(*initial_frame_size);
201201
}

chromium/content/browser/renderer_host/render_frame_proxy_host.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,8 @@ class CONTENT_EXPORT RenderFrameProxyHost
164164
// receives its size from the parent via FrameHostMsg_UpdateResizeParams
165165
// before it begins parsing the content.
166166
void SetChildRWHView(RenderWidgetHostViewChildFrame* view,
167-
const gfx::Size* initial_frame_size);
167+
const gfx::Size* initial_frame_size,
168+
bool allow_paint_holding);
168169

169170
RenderViewHostImpl* GetRenderViewHost();
170171

chromium/content/browser/renderer_host/render_widget_host_impl.cc

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@
117117
#include "third_party/blink/public/common/input/synthetic_web_input_event_builders.h"
118118
#include "third_party/blink/public/common/storage_key/storage_key.h"
119119
#include "third_party/blink/public/common/web_preferences/web_preferences.h"
120+
#include "third_party/blink/public/common/widget/constants.h"
120121
#include "third_party/blink/public/common/widget/visual_properties.h"
121122
#include "third_party/blink/public/mojom/drag/drag.mojom.h"
122123
#include "third_party/blink/public/mojom/frame/intrinsic_sizing_info.mojom.h"
@@ -165,10 +166,6 @@ using blink::WebMouseWheelEvent;
165166
namespace content {
166167
namespace {
167168

168-
// How long to wait for newly loaded content to send a compositor frame
169-
// before clearing previously displayed graphics.
170-
constexpr base::TimeDelta kNewContentRenderingDelay = base::Seconds(4);
171-
172169
constexpr gfx::Rect kInvalidScreenRect(std::numeric_limits<int>::max(),
173170
std::numeric_limits<int>::max(),
174171
0,
@@ -438,7 +435,7 @@ RenderWidgetHostImpl::RenderWidgetHostImpl(
438435
switches::kDisableHangMonitor)),
439436
latency_tracker_(delegate_),
440437
hung_renderer_delay_(kHungRendererDelay),
441-
new_content_rendering_delay_(kNewContentRenderingDelay),
438+
new_content_rendering_delay_(blink::kNewContentRenderingDelay),
442439
frame_token_message_queue_(std::move(frame_token_message_queue)),
443440
render_frame_metadata_provider_(
444441
#if BUILDFLAG(IS_MAC)

chromium/content/browser/renderer_host/render_widget_host_view_child_frame.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,7 @@ void RenderWidgetHostViewChildFrame::Destroy() {
406406
// have already been cleared when RenderWidgetHostViewBase notified its
407407
// observers of our impending destruction.
408408
if (frame_connector_) {
409-
frame_connector_->SetView(nullptr);
409+
frame_connector_->SetView(nullptr, /*allow_paint_holding=*/false);
410410
SetFrameConnector(nullptr);
411411
}
412412

0 commit comments

Comments
 (0)