From 358b51690b9b26aad31363361472dba6e7b7c19c Mon Sep 17 00:00:00 2001
From: Sayed <[email protected]>
Date: Thu, 18 Jun 2026 04:49:57 -0700
Subject: [PATCH] [AW] Manage WebView PDF export file descriptor with base::ScopedFD

This CL duplicates the file descriptor passed from the Java layer during
WebView PDF export using dup(), and wraps it in a base::ScopedFD. It updates
AwPrintManager to accept and take ownership of this ScopedFD, moving it into
the background thread pool task that writes the PDF data.

This ensures that the file descriptor's lifetime is properly managed by C++ and
is not prematurely closed by external events while the background task is still
running.

Fixed: 523592755
Change-Id: I9bbc4daea6fc69aa68393284ad469051e1ec7af5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7940556
Reviewed-by: Richard Coles <[email protected]>
Reviewed-by: Peter Pakkenberg <[email protected]>
Commit-Queue: Sayed Elabady <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1648947}
---

diff --git a/android_webview/browser/aw_pdf_exporter.cc b/android_webview/browser/aw_pdf_exporter.cc
index dd1d62ef..ad2aad86e 100644
--- a/android_webview/browser/aw_pdf_exporter.cc
+++ b/android_webview/browser/aw_pdf_exporter.cc
@@ -11,7 +11,9 @@
 #include "android_webview/browser/aw_print_manager.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
+#include "base/files/scoped_file.h"
 #include "base/functional/bind.h"
+#include "base/logging.h"
 #include "content/public/browser/browser_thread.h"
 #include "printing/print_settings.h"
 #include "printing/units.h"
@@ -66,6 +68,11 @@
                                 const JavaRef<jintArray>& pages,
                                 const JavaRef<jobject>& cancel_signal) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Wrap the file descriptor in a ScopedFD to take ownership, since we are
+  // responsible for closing it when we are done (after Java detached it).
+  base::ScopedFD scoped_fd(fd);
+
   printing::PageRanges page_ranges;
   JNI_AwPdfExporter_GetPageRanges(env, pages, &page_ranges);
 
@@ -77,7 +84,8 @@
   // Update the parameters of the current print manager.
   AwPrintManager* print_manager =
       AwPrintManager::FromWebContents(web_contents_);
