From 0bdffe8ac0cbf343c6af220ee9a803b1f3101b84 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Gravel <jpgravel@chromium.org>
Date: Fri, 25 Sep 2026 10:07:51 -0700
Subject: [PATCH] Clamp pixel readback bounds on the renderer side

On the GPU process, GraphiteImageReadPixels() was clamping the requested
read bounds to the actual image bound. This has not always been the
case. Originally, the read request was failing if the bounds spilled out
of the image. This check was however removed in
ttps://crrev.com/c/5084967. Later, https://crrev.com/c/6372122 added
clamping to avoid having the GPU read outside of the image bounds.

This however is a problem because on the client side,
RasterImplementation::ReadbackImagePixelsINTERNAL allocates shared
memory for the whole unclipped image size. Since the GPU clips the
bounds, only the subset that overlaps with the image are written,
leaving all other pixels un-initialized. This uninitialized memory is
then returned to the web page. Since these frame buffers are recycled
from a memory pool, the un-initialized pixels can hold image data from
previous image buffers, which are potentially cross-origin images.

This CL fixes the issue by having ReadbackImagePixelsINTERNAL allocate
shared memory for the exact image size that the GPU process will read.
This also has the advantage of not over-allocating memory.

Fixed: 551930637
Change-Id: I3e0ee014e168101d5a615a6497aba27dd05090e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8424835
Commit-Queue: Jean-Philippe Gravel <jpgravel@chromium.org>
Reviewed-by: Ted (Chromium) Meyer <tmathmeyer@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1704875}
---
diff --git a/cc/paint/gpu_raster_pixeltest.cc b/cc/paint/gpu_raster_pixeltest.cc
index 286ce11..6b79116 100644
--- a/cc/paint/gpu_raster_pixeltest.cc
+++ b/cc/paint/gpu_raster_pixeltest.cc
@@ -283,7 +283,8 @@
         image_size.width(), image_size.height(), color_space);
     SkBitmap result;
     result.allocPixels(image_info);
-    ri->ReadbackImagePixels(mailbox, image_info, image_info.minRowBytes(), 0, 0,
+    ri->ReadbackImagePixels(mailbox, image_size, image_info,
+                            image_info.minRowBytes(), 0, 0,
                             /*plane_index=*/0, result.getPixels());
     return result;
   }
@@ -2774,9 +2775,10 @@
     readback_bitmap.allocPixels(SkImageInfo::MakeN32Premul(
         size.width(), size.height(), dest_color_space.ToSkColorSpace()));
 
-    ri->ReadbackImagePixels(dest_client_si->mailbox(), readback_bitmap.info(),
-                            readback_bitmap.rowBytes(), 0, 0,
-                            /*plane_index=*/0, readback_bitmap.getPixels());
+    ri->ReadbackImagePixels(dest_client_si->mailbox(), size,
+                            readback_bitmap.info(), readback_bitmap.rowBytes(),
+                            0, 0, /*plane_index=*/0,
+                            readback_bitmap.getPixels());
   }
 
   // The pixel value should be unchanged, even though the source and dest are
diff --git a/cc/trees/layer_tree_host_pixeltest_readback.cc b/cc/trees/layer_tree_host_pixeltest_readback.cc
index 8fa3f13..5328414e 100644
--- a/cc/trees/layer_tree_host_pixeltest_readback.cc
+++ b/cc/trees/layer_tree_host_pixeltest_readback.cc
@@ -145,8 +145,8 @@
     bitmap.allocPixels(SkImageInfo::MakeN32Premul(
         size.width(), size.height(), color_space.ToSkColorSpace()));
 
-    ri->ReadbackImagePixels(mailbox, bitmap.info(), bitmap.rowBytes(), 0, 0,
-                            /*plane_index=*/0, bitmap.getPixels());
+    ri->ReadbackImagePixels(mailbox, size, bitmap.info(), bitmap.rowBytes(), 0,
+                            0, /*plane_index=*/0, bitmap.getPixels());
     EXPECT_EQ(ri->GetError(), static_cast<unsigned>(GL_NO_ERROR));
 
     return bitmap;
diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index 5a1246b..0340dbd 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -4633,7 +4633,8 @@
       SkColorTypeBytesPerPixel(static_cast<SkColorType>(dst_sk_color_type));
   RelaxedAtomicWriteMemcpyImageRowsSkippingPadding(
       /*dst=*/dst, /*src=*/src, /*row_bytes=*/min_row_bytes,
-      /*height=*/dst_height, /*stride=*/dst_row_bytes);
+      /*height=*/dst_height, /*dst_stride=*/dst_row_bytes,
+      /*src_stride=*/dst_row_bytes);
   return GL_TRUE;
 }
 
