Closed Bug 410071 Opened 12 years ago Closed 12 years ago

Printing pages on swedbank.se broken in Firefox 3 beta (and trunk)

Categories

(Core :: Graphics, defect, P2, critical)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: djst, Assigned: vlad)

References

Details

(Keywords: regression, top100, Whiteboard: [needs-testcase])

Attachments

(5 files, 6 obsolete files)

It is no longer possible to print out receipts generated by swedbank.se (one of the biggest Swedish internet banks) when using either Firefox 3 beta 2 or trunk (haven't tested with beta 1) on Windows XP or Vista. 

Steps to reproduce:

1. Unzip attachment (coming soon) and open privat.htm.
2. Print directly using "Microsoft XPS Document Writer" (in Vista), or to a physical paper.

Expected results:
The same output on paper/XPS as you see in the Print Preview.

Actual results:
A completely blank paper/XPS.


This seems to work fine in Mac OS X, but the bug has been verified on both Vista and XP. Also, Firefox 2.0.0.x prints flawlessly when following the steps above. Also note that Print Preview still works fine -- it's when you actually print it that the bug appears.
Confirmed. I got 4 black bars out of my printer, that was it. Seems to be the horizontal lines that separate the information.. but none of the text below that was printed. If needed I can scan the page as only 2 of the black bars are the same. 
This is the 16th most popular website in Sweden, according to Alexa, and the top banking site, also according to Alexa. This is a regression from Firefox 2. Requesting blocking1.9.
Flags: blocking1.9?
Keywords: regression, top100
Whiteboard: [needs-testcase]
Seems to be two separate bugs triggered by printing that page:
Assertion failed at cairo-surface.c:406: CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&surface->ref_count)
_cairo_win32_printing_surface_get_clip_box:SetWorldTransform: The parameter is incorrect.
Component: Printing → GFX: Thebes
QA Contact: printing → thebes
Attached patch Patch A, rev. 1 (obsolete) — Splinter Review
This fixes the assertion (but makes no difference for the print result).
Attachment #294800 - Flags: superreview?(vladimir)
Attachment #294800 - Flags: review?(vladimir)
Attached patch WIP (obsolete) — Splinter Review
This makes the page print correctly.  The problem is that we're calling
SetWorldTransform() and other methods that only works if the DC graphics
mode is GM_ADVANCED.  I don't know this code very well so I'm not sure
this is the correct fix though.
Blocks: 383960
Blocks: 404092
No longer blocks: 383960
Blocks: 410151
Bug 407595 may provide an additional test case for this one - both seem to talk about printing bugs in the firefox betas.
Comment on attachment 294800 [details] [diff] [review]
Patch A, rev. 1

Youch; good catch.  I've pushed this to upstream cairo as well.
Attachment #294800 - Flags: superreview?(vladimir)
Attachment #294800 - Flags: superreview+
Attachment #294800 - Flags: review?(vladimir)
Attachment #294800 - Flags: review+
Attachment #294800 - Flags: approval1.9+
In theory we should already be setting GM_ADVANCED here:

http://lxr.mozilla.org/mozilla/source/gfx/cairo/cairo/src/cairo-win32-printing-surface.c#1404

