From 3ac886fd67262e99f07b0107cd93d16f379a78b2 Mon Sep 17 00:00:00 2001
From: Lei Zhang <thestig@chromium.org>
Date: Mon, 30 Mar 2026 18:13:10 -0700
Subject: [PATCH] Printing: Fix locking in PrintedDocument

The PrintedDocument class has a `lock_` to guard the `mutable_` field,
but it is not used consistently. Add GUARDED_BY() to annotate `mutable_`
and get the compiler to check for cases where it is not guarded. Based
on its findings, the unsafe issues are:

1. Trying to post a task to run DebugDumpTask() on another thread later
   without a lock. To fix this, just run the task on the current thread
   while holding the lock. This has the potential to do I/O on the UI
   thread, but only when the browser has a debugging command line
   switch, which is very rare. So make an exception for this and let the
   debug-only function use base::ScopedAllowBlocking.

   Tidy the code slightly and rename it to DumpMetafileIfDebugEnabled().

2. Exposing GetMetafile() as a public method, which requires locking,
   but the callers do not have a way to acquire the lock. To fix this,
   get rid of this getter with an impossible locking requirement, and
   replace it with HasDocument() and GetDocumentData(), which are
   tailored to the two external callers. These new methods acquire the
   lock properly. For the internal GetMetafile() caller, just access the
   underlying `mutable_` field directly.

Bug: 494764371
Change-Id: I3a6ae714e01955bde474ae146416bc8ff92b4e41
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7709456
Auto-Submit: Lei Zhang <thestig@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1607530}
---

diff --git a/base/threading/thread_restrictions.h b/base/threading/thread_restrictions.h
index b53745d..a1bb1a4 100644
--- a/base/threading/thread_restrictions.h
+++ b/base/threading/thread_restrictions.h
@@ -391,9 +391,11 @@
 #if BUILDFLAG(IS_MAC)
 class PrintBackendServiceImpl;
 #endif
+class MetafilePlayer;
 class PrintBackendServiceManager;
 class PrintPreviewUIUntrusted;
 class PrinterQuery;
+void DumpMetafileIfDebugEnabled(const std::u16string&, const MetafilePlayer*);
 base::FilePath GetAbsoluteSystemDestinationLocation(const base::FilePath&);
 }  // namespace printing
 namespace proxy_resolver {
@@ -691,6 +693,9 @@
       base::FilePath* file_path);  // http://crbug.com/110709
   friend bool disk_cache::CleanupDirectorySync(const base::FilePath&);
   friend bool gl::init::InitializeStaticGLBindings(gl::GLImplementationParts);
+  friend void printing::DumpMetafileIfDebugEnabled(
+      const std::u16string&,
+      const printing::MetafilePlayer*);
   friend base::FilePath printing::GetAbsoluteSystemDestinationLocation(
       const base::FilePath&);
 