diff --git a/gpu/command_buffer/client/implementation_base.cc b/gpu/command_buffer/client/implementation_base.cc
index 8e4ce066..834a687 100644
--- a/gpu/command_buffer/client/implementation_base.cc
+++ b/gpu/command_buffer/client/implementation_base.cc
@@ -435,18 +435,19 @@
                                                       base::span<uint8_t> src,
                                                       size_t row_bytes,
                                                       size_t height,
-                                                      size_t stride) {
+                                                      size_t dst_stride,
+                                                      size_t src_stride) {
   // We need to use `RelaxedAtomicWriteMemcpy` because we might be writing into
   // memory observed by JS at the same time.
-  if (row_bytes == stride) {
+  if (row_bytes == dst_stride && row_bytes == src_stride) {
     base::subtle::RelaxedAtomicWriteMemcpy(dst, src);
   } else {
     // If there's padding between rows, copy each row separately to leave
     // padding untouched
     for (size_t y = 0; y < height; ++y) {
       base::subtle::RelaxedAtomicWriteMemcpy(
-          dst.subspan(y * stride, row_bytes),
-          src.subspan(y * stride, row_bytes));
+          dst.subspan(y * dst_stride, row_bytes),
+          src.subspan(y * src_stride, row_bytes));
     }
   }
 }
diff --git a/gpu/command_buffer/client/implementation_base.h b/gpu/command_buffer/client/implementation_base.h
index e2034047..0be3740 100644
--- a/gpu/command_buffer/client/implementation_base.h
+++ b/gpu/command_buffer/client/implementation_base.h
@@ -168,7 +168,8 @@
     base::span<uint8_t> src,
     size_t row_bytes,
     size_t height,
-    size_t stride);
+    size_t dst_stride,
+    size_t src_stride);
 
 }  // namespace gpu
 
diff --git a/gpu/command_buffer/client/raster_implementation.cc b/gpu/command_buffer/client/raster_implementation.cc
index 8edfa91..934883c 100644
--- a/gpu/command_buffer/client/raster_implementation.cc
+++ b/gpu/command_buffer/client/raster_implementation.cc
@@ -26,6 +26,7 @@
 #include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/memory/raw_ptr.h"
+#include "base/memory/raw_span.h"
 #include "base/memory/stack_allocated.h"
 #include "base/notimplemented.h"
 #include "base/numerics/safe_math.h"
