Fix for race condition where print preview hangs when attempting to print a PDF that hasn't loaded.

Fix for race condition where print preview hangs when attempting to print a PDF that hasn't loaded.

If |type| in RequestPrintPreview() equals PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME and |is_loading_| is true, RequestPrintPreview() returns,
then is called again, right after |is_loading_| is set to false in DidStopLoading(). 


BUG=376969
TEST=See bug for steps to reproduce.

Additional way to reproduce:
Build blink_tests and browser_tests in out/Release. Then in src/webkit/tools/layout_tests run this command:
./run_webkit_tests.py --platform browser_test.linux source_pdf/

you can replace linux with win or mac depending on your platform.

The program will attempt to save a PDF as a PDF through print preview. It will eventually fail because Print Preview will hang due to this bug.
If you want visual confirmation that this will happen open:
src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py.

Then add this line of code to the function cmd_line.
cmd.append('--enable-pixel-output-in-tests')

Then run the script again. Enabling pixel output will also make it significantly more likely for the bug to occur.
Its also more likely to happen on windows for some reason.

This is how I came across the bug.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287982

[ivandavid, 2014-07-29 00:23:37]

I fixed the race condition where print preview would hang when a PDF was to be
printed. Who else should I as a reviewer and what bug should I associate this
with, the print preview code or this bug

https://code.google.com/p/chromium/issues/detail?id=376969&can=5&colspec=ID%2...
?

[Lei Zhang, 2014-07-29 19:22:39]

On 2014/07/29 00:23:37, ivandavid wrote:
> I fixed the race condition where print preview would hang when a PDF was to be
> printed. Who else should I as a reviewer and what bug should I associate this
> with, the print preview code or this bug

Can you elaborate more on what the race condition is and how to reproduce it?

If you read the bug you mentioned, who do you think should review this CL?

>
https://code.google.com/p/chromium/issues/detail?id=376969&can=5&colspec=ID%2...
> ?

If your CL is intended to fix this bug, then fill out the BUG= field, rather
than mentioning it here.

[ivandavid, 2014-07-29 20:02:51]

I am submitting the code, because its a fix for a problem that I encountered
that sounds extremely similar to the problem described in the issue 376969. I
cc'd karen and Nate because you guys seemed to be working on fixing that issue,
so it seems appropriate to have you guys review it.

Also, I encountered the bug with the latest patch (the one where the delay was
removed) and I still encountered the problem until I applied this fix, so the
problems could be unrelated, though I suspect not.

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (left):

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1727: case
PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME: {
I encountered a similar problem to the one described in the bug when I was
working on a browser test that was printing PDFs. When I ran the browser test,
print preview would hang and just say 'Loading Preview...'. After some
investigation, it seemed like the problem that RequestPrintPreview() would be
called after DidStartLoading(), but before DidStopLoading(). So I modified the
code to cause it to wait for DidStopLoading() to be called to actually call
RequestPrintPreview() with the correct parameters and it fixed my problem. I was
only able to replicate this bug with the case
|PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME|, therefore, I put the code there.

[Lei Zhang, 2014-07-29 20:33:25]

-karen :)

[Nate Chapin, 2014-08-04 17:26:35]

This seems plausible to me, but I'm not really qualified to review it. Adding
vitalybuka.

