Drop extra PrintingContext calls to NewPage/PageDone

When system calls were refactored out of PrintedDocument in
https://crrev.com/948253, there were extra calls to PrintingContext
NewPage() and PageDone() left in PrintedDocument::RenderPrintedDocument.
These had no effect on Windows or Linux, but caused an issue for macOS.

This was undetected by tests which use TestPrintingContext, and would
only be detectable if a test was capable of using system drivers (which
generally does not occur on the bots).  Adding a test to detect this is
difficult at this time, but should be easier to do once later portions
of the out-of-process print drivers commit chains fill in more testing
capabilities.  At that point it should be easier to fire off a request
to have the macOS Preview be the output. https://crbug.com/1284745
has been filed to capture this needed effort.

Remove NewPage()/PageDone() as common methods to override from
PrintingContext and make these specific to macOS only.  Update the
various implementations of PrintingContext::PrintDocument() to include
the aborted and in-print-job checks that were previously in all
NewPage() implementations.  The same corresponding checks from
PageDone() can be handled by PrintedDocument::RenderPrintedDocument()
and PrintedDocumentWin::RenderPrintedPage().  There is no concern about
`in_print_job_` having changed during the lifetime of
RenderPrintedDocument()/RenderPrintedPage(), so just add extra checks
against printing having been asynchronouly aborted before allowing a
final successful return.

Bug: 1283651
Change-Id: I52992bc7550dd25d4ad9a1dc633c7d9452a27bb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3367333
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Alan Screen <awscreen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#955936}

chrome/browser/printing/print_job_worker.cc

@@ -519,12 +519,6 @@ void PrintJobWorker::SpoolPage(PrintedPage* page) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   DCHECK_NE(page_number_, PageNumber::npos());
 
