Bug 43658

Summary: Calling window.print() before the page is loaded results in a blank printed page
Product: WebKit Reporter: Lei Zhang <thestig>
Component: PrintingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, darin, dglazkov, hamaji, mitz, tony, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
URL: http://leiz.org/chromium/25027.htm
Attachments:
Description Flags
Patch to fix the bug.
none
Patch to fix the bug. (try 2)
dglazkov: review-
Defer printing patch 3 - add a changelog entry and pass check-webkit-style.
none
Defer printing patch 3 - add a manual test. none

Description Lei Zhang 2010-08-06 17:46:59 PDT
Created attachment 63790 [details]
Patch to fix the bug.

This is http://code.google.com/p/chromium/issues/detail?id=25027

I've included a minimal test case above.

The expect result is a printed page with the words "Print me".
The current result with Chrome and Safari is a blank page.

I believe the solution is to defer printing until the page has finished loading. Attached is a patch to do this.
Comment 1 WebKit Review Bot 2010-08-06 17:52:14 PDT
Attachment 63790 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/loader/FrameLoader.cpp:2231:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/page/DOMWindow.h:93:  print_deferred is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/page/DOMWindow.h:401:  m_print_deferred is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Early Warning System Bot 2010-08-06 18:02:25 PDT
Attachment 63790 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3598897
Comment 3 Lei Zhang 2010-08-06 18:23:06 PDT
Created attachment 63791 [details]
Patch to fix the bug. (try 2)
Comment 4 WebKit Review Bot 2010-08-06 18:28:49 PDT
Attachment 63791 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/loader/FrameLoader.cpp:2234:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 WebKit Review Bot 2010-08-06 18:50:59 PDT
Attachment 63790 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3668045
Comment 6 Tony Chang 2010-08-06 19:33:47 PDT
Hamaji might be able to review.  He also might have some ideas on how to write a layout test for this.  There are some general notes about layout tests here:
http://trac.webkit.org/wiki/CreatingLayoutTests
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
Comment 7 Dimitri Glazkov (Google) 2010-08-06 19:49:09 PDT
Comment on attachment 63791 [details]
Patch to fix the bug. (try 2)