@@ -91,14 +92,14 @@
 namespace gpu {
 namespace raster {
 
-namespace {
-
 // TODO(crbug.com/40058879): Disable this work-around, once call-sites are
 // handling failures correctly.
 BASE_FEATURE(kDisableErrorHandlingForReadback,
              "kDisableErrorHandlingForReadback",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
+namespace {
+
 const uint32_t kMaxTransferCacheEntrySizeForTransferBuffer = 1024;
 const size_t kMaxImmediateDeletedPaintCachePaths = 1024;
 constexpr size_t kMaxImmediateDeletedPaintCacheEffects = 10u;
@@ -412,15 +413,15 @@
 }
 
 struct RasterImplementation::AsyncARGBReadbackRequest {
-  AsyncARGBReadbackRequest(void* dst_pixels,
-                           const SkImageInfo& dst_info,
+  AsyncARGBReadbackRequest(base::span<uint8_t> dst_pixels,
+                           const SkImageInfo& shm_info,
                            GLuint dst_row_bytes,
                            GLuint pixels_offset,
                            GLuint finished_query,
                            std::unique_ptr<ScopedMappedMemoryPtr> shared_memory,
                            base::OnceCallback<void(bool)> callback)
       : dst_pixels(dst_pixels),
-        dst_info(dst_info),
+        shm_info(shm_info),
         dst_row_bytes(dst_row_bytes),
         pixels_offset(pixels_offset),
         shared_memory(std::move(shared_memory)),
@@ -429,13 +430,13 @@
         done(false),
         readback_successful(false) {}
   ~AsyncARGBReadbackRequest() {
-    // Sometimes `callback` owns `dst_pixels`, this prevents dangling raw ptr
-    dst_pixels = nullptr;
+    // Sometimes `callback` owns `dst_pixels`, this prevents dangling raw_span
+    dst_pixels = {};
     std::move(callback).Run(readback_successful);
   }
 
-  raw_ptr<void> dst_pixels;
-  SkImageInfo dst_info;
+  base::raw_span<uint8_t> dst_pixels;
+  SkImageInfo shm_info;
   GLuint dst_row_bytes;
   GLuint pixels_offset;
   std::unique_ptr<ScopedMappedMemoryPtr> shared_memory;
@@ -541,7 +542,8 @@
         static_cast<uint8_t*>(in_buffer) + plane_offset, plane_size));
     RelaxedAtomicWriteMemcpyImageRowsSkippingPadding(
         /*dst=*/dst, /*src=*/src, /*row_bytes=*/plane_width,
-        /*height=*/plane_height, /*stride=*/plane_stride);
+        /*height=*/plane_height, /*dst_stride=*/plane_stride,
+        /*src_stride=*/plane_stride);
   }
 };
 
@@ -1437,15 +1439,33 @@
 
 bool RasterImplementation::ReadbackImagePixelsINTERNAL(
     const gpu::Mailbox& source_mailbox,
+    const gfx::Size& source_size,
     const SkImageInfo& dst_info,
     GLuint dst_row_bytes,
     int src_x,
     int src_y,
     int plane_index,
     base::OnceCallback<void(bool)> readback_done,
-    void* dst_pixels) {
+    base::span<uint8_t> dst_pixels) {
   DCHECK_GE(dst_row_bytes, dst_info.minRowBytes());
 
+  // Clip the read bounds to the actual image size. Anything outside wouldn't be
+  // fill by the GPU process.
+  gfx::Rect src_rect(src_x, src_y, dst_info.width(), dst_info.height());
+  src_rect.Intersect(gfx::Rect(source_size));
+  if (src_rect.IsEmpty()) {
+    // Note, this runs callback out of order.
+    if (readback_done) {
+      std::move(readback_done).Run(/*success=*/false);
+    }
+    return false;
+  }
+
+  // Compute the shared memory info needed to transfer pixels from GPU process.
+  SkImageInfo shm_info = dst_info.makeWH(src_rect.width(), src_rect.height());
+  GLuint shm_row_bytes = shm_info.minRowBytes();
+  GLuint shm_size = shm_info.computeByteSize(shm_row_bytes);
+
   // We can't use GetResultAs<>() to store our result because it uses
   // TransferBuffer under the hood and this function is potentially
   // asynchronous. Instead, store the result at the beginning of the shared
@@ -1456,16 +1476,15 @@
 
   // Add the size of the SkColorSpace while maintaining 8-byte alignment.
   GLuint pixels_offset = color_space_offset;
-  if (dst_info.colorSpace()) {
+  if (shm_info.colorSpace()) {
     pixels_offset = base::bits::AlignUp(
-        color_space_offset + dst_info.colorSpace()->writeToMemory(nullptr),
+        color_space_offset + shm_info.colorSpace()->writeToMemory(nullptr),
         sizeof(uint64_t));
   }
 
-  GLuint dst_size = dst_info.computeByteSize(dst_row_bytes);
   GLuint total_size =
       pixels_offset +
-      base::bits::AlignUp(dst_size, static_cast<GLuint>(sizeof(uint64_t)));
+      base::bits::AlignUp(shm_size, static_cast<GLuint>(sizeof(uint64_t)));
 
   std::unique_ptr<ScopedMappedMemoryPtr> scoped_shared_memory =
       std::make_unique<ScopedMappedMemoryPtr>(total_size, helper(),
@@ -1490,9 +1509,11 @@
           shm_address);
   *readback_result = 0;
 
-  if (dst_info.colorSpace()) {
-    size_t bytes_written = dst_info.colorSpace()->writeToMemory(
-        UNSAFE_TODO(static_cast<uint8_t*>(shm_address) + color_space_offset));
+  if (shm_info.colorSpace()) {
+    size_t bytes_written = shm_info.colorSpace()->writeToMemory(
+        scoped_shared_memory->as_byte_span()
+            .subspan(color_space_offset)
+            .data());
     DCHECK_LE(bytes_written + color_space_offset, pixels_offset);
   }
 
@@ -1509,15 +1530,23 @@
   }
 
   helper_->ReadbackARGBImagePixelsINTERNALImmediate(
-      src_x, src_y, plane_index, dst_info.width(), dst_info.height(),
-      dst_row_bytes, dst_info.colorType(), dst_info.alphaType(), shm_id,
-      shm_offset, color_space_offset, pixels_offset, source_mailbox.name);
+      src_rect.x(), src_rect.y(), plane_index, shm_info.width(),
+      shm_info.height(), shm_row_bytes, shm_info.colorType(),
+      shm_info.alphaType(), shm_id, shm_offset, color_space_offset,
+      pixels_offset, source_mailbox.name);
+
+  // Compute the clipped destination span into which pixels should be copied.
+  size_t dst_x = src_rect.x() - src_x;
+  size_t dst_y = src_rect.y() - src_y;
+  base::span<uint8_t> clipped_dst_pixels =
+      dst_pixels.subspan(dst_info.computeOffset(dst_x, dst_y, dst_row_bytes),
+                         shm_info.computeByteSize(dst_row_bytes));
 
   if (is_async) {
     EndQueryEXT(GL_COMMANDS_ISSUED_CHROMIUM);
 
     auto request = std::make_unique<AsyncARGBReadbackRequest>(
-        dst_pixels, dst_info, dst_row_bytes, pixels_offset, query,
+        clipped_dst_pixels, shm_info, dst_row_bytes, pixels_offset, query,
         std::move(scoped_shared_memory), std::move(readback_done));
     auto* request_ptr = request.get();
     argb_request_queue_.push(std::move(request));
@@ -1530,14 +1559,12 @@
     if (!*readback_result) {
       return false;
     }
-    auto dst = UNSAFE_TODO(
-        base::span<uint8_t>(static_cast<uint8_t*>(dst_pixels), dst_size));
-    auto src = UNSAFE_TODO(base::span<uint8_t>(
-        static_cast<uint8_t*>(shm_address) + pixels_offset, dst_size));
+    base::span<uint8_t> src =
+        scoped_shared_memory->as_byte_span().subspan(pixels_offset, shm_size);
     RelaxedAtomicWriteMemcpyImageRowsSkippingPadding(
-        /*dst=*/dst, /*src=*/src, /*row_bytes=*/dst_info.minRowBytes(),
-        /*height=*/dst_info.height(),
-        /*stride=*/dst_row_bytes);
+        /*dst=*/clipped_dst_pixels, /*src=*/src, /*row_bytes=*/shm_row_bytes,
+        /*height=*/shm_info.height(), /*dst_stride=*/dst_row_bytes,
+        /*src_stride=*/shm_row_bytes);
   }
 
   return true;
@@ -1562,19 +1589,17 @@
         static_cast<cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result*>(
             request->shared_memory->address());
     if (*result) {
-      size_t dst_size =
-          request->dst_info.computeByteSize(request->dst_row_bytes);
-      auto dst = UNSAFE_TODO(base::span<uint8_t>(
-          static_cast<uint8_t*>(request->dst_pixels.get()), dst_size));
-      auto src = UNSAFE_TODO(base::span<uint8_t>(
-          static_cast<uint8_t*>(request->shared_memory->address()) +
-              request->pixels_offset,
-          dst_size));
+      size_t shm_row_bytes = request->shm_info.minRowBytes();
+      size_t shm_size = request->shm_info.computeByteSize(shm_row_bytes);
+      auto src = request->shared_memory->as_byte_span().subspan(
+          request->pixels_offset, shm_size);
       RelaxedAtomicWriteMemcpyImageRowsSkippingPadding(
-          /*dst=*/dst, /*src=*/src,
-          /*row_bytes=*/request->dst_info.minRowBytes(),
-          /*height=*/request->dst_info.height(),
-          /*stride=*/request->dst_row_bytes);
+          /*dst=*/request->dst_pixels,
+          /*src=*/src,
+          /*row_bytes=*/shm_row_bytes,
+          /*height=*/request->shm_info.height(),
+          /*dst_stride=*/request->dst_row_bytes,
+          /*src_stride=*/shm_row_bytes);
       request->readback_successful = true;
     }
 
