[printing] Move CheckForCancel() to printing::mojom::PrintManagerHost

On the current code, even though |preview_ui_| is checked before
calling CheckForCancel(), the renderer could not get the reply as
the print WebUI dialog is already closed on the browser. If then,
the renderer gets blocked.

Since CheckForCancel() is called multiple times while handling the
print preview request and |preview_ui_| could be disconnected once
the print WebUI is closed, this change moves CheckForCancel() to
mojom::PrintManagerHost as PrintViewManager which implements it
is alive while WebContents is available.

Bug: 1166492, 1167170, 1008939
Change-Id: Iffc5d5354be3778ec7f33cff11051858432b5bf0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632023Reviewed-by: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Commit-Queue: Julie Kim <jkim@igalia.com>
Cr-Commit-Position: refs/heads/master@{#845636}

chrome/browser/printing/print_preview_message_handler.cc

@@ -185,7 +185,7 @@ void PrintPreviewMessageHandler::OnDidPreviewPage(
 
   if (ShouldUseCompositor(print_preview_ui)) {
     // Don't bother compositing if this request has been cancelled already.
-    if (print_preview_ui->ShouldCancelRequest(ids.request_id))
+    if (PrintPreviewUI::ShouldCancelRequest(ids.ui_id, ids.request_id))
       return;
 
     auto* client = PrintCompositeClient::FromWebContents(web_contents());
@@ -238,7 +238,7 @@ void PrintPreviewMessageHandler::OnMetafileReadyForPrinting(
 
   if (composite_document_using_individual_pages) {
     // Don't bother compositing if this request has been cancelled already.
-    if (print_preview_ui->ShouldCancelRequest(ids.request_id))
+    if (PrintPreviewUI::ShouldCancelRequest(ids.ui_id, ids.request_id))
       return;
 
     auto callback = base::BindOnce(
@@ -285,7 +285,7 @@ void PrintPreviewMessageHandler::NotifyUIPreviewPageReady(
     return;
 
   // Don't bother notifying the UI if this request has been cancelled already.
-  if (print_preview_ui->ShouldCancelRequest(ids.request_id))
+  if (PrintPreviewUI::ShouldCancelRequest(ids.ui_id, ids.request_id))
     return;
 
   print_preview_ui->OnDidPreviewPage(page_number, std::move(data_bytes),
@@ -300,7 +300,7 @@ void PrintPreviewMessageHandler::NotifyUIPreviewDocumentReady(
     return;
 
   // Don't bother notifying the UI if this request has been cancelled already.
-  if (print_preview_ui->ShouldCancelRequest(ids.request_id))
+  if (PrintPreviewUI::ShouldCancelRequest(ids.ui_id, ids.request_id))
     return;
 
   print_preview_ui->OnPreviewDataIsAvailable(std::move(data_bytes),
@@ -319,7 +319,7 @@ void PrintPreviewMessageHandler::OnCompositePdfPageDone(
   if (!print_preview_ui)
     return;
 
-  if (print_preview_ui->ShouldCancelRequest(ids.request_id))
+  if (PrintPreviewUI::ShouldCancelRequest(ids.ui_id, ids.request_id))
     return;
 
   if (status != mojom::PrintCompositor::Status::kSuccess) {
@@ -403,7 +403,7 @@ void PrintPreviewMessageHandler::OnCompositeToPdfDone(
   if (!print_preview_ui)
     return;
 
-  if (print_preview_ui->ShouldCancelRequest(ids.request_id))
+  if (PrintPreviewUI::ShouldCancelRequest(ids.ui_id, ids.request_id))
     return;
 
   if (status != mojom::PrintCompositor::Status::kSuccess) {
@@ -447,7 +447,7 @@ void PrintPreviewMessageHandler::OnPrepareForDocumentToPdfDone(
   if (!print_preview_ui)
     return;
 
-  if (print_preview_ui->ShouldCancelRequest(ids.request_id))
+  if (PrintPreviewUI::ShouldCancelRequest(ids.ui_id, ids.request_id))
     return;
 
   if (status != mojom::PrintCompositor::Status::kSuccess)

chrome/browser/printing/print_view_manager.cc

@@ -351,6 +351,13 @@ void PrintViewManager::RequestPrintPreview(
                                    *params);
 }
 
+void PrintViewManager::CheckForCancel(int32_t preview_ui_id,
+                                      int32_t request_id,
+                                      CheckForCancelCallback callback) {
+  std::move(callback).Run(
+      PrintPreviewUI::ShouldCancelRequest(preview_ui_id, request_id));
+}
+
 void PrintViewManager::OnScriptedPrintPreviewReply(IPC::Message* reply_msg) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   print_preview_rfh_->Send(reply_msg);

chrome/browser/printing/print_view_manager.h

@@ -62,6 +62,9 @@ class PrintViewManager : public PrintViewManagerBase,
   void DidShowPrintDialog() override;
   void ShowScriptedPrintPreview(bool source_is_modifiable) override;
   void RequestPrintPreview(mojom::RequestPrintPreviewParamsPtr params) override;
+  void CheckForCancel(int32_t preview_ui_id,
+                      int32_t request_id,
+                      CheckForCancelCallback callback) override;
 
   // content::WebContentsObserver implementation.
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;

chrome/browser/ui/webui/print_preview/print_preview_ui.cc

@@ -593,9 +593,10 @@ void PrintPreviewUI::SetInitialParams(
   print_preview_ui->print_selection_only_ = params.selection_only;
 }
 
-bool PrintPreviewUI::ShouldCancelRequest(int request_id) const {
+// static
+bool PrintPreviewUI::ShouldCancelRequest(int preview_ui_id, int request_id) {
   int current_id = -1;
-  if (!g_print_preview_request_id_map.Get().Get(*id_, &current_id))
+  if (!g_print_preview_request_id_map.Get().Get(preview_ui_id, &current_id))
     return true;
   return request_id != current_id;
 }
@@ -798,11 +799,6 @@ void PrintPreviewUI::PrinterSettingsInvalid(int32_t document_cookie,
   handler_->OnInvalidPrinterSettings(request_id);
 }
 
-void PrintPreviewUI::CheckForCancel(int32_t request_id,
-                                    CheckForCancelCallback callback) {
-  std::move(callback).Run(ShouldCancelRequest(request_id));
-}
-
 // static
 void PrintPreviewUI::SetDelegateForTesting(TestDelegate* delegate) {
   g_test_delegate = delegate;

chrome/browser/ui/webui/print_preview/print_preview_ui.h

@@ -62,8 +62,6 @@ class PrintPreviewUI : public ConstrainedWebDialogUI,
                              int32_t request_id) override;
   void PrinterSettingsInvalid(int32_t document_cookie,
                               int32_t request_id) override;
-  void CheckForCancel(int32_t request_id,
-                      CheckForCancelCallback callback) override;
 
   bool IsBound() const;
 
@@ -132,7 +130,7 @@ class PrintPreviewUI : public ConstrainedWebDialogUI,
   // Determines whether to cancel a print preview request based on the request
   // id.
   // Can be called from any thread.
-  bool ShouldCancelRequest(int request_id) const;
+  static bool ShouldCancelRequest(int preview_ui_id, int request_id);
 
   // Returns an id to uniquely identify this PrintPreviewUI.
   base::Optional<int32_t> GetIDForPrintPreviewUI() const;

chrome/browser/ui/webui/print_preview/print_preview_ui_unittest.cc

@@ -191,20 +191,25 @@ TEST_F(PrintPreviewUIUnitTest, ShouldCancelRequest) {
   preview_ui->SetPreviewUIId();
 
   // Test the initial state.
-  EXPECT_TRUE(preview_ui->ShouldCancelRequest(0));
+  EXPECT_TRUE(PrintPreviewUI::ShouldCancelRequest(
+      *preview_ui->GetIDForPrintPreviewUI(), 0));
 
   const int kFirstRequestId = 1000;
   const int kSecondRequestId = 1001;
 
   // Test with kFirstRequestId.
   preview_ui->OnPrintPreviewRequest(kFirstRequestId);
-  EXPECT_FALSE(preview_ui->ShouldCancelRequest(kFirstRequestId));
-  EXPECT_TRUE(preview_ui->ShouldCancelRequest(kSecondRequestId));
+  EXPECT_FALSE(PrintPreviewUI::ShouldCancelRequest(
+      *preview_ui->GetIDForPrintPreviewUI(), kFirstRequestId));
+  EXPECT_TRUE(PrintPreviewUI::ShouldCancelRequest(
+      *preview_ui->GetIDForPrintPreviewUI(), kSecondRequestId));
 
   // Test with kSecondRequestId.
   preview_ui->OnPrintPreviewRequest(kSecondRequestId);
-  EXPECT_TRUE(preview_ui->ShouldCancelRequest(kFirstRequestId));
-  EXPECT_FALSE(preview_ui->ShouldCancelRequest(kSecondRequestId));
+  EXPECT_TRUE(PrintPreviewUI::ShouldCancelRequest(
+      *preview_ui->GetIDForPrintPreviewUI(), kFirstRequestId));
+  EXPECT_FALSE(PrintPreviewUI::ShouldCancelRequest(
+      *preview_ui->GetIDForPrintPreviewUI(), kSecondRequestId));
 }
 
 TEST_F(PrintPreviewUIUnitTest, ParseDataPath) {

components/printing/browser/print_manager.cc

@@ -71,6 +71,10 @@ void PrintManager::ShowScriptedPrintPreview(bool source_is_modifiable) {}
 
 void PrintManager::RequestPrintPreview(
     mojom::RequestPrintPreviewParamsPtr params) {}
+
+void PrintManager::CheckForCancel(int32_t preview_ui_id,
+                                  int32_t request_id,
+                                  CheckForCancelCallback callback) {}
 #endif
 
 bool PrintManager::IsPrintRenderFrameConnected(content::RenderFrameHost* rfh) {

components/printing/browser/print_manager.h

@@ -59,6 +59,9 @@ class PrintManager : public content::WebContentsObserver,
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
   void ShowScriptedPrintPreview(bool source_is_modifiable) override;
   void RequestPrintPreview(mojom::RequestPrintPreviewParamsPtr params) override;
+  void CheckForCancel(int32_t preview_ui_id,
+                      int32_t request_id,
+                      CheckForCancelCallback callback) override;
 #endif
 
  protected:

components/printing/common/print.mojom

@@ -234,11 +234,6 @@ interface PrintPreviewUI {
   // printer driver is bogus).
   [EnableIf=enable_print_preview]
   PrinterSettingsInvalid(int32 document_cookie, int32 request_id);
-
-  // Asks the browser whether the print preview has been cancelled. It
-  // gets true if the preview for |request_id| has been cancelled.
-  [EnableIf=enable_print_preview, Sync]
-  CheckForCancel(int32 request_id) => (bool cancel);
 };
 
 // Render process interface exposed to the browser to handle most of the
@@ -344,4 +339,10 @@ interface PrintManagerHost {
   // Asks the browser to do print preview.
   [EnableIf=enable_print_preview]
   RequestPrintPreview(RequestPrintPreviewParams params);
+
+  // Asks the browser whether the print preview has been cancelled. It
+  // gets true if the preview corresponding to |preview_ui_id| and |request_id|
+  // has been cancelled.
+  [EnableIf=enable_print_preview, Sync]
+  CheckForCancel(int32 preview_ui_id, int32 request_id) => (bool cancel);
 };

components/printing/renderer/print_render_frame_helper.cc

@@ -2481,8 +2481,11 @@ bool PrintRenderFrameHelper::CheckForCancel() {
   const mojom::PrintParams& print_params = *print_pages_params_->params;
   bool cancel = false;
 
-  if (preview_ui_)
-    preview_ui_->CheckForCancel(print_params.preview_request_id, &cancel);
+  if (!GetPrintManagerHost()->CheckForCancel(print_params.preview_ui_id,
+                                             print_params.preview_request_id,
+                                             &cancel)) {
+    cancel = true;
+  }
 
   if (cancel)
     notify_browser_of_print_failure_ = false;

components/printing/test/print_render_frame_helper_browsertest.cc

@@ -165,8 +165,7 @@ class DidPreviewPageListener : public IPC::Listener {
 
 class FakePrintPreviewUI : public mojom::PrintPreviewUI {
  public:
-  explicit FakePrintPreviewUI(PrintMockRenderThread* thread)
-      : thread_(thread) {}
+  FakePrintPreviewUI() = default;
   ~FakePrintPreviewUI() override = default;
 
   mojo::PendingAssociatedRemote<mojom::PrintPreviewUI> BindReceiver() {
@@ -198,10 +197,6 @@ class FakePrintPreviewUI : public mojom::PrintPreviewUI {
     invalid_printer_setting_ = true;
     RunQuitClosure();
   }
-  void CheckForCancel(int32_t request_id,
-                      CheckForCancelCallback callback) override {
-    std::move(callback).Run(thread_->ShouldCancelRequest());
-  }
 
  private:
   void RunQuitClosure() {
@@ -210,7 +205,6 @@ class FakePrintPreviewUI : public mojom::PrintPreviewUI {
     std::move(quit_closure_).Run();
   }
 
-  PrintMockRenderThread* thread_;
   bool preview_failed_ = false;
   bool preview_cancelled_ = false;
   bool invalid_printer_setting_ = false;
@@ -223,8 +217,14 @@ class FakePrintPreviewUI : public mojom::PrintPreviewUI {
 class TestPrintManagerHost
     : public mojom::PrintManagerHostInterceptorForTesting {
  public:
-  TestPrintManagerHost(content::RenderFrame* frame, MockPrinter* printer)
-      : printer_(printer) {
+  TestPrintManagerHost(content::RenderFrame* frame,
+                       PrintMockRenderThread* thread)
+      : printer_(thread->GetPrinter())
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
+        ,
+        thread_(thread)
+#endif
+  {
     Init(frame);
   }
   ~TestPrintManagerHost() override = default;
@@ -351,6 +351,11 @@ class TestPrintManagerHost
   void ShowScriptedPrintPreview(bool source_is_modifiable) override {}
   void RequestPrintPreview(
       mojom::RequestPrintPreviewParamsPtr params) override {}
+  void CheckForCancel(int32_t preview_ui_id,
+                      int32_t request_id,
+                      CheckForCancelCallback callback) override {
+    std::move(callback).Run(thread_->ShouldCancelRequest());
+  }
 #endif
 
   bool IsPrinted() { return is_printed_; }
@@ -391,6 +396,9 @@ class TestPrintManagerHost
   uint32_t number_pages_ = 0;
   bool is_printed_ = false;
   MockPrinter* printer_;
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
+  PrintMockRenderThread* thread_;
+#endif
   base::OnceClosure quit_closure_;
   // True to simulate user clicking print. False to cancel.
   bool print_dialog_user_response_ = true;
@@ -421,9 +429,6 @@ class PrintRenderFrameHelperTestBase : public content::RenderViewTest {
 
     content::RenderViewTest::SetUp();
     BindPrintManagerHost(content::RenderFrame::FromWebFrame(GetMainFrame()));
-#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
-    preview_ui_ = std::make_unique<FakePrintPreviewUI>(print_render_thread_);
-#endif
   }
 
   void TearDown() override {
@@ -437,8 +442,8 @@ class PrintRenderFrameHelperTestBase : public content::RenderViewTest {
   }
 
   void BindPrintManagerHost(content::RenderFrame* frame) {
-    auto print_manager = std::make_unique<TestPrintManagerHost>(
-        frame, print_render_thread_->GetPrinter());
+    auto print_manager =
+        std::make_unique<TestPrintManagerHost>(frame, print_render_thread_);
     GetPrintRenderFrameHelperForFrame(frame)->GetPrintManagerHost();
     print_manager->WaitUntilBinding();
     frame_to_print_manager_map_.emplace(frame, std::move(print_manager));
@@ -454,7 +459,7 @@ class PrintRenderFrameHelperTestBase : public content::RenderViewTest {
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
   void BindToFakePrintPreviewUI() {
     PrintRenderFrameHelper* frame_helper = GetPrintRenderFrameHelper();
-    frame_helper->SetPrintPreviewUI(preview_ui_->BindReceiver());
+    frame_helper->SetPrintPreviewUI(preview_ui_.BindReceiver());
   }
 
   void WaitMojoMessages(base::RunLoop* run_loop) {
@@ -594,12 +599,12 @@ class PrintRenderFrameHelperTestBase : public content::RenderViewTest {
     return it->second.get();
   }
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
-  FakePrintPreviewUI* preview_ui() { return preview_ui_.get(); }
+  FakePrintPreviewUI* preview_ui() { return &preview_ui_; }
 #endif
 
  private:
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
-  std::unique_ptr<FakePrintPreviewUI> preview_ui_;
+  FakePrintPreviewUI preview_ui_;
 #endif
   // Naked pointer as ownership is with
   // |content::RenderViewTest::render_thread_|.

headless/lib/browser/headless_print_manager.cc

@@ -21,9 +21,21 @@
 #include "printing/mojom/print.mojom.h"
 #include "printing/print_job_constants.h"
 #include "printing/units.h"
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
+#include "mojo/public/cpp/bindings/message.h"
+#endif
 
 namespace headless {
 
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
+namespace {
+
+constexpr char kInvalidPathForCheckForCancel[] =
+    "Invalid path for CheckForCancel";
+
+}  // namespace
+#endif
+
 HeadlessPrintSettings::HeadlessPrintSettings()
     : prefer_css_page_size(false),
       landscape(false),
@@ -238,6 +250,16 @@ void HeadlessPrintManager::PrintingFailed(int32_t cookie) {
   ReleaseJob(PRINTING_FAILED);
 }
 
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
+void HeadlessPrintManager::CheckForCancel(int32_t preview_ui_id,
+                                          int32_t request_id,
+                                          CheckForCancelCallback callback) {
+  // CheckForCancel should never be called on HeadlessPrintManager, since this
+  // is only triggered by Print Preview.
+  mojo::ReportBadMessage(kInvalidPathForCheckForCancel);
+}
+#endif
+
 void HeadlessPrintManager::DidPrintDocument(
     printing::mojom::DidPrintDocumentParamsPtr params,
     DidPrintDocumentCallback callback) {

headless/lib/browser/headless_print_manager.h

@@ -100,6 +100,11 @@ class HeadlessPrintManager
                      ScriptedPrintCallback callback) override;
   void ShowInvalidPrinterSettingsError() override;
   void PrintingFailed(int32_t cookie) override;
+#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
+  void CheckForCancel(int32_t preview_ui_id,
+                      int32_t request_id,
+                      CheckForCancelCallback callback) override;
+#endif
 
   void Reset();
   void ReleaseJob(PrintResult result);