Fix crash when displaying system print dialog on Linux.

Fix crash when displaying system print dialog on Linux.
Uses a render view ID to find a correct window handle to use as a parent
for the system dialog.
Uses a series of PostTask calls to cause the various steps of
getting print settings to occur on the correct threads.

BUG=130095
TEST=Verify 130095


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141726

[Albert Bodenhamer, 2012-06-08 23:27:41]

Tested on Linux and Win.  I'm testing on Mac now.

[Albert Bodenhamer, 2012-06-11 20:48:06]

A herd of hairy yaks stands between me and a working Mac build.  Can one of you
patch this in and and try opening the system print dialog?

[Lei Zhang, 2012-06-11 21:23:06]

I'll do a Mac build and see what happens.

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:282: params.render_view_id,
settings_params,
I'm not sure we need the |render_view_id| param. I think it's the same as
reply_msg->routing_id().

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:284: printer_query,
reply_msg), printer_query));
nit: Can you put the second |printer_query| on a separate line? Otherwise it
looks like part of the Bind().

[Albert Bodenhamer, 2012-06-11 21:54:08]

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:282: params.render_view_id,
settings_params,
On 2012/06/11 21:23:06, Lei Zhang wrote:
> I'm not sure we need the |render_view_id| param. I think it's the same as
> reply_msg->routing_id().

reply_msg->routing_id() is often 0x7fffffff which causes a crash when trying to
get the webcontents from the rvh.  I'm not sure why, but it's not reliable. 
Passing a separate render_view_id works reliably.

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:284: printer_query,
reply_msg), printer_query));
On 2012/06/11 21:23:06, Lei Zhang wrote:
> nit: Can you put the second |printer_query| on a separate line? Otherwise it
> looks like part of the Bind().

Done.

[Lei Zhang, 2012-06-11 22:17:10]

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:282: params.render_view_id,
settings_params,
On 2012/06/11 21:54:08, Albert Bodenhamer wrote:
> On 2012/06/11 21:23:06, Lei Zhang wrote:
> > I'm not sure we need the |render_view_id| param. I think it's the same as
> > reply_msg->routing_id().
> 
> reply_msg->routing_id() is often 0x7fffffff which causes a crash when trying
to
> get the webcontents from the rvh.  I'm not sure why, but it's not reliable. 
> Passing a separate render_view_id works reliably.
> 

Weird. I have not seen that in my testing. Is there a way to reproduce this
case? Presumably the routing_id from the renderer is always valid, so I wonder
how it changed in the reply message.

[Albert Bodenhamer, 2012-06-11 22:54:57]

On 2012/06/11 22:17:10, Lei Zhang wrote:
>
http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
> File chrome/browser/printing/printing_message_filter.cc (right):
> 
>
http://codereview.chromium.org/10544084/diff/12001/chrome/browser/printing/pr...
> chrome/browser/printing/printing_message_filter.cc:282: params.render_view_id,
> settings_params,
> On 2012/06/11 21:54:08, Albert Bodenhamer wrote:
> > On 2012/06/11 21:23:06, Lei Zhang wrote:
> > > I'm not sure we need the |render_view_id| param. I think it's the same as
> > > reply_msg->routing_id().
> > 
> > reply_msg->routing_id() is often 0x7fffffff which causes a crash when trying
> to
> > get the webcontents from the rvh.  I'm not sure why, but it's not reliable. 
> > Passing a separate render_view_id works reliably.
> > 
> 
> Weird. I have not seen that in my testing. Is there a way to reproduce this
> case? Presumably the routing_id from the renderer is always valid, so I wonder
> how it changed in the reply message.

Found it.  It was a side effect of converting GetDefaultPrintSettings to a
CONTROL message.  Switching it back to a ROUTED message works properly.

I also removed the extra render_view_id from the OnScriptedPrint message while I
was at it.

[Lei Zhang, 2012-06-11 23:32:28]

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
File chrome/browser/printing/printing_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.cc:165:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
If you do this, then |g_printing_file_descriptor_map| needs locking.

