Add use-counter for cross-origin scroll into views

This use counter is intended to measure how often scroll into view bubbles across origins and causes scrolling to see if it can be limited to same-origin only. Bug: 1339003 Change-Id: I7d5f8b163ea18e4fad29180fd083387043acc0db Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3787280 Reviewed-by: Steve Kobes <skobes@chromium.org> Commit-Queue: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/main@{#1029341}
marcosholgado · Jul 28, 2022 · 7f17bf4 · 7f17bf4
1 parent 2a12556
commit 7f17bf4
Show file tree

Hide file tree

Showing 9 changed files with 157 additions and 5 deletions.
diff --git a/third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom b/third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
@@ -3634,6 +3634,7 @@ enum WebFeature {
   kLinkRelPrefetchAsDocumentSameOrigin = 4313,
   kLinkRelPrefetchAsDocumentCrossOrigin = 4314,
   kPersistentQuotaType = 4315,
+  kCrossOriginScrollIntoView = 4316,
 
   // Add new features immediately above this line. Don't change assigned
   // numbers of any item, and don't reuse removed slots.

diff --git a/third_party/blink/renderer/core/frame/remote_frame.cc b/third_party/blink/renderer/core/frame/remote_frame.cc
@@ -697,7 +697,8 @@ void RemoteFrame::ScrollRectToVisible(
       PhysicalRect::EnclosingRect(rect_to_scroll), owner_object->View());
 
   scroll_into_view_util::ScrollRectToVisible(*owner_object, absolute_rect,
-                                             std::move(params));
+                                             std::move(params),
+                                             /*from_remote_frame=*/true);
 }
 
 void RemoteFrame::IntrinsicSizingInfoOfChildChanged(

diff --git a/third_party/blink/renderer/core/frame/root_frame_viewport.cc b/third_party/blink/renderer/core/frame/root_frame_viewport.cc
@@ -372,6 +372,10 @@ PhysicalRect RootFrameViewport::ScrollIntoView(
   // Return the newly moved rect to absolute coordinates.
   // TODO(szager): PaintLayerScrollableArea::ScrollIntoView clips the return
   // value to the visible content rect, but this does not.
+  // TODO(bokan): This returns an unchanged rect for scroll sequences (the PLSA
+  // version correctly computes what the rect will be when the sequence is
+  // executed) and we can't just adjust by `new_scroll_offset` since, to get to
+  // absolute coordinates, we must offset by only the layout viewport's scroll.
   rect_in_document.Move(
       -PhysicalOffset::FromVector2dFRound(LayoutViewport().GetScrollOffset()));
   return rect_in_document;

diff --git a/third_party/blink/renderer/core/page/scrolling/scroll_into_view_test.cc b/third_party/blink/renderer/core/page/scrolling/scroll_into_view_test.cc
@@ -28,6 +28,7 @@
 #include "third_party/blink/renderer/core/testing/sim/sim_compositor.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_request.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_test.h"
+#include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
 
 namespace blink {
 
@@ -777,6 +778,97 @@ TEST_F(ScrollIntoViewTest, LongDistanceSmoothScrollFinishedInThreeSeconds) {
   ASSERT_EQ(Window().scrollY(), target->OffsetTop());
 }
 
+TEST_F(ScrollIntoViewTest, OriginCrossingUseCounter) {
+  v8::HandleScope HandleScope(v8::Isolate::GetCurrent());
+  WebView().MainFrameViewWidget()->Resize(gfx::Size(800, 600));
+  SimRequest main("https://example.com/test.html", "text/html");
+  SimRequest local_child("https://example.com/child.html", "text/html");
+  SimRequest xorigin_child("https://xorigin.com/child.html", "text/html");
+  LoadURL("https://example.com/test.html");
+
+  main.Complete(
+      R"HTML(
+        <!DOCTYPE html>
+        <style>
+          body {
+            width: 2000px;
+            height: 2000px;
+          }
+
+          iframe {
+            position: absolute;
+            left: 1000px;
+            top: 1200px;
+            width: 200px;
+            height: 200px;
+          }
+        </style>
+        <iframe id="localChildFrame" src="child.html"></iframe>
+        <iframe id="xoriginChildFrame" src="https://xorigin.com/child.html"></iframe>
+      )HTML");
+
+  String child_html =
+      R"HTML(
+        <!DOCTYPE html>
+        <style>
+          body {
+            width: 1000px;
+            height: 1000px;
+          }
+
+          div {
+            position: absolute;
+            left: 300px;
+            top: 400px;
+            background-color: red;
+          }
+        </style>
+        <div id="target">Target</div>
+      )HTML";
+
+  local_child.Complete(child_html);
+  xorigin_child.Complete(child_html);
+
+  Element* local_child_frame = GetDocument().getElementById("localChildFrame");
+  Element* xorigin_child_frame =
+      GetDocument().getElementById("xoriginChildFrame");
+  Document* local_child_document =
+      To<HTMLIFrameElement>(local_child_frame)->contentDocument();
+  Document* xorigin_child_document =
+      To<HTMLIFrameElement>(xorigin_child_frame)->contentDocument();
+
+  // Same origin frames shouldn't count the scroll into view.
+  {
+    ASSERT_EQ(GetDocument().View()->GetScrollableArea()->GetScrollOffset(),
+              ScrollOffset(0, 0));
+
+    Element* target = local_child_document->getElementById("target");
+    target->scrollIntoView();
+
+    ASSERT_NE(GetDocument().View()->GetScrollableArea()->GetScrollOffset(),
+              ScrollOffset(0, 0));
+    EXPECT_FALSE(
+        GetDocument().IsUseCounted(WebFeature::kCrossOriginScrollIntoView));
+  }
+
+  GetDocument().View()->GetScrollableArea()->SetScrollOffset(
+      ScrollOffset(0, 0), mojom::blink::ScrollType::kProgrammatic);
+
+  // Cross origin frames should record the scroll into view use count.
+  {
+    ASSERT_EQ(GetDocument().View()->GetScrollableArea()->GetScrollOffset(),
+              ScrollOffset(0, 0));
+
+    Element* target = xorigin_child_document->getElementById("target");
+    target->scrollIntoView();
+
+    ASSERT_NE(GetDocument().View()->GetScrollableArea()->GetScrollOffset(),
+              ScrollOffset(0, 0));
+    EXPECT_TRUE(
+        GetDocument().IsUseCounted(WebFeature::kCrossOriginScrollIntoView));
+  }
+}
+
 TEST_F(ScrollIntoViewTest, FromDisplayNoneIframe) {
   v8::HandleScope HandleScope(v8::Isolate::GetCurrent());
   WebView().MainFrameViewWidget()->Resize(gfx::Size(800, 600));

diff --git a/third_party/blink/renderer/core/scroll/scroll_into_view_util.cc b/third_party/blink/renderer/core/scroll/scroll_into_view_util.cc
@@ -20,6 +20,7 @@
 #include "third_party/blink/renderer/core/page/frame_tree.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/scroll/smooth_scroll_sequencer.h"
+#include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
 #include "third_party/blink/renderer/platform/weborigin/security_origin.h"
 #include "ui/gfx/geometry/rect_f.h"
 
@@ -117,7 +118,8 @@ absl::optional<LayoutBox*> GetScrollParent(
 absl::optional<PhysicalRect> PerformBubblingScrollIntoView(
     const LayoutBox& box,
     const PhysicalRect& absolute_rect,
-    mojom::blink::ScrollIntoViewParamsPtr& params) {
+    mojom::blink::ScrollIntoViewParamsPtr& params,
+    bool from_remote_frame) {
   DCHECK(params->type == mojom::blink::ScrollType::kProgrammatic ||
          params->type == mojom::blink::ScrollType::kUser);
 
@@ -127,6 +129,11 @@ absl::optional<PhysicalRect> PerformBubblingScrollIntoView(
   const LayoutBox* current_box = &box;
   PhysicalRect absolute_rect_to_scroll = absolute_rect;
 
+  // TODO(bokan): Temporary, to track cross-origin scroll-into-view prevalence.
+  // https://crbug.com/1339003.
+  const SecurityOrigin* starting_frame_origin =
+      box.GetFrame()->GetSecurityContext()->GetSecurityOrigin();
+
   while (current_box) {
     if (absolute_rect_to_scroll.Width() <= 0)
       absolute_rect_to_scroll.SetWidth(LayoutUnit(1));
@@ -157,8 +164,40 @@ absl::optional<PhysicalRect> PerformBubblingScrollIntoView(
     }
 
     if (area_to_scroll) {
+      ScrollOffset scroll_before = area_to_scroll->GetScrollOffset();
+      DCHECK(area_to_scroll->GetSmoothScrollSequencer());
+      wtf_size_t num_scroll_sequences =
+          area_to_scroll->GetSmoothScrollSequencer()->GetCount();
+
       absolute_rect_to_scroll =
           area_to_scroll->ScrollIntoView(absolute_rect_to_scroll, params);
+
+      // TODO(bokan): Temporary, to track cross-origin scroll-into-view
+      // prevalence. https://crbug.com/1339003.
+      // If this is for a scroll sequence, GetScrollOffset won't change until
+      // all the animations in the sequence are run which happens
+      // asynchronously after this method returns. Thus, for scroll sequences,
+      // check instead if an entry was added to the sequence which occurs only
+      // if the scroll offset is changed as a result of ScrollIntoView.
+      bool scroll_changed =
+          params->is_for_scroll_sequence
+              ? area_to_scroll->GetSmoothScrollSequencer()->GetCount() !=
+                    num_scroll_sequences
+              : area_to_scroll->GetScrollOffset() != scroll_before;
+      if (scroll_changed && !params->for_focused_editable &&
+          params->type == mojom::blink::ScrollType::kProgrammatic) {
+        const SecurityOrigin* current_frame_origin =
+            current_box->GetFrame()->GetSecurityContext()->GetSecurityOrigin();
+        if (!current_frame_origin->CanAccess(starting_frame_origin) ||
+            from_remote_frame) {
+          // ScrollIntoView caused a visible scroll in an origin that can't be
+          // accessed from where the ScrollIntoView was initiated.
+          DCHECK(params->cross_origin_boundaries);
+          UseCounter::Count(
+              current_box->GetFrame()->LocalFrameRoot().GetDocument(),
+              WebFeature::kCrossOriginScrollIntoView);
+        }
+      }
     }
 
     bool is_fixed_to_frame =
@@ -226,7 +265,8 @@ namespace scroll_into_view_util {
 
 void ScrollRectToVisible(const LayoutObject& layout_object,
                          const PhysicalRect& absolute_rect,
-                         mojom::blink::ScrollIntoViewParamsPtr params) {
+                         mojom::blink::ScrollIntoViewParamsPtr params,
+                         bool from_remote_frame) {
   LayoutBox* enclosing_box = layout_object.EnclosingBox();
   if (!enclosing_box)
     return;
@@ -239,7 +279,8 @@ void ScrollRectToVisible(const LayoutObject& layout_object,
       params->type == mojom::blink::ScrollType::kProgrammatic;
 
   absl::optional<PhysicalRect> updated_absolute_rect =
-      PerformBubblingScrollIntoView(*enclosing_box, absolute_rect, params);
+      PerformBubblingScrollIntoView(*enclosing_box, absolute_rect, params,
+                                    from_remote_frame);
 
   frame->GetSmoothScrollSequencer().RunQueuedAnimations();
 

diff --git a/third_party/blink/renderer/core/scroll/scroll_into_view_util.h b/third_party/blink/renderer/core/scroll/scroll_into_view_util.h
@@ -24,9 +24,12 @@ namespace scroll_into_view_util {
 // LayoutObject, and scrolls the LayoutObject and all its containers such that
 // the child content of the LayoutObject at that rect is visible in the
 // viewport.
+// TODO(bokan): `from_remote_frame` is temporary, to track cross-origin
+// scroll-into-view prevalence. https://crbug.com/1339003.
 void CORE_EXPORT ScrollRectToVisible(const LayoutObject&,
                                      const PhysicalRect&,
-                                     mojom::blink::ScrollIntoViewParamsPtr);
+                                     mojom::blink::ScrollIntoViewParamsPtr,
+                                     bool from_remote_frame = false);
 
 // ScrollFocusedEditableIntoView uses the caret rect for ScrollIntoView but
 // stores enough information in `params` so that the editable element's bounds

diff --git a/third_party/blink/renderer/core/scroll/smooth_scroll_sequencer.cc b/third_party/blink/renderer/core/scroll/smooth_scroll_sequencer.cc
@@ -66,6 +66,10 @@ bool SmoothScrollSequencer::FilterNewScrollOrAbortCurrent(
   return false;
 }
 
+wtf_size_t SmoothScrollSequencer::GetCount() const {
+  return queue_.size();
+}
+
 void SmoothScrollSequencer::DidDisposeScrollableArea(
     const ScrollableArea& area) {
   for (Member<SequencedScroll>& sequenced_scroll : queue_) {

diff --git a/third_party/blink/renderer/core/scroll/smooth_scroll_sequencer.h b/third_party/blink/renderer/core/scroll/smooth_scroll_sequencer.h
@@ -60,6 +60,11 @@ class CORE_EXPORT SmoothScrollSequencer final
   // incoming scroll. It may also abort the current sequenced scroll.
   bool FilterNewScrollOrAbortCurrent(mojom::blink::ScrollType incoming_type);
 
+  // Returns the size of the scroll sequence queue.
+  // TODO(bokan): Temporary, to track cross-origin scroll-into-view prevalence.
+  // https://crbug.com/1339003.
+  wtf_size_t GetCount() const;
+
   void DidDisposeScrollableArea(const ScrollableArea&);
 
   void Trace(Visitor*) const;

diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
@@ -40890,6 +40890,7 @@ Called by update_use_counter_feature_enum.py.-->
   <int value="4313" label="LinkRelPrefetchAsDocumentSameOrigin"/>
   <int value="4314" label="LinkRelPrefetchAsDocumentCrossOrigin"/>
   <int value="4315" label="PersistentQuotaType"/>
+  <int value="4316" label="CrossOriginScrollIntoView"/>
 </enum>
 
 <enum name="FeaturePolicyAllowlistType">