@@ -1627,14 +1652,15 @@
     return;
   }
 
-  ReadbackImagePixelsINTERNAL(source_mailbox, dst_info, dst_row_bytes,
-                              source_starting_point.x(),
+  ReadbackImagePixelsINTERNAL(source_mailbox, source_size, dst_info,
+                              dst_row_bytes, source_starting_point.x(),
                               source_starting_point.y(), /*plane_index=*/0,
-                              std::move(readback_done), out.data());
+                              std::move(readback_done), out);
 }
 
 bool RasterImplementation::ReadbackImagePixels(
     const gpu::Mailbox& source_mailbox,
+    const gfx::Size& source_size,
     const SkImageInfo& dst_info,
     GLuint dst_row_bytes,
     int src_x,
@@ -1642,9 +1668,12 @@
     int plane_index,
     void* dst_pixels) {
   TRACE_EVENT0("gpu", "RasterImplementation::ReadbackImagePixels");
+  size_t dst_size = dst_info.computeByteSize(dst_row_bytes);
+  base::span<uint8_t> dst_span =
+      UNSAFE_TODO(base::span(static_cast<uint8_t*>(dst_pixels), dst_size));
   return ReadbackImagePixelsINTERNAL(
-             source_mailbox, dst_info, dst_row_bytes, src_x, src_y, plane_index,
-             base::OnceCallback<void(bool)>(), dst_pixels) ||
+             source_mailbox, source_size, dst_info, dst_row_bytes, src_x, src_y,
+             plane_index, base::OnceCallback<void(bool)>(), dst_span) ||
          base::FeatureList::IsEnabled(kDisableErrorHandlingForReadback);
 }
 
