Revert 249608 "Don't run internal message loop from PrintJob::Co..."

> Don't run internal message loop from PrintJob::ControlledWorkerShutdown()
> 
> If loops is running, and render is closed, the RenderViewHostImpl that called ControlledWorkerShutdown is going to be destroyed. So RenderViewHostImpl crashes immediately after returning from ControlledWorkerShutdown.
> 
> Solution it to use delayed messages instead of nested loop.
> 
> BUG=313274
> NOTRY=true
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249561
> 
> Review URL: https://codereview.chromium.org/136733005

TBR=vitalybuka@chromium.org

Review URL: https://codereview.chromium.org/157223003

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

3 files changed, 71 insertions, 26 deletions

diff --git a/chrome/browser/printing/print_job.cc b/chrome/browser/printing/print_job.cc
index 78d49c7..a899b17 100644
--- a/chrome/browser/printing/print_job.cc
+++ b/chrome/browser/printing/print_job.cc
@@ -37,6 +37,8 @@ PrintJob::PrintJob()
       settings_(),
       is_job_pending_(false),
       is_canceling_(false),
+      is_stopping_(false),
+      is_stopped_(false),
       quit_factory_(this),
       weak_ptr_factory_(this) {
   DCHECK(ui_message_loop_);
@@ -161,12 +163,16 @@ void PrintJob::Stop() {
   // Be sure to live long enough.
   scoped_refptr<PrintJob> handle(this);
 
-  if (worker_->message_loop()) {
+  base::MessageLoop* worker_loop = worker_->message_loop();
+  if (worker_loop) {
     ControlledWorkerShutdown();
-  } else {
-    // Flush the cached document.
-    UpdatePrintedDocument(NULL);
+
+    is_job_pending_ = false;
+    registrar_.Remove(this, chrome::NOTIFICATION_PRINT_JOB_EVENT,
+                      content::Source<PrintJob>(this));
   }
+  // Flush the cached document.
+  UpdatePrintedDocument(NULL);
 }
 
 void PrintJob::Cancel() {
@@ -220,6 +226,14 @@ bool PrintJob::is_job_pending() const {
   return is_job_pending_;
 }
 
+bool PrintJob::is_stopping() const {
+  return is_stopping_;
+}
+
+bool PrintJob::is_stopped() const {
+  return is_stopped_;
+}
+
 PrintedDocument* PrintJob::document() const {
   return document_.get();
 }
@@ -313,34 +327,53 @@ void PrintJob::ControlledWorkerShutdown() {
   // deadlock is eliminated.
   worker_->StopSoon();
 
-  // Delay shutdown until the worker terminates.  We want this code path
-  // to wait on the thread to quit before continuing.
-  if (worker_->IsRunning()) {
-    base::MessageLoop::current()->PostDelayedTask(
-        FROM_HERE,
-        base::Bind(&PrintJob::ControlledWorkerShutdown, this),
-        base::TimeDelta::FromMilliseconds(100));
-    return;
+  // 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().platform_handle();
+  for (; thread_handle;) {
+    // Note that we don't do any kind of message prioritization 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 occurred. Assume the thread quit.
+      NOTREACHED();
+      break;
+    }
   }
 #endif
 
 
   // Now make sure the thread object is cleaned up. Do this on a worker
   // thread because it may block.
+  is_stopping_ = true;
+
   base::WorkerPool::PostTaskAndReply(
       FROM_HERE,
       base::Bind(&PrintJobWorker::Stop,
                  base::Unretained(worker_.get())),
-      base::Bind(&PrintJob::HoldUntilStopIsCalled, this),
+      base::Bind(&PrintJob::HoldUntilStopIsCalled,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 scoped_refptr<PrintJob>(this)),
       false);
-
-  is_job_pending_ = false;
-  registrar_.Remove(this, chrome::NOTIFICATION_PRINT_JOB_EVENT,
-                    content::Source<PrintJob>(this));
-  UpdatePrintedDocument(NULL);
 }
 
-void PrintJob::HoldUntilStopIsCalled() {
+void PrintJob::HoldUntilStopIsCalled(const scoped_refptr<PrintJob>&) {
+  is_stopped_ = true;
+  is_stopping_ = false;
 }
 
 void PrintJob::Quit() {
diff --git a/chrome/browser/printing/print_job.h b/chrome/browser/printing/print_job.h
index 69ff756..e342250 100644
--- a/chrome/browser/printing/print_job.h
+++ b/chrome/browser/printing/print_job.h
@@ -88,6 +88,12 @@ class PrintJob : public PrintJobWorkerOwner,
   // and the end of the spooling.
   bool is_job_pending() const;
 
+  // Returns true if the worker thread is in the process of stopping.
+  bool is_stopping() const;
+
+  // Returns true if the worker thread has stopped.
+  bool is_stopped() const;
+
   // Access the current printed document. Warning: may be NULL.
   PrintedDocument* document() const;
 
@@ -112,7 +118,7 @@ class PrintJob : public PrintJobWorkerOwner,
   // Called at shutdown when running a nested message loop.
   void Quit();
 
-  void HoldUntilStopIsCalled();
+  void HoldUntilStopIsCalled(const scoped_refptr<PrintJob>& job);
 
   content::NotificationRegistrar registrar_;
 
@@ -142,6 +148,12 @@ class PrintJob : public PrintJobWorkerOwner,
   // the notified calls Cancel() again.
   bool is_canceling_;
 
+  // Is the worker thread stopping.
+  bool is_stopping_;
+
+  // Is the worker thread stopped.
+  bool is_stopped_;
+
   // Used at shutdown so that we can quit a nested message loop.
   base::WeakPtrFactory<PrintJob> quit_factory_;
 
diff --git a/chrome/browser/printing/print_job_unittest.cc b/chrome/browser/printing/print_job_unittest.cc
index 5ea1189..f67bc60 100644
--- a/chrome/browser/printing/print_job_unittest.cc
+++ b/chrome/browser/printing/print_job_unittest.cc
@@ -106,15 +106,15 @@ TEST_F(PrintJobTest, SimplePrint) {
   TestSource source;
   job->Initialize(owner.get(), &source, 1);
   job->Stop();
-  EXPECT_TRUE(job->document());
-  while (job->document()) {
+  EXPECT_FALSE(job->is_stopped());
+  EXPECT_TRUE(job->is_stopping());
+  while (!job->is_stopped())
+  {
     current.RunUntilIdle();
   }
-  EXPECT_FALSE(job->document());
+  EXPECT_TRUE(job->is_stopped());
+  EXPECT_FALSE(job->is_stopping());
   job = NULL;
-  while (!check) {
-    current.RunUntilIdle();
-  }
   EXPECT_TRUE(check);
 }