1715771 - Print preview error when printing

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Bogdan Maris, Desktop QA]

Affected versions

• Firefox 90.0b5
• Latest Nightly 91.0a1

Unaffected versions

• Firefox 89.0 RC

Affected platforms

• Windows 10
• Ubuntu 18.04

Unaffected versions (for me at least)

• macOS 11.3

Steps to reproduce

1. Start Firefox with a new profile
2. Visit youtube.com
3. Print (Ctrl+P or from hamburger etc)
4. Have Save to PDF option in Destination (to make it easier to save, I don't have a printer set up to my PC).
5. Save the pdf locally

Expected result

• Youtube pdf is successfully saved locally.

Actual result

• Print preview error, An error occurred while printing is displayed. The pdf is saved locally but it can't be opened since the file is corrupt.

Regression range

• First bad: 411d3031b1b4c0f8b9ff3009ceb21ba4cfdc601e
• Last good: d7a58cff3e920dfbfdfe29c76a2822f7b916c0d3
• Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d7a58cff3e920dfbfdfe29c76a2822f7b916c0d3&tochange=411d3031b1b4c0f8b9ff3009ceb21ba4cfdc601e
  Potential regressor: Bug 1540737 Add layerless painting

Additional notes

• I had the error displayed only on Windows 10 and Ubuntu 18 (I also tried on macOS 11.3 but I receive no error). I also tested on another w10 machine and the error was not shown so I'm not sure why that is displayed.
• This is the message from Browser console when the error hits: [Exception... "Failure" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/Prompter.jsm :: openPromptSync :: line 1116" data: no]

Suggested severity

• Not sure how many websites this could affect but I would make it an S3 for now. Please fiddle with the severity if anyone thinks this could be more serious.

Comment 1

[Marco Castelluccio [:marco]]

Bogdan, can you reproduce on other pages too, or just youtube.com?

Comment 2

[Bogdan Maris, Desktop QA]

I tried on multiple websites from top 10 Alexa, I could reproduce on two more websites https://vimeo.com/watch and https://www.amazon.com/ though not sure what they have in common.

Comment 3

[Matt Woodrow (:mattwoodrow)]

This is a bit of a pain.

_cairo_recording_surface_tag checks that the 'draw' operation for the tag fits within the current clip (via _cairo_composite_rectangles_init_for_paint) and skips emitting the tag if not (with CAIRO_INT_STATUS_NOTHING_TO_DO).

This return status gets silently ignored, and cairo_tag_begin has no return value anyway, so we have no way of knowing that the begin tag was ignored.

_cairo_gstate_tag_end however, passes an empty clip (along with empty values for all other parameters), so the end tag isn't ignored by the recording surface.

Replaying the surface fails and puts the surface into an error state, due to the mismatched tag recordings.

I think FrameLayerBuilder was slightly better at detecting the case where we clip out the entire DT, and skipping the display item, so it avoided this specific case. I think this is likely still a general problem, where we need to have the exact same logic in our code to discarding link item as cairo recording surface has internally, and I suspect there are other cases we currently get it wrong (or will in the future).

We can likely hack around this by checking cairo_clip_extents() from DrawTargetCairo::Link or similar, but I worry that it's super easy to miss cases.

Jonathan, any ideas here? It seems like the cairo API is somewhat broken.

Comment 4

[Jonathan Kew [:jfkthame]]

Yeah, this does seem like an API problem. It looks like a possible fix would be to make cairo_tag_begin return a boolean (instead of void) indicating whether it actually did begin a tag structure; then the caller can use this to determine whether to do the corresponding cairo_tag_end. I have a strawman patch that does this, and it seems to resolve the problem in my initial testing. However, I think we should raise this upstream before deciding how to proceed.

(Another option might be to ensure that the surface records the fact that it ignored a begin-tag, so that it can properly ignore the corresponding end-tag as well. But I'm not sure how easy it'll be to keep track of that properly...)

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: WIP: Bug 1715771 - Make cairo_tag_begin return a bool to indicate whether it actually did begin a tag structure.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: WIP: Bug 1715771 - Make cairo_tag_begin return a bool to indicate whether it actually did begin a tag structure.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1715771 - Make cairo_recording_surface keep track of any tag-begin that it skips, so that it can also skip the matching tag-end. r=jrmuizel

Comment 8

[Jonathan Kew [:jfkthame]]

The patch in comment 7 is probably a better approach than comment 6, as it doesn't involve any cairo API changes or modification of the client code. I'll propose it upstream as a possible solution.

Comment 9

[Jonathan Kew [:jfkthame]]

I've opened https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/194 with the proposed patch; let's see what the cairo developers think about this.

In the meantime, we may want to take this anyhow in gecko so as to avoid creating broken PDFs, even if upstream may decide to fix it differently in the end. Right now this bug means we get unusable output from certain pages (rather unpredictably, depending on the exact content).

Comment 10

[Jonathan Kew [:jfkthame]]

Bumping this to S2, as it can result in an unusable PDF, and potentially could affect many sites. (It's a bit unpredictable; e.g. with the main YouTube page, the error usually reproduces for me but occasionally it works OK - probably dependent on details of exactly what shows up on the site that particular time, and how the content gets split across pages in the output.)

With the above patch, I get a good PDF every time. I guess it's too late for 90 at this point, but we should fix this for 91 at least.

Comment 11

[Jonathan Kew [:jfkthame]]

This has just been fixed upstream in cairo (in a more fundamental way) via https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/196, so I will update the patch here to take the upstream fix.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1715771 - Cherry-pick changes from https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/196 to avoid breakage when tagging PDF output. r=jrmuizel

Depends on D120214

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cd1ac96a0ff
Cherry-pick changes from https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/196 to avoid breakage when tagging PDF output. r=jrmuizel

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 9231893 [details]
Bug 1715771 - Cherry-pick changes from https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/196 to avoid breakage when tagging PDF output. r=jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Print / Save As PDF sometimes generates a broken (unusable) output file.
• 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 original description in comment 0.

(fix has been verified in local build, not yet in Nightly)

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fix from upstream (tested in CI there) that simplifies code and removes the broken (mis)feature.
• String changes made/needed:

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5cd1ac96a0ff

Comment 16

[Bogdan Maris, Desktop QA]

Using the nightly build from https://treeherder.mozilla.org/jobs?repo=mozilla-central&revision=9c579e2ce174b063bdba7b0a7d43896949c19822 which has the fix I keep getting the error message on Ubuntu 18.04 and Windows 10. Reproduced it on a youtube video (eg: https://www.youtube.com/watch?v=kknVfOJZ1w0) and printing to PDF in Landscape (Portrait mode does not always trigger the error but in Landscape I always got it to trigger the error).

Comment 17

[Jonathan Kew [:jfkthame]]

Thanks for testing! That's a bit worrying... I wonder if it's the same failure, or if we're hitting another issue -- it might be bug 1717685, which I think could lead to the same error message if the print job hits it. I'll try to reproduce/confirm and see if that will fix it.

Comment 18

[Jonathan Kew [:jfkthame]]

Re-opening for now, pending further investigation of comment 16.

Comment 19

[Jonathan Kew [:jfkthame]]

The failure in comment 16 turns out to be unrelated to either this issue or bug 1717685; we're failing to create a context due to an out-of-memory failure within cairo, which in turn is due to a number-parsing bug. (It's not obvious, because basically any internal failure within cairo will result in the same user-level error message.) So I'll file a new bug for that failure, and report it upstream.

So the original issue here is fixed, after all.

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9231893 [details]
Bug 1715771 - Cherry-pick changes from https://gitlab.freedesktop.org/cairo/cairo/-/merge_requests/196 to avoid breakage when tagging PDF output. r=jrmuizel

Approved for 91 beta 6, thanks.

Comment 21

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f810905676b

Comment 22

[Bogdan Maris, Desktop QA]

Since the only way I could verify this is by making sure the print error is not displayed anymore I will rely on the fix from bug 1721424 for verification. Using latest Nightly from today I can no longer reproduce the error message no matter what website I tried (all the ones I reproduced in the first place).

Comment 23

[Bogdan Maris, Desktop QA]

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #22)

Since the only way I could verify this is by making sure the print error is not displayed anymore I will rely on the fix from bug 1721424 for verification. Using latest Nightly from today I can no longer reproduce the error message no matter what website I tried (all the ones I reproduced in the first place).

Since bug 1721424 got an uplift to 91 I verified that using 91.0b6 on Ubuntu 18.04 and Windows 10 the error is not thrown while printing.