Do we have to do this on the UI thread? How did CrOS get away with calling the
same methods from the FILE thread?

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
File chrome/browser/printing/printing_message_filter.h (right):

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.h:14: #include
"content/public/browser/web_contents.h"
Forward declare.

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.h:68: struct
GetPrintSettingsForRenderViewParams {
I think you can just forward declare this here and move the definition to the
.cc file as:

struct PrinterMessageFilter::GetPrintSettingsForRenderViewParams {
...
};

Then you can also keep PrinterQuery forward declared in this header.

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/renderer/pri...
File chrome/renderer/print_web_view_helper.cc (left):

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/renderer/pri...
chrome/renderer/print_web_view_helper.cc:1545: params.host_window_id =
render_view()->GetHostWindow();
FYI, this is the only user of GetHostWindow(). Shall we just remove the
GetHostWindow code in content while we're at it?

[Albert Bodenhamer, 2012-06-11 23:54:58]

http://codereview.chromium.org/10544084/diff/1009/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/10544084/diff/1009/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:165:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Argh!  Yeah.  I should have caught that.

CrOS DIDN'T get away with calling this on the FILE thread. 
WebContents::FromRenderViewHost was DCHECKing because it wasn't called from the
UI thread.  It only ever worked because it used to use BrowserList.

I'll need to call it from the FILE thread and then post a task to the UI thread
to finish the job.  Thankfully CreatePrintDialogForFile can be called from the
UI thread so it's not too complicated.

On 2012/06/11 23:32:28, Lei Zhang wrote:
> If you do this, then |g_printing_file_descriptor_map| needs locking.
> 
> Do we have to do this on the UI thread? How did CrOS get away with calling the
> same methods from the FILE thread?

[Albert Bodenhamer, 2012-06-12 01:19:02]

Adding John for the content changes.

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
File chrome/browser/printing/printing_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.cc:165:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/06/11 23:32:28, Lei Zhang wrote:
> If you do this, then |g_printing_file_descriptor_map| needs locking.
> 
> Do we have to do this on the UI thread? How did CrOS get away with calling the
> same methods from the FILE thread?

Done.

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
File chrome/browser/printing/printing_message_filter.h (right):

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.h:14: #include
"content/public/browser/web_contents.h"
On 2012/06/11 23:32:28, Lei Zhang wrote:
> Forward declare.

Done.

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.h:68: struct
GetPrintSettingsForRenderViewParams {
On 2012/06/11 23:32:28, Lei Zhang wrote:
> I think you can just forward declare this here and move the definition to the
> .cc file as:
> 
> struct PrinterMessageFilter::GetPrintSettingsForRenderViewParams {
> ...
> };
> 
> Then you can also keep PrinterQuery forward declared in this header.

Done.

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/renderer/pri...
File chrome/renderer/print_web_view_helper.cc (left):

https://chromiumcodereview.appspot.com/10544084/diff/1009/chrome/renderer/pri...
chrome/renderer/print_web_view_helper.cc:1545: params.host_window_id =
render_view()->GetHostWindow();
On 2012/06/11 23:32:28, Lei Zhang wrote:
> FYI, this is the only user of GetHostWindow(). Shall we just remove the
> GetHostWindow code in content while we're at it?

Done.

[jam, 2012-06-12 01:46:57]

content lgtm

[Lei Zhang, 2012-06-12 01:47:03]

LGTM, with a few nits below.

Works on Mac too.

https://chromiumcodereview.appspot.com/10544084/diff/11/chrome/browser/printi...
File chrome/browser/printing/printing_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10544084/diff/11/chrome/browser/printi...
chrome/browser/printing/printing_message_filter.cc:162: void
PrintingMessageFilter::OnTempFileForPrintingWrittenImpl(
nit: Can you put this below OnTempFileForPrintingWritten(), in the same order
they are declared?

https://chromiumcodereview.appspot.com/10544084/diff/11/chrome/browser/printi...
chrome/browser/printing/printing_message_filter.cc:162: void
PrintingMessageFilter::OnTempFileForPrintingWrittenImpl(
nit: Maybe rename to CreatePrintDialogForFile() ?

https://chromiumcodereview.appspot.com/10544084/diff/11/chrome/browser/printi...
chrome/browser/printing/printing_message_filter.cc:185: FilePath path =
it->second;
nit: Can this be a FilePath& ?

https://chromiumcodereview.appspot.com/10544084/diff/11/chrome/browser/printi...
chrome/browser/printing/printing_message_filter.cc:228: IPC::Message* reply_msg)
{
nit: this fits on the previous line, like before.

[Albert Bodenhamer, 2012-06-12 18:50:59]

I finally got my mac build working and it tests fine for me as well.

http://codereview.chromium.org/10544084/diff/11/chrome/browser/printing/print...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/10544084/diff/11/chrome/browser/printing/print...
chrome/browser/printing/printing_message_filter.cc:162: void
PrintingMessageFilter::OnTempFileForPrintingWrittenImpl(
On 2012/06/12 01:47:03, Lei Zhang wrote:
> nit: Can you put this below OnTempFileForPrintingWritten(), in the same order
> they are declared?

Done.

http://codereview.chromium.org/10544084/diff/11/chrome/browser/printing/print...
chrome/browser/printing/printing_message_filter.cc:162: void
PrintingMessageFilter::OnTempFileForPrintingWrittenImpl(
On 2012/06/12 01:47:03, Lei Zhang wrote:
> nit: Maybe rename to CreatePrintDialogForFile() ?

Done.

http://codereview.chromium.org/10544084/diff/11/chrome/browser/printing/print...
chrome/browser/printing/printing_message_filter.cc:185: FilePath path =
it->second;
On 2012/06/12 01:47:03, Lei Zhang wrote:
> nit: Can this be a FilePath& ?

Done.

http://codereview.chromium.org/10544084/diff/11/chrome/browser/printing/print...
chrome/browser/printing/printing_message_filter.cc:228: IPC::Message* reply_msg)
{
On 2012/06/12 01:47:03, Lei Zhang wrote:
> nit: this fits on the previous line, like before.

Done.

[commit-bot: I haz the power, 2012-06-12 18:51:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abodenha@chromium.org/10544084/11007

[commit-bot: I haz the power, 2012-06-12 20:28:12]

Change committed as 141726