diff --git a/gpu/command_buffer/client/raster_implementation.h b/gpu/command_buffer/client/raster_implementation.h
index 6a7ee17..dbb58b50 100644
--- a/gpu/command_buffer/client/raster_implementation.h
+++ b/gpu/command_buffer/client/raster_implementation.h
@@ -15,6 +15,7 @@
 #include <vector>
 
 #include "base/containers/span.h"
+#include "base/feature.h"
 #include "base/memory/raw_ptr.h"
... (file diff truncated)

diff --git a/components/viz/test/test_raster_interface.cc b/components/viz/test/test_raster_interface.cc
index 831239185..973429d 100644
--- a/components/viz/test/test_raster_interface.cc
+++ b/components/viz/test/test_raster_interface.cc
@@ -140,6 +140,7 @@
 
 bool TestRasterInterface::ReadbackImagePixels(
     const gpu::Mailbox& source_mailbox,
+    const gfx::Size& source_size,
     const SkImageInfo& dst_info,
     GLuint dst_row_bytes,
     int src_x,
diff --git a/components/viz/test/test_raster_interface.h b/components/viz/test/test_raster_interface.h
index 3ebb9aa5..94fa8cf 100644
--- a/components/viz/test/test_raster_interface.h
+++ b/components/viz/test/test_raster_interface.h
@@ -137,6 +137,7 @@
       base::OnceCallback<void()> release_mailbox,
       base::OnceCallback<void(bool)> readback_done) override {}
   bool ReadbackImagePixels(const gpu::Mailbox& source_mailbox,
+                           const gfx::Size& source_size,
                            const SkImageInfo& dst_info,
                            GLuint dst_row_bytes,
                            int src_x,
diff --git a/gpu/command_buffer/client/raster_implementation_unittest.cc b/gpu/command_buffer/client/raster_implementation_unittest.cc
index d9fd57a..991d998 100644
--- a/gpu/command_buffer/client/raster_implementation_unittest.cc
+++ b/gpu/command_buffer/client/raster_implementation_unittest.cc
@@ -15,14 +15,18 @@
 #include <algorithm>
 #include <array>
 #include <memory>
+#include <numeric>
 
 #include "base/bits.h"
 #include "base/compiler_specific.h"
 #include "base/containers/heap_array.h"
 #include "base/containers/span.h"
+#include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/raw_ptr.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/test_future.h"
 #include "cc/paint/raw_memory_transfer_cache_entry.h"
 #include "cc/paint/transfer_cache_serialize_helper.h"
 #include "gpu/command_buffer/client/client_test_helper.h"
@@ -278,16 +282,17 @@
   }
 
   bool ReadbackImagePixelsINTERNAL(const gpu::Mailbox& source_mailbox,
+                                   const gfx::Size& source_size,
                                    const SkImageInfo& dst_info,
                                    GLuint dst_row_bytes,
                                    int src_x,
                                    int src_y,
                                    int plane_index,
                                    base::OnceCallback<void(bool)> readback_done,
-                                   void* dst_pixels) {
+                                   base::span<uint8_t> dst_pixels) {
     return gl_->ReadbackImagePixelsINTERNAL(
-        source_mailbox, dst_info, dst_row_bytes, src_x, src_y, plane_index,
-        std::move(readback_done), dst_pixels);
+        source_mailbox, source_size, dst_info, dst_row_bytes, src_x, src_y,
+        plane_index, std::move(readback_done), dst_pixels);
   }
 
   static SharedMemoryLimits SharedMemoryLimitsForTesting() {
@@ -896,6 +901,7 @@
 
 // https://crbug.com/543707066
 TEST_F(RasterImplementationTest, ReadbackImagePixelsSyncPadding) {
+  base::test::ScopedFeatureList feature({}, {kDisableErrorHandlingForReadback});
   gpu::Mailbox mailbox = gpu::Mailbox::Generate();
   SkImageInfo dst_info = SkImageInfo::MakeN32Premul(2, 2);
   GLuint dst_row_bytes =
@@ -905,48 +911,48 @@
       sizeof(cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result),
       sizeof(uint64_t));
   GLuint pixels_offset = color_space_offset;
-  GLuint dst_size = dst_info.computeByteSize(dst_row_bytes);
+  GLuint shm_size = dst_info.computeMinByteSize();
   GLuint total_size =
       pixels_offset +
-      base::bits::AlignUp(dst_size, static_cast<GLuint>(sizeof(uint64_t)));
+      base::bits::AlignUp(shm_size, static_cast<GLuint>(sizeof(uint64_t)));
 
   ExpectedMemoryInfo mem = GetExpectedMappedMemory(total_size);
 
   std::vector<uint8_t> dst_pixels(dst_row_bytes * dst_info.height(), 0xAA);
 
   EXPECT_CALL(*command_buffer(), OnFlush())
-      .WillOnce([mem, pixels_offset, dst_size]() {
+      .WillOnce([mem, pixels_offset, shm_size]() {
         // Write 1 to readback_result (at the beginning of shm).
         auto* result = reinterpret_cast<
             cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result*>(mem.ptr);
         *result = 1;
 
         // Write test data to the pixel portion of the shared memory.
-        auto src_pixels = mem.span.subspan(pixels_offset, dst_size);
-        // Fill src_pixels with distinct values, e.g. 1 to dst_size
-        for (size_t i = 0; i < dst_size; ++i) {
+        auto src_pixels = mem.span.subspan(pixels_offset, shm_size);
+        // Fill src_pixels with distinct values, e.g. 1 to shm_size
+        for (size_t i = 0; i < shm_size; ++i) {
           src_pixels[i] = static_cast<uint8_t>(i + 1);
         }
       })
       .RetiresOnSaturation();
 
-  bool success = gl_->ReadbackImagePixels(mailbox, dst_info, dst_row_bytes,
-                                          /*src_x=*/0, /*src_y=*/0,
-                                          /*plane_index=*/0, dst_pixels.data());
+  bool success = gl_->ReadbackImagePixels(
+      mailbox, /*source_size=*/gfx::Size(2, 2), dst_info, dst_row_bytes,
+      /*src_x=*/0, /*src_y=*/0, /*plane_index=*/0, dst_pixels.data());
 
   EXPECT_TRUE(success);
 
   // Expected output:
-  // Row 1 (pixels: 0 to 7) copied from src_pixels (0 to 7): 1, 2, 3, 4, 5, 6,
-  // 7, 8. Row 1 (padding: 8 to 11) untouched: 0xAA, 0xAA, 0xAA, 0xAA. Row 2
-  // (pixels: 12 to 19) copied from src_pixels (12 to 19): 13, 14, 15, 16, 17,
-  // 18, 19, 20. Row 2 (padding: 20 to 23) untouched: 0xAA, 0xAA, 0xAA, 0xAA.
+  // - Row 1 (pixels: 0 to 7) copied from src_pixels (0 to 7): 1..8.
+  // - Row 1 (padding: 8 to 11) untouched: 0xAA, 0xAA, 0xAA, 0xAA.
+  // - Row 2 (pixels: 12 to 19) copied from src_pixels (8 to 15): 9..16.
+  // - Row 2 (padding: 20 to 23) untouched: 0xAA, 0xAA, 0xAA, 0xAA.
 
   std::vector<uint8_t> expected_pixels(dst_row_bytes * dst_info.height(), 0xAA);
   for (int y = 0; y < dst_info.height(); ++y) {
     for (size_t x = 0; x < dst_info.minRowBytes(); ++x) {
       size_t dst_idx = y * dst_row_bytes + x;
-      size_t src_idx = y * dst_row_bytes + x;
+      size_t src_idx = y * dst_info.minRowBytes() + x;
       expected_pixels[dst_idx] = static_cast<uint8_t>(src_idx + 1);
     }
   }
@@ -965,10 +971,10 @@
       sizeof(cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result),
       sizeof(uint64_t));
   GLuint pixels_offset = color_space_offset;
-  GLuint dst_size = dst_info.computeByteSize(dst_row_bytes);
+  GLuint shm_size = dst_info.computeMinByteSize();
   GLuint total_size =
       pixels_offset +
-      base::bits::AlignUp(dst_size, static_cast<GLuint>(sizeof(uint64_t)));
+      base::bits::AlignUp(shm_size, static_cast<GLuint>(sizeof(uint64_t)));
 
   ExpectedMemoryInfo mem = GetExpectedMappedMemory(total_size);
 
@@ -991,8 +997,9 @@
       &callback_run, &callback_success);
 
   bool success = ReadbackImagePixelsINTERNAL(
-      mailbox, dst_info, dst_row_bytes, /*src_x=*/0, /*src_y=*/0,
-      /*plane_index=*/0, std::move(readback_done), dst_pixels.data());
+      mailbox, /*source_size=*/gfx::Size(2, 2), dst_info, dst_row_bytes,
+      /*src_x=*/0, /*src_y=*/0, /*plane_index=*/0, std::move(readback_done),
+      dst_pixels);
 
   EXPECT_TRUE(success);
   EXPECT_FALSE(callback_run);
@@ -1004,8 +1011,8 @@
           mem.ptr);
   *result = 1;
 