[Vitaly Buka (NO REVIEWS), 2014-08-05 05:49:58]

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (left):

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1727: case
PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME: {
My understanding is that it's by design.
is_loading_ should be checked only for scripted preview.

For user initiated preview of web page it should be called immediately.
Page could be loading continuously for significant time, still user may wish to
print already loaded content.

Still maybe it's OK to have this exception for plugins. 
So it needs GetPlugin() check here like in case above.

On 2014/07/29 20:02:51, ivandavid wrote:
> I encountered a similar problem to the one described in the bug when I was
> working on a browser test that was printing PDFs. When I ran the browser test,
> print preview would hang and just say 'Loading Preview...'. After some
> investigation, it seemed like the problem that RequestPrintPreview() would be
> called after DidStartLoading(), but before DidStopLoading(). So I modified the
> code to cause it to wait for DidStopLoading() to be called to actually call
> RequestPrintPreview() with the correct parameters and it fixed my problem. I
was
> only able to replicate this bug with the case
> |PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME|, therefore, I put the code there.

[Vitaly Buka (NO REVIEWS), 2014-08-05 06:05:07]

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:824:
ShowScriptedPrintPreview();
please remove ShowScriptedPrintPreview() from here use
request_print_preview_closure_ to call it.

request_print_preview_closure_ -> on_stop_loading_closure_ ?

[ivandavid, 2014-08-05 19:30:17]

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (left):

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1727: case
PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME: {
On 2014/08/05 05:49:57, Vitaly Buka wrote:
> My understanding is that it's by design.
> is_loading_ should be checked only for scripted preview.
> 
> For user initiated preview of web page it should be called immediately.
> Page could be loading continuously for significant time, still user may wish
to
> print already loaded content.
> 
> Still maybe it's OK to have this exception for plugins. 
> So it needs GetPlugin() check here like in case above.
> 
> On 2014/07/29 20:02:51, ivandavid wrote:
> > I encountered a similar problem to the one described in the bug when I was
> > working on a browser test that was printing PDFs. When I ran the browser
test,
> > print preview would hang and just say 'Loading Preview...'. After some
> > investigation, it seemed like the problem that RequestPrintPreview() would
be
> > called after DidStartLoading(), but before DidStopLoading(). So I modified
the
> > code to cause it to wait for DidStopLoading() to be called to actually call
> > RequestPrintPreview() with the correct parameters and it fixed my problem. I
> was
> > only able to replicate this bug with the case
> > |PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME|, therefore, I put the code
there.
> 

Done.

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/427723004/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:824:
ShowScriptedPrintPreview();
On 2014/08/05 06:05:06, Vitaly Buka wrote:
> please remove ShowScriptedPrintPreview() from here use
> request_print_preview_closure_ to call it.
> 
> request_print_preview_closure_ -> on_stop_loading_closure_ ?

Done.

[Vitaly Buka (NO REVIEWS), 2014-08-06 08:53:57]

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:826: NOTREACHED();
NOTREACHED is incorrect here. DidStopLoading is going to be called even without
print preview.

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1705: on_stop_loading_closure_
=
this piece should be on line 1718

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1738: if (is_loading_ &&
GetPlugin(print_preview_context_.source_frame())) {
Same is needed for PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE

[ivandavid, 2014-08-06 19:00:17]

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:826: NOTREACHED();
On 2014/08/06 08:53:57, Vitaly Buka wrote:
> NOTREACHED is incorrect here. DidStopLoading is going to be called even
without
> print preview.

Done.

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1705: on_stop_loading_closure_
=
On 2014/08/06 08:53:57, Vitaly Buka wrote:
> this piece should be on line 1718
Ok. I put it after the comment.

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1705: on_stop_loading_closure_
=
On 2014/08/06 08:53:57, Vitaly Buka wrote:
> this piece should be on line 1718

Done.

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1738: if (is_loading_ &&
GetPlugin(print_preview_context_.source_frame())) {
On 2014/08/06 08:53:57, Vitaly Buka wrote:
> Same is needed for PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE
Is it supposed to bind RequestPrintPreview() or ShowScriptPrintPreview()? I used
RequestPrintPreview(), but since I am not familiar with this piece of code, I
don't know if that's what should be used.

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1738: if (is_loading_ &&
GetPlugin(print_preview_context_.source_frame())) {
On 2014/08/06 08:53:57, Vitaly Buka wrote:
> Same is needed for PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE

Done.

[Vitaly Buka (NO REVIEWS), 2014-08-06 20:57:22]

lgtm

[Vitaly Buka (NO REVIEWS), 2014-08-06 21:00:37]

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/427723004/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:1738: if (is_loading_ &&
GetPlugin(print_preview_context_.source_frame())) {
Yes, it should be RequestPrintPreview. Bind of RequestPrintPreview is just delay
of the current function until frame is loaded. We'd like to do the same even for
PRINT_PREVIEW_SCRIPTED, but it's exceptional case because we need to block
message handler and javascript.

PRINT_PREVIEW_USER_INITIATED_SELECTION should not be possible for plugin frames

On 2014/08/06 19:00:16, ivandavid wrote:
> On 2014/08/06 08:53:57, Vitaly Buka wrote:
> > Same is needed for PRINT_PREVIEW_USER_INITIATED_CONTEXT_NODE
> Is it supposed to bind RequestPrintPreview() or ShowScriptPrintPreview()? I
used
> RequestPrintPreview(), but since I am not familiar with this piece of code, I
> don't know if that's what should be used.

[ivandavid, 2014-08-06 21:39:25]

The CQ bit was checked by ivandavid@chromium.org

[commit-bot: I haz the power, 2014-08-06 21:40:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ivandavid@chromium.org/427723004/120001

[commit-bot: I haz the power, 2014-08-07 07:17:40]

Change committed as 287982

[ivandavid, 2014-08-12 18:51:03]

A revert of this CL has been created in
https://codereview.chromium.org/463123004/ by ivandavid@chromium.org.

The reason for reverting is: The changes fix an issue, but cause a more serious
one. 

crbug.com/402402.