Cleanup jobs which have failed.

chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/printing/cups_print_job_manager_impl.h"
 
 #include <cups/cups.h>
 #include <algorithm>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/printing/cups_print_job.h"
 #include "chrome/browser/chromeos/printing/printers_manager.h"
 #include "chrome/browser/chromeos/printing/printers_manager_factory.h"
 #include "chrome/browser/printing/print_job.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
@@ skipping 105 matching lines @@
     print_job->set_state(State::STATE_STARTED);
   } else {
     pages_updated = job.current_pages != print_job->printed_page_number();
     print_job->set_printed_page_number(job.current_pages);
     print_job->set_state(State::STATE_PAGE_DONE);
   }
 
   return pages_updated;
 }
 
-// Updates the state of a print job based on |printer_status| and |job|. Returns
-// true if observers need to be notified of an update.
-bool UpdatePrintJob(const printing::PrinterStatus& printer_status,
-                    const printing::CupsJob& job,
-                    chromeos::CupsPrintJob* print_job) {
-  DCHECK_EQ(job.id, print_job->job_id());
-
-  State old_state = print_job->state();
-
-  bool pages_updated = false;
-  switch (job.state) {
-    case printing::CupsJob::PROCESSING:
-      if (ContainsReason(printer_status, PrinterReason::CONNECTING_TO_DEVICE)) {
-        if (EnforceTimeout(job, print_job)) {
-          LOG(WARNING) << "Connecting to printer timed out";
-          // TODO(skau): Purge job from queue.
-        }
-        break;
-      }
-      pages_updated = UpdateCurrentPage(job, print_job);
-      break;
-    case printing::CupsJob::COMPLETED:
-      DCHECK_GE(job.current_pages, print_job->total_page_number());
-      print_job->set_state(State::STATE_DOCUMENT_DONE);
-      break;
-    case printing::CupsJob::ABORTED:
-    case printing::CupsJob::CANCELED:
-      print_job->set_error_code(ErrorCodeFromReasons(printer_status));
-    // fall through
-    default:
-      print_job->set_state(ConvertState(job.state));
-      break;
-  }
-
-  return print_job->state() != old_state || pages_updated;
-}
-
 }  // namespace
 
 namespace chromeos {
 
 CupsPrintJobManagerImpl::CupsPrintJobManagerImpl(Profile* profile)
     : CupsPrintJobManager(profile),
       cups_connection_(GURL(), HTTP_ENCRYPT_NEVER, false),
       weak_ptr_factory_(this) {
   registrar_.Add(this, chrome::NOTIFICATION_PRINT_JOB_EVENT,
                  content::NotificationService::AllSources());
 }
 
 CupsPrintJobManagerImpl::~CupsPrintJobManagerImpl() {}
 
-bool CupsPrintJobManagerImpl::CancelPrintJob(CupsPrintJob* job) {
-  std::string printer_id = job->printer().id();
-  std::unique_ptr<::printing::CupsPrinter> printer =
-      cups_connection_.GetPrinter(printer_id);
-  if (!printer) {
-    LOG(WARNING) << "Printer not found";
-    return false;
-  }
+// Must be run from the UI thread or jobs_ could become inconsistent.

[Carlson, 2017/03/16 18:36:48]

the "or jobs_ could become inconsistent" suffix here probably is something most
casual users won't care about.

[skau, 2017/03/16 22:14:36]

Done.

+void CupsPrintJobManagerImpl::CancelPrintJob(CupsPrintJob* job) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  return printer->CancelJob(job->job_id());
+  const int job_id = job->job_id();
+
+  // Copy printer_id.  |job| is about to be freed.
+  const std::string printer_id = job->printer().id();
+
+  // Stop montioring jobs after we cancel them.  The user no longer cares.
+  jobs_.erase(job->GetUniqueId());
+
+  // Be sure to copy out all relevant fields.  |job| may be destroyed after we
+  // exit this scope.
+  content::BrowserThread::GetBlockingPool()->PostTask(
+      FROM_HERE,
+      base::Bind(&CupsPrintJobManagerImpl::CancelJobImpl,
+                 weak_ptr_factory_.GetWeakPtr(), printer_id, job_id));
 }
 
 bool CupsPrintJobManagerImpl::SuspendPrintJob(CupsPrintJob* job) {
   NOTREACHED() << "Pause printer is not implemented";
   return false;
 }
 
 bool CupsPrintJobManagerImpl::ResumePrintJob(CupsPrintJob* job) {
   NOTREACHED() << "Resume printer is not implemented";
   return false;
@@ skipping 16 matching lines @@
                    base::UTF16ToUTF8(document->settings().title()),
                    job_details->job_id(), document->page_count());
   }
 }
 
 bool CupsPrintJobManagerImpl::CreatePrintJob(const std::string& printer_name,
                                              const std::string& title,
                                              int job_id,
                                              int total_page_number) {
   auto printer =
-      chromeos::PrintersManagerFactory::GetForBrowserContext(profile_)
-          ->GetPrinter(printer_name);
+      PrintersManagerFactory::GetForBrowserContext(profile_)->GetPrinter(
+          printer_name);
   if (!printer) {
     LOG(WARNING) << "Printer was removed while job was in progress.  It cannot "
                     "be tracked";
     return false;
   }
 
   // Create a new print job.
   auto cpj = base::MakeUnique<CupsPrintJob>(*printer, job_id, title,
                                             total_page_number);
   std::string key = cpj->GetUniqueId();
@@ skipping 35 matching lines @@
   }
   std::vector<std::string> ids{printer_ids.begin(), printer_ids.end()};
 
   content::BrowserThread::PostTaskAndReplyWithResult(
       content::BrowserThread::FILE_USER_BLOCKING, FROM_HERE,
       base::Bind(&QueryCups, &cups_connection_, ids),
       base::Bind(&CupsPrintJobManagerImpl::UpdateJobs,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
+bool CupsPrintJobManagerImpl::UpdatePrintJob(
+    const ::printing::PrinterStatus& printer_status,
+    const ::printing::CupsJob& job,
+    CupsPrintJob* print_job) {
+  DCHECK_EQ(job.id, print_job->job_id());
+
+  State old_state = print_job->state();
+
+  bool pages_updated = false;
+  switch (job.state) {
+    case ::printing::CupsJob::PROCESSING:
+      if (ContainsReason(printer_status, PrinterReason::CONNECTING_TO_DEVICE)) {
+        if (EnforceTimeout(job, print_job)) {
+          LOG(WARNING) << "Connecting to printer timed out";
+          print_job->set_expired(true);
+        }
+        break;

[Carlson, 2017/03/16 18:36:48]

Nit:  I think it would be slightly more natural to put the UpdateCurrentPage in
an else block instead of breaking at the end of the if to avoid executing it.

[skau, 2017/03/16 22:14:36]

Done.

+      }
+      pages_updated = UpdateCurrentPage(job, print_job);
+      break;
+    case ::printing::CupsJob::COMPLETED:
+      DCHECK_GE(job.current_pages, print_job->total_page_number());
+      print_job->set_state(State::STATE_DOCUMENT_DONE);
+      break;
+    case ::printing::CupsJob::ABORTED:
+    case ::printing::CupsJob::CANCELED:
+      print_job->set_error_code(ErrorCodeFromReasons(printer_status));
+    // fall through
+    default:
+      print_job->set_state(ConvertState(job.state));
+      break;
+  }
+
+  return print_job->state() != old_state || pages_updated;
+}
+
 // Use job information to update local job states.  Previously completed jobs
 // could be in |jobs| but those are ignored as we will not emit updates for them
 // after they are completed.
 void CupsPrintJobManagerImpl::UpdateJobs(const QueryResult& result) {
   const std::vector<::printing::QueueStatus>& queues = result.queues;
 
   // Query has completed.  Allow more queries.
   in_query_ = false;
 
   // If the query failed, either retry or purge.
@@ skipping 24 matching lines @@
       if (entry == jobs_.end())
         continue;
 
       CupsPrintJob* print_job = entry->second.get();
 
       if (UpdatePrintJob(queue.printer_status, job, print_job)) {
         // The state of the job changed, notify observers.
         NotifyJobStateUpdate(print_job);
       }
 
-      // Cleanup completed jobs.
-      if (print_job->IsJobFinished()) {
+      if (print_job->expired()) {
+        // Job needs to be forcibly cancelled.
+        CancelPrintJob(print_job);
+        // Beware, print_job was removed from jobs_ and deleted.
+      } else if (print_job->IsJobFinished()) {
+        // Cleanup completed jobs.
         jobs_.erase(entry);
       } else {
         active_jobs.push_back(key);
       }
     }
   }
 
   // Keep polling until all jobs complete or error.
   if (!active_jobs.empty()) {
     // During normal operations, we poll at the default rate.
@@ skipping 10 matching lines @@
   for (const auto& entry : jobs_) {
     // Declare all lost jobs errors.
     CupsPrintJob* job = entry.second.get();
     job->set_state(CupsPrintJob::State::STATE_ERROR);
     NotifyJobStateUpdate(job);
   }
 
   jobs_.clear();
 }
 
+void CupsPrintJobManagerImpl::CancelJobImpl(const std::string& printer_id,
+                                            const int job_id) {
+  std::unique_ptr<::printing::CupsPrinter> printer =
+      cups_connection_.GetPrinter(printer_id);
+  if (!printer) {
+    LOG(WARNING) << "Printer not found";

[Carlson, 2017/03/16 18:36:48]

Would it be useful to add printer_id and/or job_id to this log message and the
one below to help track down what happened?

[skau, 2017/03/16 22:14:36]

I'll log the printer_id.  I don't think the job_id will have much meaning in the
logs.

+    return;
+  }
+
+  if (!printer->CancelJob(job_id)) {
+    // This is not expected to fail but log it if it does.
+    LOG(WARNING) << "Cancelling job failed.  Job may be stuck in queue.";
+  }
+}
+
 void CupsPrintJobManagerImpl::NotifyJobStateUpdate(CupsPrintJob* job) {
   switch (job->state()) {
     case State::STATE_NONE:
       // State does not require notification.
       break;
     case State::STATE_WAITING:
       NotifyJobUpdated(job);
       break;
     case State::STATE_STARTED:
       NotifyJobStarted(job);
@@ skipping 18 matching lines @@
       break;
   }
 }
 
 // static
 CupsPrintJobManager* CupsPrintJobManager::CreateInstance(Profile* profile) {
   return new CupsPrintJobManagerImpl(profile);
 }
 
 }  // namespace chromeos