-  auto src_pixels = mem.span.subspan(pixels_offset, dst_size);
-  for (size_t i = 0; i < dst_size; ++i) {
+  auto src_pixels = mem.span.subspan(pixels_offset, shm_size);
+  for (size_t i = 0; i < shm_size; ++i) {
     src_pixels[i] = static_cast<uint8_t>(i + 1);
   }
 
@@ -1023,7 +1030,7 @@
   for (int y = 0; y < dst_info.height(); ++y) {
     for (size_t x = 0; x < dst_info.minRowBytes(); ++x) {
       size_t dst_idx = y * dst_row_bytes + x;
-      size_t src_idx = y * dst_row_bytes + x;
+      size_t src_idx = y * dst_info.minRowBytes() + x;
       expected_pixels[dst_idx] = static_cast<uint8_t>(src_idx + 1);
     }
   }
@@ -1031,6 +1038,121 @@
   EXPECT_EQ(dst_pixels, expected_pixels);
 }
 
+TEST_F(RasterImplementationTest, ReadbackImagePixelsSyncOutOfBounds) {
+  base::test::ScopedFeatureList feature({}, {kDisableErrorHandlingForReadback});
+  // Request a 4x4 read starting at (-1, -1) with row padding (20 bytes/row).
+  // The intersection with the 2x2 source image is (0, 0, 2, 2), which lands at
+  // offset (1, 1) in the 4x4 destination buffer.
+  SkImageInfo dst_info = SkImageInfo::MakeN32Premul(4, 4);
+  GLuint dst_row_bytes = 20;  // 4 pixels * 4 bytes/pixel = 16 + 4 padding.
+  SkImageInfo clipped_info = SkImageInfo::MakeN32Premul(2, 2);
+
+  GLuint pixels_offset = base::bits::AlignUp(
+      sizeof(cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result),
+      sizeof(uint64_t));
+  GLuint shm_size = clipped_info.computeMinByteSize();
+  GLuint total_size =
+      pixels_offset +
+      base::bits::AlignUp(shm_size, static_cast<GLuint>(sizeof(uint64_t)));
+  ExpectedMemoryInfo mem = GetExpectedMappedMemory(total_size);
+
+  EXPECT_CALL(*command_buffer(), OnFlush()).WillOnce([&]() {
+    *reinterpret_cast<cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result*>(
+        mem.ptr) = 1;
+    std::ranges::iota(mem.span.subspan(pixels_offset, shm_size), 0);
+  });
+
+  std::vector<uint8_t> dst_pixels(dst_row_bytes * dst_info.height(), 0xAA);
+  EXPECT_TRUE(gl_->ReadbackImagePixels(
+      gpu::Mailbox::Generate(), /*source_size=*/gfx::Size(2, 2), dst_info,
+      dst_row_bytes, /*src_x=*/-1, /*src_y=*/-1, /*plane_index=*/0,
+      dst_pixels.data()));
+
+  std::vector<uint8_t> expected_pixels(dst_row_bytes * dst_info.height(), 0xAA);
+  for (int y = 0; y < clipped_info.height(); ++y) {
+    for (size_t x = 0; x < clipped_info.minRowBytes(); ++x) {
+      size_t dst_idx = (y + 1) * dst_row_bytes + dst_info.bytesPerPixel() + x;
+      size_t src_idx = y * clipped_info.minRowBytes() + x;
+      expected_pixels[dst_idx] = static_cast<uint8_t>(src_idx);
+    }
+  }
+  EXPECT_EQ(dst_pixels, expected_pixels);
+}
+
+TEST_F(RasterImplementationTest, ReadbackImagePixelsSyncCompletelyOutOfBounds) {
+  base::test::ScopedFeatureList feature({}, {kDisableErrorHandlingForReadback});
+  // Read a 2x2 image at index (3, 3), that is, completely out of bounds.
+  SkImageInfo dst_info = SkImageInfo::MakeN32Premul(2, 2);
+  std::vector<uint8_t> dst_pixels(dst_info.computeMinByteSize(), 0xAA);
+  EXPECT_FALSE(gl_->ReadbackImagePixels(
+      gpu::Mailbox::Generate(), /*source_size=*/gfx::Size(2, 2), dst_info,
+      dst_info.minRowBytes(), /*src_x=*/3, /*src_y=*/3, /*plane_index=*/0,
+      dst_pixels.data()));
+
+  EXPECT_EQ(dst_pixels,
+            std::vector<uint8_t>(dst_info.computeMinByteSize(), 0xAA));
+}
+
+TEST_F(RasterImplementationTest, ReadbackImagePixelsAsyncOutOfBounds) {
+  // Request a 4x4 read starting at (-1, -1) with row padding (20 bytes/row).
+  // The intersection with the 2x2 source image is (0, 0, 2, 2), which lands at
+  // offset (1, 1) in the 4x4 destination buffer.
+  SkImageInfo dst_info = SkImageInfo::MakeN32Premul(4, 4);
+  GLuint dst_row_bytes = 20;
+  SkImageInfo clipped_info = SkImageInfo::MakeN32Premul(2, 2);
+
+  GLuint pixels_offset = base::bits::AlignUp(
+      sizeof(cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result),
+      sizeof(uint64_t));
+  GLuint shm_size = clipped_info.computeMinByteSize();
+  GLuint total_size =
+      pixels_offset +
+      base::bits::AlignUp(shm_size, static_cast<GLuint>(sizeof(uint64_t)));
+  ExpectedMemoryInfo mem = GetExpectedMappedMemory(total_size);
+
+  EXPECT_CALL(*gpu_control_, DoSignalQuery(_, _))
+      .WillOnce([&](uint32_t query, base::OnceClosure* callback) {
+        *reinterpret_cast<
+            cmds::ReadbackARGBImagePixelsINTERNALImmediate::Result*>(mem.ptr) =
+            1;
+        std::ranges::iota(mem.span.subspan(pixels_offset, shm_size), 0);
+        std::move(*callback).Run();
+      });
+
+  base::test::TestFuture<bool> result_future;
+  std::vector<uint8_t> dst_pixels(dst_row_bytes * dst_info.height(), 0xAA);
+  EXPECT_TRUE(ReadbackImagePixelsINTERNAL(
+      gpu::Mailbox::Generate(), /*source_size=*/gfx::Size(2, 2), dst_info,
+      dst_row_bytes, /*src_x=*/-1, /*src_y=*/-1, /*plane_index=*/0,
+      result_future.GetCallback(), dst_pixels));
+  EXPECT_TRUE(result_future.Get());
+
+  std::vector<uint8_t> expected_pixels(dst_row_bytes * dst_info.height(), 0xAA);
+  for (int y = 0; y < clipped_info.height(); ++y) {
+    for (size_t x = 0; x < clipped_info.minRowBytes(); ++x) {
+      size_t dst_idx = (y + 1) * dst_row_bytes + dst_info.bytesPerPixel() + x;
+      size_t src_idx = y * clipped_info.minRowBytes() + x;
+      expected_pixels[dst_idx] = static_cast<uint8_t>(src_idx);
+    }
+  }
+  EXPECT_EQ(dst_pixels, expected_pixels);
+}
+
+TEST_F(RasterImplementationTest,
+       ReadbackImagePixelsAsyncCompletelyOutOfBounds) {
+  // Read a 2x2 image at index (3, 3), that is, completely out of bounds.
+  SkImageInfo dst_info = SkImageInfo::MakeN32Premul(2, 2);
+  std::vector<uint8_t> dst_pixels(dst_info.computeMinByteSize(), 0xAA);
+  base::test::TestFuture<bool> result_future;
+  EXPECT_FALSE(ReadbackImagePixelsINTERNAL(
+      gpu::Mailbox::Generate(), /*source_size=*/gfx::Size(2, 2), dst_info,
+      dst_info.minRowBytes(), /*src_x=*/3, /*src_y=*/3, /*plane_index=*/0,
+      result_future.GetCallback(), dst_pixels));
+  EXPECT_FALSE(result_future.Get());
+  EXPECT_EQ(dst_pixels,
... (truncated)