-  // Preprocess.
-  if (printing_context_->NewPage() != mojom::ResultCode::kSuccess) {
-    OnFailure();
-    return;
-  }
-
   // Actual printing.
   if (document_->RenderPrintedPage(*page, printing_context_.get()) !=
       mojom::ResultCode::kSuccess) {
@@ -532,12 +526,6 @@ void PrintJobWorker::SpoolPage(PrintedPage* page) {
     return;
   }
 
-  // Postprocess.
-  if (printing_context_->PageDone() != mojom::ResultCode::kSuccess) {
-    OnFailure();
-    return;
-  }
-
   // Signal everyone that the page is printed.
   DCHECK(print_job_);
   print_job_->PostTask(FROM_HERE,

printing/emf_win_unittest.cc

@@ -115,7 +115,6 @@ TEST_F(EmfPrintingTest, Enumerate) {
   // current directory.
   // TODO(maruel):  Clean the .PRN file generated in current directory.
   context.NewDocument(u"EmfTest.Enumerate");
-  context.NewPage();
   // Process one at a time.
   RECT page_bounds = emf.GetPageBounds(1).ToRECT();
   Emf::Enumerator emf_enum(emf, context.context(), &page_bounds);
@@ -129,7 +128,6 @@ TEST_F(EmfPrintingTest, Enumerate) {
     EXPECT_TRUE(itr->SafePlayback(&emf_enum.context_))
         << " index: " << index << " type: " << itr->record()->iType;
   }
-  context.PageDone();
   context.DocumentDone();
 }
 

printing/printed_document.cc

@@ -187,17 +187,18 @@ const MetafilePlayer* PrintedDocument::GetMetafile() {
 
 mojom::ResultCode PrintedDocument::RenderPrintedDocument(
     PrintingContext* context) {
-  mojom::ResultCode result = context->NewPage();
+  base::AutoLock lock(lock_);
+  mojom::ResultCode result = context->PrintDocument(
+      *GetMetafile(), *immutable_.settings_, mutable_.expected_page_count_);
   if (result != mojom::ResultCode::kSuccess)
     return result;
-  {
-    base::AutoLock lock(lock_);
-    result = context->PrintDocument(*GetMetafile(), *immutable_.settings_,
-                                    mutable_.expected_page_count_);
-    if (result != mojom::ResultCode::kSuccess)
-      return result;
-  }
-  return context->PageDone();
+
+  // Beware of any asynchronous aborts of the print job that happened during
+  // printing.
+  if (context->PrintingAborted())
+    return mojom::ResultCode::kCanceled;
+
+  return mojom::ResultCode::kSuccess;
 }
 
 bool PrintedDocument::IsComplete() const {

printing/printed_document_win.cc

@@ -24,8 +24,17 @@ mojom::ResultCode PrintedDocument::RenderPrintedPage(
 #endif
 
   DCHECK(context);
-  return context->RenderPage(page,
-                             immutable_.settings_->page_setup_device_units());
+  mojom::ResultCode result = context->RenderPage(
+      page, immutable_.settings_->page_setup_device_units());
+  if (result != mojom::ResultCode::kSuccess)
+    return result;
+
+  // Beware of any asynchronous aborts of the print job that happened during
+  // printing.
+  if (context->PrintingAborted())
+    return mojom::ResultCode::kCanceled;
+
+  return mojom::ResultCode::kSuccess;
 }
 
 }  // namespace printing

printing/printing_context.h

@@ -122,18 +122,12 @@ class COMPONENT_EXPORT(PRINTING) PrintingContext {
   virtual mojom::ResultCode NewDocument(
       const std::u16string& document_name) = 0;
 
-  // Starts a new page.
-  virtual mojom::ResultCode NewPage() = 0;
-
 #if defined(OS_WIN)
   // Renders a page.
   virtual mojom::ResultCode RenderPage(const PrintedPage& page,
                                        const PageSetup& page_setup) = 0;
 #endif
 
-  // Closes the printed page.
-  virtual mojom::ResultCode PageDone() = 0;
-
   // Prints the document contained in `metafile`.
   virtual mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                           const PrintSettings& settings,
@@ -175,6 +169,8 @@ class COMPONENT_EXPORT(PRINTING) PrintingContext {
 
   std::unique_ptr<PrintSettings> TakeAndResetSettings();
 
+  bool PrintingAborted() const { return abort_printing_; }
+
   int job_id() const { return job_id_; }
 
  protected:

printing/printing_context_android.cc

@@ -213,30 +213,13 @@ mojom::ResultCode PrintingContextAndroid::NewDocument(
   return mojom::ResultCode::kSuccess;
 }
 
-mojom::ResultCode PrintingContextAndroid::NewPage() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
-mojom::ResultCode PrintingContextAndroid::PageDone() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
 mojom::ResultCode PrintingContextAndroid::PrintDocument(
     const MetafilePlayer& metafile,
     const PrintSettings& settings,
     uint32_t num_pages) {
+  if (abort_printing_)
+    return mojom::ResultCode::kCanceled;
+  DCHECK(in_print_job_);
   DCHECK(is_file_descriptor_valid());
 
   return metafile.SaveToFileDescriptor(fd_) ? mojom::ResultCode::kSuccess

printing/printing_context_android.h

@@ -60,8 +60,6 @@ class COMPONENT_EXPORT(PRINTING) PrintingContextAndroid
   mojom::ResultCode UpdatePrinterSettings(
       const PrinterSettings& printer_settings) override;
   mojom::ResultCode NewDocument(const std::u16string& document_name) override;
-  mojom::ResultCode NewPage() override;
-  mojom::ResultCode PageDone() override;
   mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                   const PrintSettings& settings,
                                   uint32_t num_pages) override;

printing/printing_context_chromeos.cc

@@ -402,32 +402,14 @@ mojom::ResultCode PrintingContextChromeos::NewDocument(
   return mojom::ResultCode::kSuccess;
 }
 
-mojom::ResultCode PrintingContextChromeos::NewPage() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
-mojom::ResultCode PrintingContextChromeos::PageDone() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
 mojom::ResultCode PrintingContextChromeos::PrintDocument(
     const MetafilePlayer& metafile,
     const PrintSettings& settings,
     uint32_t num_pages) {
+  if (abort_printing_)
+    return mojom::ResultCode::kCanceled;
+  DCHECK(in_print_job_);
+
 #if defined(USE_CUPS)
   std::vector<char> buffer;
   if (!metafile.GetDataAsVector(&buffer))

printing/printing_context_chromeos.h

@@ -39,8 +39,6 @@ class COMPONENT_EXPORT(PRINTING) PrintingContextChromeos
   mojom::ResultCode UpdatePrinterSettings(
       const PrinterSettings& printer_settings) override;
   mojom::ResultCode NewDocument(const std::u16string& document_name) override;
-  mojom::ResultCode NewPage() override;
-  mojom::ResultCode PageDone() override;
   mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                   const PrintSettings& settings,
                                   uint32_t num_pages) override;

printing/printing_context_linux.cc

@@ -151,30 +151,13 @@ mojom::ResultCode PrintingContextLinux::NewDocument(
   return mojom::ResultCode::kSuccess;
 }
 
-mojom::ResultCode PrintingContextLinux::NewPage() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
-mojom::ResultCode PrintingContextLinux::PageDone() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
 mojom::ResultCode PrintingContextLinux::PrintDocument(
     const MetafilePlayer& metafile,
     const PrintSettings& settings,
     uint32_t num_pages) {
+  if (abort_printing_)
+    return mojom::ResultCode::kCanceled;
+  DCHECK(in_print_job_);
   DCHECK(print_dialog_);
   // TODO(crbug.com/1252685)  Plumb error code back from
   // `PrintDialogGtkInterface`.

printing/printing_context_linux.h

@@ -45,8 +45,6 @@ class COMPONENT_EXPORT(PRINTING) PrintingContextLinux : public PrintingContext {
   mojom::ResultCode UpdatePrinterSettings(
       const PrinterSettings& printer_settings) override;
   mojom::ResultCode NewDocument(const std::u16string& document_name) override;
-  mojom::ResultCode NewPage() override;
-  mojom::ResultCode PageDone() override;
   mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                   const PrintSettings& settings,
                                   uint32_t num_pages) override;

printing/printing_context_mac.h

@@ -35,8 +35,6 @@ class COMPONENT_EXPORT(PRINTING) PrintingContextMac : public PrintingContext {
   mojom::ResultCode UpdatePrinterSettings(
       const PrinterSettings& printer_settings) override;
   mojom::ResultCode NewDocument(const std::u16string& document_name) override;
-  mojom::ResultCode NewPage() override;
-  mojom::ResultCode PageDone() override;
   mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                   const PrintSettings& settings,
                                   uint32_t num_pages) override;
@@ -101,6 +99,12 @@ class COMPONENT_EXPORT(PRINTING) PrintingContextMac : public PrintingContext {
   // Returns true is the pair is set.
   bool SetKeyValue(base::StringPiece key, base::StringPiece value);
 
+  // Starts a new page.
+  mojom::ResultCode NewPage();
+
+  // Closes the printed page.
+  mojom::ResultCode PageDone();
+
   // The native print info object.
   base::scoped_nsobject<NSPrintInfo> print_info_;
 

printing/printing_context_no_system_dialog.cc

@@ -98,26 +98,6 @@ mojom::ResultCode PrintingContextNoSystemDialog::NewDocument(
   return mojom::ResultCode::kSuccess;
 }
 
-mojom::ResultCode PrintingContextNoSystemDialog::NewPage() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
-mojom::ResultCode PrintingContextNoSystemDialog::PageDone() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // Intentional No-op.
-
-  return mojom::ResultCode::kSuccess;
-}
-
 mojom::ResultCode PrintingContextNoSystemDialog::PrintDocument(
     const MetafilePlayer& metafile,
     const PrintSettings& settings,

printing/printing_context_no_system_dialog.h

@@ -32,8 +32,6 @@ class COMPONENT_EXPORT(PRINTING) PrintingContextNoSystemDialog
   mojom::ResultCode UpdatePrinterSettings(
       const PrinterSettings& printer_settings) override;
   mojom::ResultCode NewDocument(const std::u16string& document_name) override;
-  mojom::ResultCode NewPage() override;
-  mojom::ResultCode PageDone() override;
   mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                   const PrintSettings& settings,
                                   uint32_t num_pages) override;

printing/printing_context_win.cc

@@ -366,18 +366,6 @@ mojom::ResultCode PrintingContextWin::NewDocument(
   return mojom::ResultCode::kSuccess;
 }
 
-mojom::ResultCode PrintingContextWin::NewPage() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(context_);
-  DCHECK(in_print_job_);
-
-  // Intentional No-op. MetafileSkia::SafePlayback takes care of calling
-  // ::StartPage().
-
-  return mojom::ResultCode::kSuccess;
-}
-
 mojom::ResultCode PrintingContextWin::RenderPage(const PrintedPage& page,
                                                  const PageSetup& page_setup) {
   if (abort_printing_)
@@ -416,17 +404,6 @@ mojom::ResultCode PrintingContextWin::RenderPage(const PrintedPage& page,
   return mojom::ResultCode::kSuccess;
 }
 
-mojom::ResultCode PrintingContextWin::PageDone() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // Intentional No-op. MetafileSkia::SafePlayback takes care of calling
-  // ::EndPage().
-
-  return mojom::ResultCode::kSuccess;
-}
-
 mojom::ResultCode PrintingContextWin::PrintDocument(
     const MetafilePlayer& metafile,
     const PrintSettings& settings,

printing/printing_context_win.h

@@ -35,10 +35,8 @@ class COMPONENT_EXPORT(PRINTING) PrintingContextWin : public PrintingContext {
   mojom::ResultCode UpdatePrinterSettings(
       const PrinterSettings& printer_settings) override;
   mojom::ResultCode NewDocument(const std::u16string& document_name) override;
-  mojom::ResultCode NewPage() override;
   mojom::ResultCode RenderPage(const PrintedPage& page,
                                const PageSetup& page_setup) override;
-  mojom::ResultCode PageDone() override;
   mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                   const PrintSettings& settings,
                                   uint32_t num_pages) override;

printing/test_printing_context.cc

@@ -116,15 +116,6 @@ mojom::ResultCode TestPrintingContext::NewDocument(
   return mojom::ResultCode::kSuccess;
 }
 
-mojom::ResultCode TestPrintingContext::NewPage() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // No-op.
-  return mojom::ResultCode::kSuccess;
-}
-
 #if defined(OS_WIN)
 mojom::ResultCode TestPrintingContext::RenderPage(const PrintedPage& page,
                                                   const PageSetup& page_setup) {
@@ -138,15 +129,6 @@ mojom::ResultCode TestPrintingContext::RenderPage(const PrintedPage& page,
 }
 #endif  // defined(OS_WIN)
 
-mojom::ResultCode TestPrintingContext::PageDone() {
-  if (abort_printing_)
-    return mojom::ResultCode::kCanceled;
-  DCHECK(in_print_job_);
-
-  // No-op.
-  return mojom::ResultCode::kSuccess;
-}
-
 mojom::ResultCode TestPrintingContext::PrintDocument(
     const MetafilePlayer& metafile,
     const PrintSettings& settings,

printing/test_printing_context.h

@@ -58,12 +58,10 @@ class TestPrintingContext : public PrintingContext {
   mojom::ResultCode UpdatePrinterSettings(
       const PrinterSettings& printer_settings) override;
   mojom::ResultCode NewDocument(const std::u16string& document_name) override;
-  mojom::ResultCode NewPage() override;
 #if defined(OS_WIN)
   mojom::ResultCode RenderPage(const PrintedPage& page,
                                const PageSetup& page_setup) override;
 #endif
-  mojom::ResultCode PageDone() override;
   mojom::ResultCode PrintDocument(const MetafilePlayer& metafile,
                                   const PrintSettings& settings,
                                   uint32_t num_pages) override;