It needs a ChangeLog and a layout test (if possible). Tony's right; Hamaji-san is the right person to help here.
Comment 8 mitz 2010-08-06 19:51:35 PDT
Is this change compatible with what other browsers do? Is it expected that if the script calls print() multiple times while loading, printing will only happen once at most. What happens if the script has called print() while loading, and before the page is done loading, the user closes the page or navigates to another page?
Comment 9 Lei Zhang 2010-08-09 14:03:36 PDT
(In reply to comment #8)
> Is this change compatible with what other browsers do? Is it expected that if the script calls print() multiple times while loading, printing will only happen once at most. What happens if the script has called print() while loading, and before the page is done loading, the user closes the page or navigates to another page?

I tried changing the test page to call print() multiple times. On Windows, we have the following behavior:

IE 8: prompts once always
FF 3.6: prompts once always
Chrome 5: prompts once if you hit cancel, multiple prompts if you hit ok
Safari 4: prompts multiple times always
Opera 10.60: does not prompt at all

Based on this, I'd say it's ok if the WebKit based browser match IE/FF. It doesn't make sense to me to prompt multiple times on initial load. If you know of any cases where one would want to do that, please let me know.


For the question of what happens when a script calls printing while the page is loading, but then the user navigates away, I made a simple html page that calls print(), followed by a few MBs of text. I restricted the bandwidth on the web server to 10 kbps to simulate slow rendering and found the following:

IE 8: If you navigate away before the page loads, the print prompt never shows up.
FF 3.6: If you navigate away, FF will pop up a print prompt and print out what it has loaded - which can be blank or a partial page. This behavior is also a bit weird because you could have closed the last FF window, but you'd get a lone print dialog.

There's no clear defined behavior here either. For WebKit, with my patch, we'd match IE's behavior. Since the page never finished loading, we wouldn't get a print prompt.


It appears I'd need to add a layout test to LayoutTests/printing ? I'll take a look there.
Comment 10 Lei Zhang 2010-08-09 19:47:34 PDT
Created attachment 63966 [details]
Defer printing patch 3 - add a changelog entry and pass check-webkit-style.

Just a clean up patch, no real code change. I looked at the printing layout tests and it's not obvious to me how we can test the behavior in this bug. I'm open to suggestions.
Comment 11 Shinichiro Hamaji 2010-08-13 00:52:46 PDT
Comment on attachment 63966 [details]
Defer printing patch 3 - add a changelog entry and pass check-webkit-style.

Oops. Sorry for the latency. I think the test method I've added cannot help testing window.print(). I think we can add WebCore/manual-tests for this test.

This change looks good to me as the new behavior sounds more compatible to other browsers. I hesitate to r+ this change now because this patch is related to loader which I don't know well.

WebCore/loader/FrameLoader.cpp:2233
 +          window->print();
A question: don't we need to reset m_printDeferred?
Comment 12 Lei Zhang 2010-08-13 13:48:24 PDT
Created attachment 64371 [details]
Defer printing patch 3 - add a manual test.

(In reply to comment #11)
> (From update of attachment 63966 [details])
> Oops. Sorry for the latency. I think the test method I've added cannot help testing window.print(). I think we can add WebCore/manual-tests for this test.

Ok, I added a copy of 25027.htm to manual-tests. See try 4.

> This change looks good to me as the new behavior sounds more compatible to other browsers. I hesitate to r+ this change now because this patch is related to loader which I don't know well.
> 
> WebCore/loader/FrameLoader.cpp:2233
>  +          window->print();
> A question: don't we need to reset m_printDeferred?

DOMWindow::print() resets m_printDeferred.
Comment 13 Shinichiro Hamaji 2010-08-24 20:10:43 PDT
Comment on attachment 64371 [details]
Defer printing patch 3 - add a manual test.

Looks good. As I wrote in Comment 11, I'm not sure I'm a good reviewer for loader code. Could you wait for a few days before you land this to see if no one objects?

> DOMWindow::print() resets m_printDeferred.

Ah, I see. Thanks for clarification!
Comment 14 WebKit Commit Bot 2010-08-30 17:02:56 PDT
Comment on attachment 64371 [details]
Defer printing patch 3 - add a manual test.

Clearing flags on attachment: 64371

Committed r66428: <http://trac.webkit.org/changeset/66428>
Comment 15 WebKit Commit Bot 2010-08-30 17:03:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Darin Adler 2010-08-30 17:48:14 PDT
I see multiple problems with this patch:

It’s not good how this patch spreads the feature over two classes. The code in both FrameLoader and DOMWindow is inelegant. All the code should be in one class, with the other class calling over as needed. That way you would not have to expose a printDeferred function.

The name printDeferred is not a good boolean function or data member name. It sounds like a verb "print deferred". I would name it "should print when finished loading":

    m_shouldPrintWhenFinishedLoading

Also, I think FrameLoader::finishedLoading may not be the right place to call this. That's the function called when the main resources is loaded, not when the entire page is loaded.
Comment 17 Lei Zhang 2010-08-30 18:42:21 PDT
(In reply to comment #16)
> I see multiple problems with this patch:
> 
> It’s not good how this patch spreads the feature over two classes. The code in both FrameLoader and DOMWindow is inelegant. All the code should be in one class, with the other class calling over as needed. That way you would not have to expose a printDeferred function.
> 
> The name printDeferred is not a good boolean function or data member name. It sounds like a verb "print deferred". I would name it "should print when finished loading":
> 
>     m_shouldPrintWhenFinishedLoading
> 
> Also, I think FrameLoader::finishedLoading may not be the right place to call this. That's the function called when the main resources is loaded, not when the entire page is loaded.

I'd be happy to fix this to address your concerns. Should I post a follow up patch to this bug / a separate bug, or revert?

If FrameLoader::finishedLoading is the wrong place, then do you know where the right place is?
Comment 18 Darin Adler 2010-08-30 18:53:25 PDT
(In reply to comment #17)
> I'd be happy to fix this to address your concerns. Should I post a follow up patch to this bug / a separate bug, or revert?

I think a separate bug is the cleanest way to attack it.

> If FrameLoader::finishedLoading is the wrong place, then do you know where the right place is?

It would take a little thought to be sure. I would want it to be a place that exactly corresponded to whatever test we're doing to check if it should be deferred. So if the test is "isLoading" then it should be the place that makes isLoading false. Or if the test is "did not fire load event yet", then it should be the place that the load event is fired. Sorry, I don’t have time to do the research myself right now.
Comment 19 Alexey Proskuryakov 2011-01-20 12:16:06 PST
> I tried changing the test page to call print() multiple times. On Windows, we have the following behavior:
>
> IE 8: prompts once always
> FF 3.6: prompts once always
<...>
> Based on this, I'd say it's ok if the WebKit based browser match IE/FF.

This is not what I see in Firefox 3.6 on Mac or Windows. What I see is that window.print() is always synchronous in Firefox, just like in Safari 5 - as evidenced by the included test case, which fails in Firefox.

So, this patch has changed WebKit from matching Firefox to matching IE. What am I missing? Why does the original issue with Bank of America not occur with Firefox?
Comment 20 Lei Zhang 2011-01-20 13:41:57 PST
(In reply to comment #19)
> > I tried changing the test page to call print() multiple times. On Windows, we have the following behavior:
> >
> > IE 8: prompts once always
> > FF 3.6: prompts once always
> <...>
> > Based on this, I'd say it's ok if the WebKit based browser match IE/FF.
> 
> This is not what I see in Firefox 3.6 on Mac or Windows. What I see is that window.print() is always synchronous in Firefox, just like in Safari 5 - as evidenced by the included test case, which fails in Firefox.
> 
> So, this patch has changed WebKit from matching Firefox to matching IE. What am I missing? Why does the original issue with Bank of America not occur with Firefox?

Which test case did you try exactly? On Windows XP, with Firefox 3.6.12 and a CutePDF dummy printer, I loaded up http://leiz.org/chromium/25027_test2.html, which calls window.print() 3 times, but only pops up the print dialog once.

screen capture of what I described above:
http://leiz.org/chromium/25027_ff.avi
Comment 21 Alexey Proskuryakov 2011-01-20 14:31:19 PST
I've been testing with local files, and your test case also results in three synchronous print dialogs when loaded from a local file. Can you confirm that the regression test that went to manual-tests fails in Firefox for you?

Looks like Firefox has a special case for file:///, or maybe it just has some kind of timing issue. Interesting.
Comment 22 Lei Zhang 2011-01-20 14:50:06 PST
(In reply to comment #21)
> I've been testing with local files, and your test case also results in three synchronous print dialogs when loaded from a local file. Can you confirm that the regression test that went to manual-tests fails in Firefox for you?
> 
> Looks like Firefox has a special case for file:///, or maybe it just has some kind of timing issue. Interesting.

Yes, I do see the different behavior from Firefox when using a file:// url rather than http://. Perhaps some kind of safeguard against malicious webpages?

The regression test did not fail for me with Firefox. When I loaded print-before-load.html, the CutePDF 'printer' output had the text from the webpage. Same for both file:// and http://.
Comment 23 Alexey Proskuryakov 2011-01-20 15:21:20 PST
> The regression test did not fail for me with Firefox.

That's still strange, but is less important than the fact that Firefox also delays printing web pages until the page is loaded.
Comment 24 mitz 2011-01-20 15:29:59 PST
(In reply to comment #23)
> That's still strange, but is less important than the fact that Firefox also delays printing web pages until the page is loaded.

As far as I can tell from <http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#printing>, the user agent may delay printing, but if it does so, it should still print the page as it was when print() was called. At any rate, it may not defer the printing prompt.

It appears as if Firefox (and perhaps WebKit browsers after this fix) or not in compliance with HTML.
Comment 25 mitz 2011-01-20 15:30:30 PST
s/or not/are not/
Comment 26 Lei Zhang 2011-01-20 15:41:15 PST
Then that means IE / FF / Webkit all violates the WHATWG spec, but at least they are consistent with respect to each other.
Comment 27 Alexey Proskuryakov 2011-01-20 15:52:19 PST
I've sent an e-mail to WHATWG about this.