-  print_manager->UpdateParam(CreatePdfSettings(env, obj, page_ranges), fd,
+  print_manager->UpdateParam(CreatePdfSettings(env, obj, page_ranges),
+                             std::move(scoped_fd),
                              base::BindRepeating(&AwPdfExporter::DidExportPdf,
                                                  base::Unretained(this)));
 
diff --git a/android_webview/browser/aw_print_manager.cc b/android_webview/browser/aw_print_manager.cc
index 6415c94..b916d1a 100644
--- a/android_webview/browser/aw_print_manager.cc
+++ b/android_webview/browser/aw_print_manager.cc
@@ -10,6 +10,7 @@
 #include "base/check_op.h"
 #include "base/file_descriptor_posix.h"
 #include "base/files/file_util.h"
+#include "base/files/scoped_file.h"
 #include "base/functional/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -30,14 +31,15 @@
 
 namespace {
 
-uint32_t SaveDataToFd(int fd,
+uint32_t SaveDataToFd(base::ScopedFD fd,
                       uint32_t page_count,
                       scoped_refptr<base::RefCountedSharedMemoryMapping> data) {
-  bool result = fd > base::kInvalidFd &&
-                base::IsValueInRangeForNumericType<int>(data->size());
-  if (result)
-    result = base::WriteFileDescriptor(fd, *data);
-  return result ? page_count : 0;
+  bool did_write_successfully =
+      fd.is_valid() && base::IsValueInRangeForNumericType<int>(data->size());
+  if (did_write_successfully) {
+    did_write_successfully = base::WriteFileDescriptor(fd.get(), *data);
+  }
+  return did_write_successfully ? page_count : 0;
 }
 
 }  // namespace
@@ -71,7 +73,7 @@
 
 void AwPrintManager::PdfWritingDone(int page_count) {
   // The fd_ should have been reset when printing started.
-  CHECK_EQ(fd_, base::kInvalidFd);
+  CHECK(!fd_.is_valid());
   // Trigger the callback to notify the embedding application that printing is
   // done. A non-positive `page_count` value (<=0) will be presented as an error
   // callback to the application.
@@ -106,12 +108,12 @@
 
 void AwPrintManager::UpdateParam(
     std::unique_ptr<printing::PrintSettings> settings,
-    int file_descriptor,
+    base::ScopedFD file_descriptor,
     PrintManager::PdfWritingDoneCallback callback) {
   DCHECK(settings);
   DCHECK(callback);
   settings_ = std::move(settings);
-  fd_ = file_descriptor;
+  fd_ = std::move(file_descriptor);
   set_pdf_writing_done_callback(std::move(callback));
   set_cookie(printing::PrintSettings::NewCookie());
 }
@@ -153,11 +155,10 @@
 void AwPrintManager::DidPrintDocument(
     printing::mojom::DidPrintDocumentParamsPtr params,
     DidPrintDocumentCallback callback) {
-  // Exchange the fd_ with kInvalidFd here to prevent it from being used more
-  // than once.
-  int print_fd = std::exchange(fd_, base::kInvalidFd);
+  // Extract the fd_ here to prevent it from being used more than once.
+  base::ScopedFD print_fd = std::move(fd_);
 
-  if (print_fd == base::kInvalidFd) {
+  if (!print_fd.is_valid()) {
     PdfWritingDone(0);
     std::move(callback).Run(false);
     return;
@@ -195,7 +196,8 @@
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})
       ->PostTaskAndReplyWithResult(
           FROM_HERE,
-          base::BindOnce(&SaveDataToFd, print_fd, number_pages(), data),
+          base::BindOnce(&SaveDataToFd, std::move(print_fd), number_pages(),
+                         data),
           base::BindOnce(&AwPrintManager::OnDidPrintDocumentWritingDone,
                          weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
diff --git a/android_webview/browser/aw_print_manager.h b/android_webview/browser/aw_print_manager.h
index 58111a4..5a4cf11 100644
--- a/android_webview/browser/aw_print_manager.h
+++ b/android_webview/browser/aw_print_manager.h
@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/file_descriptor_posix.h"
+#include "base/files/scoped_file.h"
 #include "base/memory/weak_ptr.h"
 #include "components/printing/browser/print_manager.h"
 #include "components/printing/common/print.mojom-forward.h"
@@ -38,7 +39,7 @@
 
   // Updates the parameters for printing.
   void UpdateParam(std::unique_ptr<printing::PrintSettings> settings,
-                   int file_descriptor,
+                   base::ScopedFD file_descriptor,
                    PdfWritingDoneCallback callback);
 
  private:
@@ -61,7 +62,7 @@
   std::unique_ptr<printing::PrintSettings> settings_;
 
   // The file descriptor into which the PDF of the document will be written.
-  int fd_ = base::kInvalidFd;
+  base::ScopedFD fd_;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
diff --git a/android_webview/browser/aw_print_manager_unittest.cc b/android_webview/browser/aw_print_manager_unittest.cc
index 6b729c8..4e0930a 100644
--- a/android_webview/browser/aw_print_manager_unittest.cc
+++ b/android_webview/browser/aw_print_manager_unittest.cc
@@ -4,17 +4,20 @@
 
 #include "android_webview/browser/aw_print_manager.h"
 
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+
 #include <utility>
 
-#include "base/files/file.h"
 #include "base/files/file_util.h"
-#include "base/files/scoped_temp_dir.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/memory/read_only_shared_memory_region.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
+#include "base/test/run_until.h"
 #include "base/test/task_environment.h"
 #include "components/printing/common/print.mojom.h"
 #include "content/public/browser/web_contents.h"
@@ -68,59 +71,20 @@

diff --git a/android_webview/browser/aw_print_manager_unittest.cc b/android_webview/browser/aw_print_manager_unittest.cc
index 6b729c8..4e0930a 100644
--- a/android_webview/browser/aw_print_manager_unittest.cc
+++ b/android_webview/browser/aw_print_manager_unittest.cc
@@ -4,17 +4,20 @@
 
 #include "android_webview/browser/aw_print_manager.h"
 
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+
 #include <utility>
 
-#include "base/files/file.h"
 #include "base/files/file_util.h"
-#include "base/files/scoped_temp_dir.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/memory/read_only_shared_memory_region.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
 #include "base/test/bind.h"
+#include "base/test/run_until.h"
 #include "base/test/task_environment.h"
 #include "components/printing/common/print.mojom.h"
 #include "content/public/browser/web_contents.h"
@@ -68,59 +71,20 @@
   std::unique_ptr<content::WebContents> web_contents_;
 };
 
-TEST_F(AwPrintManagerTest, FdIsResetOncePrintingStarts) {
-  auto* print_manager = AwPrintManager::FromWebContents(web_contents());
-  ASSERT_TRUE(print_manager);
-
-  int test_fd = 123;
-  auto settings = std::make_unique<printing::PrintSettings>();
-
-  bool callback_called = false;
-  print_manager->UpdateParam(
-      std::move(settings), test_fd,
-      base::BindLambdaForTesting(
-          [&callback_called](int page_count) { callback_called = true; }));
-
-  auto params = printing::mojom::DidPrintDocumentParams::New();
-  params->document_cookie = 0;  // Wrong cookie to fail early
-  params->content = printing::mojom::DidPrintContentParams::New();
-
-  auto* host = static_cast<printing::mojom::PrintManagerHost*>(print_manager);
-
-  bool did_print_callback_called = false;
-
-  // We pass a wrong cookie, which causes DidPrintDocument to fail early.
-  // DidPrintDocument will call PdfWritingDone(0).
-  // Inside PdfWritingDone(), it CHECKs that fd_ is set to base::kInvalidFd.
-  // If fd_ was not reset when printing started (i.e. at the beginning of
-  // DidPrintDocument), the test will crash.
-  host->DidPrintDocument(
-      std::move(params),
-      base::BindLambdaForTesting([&did_print_callback_called](bool success) {
-        did_print_callback_called = true;
-        EXPECT_FALSE(success);
-      }));
-
-  EXPECT_TRUE(callback_called);
-  EXPECT_TRUE(did_print_callback_called);
-}
-
 TEST_F(AwPrintManagerTest, FdIsResetAfterSuccessfulPrint) {
   auto* print_manager = AwPrintManager::FromWebContents(web_contents());
   ASSERT_TRUE(print_manager);
 
-  base::ScopedTempDir temp_dir;
-  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-  base::FilePath temp_file;
-  ASSERT_TRUE(base::CreateTemporaryFileInDir(temp_dir.GetPath(), &temp_file));
-  base::File file(temp_file, base::File::FLAG_OPEN | base::File::FLAG_WRITE);
-  int valid_fd = file.GetPlatformFile();
+  int fds[2];
+  ASSERT_EQ(0, pipe2(fds, O_CLOEXEC));
+  base::ScopedFD read_fd(fds[0]);
+  base::ScopedFD write_fd(fds[1]);
 
   auto settings = std::make_unique<printing::PrintSettings>();
 
   bool callback_called = false;
   print_manager->UpdateParam(
-      std::move(settings), valid_fd,
+      std::move(settings), std::move(write_fd),
       base::BindLambdaForTesting([&callback_called](int page_count) {
         callback_called = true;
         EXPECT_EQ(1, page_count);
@@ -160,6 +124,123 @@
 
   EXPECT_TRUE(callback_called);
   EXPECT_TRUE(did_print_callback_called);
+
+  // Verify that the data was written to the pipe successfully.
+  std::string read_buf;
+  char buf[128];
+  ssize_t bytes_read;
+  while ((bytes_read = read(read_fd.get(), buf, sizeof(buf))) > 0) {
+    read_buf.append(buf, bytes_read);
+  }
+  EXPECT_EQ(read_buf, std::string(reinterpret_cast<const char*>(test_data),
+                                  sizeof(test_data)));
+}
+
+TEST_F(AwPrintManagerTest,
+       FdLifecycleManagedByBackgroundTaskEvenIfManagerDestroyed) {
+  auto* print_manager = AwPrintManager::FromWebContents(web_contents());
+  ASSERT_TRUE(print_manager);
+
+  int fds[2];
+  ASSERT_EQ(0, pipe2(fds, O_CLOEXEC));
+  base::ScopedFD read_fd(fds[0]);
+  base::ScopedFD write_fd(fds[1]);
+  ASSERT_TRUE(base::SetNonBlocking(read_fd.get()));
+
+  auto settings = std::make_unique<printing::PrintSettings>();
+
+  bool callback_called = false;
+  print_manager->UpdateParam(
+      std::move(settings), std::move(write_fd),
+      base::BindLambdaForTesting(
+          [&callback_called](int page_count) { callback_called = true; }));
+
+  auto params = printing::mojom::DidPrintDocumentParams::New();
+  params->document_cookie = PrintManagerPeer::GetCookie(print_manager);
+
+  auto content = printing::mojom::DidPrintContentParams::New();
+  const uint8_t test_data[] = "test data for lifecycle test";
+  base::MappedReadOnlyRegion mapped_region =
+      base::ReadOnlySharedMemoryRegion::Create(sizeof(test_data));
+  ASSERT_TRUE(mapped_region.IsValid());
+  mapped_region.mapping.GetMemoryAsSpan<uint8_t>().copy_from(test_data);
+
+  content->metafile_data_region = std::move(mapped_region.region);
+  params->content = std::move(content);
+
+  auto* host = static_cast<printing::mojom::PrintManagerHost*>(print_manager);
+
+  host->DidGetPrintedPagesCount(PrintManagerPeer::GetCookie(print_manager), 1);
+
+  host->DidPrintDocument(std::move(params),
+                         base::BindLambdaForTesting([](bool success) {
+                           ADD_FAILURE() << "Callback should not run";
+                         }));
+
+  // Destroy the print manager before running the background task.
+  TearDown();
+
+  // Since the print manager is destroyed, the completion callback will not run.
+  // We use RunUntil to wait for the background file writing task to complete.
+  // Note: RunUntilIdle() is a banned pattern and cannot be used here.
+  std::string read_buf;
+  EXPECT_TRUE(base::test::RunUntil([&]() {
+    char buf[128];
+    ssize_t bytes_read = read(read_fd.get(), buf, sizeof(buf));
+    if (bytes_read > 0) {
+      read_buf.append(buf, bytes_read);
+    }
+    return read_buf == std::string(reinterpret_cast<const char*>(test_data),
+                                   sizeof(test_data));
+  }));
+}
+
+// Verifies that when DidPrintDocument fails early (e.g., due to a cookie
+// mismatch), the file descriptor is properly closed. This also implicitly
+// verifies that the print manager resets/extracts the fd_ when printing starts,
+// because PdfWritingDone(0) CHECKs that fd_ is invalid.
+TEST_F(AwPrintManagerTest, FdIsClosedOnCookieMismatch) {
+  auto* print_manager = AwPrintManager::FromWebContents(web_contents());
+  ASSERT_TRUE(print_manager);
+
+  int fds[2];
+  ASSERT_EQ(0, pipe2(fds, O_CLOEXEC));
+  base::ScopedFD read_fd(fds[0]);
+  base::ScopedFD write_fd(fds[1]);
+  ASSERT_TRUE(base::SetNonBlocking(read_fd.get()));
+
+  auto settings = std::make_unique<printing::PrintSettings>();
+
+  bool callback_called = false;
+  print_manager->UpdateParam(
+      std::move(settings), std::move(write_fd),
+      base::BindLambdaForTesting([&callback_called](int page_count) {
+        callback_called = true;
+        EXPECT_EQ(0, page_count);
+      }));
+
+  auto params = printing::mojom::DidPrintDocumentParams::New();
+  params->document_cookie =
+      PrintManagerPeer::GetCookie(print_manager) + 1;  // Wrong cookie
+  params->content = printing::mojom::DidPrintContentParams::New();
+
+  auto* host = static_cast<printing::mojom::PrintManagerHost*>(print_manager);
+
+  bool did_print_callback_called = false;
+  host->DidPrintDocument(
+      std::move(params),
+      base::BindLambdaForTesting([&did_print_callback_called](bool success) {
+        did_print_callback_called = true;
+        EXPECT_FALSE(success);
+      }));
+
+  EXPECT_TRUE(callback_called);
+  EXPECT_TRUE(did_print_callback_called);
+
+  // The write_fd should be closed now due to cookie mismatch, meaning a read
+  // on read_fd should return 0 (EOF).
+  char buf;
+  EXPECT_EQ(0, read(read_fd.get(), &buf, 1));
 }
 
 }  // namespace android_webview