Closed
Bug 410071
Opened 17 years ago
Closed 17 years ago
Printing pages on swedbank.se broken in Firefox 3 beta (and trunk)
Categories
(Core :: Graphics, defect, P2)
Tracking
()
VERIFIED
FIXED
People
(Reporter: djst, Assigned: vlad)
References
Details
(Keywords: regression, top100, Whiteboard: [needs-testcase])
Attachments
(5 files, 6 obsolete files)
(deleted),
application/x-zip-compressed
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•17 years ago
|
||
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.
Comment 3•17 years ago
|
||
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.
Comment 4•17 years ago
|
||
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
Comment 5•17 years ago
|
||
This fixes the assertion (but makes no difference for the print result).
Attachment #294800 -
Flags: superreview?(vladimir)
Attachment #294800 -
Flags: review?(vladimir)
Comment 6•17 years ago
|
||
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.
Updated•17 years ago
|
Comment 7•17 years ago
|
||
Bug 407595 may provide an additional test case for this one - both seem to talk about printing bugs in the firefox betas.
Assignee | ||
Comment 8•17 years ago
|
||
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+
Assignee | ||
Comment 9•17 years ago
|
||
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?
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Comment 11•17 years ago
|
||
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)
Comment 12•17 years ago
|
||
(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.
Comment 13•17 years ago
|
||
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.
Comment 14•17 years ago
|
||
Here's a reduced testcase based on the united boarding pass from bug 407595.
Comment 15•17 years ago
|
||
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)
Comment 16•17 years ago
|
||
(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
Comment 17•17 years ago
|
||
This is basically the same as attachment 294801 [details] [diff] [review] ("WIP"), except it sets GM_ADVANCED a bit later.
Comment 18•17 years ago
|
||
(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.
Comment 19•17 years ago
|
||
Regression range is:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-11-29+04%3A00%3A00&maxdate=2007-11-30+06%3A00%3A00&cvsroot=%2Fcvsroot
which points to the cairo update, bug 404092
Comment 20•17 years ago
|
||
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.
Assignee | ||
Comment 21•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #297865 -
Attachment is patch: true
Attachment #297865 -
Attachment mime type: application/octet-stream → text/plain
Comment 22•17 years ago
|
||
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
Assignee | ||
Comment 23•17 years ago
|
||
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)
Comment 24•17 years ago
|
||
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);
Assignee | ||
Comment 25•17 years ago
|
||
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)
Assignee | ||
Comment 26•17 years ago
|
||
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)
Comment 27•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Attachment #298839 -
Flags: review?(ajohnson)
Assignee | ||
Comment 28•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 29•17 years ago
|
||
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.
Description
•