Are we never calling this start_page?  We may not be -- as I remember stuart had issues with the printing api (e.g. there's no corresponding end_page that you can call); but I -think- we still want to call start_page before we start drawing each page even if we handle all the pagination ourselves.  Does that sound right, stuart?
Duplicate of this bug: 407595
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
The WIP patch (attachment 294801 [details] [diff] [review]) also fixes Bug 407595, which I've now marked as a dupe.

We should definitely get this fixed for the next beta release -- it's pretty important that we knock out Bug 407595 so that people can print their boarding passes (and Swedish banking records)
(In reply to comment #9)
> In theory we should already be setting GM_ADVANCED here:
> 
> http://mxr.mozilla.org/mozilla/source/gfx/cairo/cairo/src/cairo-win32-printing-surface.c#1404

That's inside of 
   _cairo_win32_printing_surface_start_page
which should in theory be called by 
   _cairo_paginated_surface_show_page
at this location:
http://mxr.mozilla.org/seamonkey/source/gfx/cairo/cairo/src/cairo-paginated-surface.c#436

Was gonna investigate that today, but my Windows VM isn't cooperating right now, so I'll probably check back tomorrow.
Attached file testcase
I also found a case where it didn't print out some text, this is a minimized version of it, which I think is basically this bug.
Attached file testcase 2
Here's a reduced testcase based on the united boarding pass from bug 407595.
Notes on testcase 2's printed output:

Expected:        "This whole line should be visible when printed."
Actual:          "This _____"
After WIP patch: "This _____ line should be visible when printed."

(where "_____" is the link underline)

So, the WIP patch doesn't fix the link-text-is-missing issue.  (which is the issue shown in martijn's testcase)
(In reply to comment #12)
> (In reply to comment #9)
> > In theory we should already be setting GM_ADVANCED here:
> > 
> > http://mxr.mozilla.org/mozilla/source/gfx/cairo/cairo/src/cairo-win32-printing-surface.c#1404


We are, actually -- the WIP patch just sets it earlier, in gfxWindowsSurface::BeginPrinting() rather than within gfxWindowsSurface::EndPage() where it currently occurs.  (see callstacks below)

The WIP patch sets GM_ADVANCED here:
 	thebes.dll!gfxWindowsSurface::BeginPrinting   Line 183
 	gkgfxthebes.dll!nsThebesDeviceContext::BeginDocument  Line 535
 	gklayout.dll!nsPrintEngine::SetupToPrintContent  Line 1706
 	gklayout.dll!nsPrintEngine::DocumentReadyForPrinting()  Line 1418
	gklayout.dll!nsPrintEngine::Observe  Line 3108
 	embedcomponents.dll!nsPrintProgress::DoneIniting()  Line 223

The pre-existing code sets GM_ADVANCED later, here:
       	thebes.dll!_cairo_win32_printing_surface_start_page()  Line 1404
 	thebes.dll!_start_page()  Line 406
 	thebes.dll!_cairo_paginated_surface_show_page()  Line 441
 	thebes.dll!_moz_cairo_surface_show_page()  Line 1703
 	thebes.dll!gfxWindowsSurface::EndPage()  Line 239
 	gkgfxthebes.dll!nsThebesDeviceContext::EndPage()  Line 593
 	gklayout.dll!nsSimplePageSequenceFrame::DoPageEnd()  Line 651
 	gklayout.dll!nsPrintEngine::PrintPage()  Line 2346
 	gklayout.dll!nsPagePrintTimer::Notify()  Line 90
Attached patch WIP 2 (obsolete) — Splinter Review
This is basically the same as attachment 294801 [details] [diff] [review] ("WIP"), except it sets GM_ADVANCED a bit later.
(In reply to comment #15)
> So, the WIP patch doesn't fix the link-text-is-missing issue.  (which is the
> issue shown in martijn's testcase)

I've filed this link-text-is-missing issue as bug 413024.
This looks like there could be an extra RestoreDC() with no corresponding SaveDC. That would explain why the the win32 printing surface appears to lose the  GM_ADVANCED setting while setting GM_ADVANCED outside of the top level SaveDC/RestoreDC makes it work.

I had a look through the code and couldn't see anything obviously wrong. ie every SaveDC() appeared to have a corresponding RestoreDC. I would suggest that someone who can compile Firefox on Windows add some printfs to track each call to SaveDC and RestoreDC while printing one of the pages that doesn't print.
That's not a bad idea -- I wasn't thinking of that.  I've got this on my plate, I'll try to get to the bottom of it tomorrow.
Attachment #297865 - Attachment is patch: true
Attachment #297865 - Attachment mime type: application/octet-stream → text/plain
Attached patch WIP 2a (obsolete) — Splinter Review
oops -- just noticed that the previously posted WIP 2 wasn't using the right patch format/context -- I hand't put my .cvsrc on my windows box yet.  This version's nicer.
Attachment #297865 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
OK, Adrian's comment was dead on.  We were, in fact, blowing away things due to misbalanced SaveDC/RestoreDC.  What was going on is that the printing surface's intersect_clip_path tries to reset the clip by calling RestoreDC with a previously-saved state number; but this doesn't work if anything calls SaveDC and then tries to reset the clip path.  This situation was happening during a paint() operation with a meta surface pattern; we SaveDC() around the paint(), and then try to replay the meta surface, which fiddles with the clip.

This patch reworks initial-clip saving in both the printing and non-printing win32 surfaces, moving all the shared code into two new helper functions.  It also fixes a minor inconsistency in how restoring was done before (if we didn't have a complex region initially, but had a simple rect clip, we'd never reset to that simple rect clip when we had to reset to no clipping).
Assignee: nobody → vladimir
Attachment #294800 - Attachment is obsolete: true
Attachment #294801 - Attachment is obsolete: true
Attachment #298513 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #298555 - Flags: review?(ajohnson)
I tested the patch with cairo standalone with some test code. There was a  compile problem with NIL_SURFACE not defined. With this patch I am unable to get any output from the win32-printing surface. Not even a simple stroke of one line would work. Running the same code works fine with the win32-surface or the image surface. win32-printing also worked (except for the clip problem) with the same code before your patch.

I have not had time to debug it. I got as far as finding that _cairo_win32_printing_surface_start_page is getting called but the drawing functions like _cairo_win32_printing_surface_stroke are not getting called. I couldn't see any obvious reason why your patch would prevent the win32-printing drawing functions from being called.

There is always the possibility that there is a problem with my build environment. I have had some issues in the past with getting cairo to build on windows.

What I was interested in checking was how using Get/SelectClipRgn affected the PostScript output from the win32-printing-surface with a PS driver compared with using SaveDC/RestoreDC. We want to always have high level vector output and avoid unnecessary rasterization and/or curve flattening from the vector based printer drivers.

The following is the test code I was intending to use to check how the PS output looks with your changes:

    cairo_move_to (cr, 270, 200);
    cairo_arc (cr, 200, 200, 70, 0, 2*M_PI);
    cairo_close_path (cr);
    cairo_set_source_rgb (cr, 1, 0, 0);
    cairo_stroke_preserve (cr);
    cairo_clip (cr);

    cairo_push_group_with_content (cr, CAIRO_CONTENT_COLOR_ALPHA);

    cairo_move_to (cr, 200, 160);
    cairo_rel_line_to (cr, 100, 140);
    cairo_rel_line_to (cr, -170, 0);
    cairo_close_path (cr);
    cairo_clip (cr);

    cairo_set_source_rgb (cr, 0, 1, 0);
    cairo_paint (cr);

    cairo_reset_clip (cr);

    cairo_move_to (cr, 0, 0);
    cairo_line_to (cr, 400, 400);
    cairo_set_source_rgb (cr, 0, 0, 1);
    cairo_stroke (cr);
    
    cairo_pop_group_to_source (cr);

    cairo_paint (cr);
Ok, here's an updated patch.  Output looks good to me, using the HP 4550 driver.

The problem was essentially that the ClipRgn functions all work in device units, whereas IntersectClipRect/SelectClipPath/etc. all work in logical coordinates -- and the SetWorldTransform in the boilerplate code was throwing things off initially.  I didn't see this when testing with Mozilla because we don't use the global SWT before creating the cairo surface (that is, we use the win32 printing surface in native device coords there, not in points).
Attachment #298555 - Attachment is obsolete: true
Attachment #298812 - Flags: review?(ajohnson)
Attachment #298555 - Flags: review?(ajohnson)
Even more updated patch.  I broke the win32 (non-printing) surface's clipping routine slightly; this fixes it.
Attachment #298812 - Attachment is obsolete: true
Attachment #298839 - Flags: review?(ajohnson)
Attachment #298812 - Flags: review?(ajohnson)
Attached patch patchSplinter Review
I've tested the patch and it seems to work fine. There is one issue
with the clipping. In the cairo test code in comment 24 the blue line
drawn at the end of the group is not clipped to the circle clip that
is set before the group is painted. This problem also exists in the
win32 non printing surface so I don't think it is a regression. It can be fixed later in cairo. Given the severity of this bug I would not hold this patch up for this issue.

The clipping problem can be fixed by saving the clip region before
replaying the meta surface. The old clip region would need to be saved
as a local variable in
_cairo_win32_printing_surface_paint_meta_pattern() and restored when
exiting the function similar to how the ctm is saved and restored.

I noticed while checking the PS output using the 4550 driver that the
clip path for non rectangular paths is converted to lots of little
rectangles. I tried replacing the GetClipRgn()/SelectClipRgn() with
SaveDC()/RestoreDC() and I still get the same result. This is
disappointing as the win32-printing surface relies on clipping to
implement features not directly supported by GDI such as filling
arbitrary paths with image or gradient patterns.

I've attached my patch that uses SaveDC/RestoreDC. It wraps the page
and meta surface patterns with a SaveDC/RestoreDC after all other
transformations have been done. Then when resetting the clip path I do
a RestoreDC(); SaveDC().  This is the same as how the PS and PDF
surfaces work and it displays the test in comment 24 correctly. It
also results in a smaller patch.

Take your pick which patch you prefer. I don't know how the memory use
and performance of SaveDC/RestoreDC compares with
GetClipRgn()/SelectClipRgn() although for printing I don't think it
matters as much as it does for the display.
Ok, I ended up merging the two patches, as there were related fixes of the non-printing surface in my version; but I kept the SaveDC/RestoreDC approach in the printing surface to ensure that printers have the most information possible.  I noticed the issue with non-rectangular clips as well; I really think under the hood GDI will tessellate non-rectangular clips down before letting drivers get at it, sadly.

I've pushed this upstream into cairo now, in preparation for taking a cairo upgrade into mozilla to catch this and a few other fixes.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
All three test cases here work, as well as the united boarding passes from the duplicate bugs.  

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012704 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.