Use the new Thread::StopSoon() facility. Add a new tight loop to control the worker thread shutdown in a way that is safe even for printer driver that displays a dialog box and attaches to the browser window.

BUG=1274015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@729 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 66 insertions, 32 deletions

diff --git a/chrome/browser/printing/print_job.cc b/chrome/browser/printing/print_job.cc
index 54713cf..0b6ec2f 100644
--- a/chrome/browser/printing/print_job.cc
+++ b/chrome/browser/printing/print_job.cc
@@ -46,7 +46,6 @@ PrintJob::PrintJob(PrintedPagesSource* source)
       source_(source),
       is_job_pending_(false),
       is_print_dialog_box_shown_(false),
-      is_blocking_(false),
       is_canceling_(false) {
 }
 
@@ -57,7 +56,6 @@ PrintJob::PrintJob()
       settings_(),
       is_job_pending_(false),
       is_print_dialog_box_shown_(false),
-      is_blocking_(false),
       is_canceling_(false) {
 }
 
@@ -65,7 +63,6 @@ PrintJob::~PrintJob() {
   // The job should be finished (or at least canceled) when it is destroyed.
   DCHECK(!is_job_pending_);
   DCHECK(!is_print_dialog_box_shown_);
-  DCHECK(!is_blocking_);
   DCHECK(!is_canceling_);
   DCHECK(worker_->message_loop() == NULL);
   DCHECK_EQ(ui_message_loop_, MessageLoop::current());
@@ -77,7 +74,6 @@ void PrintJob::Initialize(PrintJobWorkerOwner* job,
   DCHECK(!worker_.get());
   DCHECK(!is_job_pending_);
   DCHECK(!is_print_dialog_box_shown_);
-  DCHECK(!is_blocking_);
   DCHECK(!is_canceling_);
   DCHECK(!document_.get());
   source_ = source;
@@ -124,7 +120,6 @@ void PrintJob::Observe(NotificationType type,
 void PrintJob::GetSettingsDone(const PrintSettings& new_settings,
                                PrintingContext::Result result) {
   DCHECK(!is_job_pending_);
-  DCHECK(!is_blocking_);
 
   if (!source_ || result == PrintingContext::FAILED) {
     // The source is gone, there's nothing to do.
@@ -175,9 +170,8 @@ void PrintJob::GetSettings(GetSettingsAskParam ask_user_for_settings,
   DCHECK_EQ(ui_message_loop_, MessageLoop::current());
   DCHECK(!is_job_pending_);
   DCHECK(!is_print_dialog_box_shown_);
-  DCHECK(!is_blocking_);
   // Is not reentrant.
-  if (is_job_pending_ || is_blocking_)
+  if (is_job_pending_)
     return;
 
   // Lazy create the worker thread. There is one worker thread per print job.
@@ -207,8 +201,7 @@ void PrintJob::StartPrinting() {
   DCHECK(worker_->message_loop());
   DCHECK(!is_job_pending_);
   DCHECK(!is_print_dialog_box_shown_);
-  DCHECK(!is_blocking_);
-  if (!worker_->message_loop() || is_job_pending_ || is_blocking_)
+  if (!worker_->message_loop() || is_job_pending_)
     return;
 
   // Real work is done in PrintJobWorker::StartPrinting().
@@ -235,25 +228,20 @@ void PrintJob::Stop() {
   MessageLoop* worker_loop = worker_->message_loop();
   if (worker_loop) {
     if (is_print_dialog_box_shown_) {
-      // Make sure there is no dialog box.
+      // Make sure there is no Print... dialog box.
       worker_loop->PostTask(FROM_HERE, NewRunnableMethod(
           worker_.get(), &PrintJobWorker::DismissDialog));
       is_print_dialog_box_shown_ = false;
     }
-    // It will wait infinitely for the worker thread to quit.
-    worker_->NonBlockingStop();
+
+    ControlledWorkerShutdown();
+
     is_job_pending_ = false;
     NotificationService::current()->RemoveObserver(
       this, NOTIFY_PRINT_JOB_EVENT, Source<PrintJob>(this));
   }
   // Flush the cached document.
   UpdatePrintedDocument(NULL);
-
-  if (is_blocking_) {
-    // Make sure we don't get stuck in an inner message loop.
-    MessageLoop::current()->Quit();
-    is_blocking_ = false;
-  }
 }
 
 void PrintJob::Cancel() {
@@ -285,8 +273,7 @@ void PrintJob::Cancel() {
 bool PrintJob::RequestMissingPages() {
   DCHECK_EQ(ui_message_loop_, MessageLoop::current());
   DCHECK(!is_print_dialog_box_shown_);
-  DCHECK(!is_blocking_);
-  if (!is_job_pending_ || is_print_dialog_box_shown_ || is_blocking_)
+  if (!is_job_pending_ || is_print_dialog_box_shown_)
     return false;
 
   MessageLoop* worker_loop = worker_.get() ? worker_->message_loop() : NULL;
@@ -314,7 +301,6 @@ bool PrintJob::FlushJob(int timeout_ms) {
         false));
   }
 
-  is_blocking_ = true;
   // Stop() will eventually be called, which will get out of the inner message
   // loop. But, don't take it for granted and set a timer in case something goes
   // wrong.
@@ -431,6 +417,52 @@ void PrintJob::OnDocumentDone() {
       Details<JobEventDetails>(details.get()));
 }
 
+void PrintJob::ControlledWorkerShutdown() {
+  DCHECK_EQ(ui_message_loop_, MessageLoop::current());
+  // We could easily get into a deadlock case if worker_->Stop() is used; the
+  // printer driver created a window as a child of the browser window. By
+  // canceling the job, the printer driver initiated dialog box is destroyed,
+  // which sends a blocking message to its parent window. If the browser window
+  // thread is not processing messages, a deadlock occurs.
+  //
+  // This function ensures that the dialog box will be destroyed in a timely
+  // manner by the mere fact that the thread will terminate. So the potential
+  // deadlock is eliminated.
+  worker_->StopSoon();
+
+  // Run a tight message loop until the worker terminates. It may seems like a
+  // hack but I see no other way to get it to work flawlessly. The issues here
+  // are:
+  // - We don't want to run tasks while the thread is quitting.
+  // - We want this code path to wait on the thread to quit before continuing.
+  MSG msg;
+  HANDLE thread_handle = worker_->thread_handle();
+  for (; thread_handle;) {
+    // Note that we don't do any kind of message priorization since we don't
+    // execute any pending task or timer.
+    DWORD result = MsgWaitForMultipleObjects(1, &thread_handle,
+                                             FALSE, INFINITE, QS_ALLINPUT);
+    if (result == WAIT_OBJECT_0 + 1) {
+      while (PeekMessage(&msg, NULL, 0, 0, TRUE) > 0) {
+        TranslateMessage(&msg);
+        DispatchMessage(&msg);
+      }
+      // Continue looping.
+    }
+    else if (result == WAIT_OBJECT_0) {
+      // The thread quit.
+      break;
+    } else {
+      // An error occured. Assume the thread quit.
+      NOTREACHED();
+      break;
+    }
+  }
+
+  // Now make sure the thread object is cleaned up.
+  worker_->Stop();
+}
+
 // Takes settings_ ownership and will be deleted in the receiving thread.
 JobEventDetails::JobEventDetails(Type type,
                                  PrintedDocument* document,
diff --git a/chrome/browser/printing/print_job.h b/chrome/browser/printing/print_job.h
index 6f0accd..6d9a6bf 100644
--- a/chrome/browser/printing/print_job.h
+++ b/chrome/browser/printing/print_job.h
@@ -27,8 +27,8 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-#ifndef CHROME_BROWSER_PRINTING_PRINT_JOB_H__
-#define CHROME_BROWSER_PRINTING_PRINT_JOB_H__
+#ifndef CHROME_BROWSER_PRINTING_PRINT_JOB_H_
+#define CHROME_BROWSER_PRINTING_PRINT_JOB_H_
 
 #include "base/ref_counted.h"
 #include "chrome/browser/printing/print_job_worker_owner.h"
@@ -153,6 +153,10 @@ class PrintJob : public base::RefCountedThreadSafe<PrintJob>,
   // notification.
   void OnDocumentDone();
 
+  // Terminates the worker thread in a very controlled way, to work around any
+  // eventual deadlock.
+  void ControlledWorkerShutdown();
+
   // Main message loop reference. Used to send notifications in the right
   // thread.
   MessageLoop* const ui_message_loop_;
@@ -178,14 +182,11 @@ class PrintJob : public base::RefCountedThreadSafe<PrintJob>,
   // Is the Print... dialog box currently shown.
   bool is_print_dialog_box_shown_;
 
-  // Is GetSettings() blocking to act like a synchronous function?
-  bool is_blocking_;
-
   // Is Canceling? If so, try to not cause recursion if on FAILED notification,
   // the notified calls Cancel() again.
   bool is_canceling_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(PrintJob);
+  DISALLOW_COPY_AND_ASSIGN(PrintJob);
 };
 
 // Details for a NOTIFY_PRINT_JOB_EVENT notification. The members may be NULL.
@@ -241,9 +242,9 @@ class JobEventDetails : public base::RefCountedThreadSafe<JobEventDetails> {
   scoped_refptr<PrintedPage> page_;
   const Type type_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(JobEventDetails);
+  DISALLOW_COPY_AND_ASSIGN(JobEventDetails);
 };
 
 }  // namespace printing
 
-#endif  // CHROME_BROWSER_PRINTING_PRINT_JOB_H__
+#endif  // CHROME_BROWSER_PRINTING_PRINT_JOB_H_
diff --git a/chrome/browser/printing/print_job_worker.cc b/chrome/browser/printing/print_job_worker.cc
index 5a26ef91..39a92bb 100644
--- a/chrome/browser/printing/print_job_worker.cc
+++ b/chrome/browser/printing/print_job_worker.cc
@@ -162,11 +162,12 @@ void PrintJobWorker::OnDocumentChanged(PrintedDocument* new_document) {
 }
 
 void PrintJobWorker::OnNewPage() {
-  DCHECK_EQ(message_loop(), MessageLoop::current());
   if (!document_.get()) {
     // Spurious message.
     return;
   }
+  // message_loop() could return NULL when the print job is cancelled.
+  DCHECK_EQ(message_loop(), MessageLoop::current());
   DCHECK(printing_context_.context());
   if (!printing_context_.context())
     return;
diff --git a/chrome/browser/printing/printing_layout_uitest.cc b/chrome/browser/printing/printing_layout_uitest.cc
index 9a6d04c..ffd20d7 100644
--- a/chrome/browser/printing/printing_layout_uitest.cc
+++ b/chrome/browser/printing/printing_layout_uitest.cc
@@ -565,7 +565,7 @@ TEST_F(PrintingLayoutTestHidden, ManyTimes) {
 }
 
 // Prints a popup and immediately closes it.
-TEST_F(PrintingLayoutTest, DISABLED_Delayed) {
+TEST_F(PrintingLayoutTest, Delayed) {
   if (IsTestCaseDisabled())
     return;
 
@@ -605,7 +605,7 @@ TEST_F(PrintingLayoutTest, DISABLED_Delayed) {
 }
 
 // Prints a popup and immediately closes it.
-TEST_F(PrintingLayoutTest, DISABLED_IFrame) {
+TEST_F(PrintingLayoutTest, IFrame) {
   if (IsTestCaseDisabled())
     return;