Closed Bug 1689789 Opened 3 years ago Closed 3 years ago

Bonus extra blank page, when printing A4-formatted PDF to A4-sized paper in Firefox, with layout.display-list.improve-fragmentation enabled

Categories

(Core :: Printing: Output, defect)

Firefox 85
defect

Tracking

()

VERIFIED FIXED
87 Branch
Tracking Status
relnote-firefox --- 85+
firefox-esr78 --- unaffected
firefox85 --- verified disabled
firefox86 --- verified
firefox87 --- verified

People

(Reporter: k.fatz, Assigned: MatsPalmgren_bugz)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [print2020_v86])

Attachments

(6 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

Open a website with integrated pdf in FF84 oder in FF85.

https://www.jena.de/fm/41/test.pdf

Actual results:

Print it in FF84 and you get one 1 printed Page.

Do the same in FF85 and you get the 1 Page with content and 1 other emty page without content.

Expected results:

In both Versions, FF84 and FF85 you should get only the ONE PAGE with content as it is shown in the browser!

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Printing: Output
Product: Firefox → Core

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #2)

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

???

(In reply to Kai from comment #3)

???

(You can disregard that Bugbug comment -- that's just a bot, auto-classifying the bug into the appropriate Bugzilla component, and documenting the fact that it's doing so.)

I can reproduce the described behavior (unexpected blank page of output, shown in print preview), if I choose A4 as the paper size in the print dialog.

Severity: -- → S3
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [print2020]

(In reply to Daniel Holbert [:dholbert] from comment #4)

I can reproduce the described behavior (unexpected blank page of output, shown in print preview), if I choose A4 as the paper size in the print dialog.

...and after selecting A4 to make the bug reproduce, I can then make it go away by toggling the about:config pref layout.display-list.improve-fragmentation to false.

So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.

Also notable: the PDF testcase itself is in A4 format, according to evince (the default Ubuntu PDF viewer).

Severity: S3 → S2
Regressed by: 1681052
Summary: FF85 Print Bug: FF adds a emty site to every print at printing integrated PDFs → Bonus blank page, when printing A4-formatted PDF to A4-sized paper in Firefox, with layout.display-list.improve-fragmentation enabled
Has Regression Range: --- → yes

ni=mats to investigate this fragmentation-fallback regression, if he has cycles to do so.

Flags: needinfo?(mats)
Attachment #9200176 - Attachment description: testPdfFF85.pdf → the result of printing the testcase (note the extra blank page)

Here's the reporter's testcase, as a local attachment in Bugzilla, BTW. (originally hosted at https://www.jena.de/fm/41/test.pdf )

The fragmentation-fallback is working as intended. The problem is that the content actually overflows.

We have code to scale down pdf.sj content to fit the page here:
https://searchfox.org/mozilla-central/rev/0a489e67575540f5aeb968208ae03ff17eb71e94/layout/generic/nsPageContentFrame.cpp#98-112
I guess it's not working correctly... Hmm, doesn't mShrinkToFitRatio affect the frame tree at some point?

Flags: needinfo?(mats)

@Daniel: Thanks a lot for your fix!

So... for this document we calculate mShrinkToFitRatio = 0.999406 but then:
https://searchfox.org/mozilla-central/rev/8d290159a6f80f5e33e2bd35c0e4b2df283a18c5/layout/printing/nsPrintJob.cpp#1290
ROFL

If I change that constant to 0.99999f then the extra page disappears...

The root cause of the mShrinkToFitRatio not being 1 is this:
maxSize.height=67280 (2.966861e+01 centimeter), heightToFit=67320 (2.968625e+01 centimeter)
(where maxSize.height is the Print Preview canvas size, heightToFit is the size of the pdf.js content)

The size of an A4 is 210 by 297 millimeters. So it appears that we have setup a Print Preview canvas size that is too small to fit an A4. (The document is 296.8625mm so it really should fit without scaling.)

67280 above is in our internal AU units (60 per CSS pixel). To be at least 297mm we would need 67352 (2.970036e+01 centimeter) (the next lower value 67351 is 2.969992e+01 centimeter). The difference between 67280 and 67352 (72) is much more than a rounding error though, it's even more than 1px, so I wonder where this bogus size came from...

So, nsPageFrame::ReflowPageContent sets up the canvas size. It gets its size from aPresContext->GetPageSize(), and that size came from nsDeviceContext::CalcPrintingSize(), which has:
(rr) p this->mPrintTarget.mRawPtr->mSize
$27 = {
<mozilla::gfx::BaseSize<int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> >> = {
{
{
width = 595,
height = 841
},
(rr) p this->mAppUnitsPerPhysicalInch
$28 = 5760

and POINTS_PER_INCH_FLOAT = 72.

The mPrintTarget->GetSize() got its size from a gfxImageSurface, which got its size from nsDeviceContextSpecProxy::MakePrintTarget which got its size from mPrintSettings->GetEffectiveSheetSize which leads to nsPrintSettingsGTK::GetEffectivePageSize which got its height from gtk_paper_size_get_height. The value it returned is 11.6929131 inch, which is 29.699999274 centimeters. So it appears we start with a slightly too small value and then have compounding rounding errors through various unit conversions after that...

Fwiw, if I call gtk_paper_size_get_height(paperSize, GTK_UNIT_MM) there I get the correct 297mm size. gtk_paper_size_get_name(paperSize) returns "iso_a4".

Assignee: nobody → mats

Also, for GTK, request the paper size in millimeter for
millimeter based paper units to avoid depending on GTK's
rounding behavior.

Depends on D103608

Note that these two parts both fixes the problem for this given PDF by themselves. I think it's worth taking both.
I'd be happy to add a test for this but I have no clue how to write a test for Print Preview of PDF documents.

FTR, the 0.998f number comes from this patch (it used to be 1.0):
https://searchfox.org/mozilla-central/diff/b1cb2953ae82a8d5ca453930b739de76ada00c18/layout/base/nsDocumentViewer.cpp#4464
There's no explanation why it was chosen, nor does it seem related to the rest of that patch... the commit message is "Add the new pluggable dialog work, in build, not used", without any bug number. smh

FWIW I think it should probably be 1.0...

Set release status flags based on info from the regressing bug 1681052

FWIW I think it should probably be 1.0...

Yeah, but it is kind of silly to re-reflow just to scale it up 1px or less too. After digging through all the conversion steps the paper size goes through I think it probably is a good idea to have some tolerance for rounding errors etc. But yes, 0.998 is rather random...
I'll file a follow-up bug on it... (EDIT: filed bug 1690028)

We should probably uplift this fix since this bug has multiple dupes already.

For us after version 85 not only blank pages appeared but also the orientation got flipped.
Imgur album:
https://imgur.com/a/3UVFdwn

(In reply to serpher from comment #25)

For us after version 85 not only blank pages appeared but also the orientation got flipped.
Imgur album:
https://imgur.com/a/3UVFdwn

That is extremely unexpected. Would you mind filing a new bug on that?
(You can use the "new/clone" button at top right of this page, and choose "...in this component")

Flags: needinfo?(serpher)

(In reply to Daniel Holbert [:dholbert] from comment #5)

(In reply to Daniel Holbert [:dholbert] from comment #4)

I can reproduce the described behavior (unexpected blank page of output, shown in print preview), if I choose A4 as the paper size in the print dialog.

...and after selecting A4 to make the bug reproduce, I can then make it go away by toggling the about:config pref layout.display-list.improve-fragmentation to false.

So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.

Also notable: the PDF testcase itself is in A4 format, according to evince (the default Ubuntu PDF viewer).

This solved (more like a workaround) the problem for now. But they need to fix it.

(In reply to Daniel Holbert [:dholbert] from comment #5)

So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.

Is this something we could (or would want to) flip back, or would that be worse?

Flags: needinfo?(dholbert)

(In reply to Daniel Holbert [:dholbert] from comment #26)

(In reply to serpher from comment #25)

For us after version 85 not only blank pages appeared but also the orientation got flipped.
Imgur album:
https://imgur.com/a/3UVFdwn

That is extremely unexpected. Would you mind filing a new bug on that?
(You can use the "new/clone" button at top right of this page, and choose "...in this component")

I hope I did that properly.
https://bugzilla.mozilla.org/show_bug.cgi?id=1690335

Flags: needinfo?(serpher)

(In reply to Julien Cristau [:jcristau] from comment #28)

(In reply to Daniel Holbert [:dholbert] from comment #5)

So, that pref-tweak (from bug 1681052) is probably the relevant change in Firefox 85 that caused this issue to start happening.

Is this something we could (or would want to) flip back, or would that be worse?

Yeah, it's probably worth turning off the pref. The pref doesn't require a restart, for what it's worth (which I think gives us some flexibility about how we flip it).

(There are long-standing bugs that are mitigated by the pref's current setting, which would obviously become re-broken if we do turn off the pref; but on the other hand, we've been living with those bugs for quite some time vs. this bug seems to be a new & fairly-obvious issue for lots of users, and new bugs are more jarring than long-standing bugs, so it probably makes sense to turn the pref off & accept those old bugs for a few weeks more, and let the pref turn back on with the "real" fix in the next release.)

(In reply to serpher from comment #29)

I hope I did that properly.

Yup, thank you!

Flags: needinfo?(dholbert)

I chatted with @dholbert and I agree it's probably worth turning the pref off for now until the fixes in this bug are released.
To turn it off, set layout.display-list.improve-fragmentation to false.

After some discussion with jcristau and mythmon: It sounds like our best bet to turn off the pref would be via a traditional patch-uplift, which we could ship as a ridealong if there's a dot release to fix some other bug. (Normandy is also sort-of an option that doesn't involve shipping an update, but it turns out that approach isn't currently well-suited to turning off a pref on just a single release.)

So: if we don't end up having a dot release, then we'll have to live with this bug for a few weeks until Firefox 86 ships to release, which will hopefully include the real fix (mats' attached patches).

Summary: Bonus blank page, when printing A4-formatted PDF to A4-sized paper in Firefox, with layout.display-list.improve-fragmentation enabled → Bonus extra blank page, when printing A4-formatted PDF to A4-sized paper in Firefox, with layout.display-list.improve-fragmentation enabled

Should this really wait? In my experience 5 duplicates is a lot.

Comment on attachment 9200953 [details]
Bug 1689789 - Disable the fragmentation fallback feature on the RELEASE channel. r=dholbert

Beta/Release Uplift Approval Request

  • User impact if declined: An extra blank page is generated in Print/Print Preview for some PDF files (A4 at least).
    Note that this will also bring back bugs that were fixed by said feature (missing content in Print/Print Preview in some cases).
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: see bug description and duplicates
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): just a pref flip to revert to earlier behavior

Note that this patch should ONLY land on the RELEASE channel. It's mostly intended as a ride-along for any dot-release; judgement call for release managers.
(The other two patches will fix the underlying bug without disabling the pref for Nightly/Beta)

  • String changes made/needed:
Attachment #9200953 - Flags: approval-mozilla-release?
Flags: qe-verify+

Comment on attachment 9200953 [details]
Bug 1689789 - Disable the fragmentation fallback feature on the RELEASE channel. r=dholbert

approved for 85.0.1

Attachment #9200953 - Flags: approval-mozilla-release? → approval-mozilla-release+

Toggled layout.display-list.improve-fragmentation to false. No extra page any more.

Hello,

Confirming this issue as verified fixed on 85.0.1(20210204182252) with Win10x64, macOS 10.15.7 and Ubuntu 20.
layout.display-list.improve-fragmentation is switched to false and no extra pages are displayed in Print Preview.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Flags: qe-verify+

Adding to the 85.0.1 release notes:
"Avoid printing an extra blank page at the end of some documents (bug 1689789)."

Reopening this, as the patch that landed (comment 40) didn't really fix the underlying issue, it just disabled the problematic feature on the release channel.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25d8b783c514
part 1 - Ensure we don't have any overflow areas for pdf.js content.  r=jfkthame
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba7cdc0b2d91
part 2 - Use Ceil instead of Truncate the ensure the print surface is large enough for the requested paper size.  r=jfkthame
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

The patch landed in nightly and beta is affected.
:mats, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(mats)

Comment on attachment 9200347 [details]
Bug 1689789 part 2 - Use Ceil instead of Truncate the ensure the print surface is large enough for the requested paper size. r=jfkthame

Beta/Release Uplift Approval Request

  • User impact if declined: An extra blank page is generated in Print/Print Preview for some PDF files (A4 at least).
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The changes are limited to print media, and are fairly non-invasive.
    AFAIK, we don't have any automated regression tests for print rendering of PDF though, which adds risk.
  • String changes made/needed:
Flags: needinfo?(mats)
Attachment #9200347 - Flags: approval-mozilla-beta?
Attachment #9200346 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9200347 [details]
Bug 1689789 part 2 - Use Ceil instead of Truncate the ensure the print surface is large enough for the requested paper size. r=jfkthame

We already have 7 bugs filed for this issue and the patch is evaluated by the author as low risk, let's take it in 86 beta 9.

Attachment #9200347 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9200346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Hello,

Confirming this issue as verified fixed on 86.0b9(20210211195159) and 87.0a1(20210211213143).

Verified using macOS 10.15.7, Win10x64 and Ubuntu 20x64.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [print2020] → [print2020_v86]
You need to log in before you can comment on or make changes to this bug.