Skip to content

Commit

Permalink
Revert 249561 "Don't run internal message loop from PrintJob::Co..."
Browse files Browse the repository at this point in the history
> 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
> 
> Review URL: https://codereview.chromium.org/136733005

Suspect CL for http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/26310/steps/unit_tests/logs/stdio

[ RUN      ] PrintJobTest.SimplePrint
c:\b\build\slave\cr-win-dbg\build\src\chrome\browser\printing\print_job_unittest.cc(110): error: Value of: job->is_stopping()
  Actual: false
Expected: true
[  FAILED  ] PrintJobTest.SimplePrint (218 ms)

TBR=vitalybuka@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@249585 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
piman@chromium.org committed Feb 7, 2014
1 parent 04b5b37 commit f068891
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 20 deletions.
57 changes: 38 additions & 19 deletions chrome/browser/printing/print_job.cc
Original file line number Diff line number Diff line change
Expand Up @@ -163,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() {
Expand Down Expand Up @@ -323,14 +327,32 @@ 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

Expand All @@ -343,16 +365,13 @@ void PrintJob::ControlledWorkerShutdown() {
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;
}
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/printing/print_job.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,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_;

Expand Down

0 comments on commit f068891

Please sign in to comment.