Revert "Fenced Frames: Navigations always replace the current entry"

This reverts commit 5e37873. Reason for revert: Appears to be breaking the tree https://ci.chromium.org/ui/p/chromium/builders/ci/Mac%20Builder%20(dbg)/202959/blamelist Original change's description: > Fenced Frames: Navigations always replace the current entry > > Background: > Fenced frames can navigate themselves but their history is not part of > the browser back/forward list as that could be a communication channel > from the fenced frame to the embedding page. This aligns with MPArch's > disjoint back/forward list for nested frame trees. (For shadowDOM, this > would be achieved with additional API level restrictions like > history.length always returning 1 etc.) > > Current Change: > This CL focuses on fenced frames to always have a replacement-only > navigation which was decided due to being a simpler model since it > doesn't imply that there's a hidden list of back/forward entries for > the nested page, only accessible via history APIs and not via the > back/forward buttons. This is also consistent with the iframes new > opt-in mode for disjoint session history as discussed in > whatwg/html#6501. > This change affects both shadowDOM and MPArch versions. > > Design: > https://docs.google.com/document/d/17rtX55WkxMcfh6ipuhP4mNULIVxUApvYt4ZYXfX2x-s/edit#heading=h.af2cik2j1rbs > > This CL includes browser tests to check the NavigationController's entry > count and > https://chromium-review.googlesource.com/c/chromium/src/+/3227344 > added WPTs for all the history API surface. > > > Bug: 1242533 > Change-Id: Ic574ee1bf87ce3a53dde7d280abaa46233d85b0d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3216452 > Reviewed-by: Reilly Grant <reillyg@chromium.org> > Reviewed-by: Dave Tapuska <dtapuska@chromium.org> > Reviewed-by: Rakina Zata Amni <rakina@chromium.org> > Reviewed-by: Jeremy Roman <jbroman@chromium.org> > Reviewed-by: Lei Zhang <thestig@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Commit-Queue: Dave Tapuska <dtapuska@chromium.org> > Commit-Queue: Shivani Sharma <shivanisha@chromium.org> > Cr-Commit-Position: refs/heads/main@{#938761} Bug: 1242533 Change-Id: Ib946f629447de53164e1d335cf4b983e1fbdaa35 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3260236 Auto-Submit: anthonyvd <anthonyvd@chromium.org> Owners-Override: anthonyvd <anthonyvd@chromium.org> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/main@{#938770}
blueboxd · Nov 5, 2021 · 367479e · 367479e
1 parent b5d85de
commit 367479e
Show file tree

Hide file tree

