Cancel prerenders for pages that call window.print()

Cancel prerenders for pages that call window.print()

BUG=82206
TEST=PrerenderBrowserTest.PrerenderPopup


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

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

[cbentzel, 2011-05-13 12:00:28]

Generally looks good.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:80:
host->channel()->AddFilter(new prerender::PrerenderMessageFilter(
Does ordering of filters matter? I'm guessing it does in this case, where you
want the prerendermessagefilter to intercept the printing IPCs before
PrintingMessageFilter does.

If so, please comment on the ordering dependency.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:42:
PrintMsg_PrintPages_Params* settings) {
DCHECK that this on the UI thread? You'll get that anyway if you have a weak
pointer for PrerenderManager and call .get(), but it might be worth keeping.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:49: // Shamelessly ripped
from PrintViewManager to clean up the print worker.
Should this be abstracted to a separate function?

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.h (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.h:35: PrerenderManager*
prerender_manager_;
This should be using a WeakPtr for the PrerenderManager, I think.

You may run into a corner-case race where a message is seen on IO thread and
forwarded to the UI thread. In the interim, the PrerenderManager is destroyed on
the UI thread. Stale memory is then accessed.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.h:37: // 
DISALLOW_COPY_AND_ASSIGN(PrerenderMessageFilter);
Why is this commented out?

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_observer.cc (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_observer.cc:12: #include
"googleurl/src/gurl.h"
This include doesn't look necessary. Or is this one of those IWYU issues?

[cbentzel, 2011-05-13 14:36:21]

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:44: // The print query will
be destroyed by the TabContents destruction.
BUG: The PrerenderContents may have gone away at this point in time because the
prerendered page was swapped in during the IO->UI transition. Don't cancel
printing in that case.

[dominich, 2011-05-13 15:05:33]

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:80:
host->channel()->AddFilter(new prerender::PrerenderMessageFilter(
On 2011/05/13 12:00:28, cbentzel wrote:
> Does ordering of filters matter? I'm guessing it does in this case, where you
> want the prerendermessagefilter to intercept the printing IPCs before
> PrintingMessageFilter does.
> 
> If so, please comment on the ordering dependency.

Done.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:42:
PrintMsg_PrintPages_Params* settings) {
On 2011/05/13 12:00:28, cbentzel wrote:
> DCHECK that this on the UI thread? You'll get that anyway if you have a weak
> pointer for PrerenderManager and call .get(), but it might be worth keeping.

Done.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:44: // The print query will
be destroyed by the TabContents destruction.
On 2011/05/13 14:36:21, cbentzel wrote:
> BUG: The PrerenderContents may have gone away at this point in time because
the
> prerendered page was swapped in during the IO->UI transition. Don't cancel
> printing in that case. 

Done.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:49: // Shamelessly ripped
from PrintViewManager to clean up the print worker.
On 2011/05/13 12:00:28, cbentzel wrote:
> Should this be abstracted to a separate function?

Done.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.h (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.h:35: PrerenderManager*
prerender_manager_;
On 2011/05/13 12:00:28, cbentzel wrote:
> This should be using a WeakPtr for the PrerenderManager, I think.
> 
> You may run into a corner-case race where a message is seen on IO thread and
> forwarded to the UI thread. In the interim, the PrerenderManager is destroyed
on
> the UI thread. Stale memory is then accessed.

Done.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.h:37: // 
DISALLOW_COPY_AND_ASSIGN(PrerenderMessageFilter);
On 2011/05/13 12:00:28, cbentzel wrote:
> Why is this commented out?

Done.

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_observer.cc (right):

http://codereview.chromium.org/7015026/diff/5001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_observer.cc:12: #include
"googleurl/src/gurl.h"
On 2011/05/13 12:00:28, cbentzel wrote:
> This include doesn't look necessary. Or is this one of those IWYU issues?

Done.

[cbentzel, 2011-05-13 15:35:33]

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:21:
prerender_manager_(prerender_manager->AsWeakPtr()) {
Is this created on the UI thread? If not, you'll need to do AsWeakPtr somewhere
else and pass the weak pointer along.

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:58: }
Nit: add // namespace prerender

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.h (right):

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.h:31: virtual
~PrerenderMessageFilter() {}
Looks like you need to remove the inline function body here to appease clang.

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/printing/prin...
File chrome/browser/printing/print_job_manager.cc (right):

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/printing/prin...
chrome/browser/printing/print_job_manager.cc:75: // Shamelessly ripped from
PrintViewManager to clean up the print worker.
This comment is likely not needed anymore.

[dominich, 2011-05-13 16:25:08]

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:21:
prerender_manager_(prerender_manager->AsWeakPtr()) {
On 2011/05/13 15:35:33, cbentzel wrote:
> Is this created on the UI thread? If not, you'll need to do AsWeakPtr
somewhere
> else and pass the weak pointer along. 

Added a DCHECK to make sure.

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.cc:58: }
On 2011/05/13 15:35:33, cbentzel wrote:
> Nit: add // namespace prerender

Done.

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_message_filter.h (right):

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_message_filter.h:31: virtual
~PrerenderMessageFilter() {}
On 2011/05/13 15:35:33, cbentzel wrote:
> Looks like you need to remove the inline function body here to appease clang.

Done.

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/printing/prin...
File chrome/browser/printing/print_job_manager.cc (right):

http://codereview.chromium.org/7015026/diff/6011/chrome/browser/printing/prin...
chrome/browser/printing/print_job_manager.cc:75: // Shamelessly ripped from
PrintViewManager to clean up the print worker.
On 2011/05/13 15:35:33, cbentzel wrote:
> This comment is likely not needed anymore. 

Done.

[cbentzel, 2011-05-13 17:59:37]

LGTM

You should get someone who's accessed chrome/browser/printing to take a look,
not just ben.

[dominich, 2011-05-13 18:28:51]

ben, thestig: I included you based on OWNERS and 'git blame' as people who know
about browser and printing.

[Ben Goodger (Google), 2011-05-13 18:31:47]

Replacing myself with jam for the message filter bits.

[jam, 2011-05-13 20:31:12]

BrowserMessageFilter is not really meant to filter stuff on the UI.  For that,
we have RenderViewObserver and TabContentsObserver.  Can't you listen to this in
PrerenderObserver?

[Lei Zhang, 2011-05-13 20:32:36]

LGTM on the printing bits.

[dominich, 2011-05-13 20:34:24]

On 2011/05/13 20:31:12, John Abd-El-Malek wrote:
> BrowserMessageFilter is not really meant to filter stuff on the UI.  For that,
> we have RenderViewObserver and TabContentsObserver.  Can't you listen to this
in
> PrerenderObserver?

That was my first thought but the message seemed to be filtered out somewhere
upstream. I'll take another crack at getting it there instead of adding a new
message filter class.

[jam, 2011-05-13 20:41:26]

On 2011/05/13 20:34:24, dominic wrote:
> On 2011/05/13 20:31:12, John Abd-El-Malek wrote:
> > BrowserMessageFilter is not really meant to filter stuff on the UI.  For
that,
> > we have RenderViewObserver and TabContentsObserver.  Can't you listen to
this
> in
> > PrerenderObserver?
> 
> That was my first thought but the message seemed to be filtered out somewhere
> upstream. I'll take another crack at getting it there instead of adding a new
> message filter class.

(looking deeper) that's because PrintingMessageFilter is filtering it on the IO
thread, so the message won't get to the UI thread.

It's better to avoid making order of filter registration matter in general,
since then we can end up in a situation where two filters want to filter some
messages before each other.  In this case, you can just have the printing fitler
post a task to the UI thread to do  whatever you want.

[dominich, 2011-05-13 20:51:36]

On 2011/05/13 20:41:26, John Abd-El-Malek wrote:
> On 2011/05/13 20:34:24, dominic wrote:
> > On 2011/05/13 20:31:12, John Abd-El-Malek wrote:
> > > BrowserMessageFilter is not really meant to filter stuff on the UI.  For
> that,
> > > we have RenderViewObserver and TabContentsObserver.  Can't you listen to
> this
> > in
> > > PrerenderObserver?
> > 
> > That was my first thought but the message seemed to be filtered out
somewhere
> > upstream. I'll take another crack at getting it there instead of adding a
new
> > message filter class.
> 
> (looking deeper) that's because PrintingMessageFilter is filtering it on the
IO
> thread, so the message won't get to the UI thread.
> 
> It's better to avoid making order of filter registration matter in general,
> since then we can end up in a situation where two filters want to filter some
> messages before each other.  In this case, you can just have the printing
fitler
> post a task to the UI thread to do  whatever you want.

I'm not sure that's an option in this case as I would like to avoid the
PrintingMessageFilter getting the message at all. I don't want to have a
situation where the Print Settings dialog appears, and that happens in the
PrintingMessageFilter message handler.

Similarly I don't want to introduce any special cases in the Printing system for
Prerender.

I agree that order of operation dependence is fragile at best, and broken at
worst, but I'm not sure how else to ensure that the Prerender system gets the
chance to filter out these messages early enough.

[cbentzel, 2011-05-13 20:59:42]

On Fri, May 13, 2011 at 4:51 PM, <dominich@chromium.org> wrote:

> On 2011/05/13 20:41:26, John Abd-El-Malek wrote:
>
>> On 2011/05/13 20:34:24, dominic wrote:
>> > On 2011/05/13 20:31:12, John Abd-El-Malek wrote:
>> > > BrowserMessageFilter is not really meant to filter stuff on the UI.
>>  For
>> that,
>> > > we have RenderViewObserver and TabContentsObserver.  Can't you listen
>> to
>> this
>> > in
>> > > PrerenderObserver?
>> >
>> > That was my first thought but the message seemed to be filtered out
>>
> somewhere
>
>> > upstream. I'll take another crack at getting it there instead of adding
>> a
>>
> new
>
>> > message filter class.
>>
>
>  (looking deeper) that's because PrintingMessageFilter is filtering it on
>> the
>>
> IO
>
>> thread, so the message won't get to the UI thread.
>>
>
>  It's better to avoid making order of filter registration matter in
>> general,
>> since then we can end up in a situation where two filters want to filter
>> some
>> messages before each other.  In this case, you can just have the printing
>>
> fitler
>
>> post a task to the UI thread to do  whatever you want.
>>
>
> I'm not sure that's an option in this case as I would like to avoid the
> PrintingMessageFilter getting the message at all. I don't want to have a
> situation where the Print Settings dialog appears, and that happens in the
> PrintingMessageFilter message handler.
>
> Similarly I don't want to introduce any special cases in the Printing
> system for
> Prerender.
>
> I agree that order of operation dependence is fragile at best, and broken
> at
> worst, but I'm not sure how else to ensure that the Prerender system gets
> the
> chance to filter out these messages early enough.


Actually, I think your patch will disable any window.print() calls from
being supported, even on non-prerendered pages, since the
PrintingMessageFilter will never handle the message.

Sorry for not catching this earlier.



>
>
>
> http://codereview.chromium.org/7015026/
>

[dominich, 2011-05-13 21:00:57]

On 2011/05/13 20:59:42, cbentzel wrote:
> On Fri, May 13, 2011 at 4:51 PM, <mailto:dominich@chromium.org> wrote:
> 
> > On 2011/05/13 20:41:26, John Abd-El-Malek wrote:
> >
> >> On 2011/05/13 20:34:24, dominic wrote:
> >> > On 2011/05/13 20:31:12, John Abd-El-Malek wrote:
> >> > > BrowserMessageFilter is not really meant to filter stuff on the UI.
> >>  For
> >> that,
> >> > > we have RenderViewObserver and TabContentsObserver.  Can't you listen
> >> to
> >> this
> >> > in
> >> > > PrerenderObserver?
> >> >
> >> > That was my first thought but the message seemed to be filtered out
> >>
> > somewhere
> >
> >> > upstream. I'll take another crack at getting it there instead of adding
> >> a
> >>
> > new
> >
> >> > message filter class.
> >>
> >
> >  (looking deeper) that's because PrintingMessageFilter is filtering it on
> >> the
> >>
> > IO
> >
> >> thread, so the message won't get to the UI thread.
> >>
> >
> >  It's better to avoid making order of filter registration matter in
> >> general,
> >> since then we can end up in a situation where two filters want to filter
> >> some
> >> messages before each other.  In this case, you can just have the printing
> >>
> > fitler
> >
> >> post a task to the UI thread to do  whatever you want.
> >>
> >
> > I'm not sure that's an option in this case as I would like to avoid the
> > PrintingMessageFilter getting the message at all. I don't want to have a
> > situation where the Print Settings dialog appears, and that happens in the
> > PrintingMessageFilter message handler.
> >
> > Similarly I don't want to introduce any special cases in the Printing
> > system for
> > Prerender.
> >
> > I agree that order of operation dependence is fragile at best, and broken
> > at
> > worst, but I'm not sure how else to ensure that the Prerender system gets
> > the
> > chance to filter out these messages early enough.
> 
> 
> Actually, I think your patch will disable any window.print() calls from
> being supported, even on non-prerendered pages, since the
> PrintingMessageFilter will never handle the message.
> 

Hmm. You're right. I'm curious why the unit tests/browser tests didn't catch it.

Ok, back to the drawing (printing?) board.

[Lei Zhang (Do not use), 2011-05-13 21:10:30]

On 2011/05/13 21:00:57, dominic wrote:
> Hmm. You're right. I'm curious why the unit tests/browser tests didn't catch
it.
> 
> Ok, back to the drawing (printing?) board.

That's a good point. NOw you have me curious, so I'm going to apply your patch
locally and take a look at that. I did write tests for window.print().

[dominich, 2011-05-13 22:17:35]

A cleaner implementation with a smaller intrusion on the printing system and
cancellation of the print process much earlier.

[jam, 2011-05-13 22:22:39]

lgtm

[Lei Zhang, 2011-05-14 04:08:57]

On 2011/05/13 21:10:30, thestig wrote:
> On 2011/05/13 21:00:57, dominic wrote:
> > Hmm. You're right. I'm curious why the unit tests/browser tests didn't catch
> it.
> > 
> > Ok, back to the drawing (printing?) board.
> 
> That's a good point. NOw you have me curious, so I'm going to apply your patch
> locally and take a look at that. I did write tests for window.print().

And that's because the tests use MockRenderThread, which isn't affected by your
change.

[Lei Zhang, 2011-05-14 04:11:56]

http://codereview.chromium.org/7015026/diff/10036/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7015026/diff/10036/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:274: // Allow Prerendering to cancel
this if necessary.
I would put this at the top of PrintWebViewHelper::PrintPage(). If you put it
here, it doesn't handle the case of when print preview is enabled.

[dominich, 2011-05-16 16:28:00]

http://codereview.chromium.org/7015026/diff/10036/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7015026/diff/10036/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:274: // Allow Prerendering to cancel
this if necessary.
On 2011/05/14 04:11:56, Lei Zhang wrote:
> I would put this at the top of PrintWebViewHelper::PrintPage(). If you put it
> here, it doesn't handle the case of when print preview is enabled.

Done.

[Lei Zhang, 2011-05-16 19:25:26]

LGTM for PrintWebViewHelper.

[tburkard, 2011-05-16 20:27:32]

LGTM

[commit-bot: I haz the power, 2011-05-16 22:19:02]

Change committed as 85545

[commit-bot: I haz the power, 2011-05-17 16:17:47]

Change committed as 85639