diff --git a/chrome/browser/printing/print_job_worker.cc b/chrome/browser/printing/print_job_worker.cc
index 403e4060..271d8e44 100644
--- a/chrome/browser/printing/print_job_worker.cc
+++ b/chrome/browser/printing/print_job_worker.cc
@@ -180,7 +180,7 @@
 #endif  // BUILDFLAG(IS_WIN)
 
   if (do_spool_document) {
-    if (!document_->GetMetafile()) {
+    if (!document_->HasDocument()) {
       PostWaitForPage();
       return;
     }
diff --git a/chrome/browser/printing/print_job_worker_oop.cc b/chrome/browser/printing/print_job_worker_oop.cc
index b4be18a4..99a8c91 100644
--- a/chrome/browser/printing/print_job_worker_oop.cc
+++ b/chrome/browser/printing/print_job_worker_oop.cc
@@ -285,11 +285,8 @@
 bool PrintJobWorkerOop::SpoolDocument() {
   DCHECK(task_runner()->RunsTasksInCurrentSequence());
 
-  const MetafilePlayer* metafile = document()->GetMetafile();
-  DCHECK(metafile);
-  base::MappedReadOnlyRegion region_mapping =
-      metafile->GetDataAsSharedMemoryRegion();
-  if (simulate_spooling_memory_errors_ || !region_mapping.IsValid()) {
+  PrintedDocument::DocumentData document_data = document()->GetDocumentData();
+  if (simulate_spooling_memory_errors_ || !document_data.data.IsValid()) {
     PRINTER_LOG(ERROR)
         << "Spooling document via service failed due to shared memory error.";
     content::GetUIThreadTaskRunner({})->PostTask(
@@ -303,8 +300,8 @@
   content::GetUIThreadTaskRunner({})->PostTask(
       FROM_HERE,
       base::BindOnce(&PrintJobWorkerOop::SendRenderPrintedDocument,
-                     ui_weak_factory_.GetWeakPtr(), metafile->GetDataType(),
-                     std::move(region_mapping.region)));
+                     ui_weak_factory_.GetWeakPtr(), document_data.data_type,
+                     std::move(document_data.data)));
   return true;
 }
 
diff --git a/printing/printed_document.cc b/printing/printed_document.cc
index 9467b19..546c55c 100644
--- a/printing/printed_document.cc
+++ b/printing/printed_document.cc
@@ -23,6 +23,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/thread_pool.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "build/build_config.h"
@@ -62,15 +63,21 @@
 }
 #endif  // BUILDFLAG(IS_WIN)
 
-void DebugDumpTask(const std::u16string& doc_name,
-                   const MetafilePlayer* metafile) {
-  DCHECK(PrintedDocument::HasDebugDumpPath());
+}  // namespace
+
+// Not in an anonymous namespace so it can be friends with
+// base::ScopedAllowBlocking.
+void DumpMetafileIfDebugEnabled(const std::u16string& doc_name,
+                                const MetafilePlayer* metafile) {
+  if (!PrintedDocument::HasDebugDumpPath()) {
+    return;
+  }
 
   static constexpr base::FilePath::CharType kExtension[] =
       FILE_PATH_LITERAL(".pdf");
-
-  std::u16string name = doc_name;
-  base::FilePath path = PrintedDocument::CreateDebugDumpPath(name, kExtension);
+  base::ScopedAllowBlocking allow_blocking;
+  base::FilePath path =
+      PrintedDocument::CreateDebugDumpPath(doc_name, kExtension);
   base::File file(path,
                   base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
 #if BUILDFLAG(IS_ANDROID)
@@ -80,6 +87,8 @@
 #endif  // BUILDFLAG(IS_ANDROID)
 }
 
+namespace {
+
 void DebugDumpDataTask(const std::u16string& doc_name,
                        const base::FilePath::StringType& extension,
                        const base::RefCountedMemory* data) {
@@ -170,27 +179,30 @@
 #endif  // BUILDFLAG(IS_WIN)
 
 void PrintedDocument::SetDocument(std::unique_ptr<MetafilePlayer> metafile) {
-  {
-    base::AutoLock lock(lock_);
-    mutable_.metafile_ = std::move(metafile);
-  }
+  DumpMetafileIfDebugEnabled(name(), metafile.get());
 
-  if (HasDebugDumpPath()) {
-    base::ThreadPool::PostTask(
-        FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
-        base::BindOnce(&DebugDumpTask, name(), mutable_.metafile_.get()));
-  }
+  base::AutoLock lock(lock_);
+  mutable_.metafile_ = std::move(metafile);
 }
 
-const MetafilePlayer* PrintedDocument::GetMetafile() {
-  return mutable_.metafile_.get();
+bool PrintedDocument::HasDocument() const {
+  base::AutoLock lock(lock_);
+  return !!mutable_.metafile_;
+}
+
+PrintedDocument::DocumentData PrintedDocument::GetDocumentData() const {
+  base::AutoLock lock(lock_);
+  base::MappedReadOnlyRegion region_mapping =
+      mutable_.metafile_->GetDataAsSharedMemoryRegion();
+  return {std::move(region_mapping.region), mutable_.metafile_->GetDataType()};
 }
 
 mojom::ResultCode PrintedDocument::RenderPrintedDocument(
     PrintingContext* context) {
   base::AutoLock lock(lock_);
-  mojom::ResultCode result = context->PrintDocument(
-      *GetMetafile(), *immutable_.settings_, mutable_.expected_page_count_);
+  mojom::ResultCode result =
+      context->PrintDocument(*mutable_.metafile_, *immutable_.settings_,
+                             mutable_.expected_page_count_);
   if (result != mojom::ResultCode::kSuccess)
     return result;
 
diff --git a/printing/printed_document.h b/printing/printed_document.h
index 5c9b6b70..4b4498bf 100644
--- a/printing/printed_document.h