Showing 20 changed files with 101 additions and 388 deletions.
diff --git a/components/plugins/renderer/webview_plugin.cc b/components/plugins/renderer/webview_plugin.cc
@@ -272,7 +272,6 @@ WebViewPlugin::WebViewHelper::WebViewHelper(
                       /*is_hidden=*/false,
                       /*is_prerendering=*/false,
                       /*is_inside_portal=*/false,
-                      /*is_fenced_frame=*/false,
                       /*compositing_enabled=*/false,
                       /*widgets_never_composited=*/false,
                       /*opener=*/nullptr, mojo::NullAssociatedReceiver(),

diff --git a/components/printing/renderer/print_render_frame_helper.cc b/components/printing/renderer/print_render_frame_helper.cc
@@ -708,7 +708,6 @@ void PrintRenderFrameHelper::PrintHeaderAndFooter(
       /*client=*/nullptr,
       /*is_hidden=*/false, /*is_prerendering=*/false,
       /*is_inside_portal=*/false,
-      /*is_fenced_frame=*/false,
       /*compositing_enabled=*/false, /*widgets_never_composited=*/false,
       /*opener=*/nullptr, mojo::NullAssociatedReceiver(),
       *source_frame.GetAgentGroupScheduler(),
@@ -989,7 +988,6 @@ void PrepareFrameAndViewForPrint::CopySelection(
       /*is_hidden=*/false,
       /*is_prerendering=*/false,
       /*is_inside_portal=*/false,
-      /*is_fenced_frame=*/false,
       /*compositing_enabled=*/false,
       /*widgets_never_composited=*/false,
       /*opener=*/nullptr, mojo::NullAssociatedReceiver(),

diff --git a/content/browser/fenced_frame/fenced_frame.cc b/content/browser/fenced_frame/fenced_frame.cc
@@ -78,7 +78,7 @@ void FencedFrame::Navigate(const GURL& url) {
       owner_render_frame_host_->GetLastCommittedOrigin(),
       owner_render_frame_host_->GetSiteInstance(), content::Referrer(),
       ui::PAGE_TRANSITION_LINK,
-      /*should_replace_current_entry=*/true, download_policy, "GET",
+      /*should_replace_current_entry=*/false, download_policy, "GET",
       /*post_body=*/nullptr, /*extra_headers=*/"",
       /*blob_url_loader_factory=*/nullptr,
       network::mojom::SourceLocation::New(), /*has_user_gesture=*/false,

diff --git a/content/browser/renderer_host/frame_tree_browsertest.cc b/content/browser/renderer_host/frame_tree_browsertest.cc
diff --git a/content/browser/renderer_host/navigation_controller_impl.cc b/content/browser/renderer_host/navigation_controller_impl.cc
@@ -1137,8 +1137,7 @@ bool NavigationControllerImpl::RendererDidNavigate(
     bool previous_document_was_activated,
     NavigationRequest* navigation_request) {
   DCHECK(navigation_request);
-  if (ShouldMaintainTrivialSessionHistory(rfh->frame_tree_node()) &&
-      GetLastCommittedEntry()) {
+  if (ShouldMaintainTrivialSessionHistory() && GetLastCommittedEntry()) {
     // Ensure that this navigation does not add a navigation entry, since
     // ShouldMaintainTrivialSessionHistory() means we should not add an entry
     // beyond the last committed one. Therefore, `should_replace_current_entry`
@@ -3140,10 +3139,9 @@ base::WeakPtr<NavigationHandle> NavigationControllerImpl::NavigateWithoutEntry(
   //
   // If there is an entry, an entry replacement must happen if the current
   // browsing context should maintain a trivial session history.
-  bool should_replace_current_entry =
-      (params.should_replace_current_entry ||
-       ShouldMaintainTrivialSessionHistory(node)) &&
-      entries_.size();
+  bool should_replace_current_entry = (params.should_replace_current_entry ||
+                                       ShouldMaintainTrivialSessionHistory()) &&
+                                      entries_.size();
 
   // Javascript URLs should not create NavigationEntries. All other navigations
   // do, including navigations to chrome renderer debug URLs.
@@ -4259,13 +4257,11 @@ void NavigationControllerImpl::NavigateToAppHistoryKey(FrameTreeNode* node,
   }
 }
 
-bool NavigationControllerImpl::ShouldMaintainTrivialSessionHistory(
-    const FrameTreeNode* frame_tree_node) const {
-  // TODO(https://crbug.com/1197384): We may have to add portals in addition to
-  // prerender and fenced frames. This should be kept in sync with
+bool NavigationControllerImpl::ShouldMaintainTrivialSessionHistory() const {
+  // TODO(https://crbug.com/1197384): We may have to add portals and fenced
+  // frames in addition to prerender. This should be kept in sync with
   // LocalFrame version, LocalFrame::ShouldMaintainTrivialSessionHistory.
-  return frame_tree_.is_prerendering() ||
-         frame_tree_node->IsInFencedFrameTree();
+  return frame_tree_.is_prerendering();
 }
 
 }  // namespace content
diff --git a/content/browser/renderer_host/navigation_controller_impl.h b/content/browser/renderer_host/navigation_controller_impl.h
@@ -725,17 +725,12 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
       FrameNavigationEntry* target_entry,
       const std::string& app_history_key);
 
-  // Whether to maintain a session history with just one entry.
+  // Whether to maintain a trivial session history.
   //
-  // This returns true for a prerendering page and for fenced frames.
-  // `frame_tree_node` is checked to see if it belongs to a frame tree for
-  // prerendering or for a fenced frame.
+  // One example is prerender.
   // Explainer:
-  // https://github.com/jeremyroman/alternate-loading-modes/blob/main/browsing-context.md#session-history)
-  //
-  // Portals will be added to this in the future.
-  bool ShouldMaintainTrivialSessionHistory(
-      const FrameTreeNode* frame_tree_node) const;
+  // https://github.com/jeremyroman/alternate-loading-modes/blob/main/browsing-context.md#session-history
+  bool ShouldMaintainTrivialSessionHistory() const;
 
   // ---------------------------------------------------------------------------
 

diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
@@ -121,7 +121,6 @@ void RenderViewImpl::Initialize(
   webview_ = WebView::Create(
       this, params->hidden, params->is_prerendering,
       params->type == mojom::ViewWidgetType::kPortal ? true : false,
-      params->type == mojom::ViewWidgetType::kFencedFrame ? true : false,
       /*compositing_enabled=*/true, params->never_composited,
       opener_frame ? opener_frame->View() : nullptr,
       std::move(params->blink_page_broadcast),

diff --git a/content/test/data/fenced_frames/basic_fenced_frame_src.html b/content/test/data/fenced_frames/basic_fenced_frame_src.html
diff --git a/extensions/renderer/scoped_web_frame.cc b/extensions/renderer/scoped_web_frame.cc
@@ -23,7 +23,6 @@ ScopedWebFrame::ScopedWebFrame()
           /*is_hidden=*/false,
           /*is_prerendering=*/false,
           /*is_inside_portal=*/false,
-          /*is_fenced_frame=*/false,
           /*compositing_enabled=*/false,
           /*widgets_never_composited=*/false,
           /*opener=*/nullptr,

diff --git a/third_party/blink/public/web/web_view.h b/third_party/blink/public/web/web_view.h
@@ -101,7 +101,6 @@ class WebView {
   // |is_prerendering| defines whether the page is being prerendered by the
   // Prerender2 feature (see content/browser/prerender/README.md).
   // [is_inside_portal] defines whether the page is inside_portal.
-  // [is_fenced_frame] defines whether the page is for a fenced frame.
   // |compositing_enabled| dictates whether accelerated compositing should be
   // enabled for the page. It must be false if no clients are provided, or if a
   // LayerTreeView will not be set for the WebWidget.
@@ -126,7 +125,6 @@ class WebView {
       bool is_hidden,
       bool is_prerendering,
       bool is_inside_portal,
-      bool is_fenced_frame,
       bool compositing_enabled,
       bool widgets_never_composited,
       WebView* opener,

diff --git a/third_party/blink/renderer/core/exported/web_view_impl.cc b/third_party/blink/renderer/core/exported/web_view_impl.cc
@@ -448,7 +448,6 @@ WebView* WebView::Create(
     bool is_hidden,
     bool is_prerendering,
     bool is_inside_portal,
-    bool is_fenced_frame,
     bool compositing_enabled,
     bool widgets_never_composited,
     WebView* opener,
@@ -461,7 +460,7 @@ WebView* WebView::Create(
       client,
       is_hidden ? mojom::blink::PageVisibilityState::kHidden
                 : mojom::blink::PageVisibilityState::kVisible,
-      is_prerendering, is_inside_portal, is_fenced_frame, compositing_enabled,
+      is_prerendering, is_inside_portal, compositing_enabled,
       widgets_never_composited, To<WebViewImpl>(opener), std::move(page_handle),
       agent_group_scheduler, session_storage_namespace_id,
       std::move(page_base_background_color));
@@ -472,7 +471,6 @@ WebViewImpl* WebViewImpl::Create(
     mojom::blink::PageVisibilityState visibility,
     bool is_prerendering,
     bool is_inside_portal,
-    bool is_fenced_frame,
     bool compositing_enabled,
     bool widgets_never_composited,
     WebViewImpl* opener,
@@ -483,7 +481,7 @@ WebViewImpl* WebViewImpl::Create(
   // Take a self-reference for WebViewImpl that is released by calling Close(),
   // then return a raw pointer to the caller.
   auto web_view = base::AdoptRef(new WebViewImpl(
-      client, visibility, is_prerendering, is_inside_portal, is_fenced_frame,
+      client, visibility, is_prerendering, is_inside_portal,
       compositing_enabled, widgets_never_composited, opener,
       std::move(page_handle), agent_group_scheduler,
       session_storage_namespace_id, std::move(page_base_background_color)));
@@ -546,7 +544,6 @@ WebViewImpl::WebViewImpl(
     mojom::blink::PageVisibilityState visibility,
     bool is_prerendering,
     bool is_inside_portal,
-    bool is_fenced_frame,
     bool does_composite,
     bool widgets_never_composited,
     WebViewImpl* opener,
@@ -582,11 +579,6 @@ WebViewImpl::WebViewImpl(
   // page.
   SetInsidePortal(is_inside_portal);
 
-  if (is_fenced_frame && features::IsFencedFramesEnabled() &&
-      features::IsFencedFramesMPArchBased()) {
-    page_->SetIsMainFrameFencedFrameRoot();
-  }
-
   // When not compositing, keep the Page in the loop so that it will paint all
   // content into the root layer, as multiple layers can only be used when
   // compositing them together later.

diff --git a/third_party/blink/renderer/core/exported/web_view_impl.h b/third_party/blink/renderer/core/exported/web_view_impl.h
@@ -117,7 +117,6 @@ class CORE_EXPORT WebViewImpl final : public WebView,
       mojom::blink::PageVisibilityState visibility,
       bool is_prerendering,
       bool is_inside_portal,
-      bool is_fenced_frame,
       bool compositing_enabled,
       bool widgets_never_composited,
       WebViewImpl* opener,
@@ -650,7 +649,6 @@ class CORE_EXPORT WebViewImpl final : public WebView,
       mojom::blink::PageVisibilityState visibility,
       bool is_prerendering,
       bool is_inside_portal,
-      bool is_fenced_frame,
       bool does_composite,
       bool widgets_never_composite,
       WebViewImpl* opener,

diff --git a/third_party/blink/renderer/core/exported/web_view_test.cc b/third_party/blink/renderer/core/exported/web_view_test.cc
@@ -489,7 +489,6 @@ TEST_F(WebViewTest, SetBaseBackgroundColorBeforeMainFrame) {
                       /*is_hidden=*/false,
                       /*is_prerendering=*/false,
                       /*is_inside_portal=*/false,
-                      /*is_fenced_frame=*/false,
                       /*compositing_enabled=*/true,
                       /*widgets_never_composited=*/false,
                       /*opener=*/nullptr, mojo::NullAssociatedReceiver(),
@@ -2744,7 +2743,6 @@ TEST_F(WebViewTest, ClientTapHandlingNullWebViewClient) {
   WebViewImpl* web_view = To<WebViewImpl>(WebView::Create(
       /*client=*/nullptr, /*is_hidden=*/false, /*is_prerendering=*/false,
       /*is_inside_portal=*/false,
-      /*is_fenced_frame=*/false,
       /*compositing_enabled=*/false,
       /*widgets_never_composited=*/false,
       /*opener=*/nullptr, mojo::NullAssociatedReceiver(),

diff --git a/third_party/blink/renderer/core/frame/frame_test_helpers.cc b/third_party/blink/renderer/core/frame/frame_test_helpers.cc
@@ -621,7 +621,6 @@ void WebViewHelper::InitializeWebView(TestWebViewClient* web_view_client,
                       /*is_hidden=*/false,
                       /*is_prerendering=*/false,
                       /*is_inside_portal=*/false,
-                      /*is_fenced_frame=*/false,
                       /*compositing_enabled=*/true,
                       /*widgets_never_composited=*/false,
                       /*opener=*/opener, mojo::NullAssociatedReceiver(),

diff --git a/third_party/blink/renderer/core/frame/local_frame.cc b/third_party/blink/renderer/core/frame/local_frame.cc
@@ -512,27 +512,12 @@ bool LocalFrame::NavigationShouldReplaceCurrentHistoryEntry(
 }
 
 bool LocalFrame::ShouldMaintainTrivialSessionHistory() const {
-  // This should be kept in sync with
+  // TODO(https://crbug.com/1197384): We may have to add fenced frames. This
+  // should be kept in sync with NavigationControllerImpl version,
   // NavigationControllerImpl::ShouldMaintainTrivialSessionHistory.
   //
   // TODO(mcnee): Similarly, we need to restrict orphaned contexts.
-  return GetPage()->InsidePortal() || GetDocument()->IsPrerendering() ||
-         IsInFencedFrameTree();
-}
-
-bool LocalFrame::IsInFencedFrameTree() const {
-  if (!blink::features::IsFencedFramesEnabled())
-    return false;
-
-  switch (blink::features::kFencedFramesImplementationTypeParam.Get()) {
-    case blink::features::FencedFramesImplementationType::kMPArch:
-      return GetPage()->IsMainFrameFencedFrameRoot();
-    case blink::features::FencedFramesImplementationType::kShadowDOM:
-      return Tree().Top(FrameTreeBoundary::kFenced) !=
-             Tree().Top(FrameTreeBoundary::kIgnoreFence);
-    default:
-      return false;
-  }
+  return GetPage()->InsidePortal() || GetDocument()->IsPrerendering();
 }
 
 bool LocalFrame::DetachImpl(FrameDetachType type) {

diff --git a/third_party/blink/renderer/core/frame/local_frame.h b/third_party/blink/renderer/core/frame/local_frame.h
@@ -804,13 +804,6 @@ class CORE_EXPORT LocalFrame final : public Frame,
       const FrameLoadRequest& request,
       WebFrameLoadType frame_load_type);
 
-  // Returns false if fenced frames are disabled. Returns true if the
-  // feature is enabled and if |this| or any of its ancestor nodes is a
-  // fenced frame. For MPArch returns the value of
-  // Page::IsMainFrameFencedFrameRoot and for shadowDOM returns true, if
-  // the FrameTree that this frame is in is not the outermost FrameTree.
-  bool IsInFencedFrameTree() const;
-
   std::unique_ptr<FrameScheduler> frame_scheduler_;
 
   // Holds all PauseSubresourceLoadingHandles allowing either |this| to delete

diff --git a/third_party/blink/renderer/core/page/page.cc b/third_party/blink/renderer/core/page/page.cc
@@ -1079,14 +1079,6 @@ bool Page::InsidePortal() const {
   return inside_portal_;
 }
 
-void Page::SetIsMainFrameFencedFrameRoot() {
-  is_fenced_frame_tree_ = true;
-}
-
-bool Page::IsMainFrameFencedFrameRoot() const {
-  return is_fenced_frame_tree_;
-}
-
 void Page::SetMediaFeatureOverride(const AtomicString& media_feature,
                                    const String& value) {
   if (!media_feature_overrides_) {

diff --git a/third_party/blink/renderer/core/page/page.h b/third_party/blink/renderer/core/page/page.h
@@ -385,11 +385,6 @@ class CORE_EXPORT Page final : public GarbageCollected<Page>,
   // Fully invalidate paint of all local frames in this page.
   void InvalidatePaint();
 
-  // Should be invoked when the main frame of this frame tree is a fenced frame.
-  void SetIsMainFrameFencedFrameRoot();
-  // Returns if the main frame of this frame tree is a fenced frame.
-  bool IsMainFrameFencedFrameRoot() const;
-
  private:
   friend class ScopedPagePauser;
 
@@ -511,11 +506,6 @@ class CORE_EXPORT Page final : public GarbageCollected<Page>,
   // prerender activation; it does not go from false to true.
   bool is_prerendering_ = false;
 
-  // Whether the the Page's main document is a Fenced Frame document. This is
-  // only set for the MPArch implementation and is true when the corresponding
-  // browser side FrameTree has the FrameTree::Type of kFencedFrame.
-  bool is_fenced_frame_tree_ = false;
-
   mojom::blink::TextAutosizerPageInfo web_text_autosizer_page_info_;
 
   WebScopedVirtualTimePauser history_navigation_virtual_time_pauser_;

diff --git a/third_party/blink/renderer/modules/peerconnection/webrtc_audio_renderer_test.cc b/third_party/blink/renderer/modules/peerconnection/webrtc_audio_renderer_test.cc
@@ -144,7 +144,6 @@ class MAYBE_WebRtcAudioRendererTest : public testing::Test {
             /*is_hidden=*/false,
             /*is_prerendering=*/false,
             /*is_inside_portal=*/false,
-            /*is_fenced_frame=*/false,
             /*compositing_enabled=*/false,
             /*widgets_never_composited=*/false,
             /*opener=*/nullptr,

diff --git a/...d_frame/history-length-fenced-navigations-replace-do-not-contribute-to-joint-expected.txt b/...d_frame/history-length-fenced-navigations-replace-do-not-contribute-to-joint-expected.txt
@@ -0,0 +1,4 @@
+This is a testharness.js-based test.
+FAIL All fenced navigations should be replace-only and not contribute to joint session history assert_equals: Navigations inside of a fenced frame are always replacement navigations and never increase `history.length` inside the fence: top-level-fenced-frame expected "PASS > top-level-fenced-frame" but got "FAIL > top-level-fenced-frame history.length: 10"
+Harness: the test ran to completion.
+