1308259 - mozPrintCallback stoped producing vector output

Status: VERIFIED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Yury Delendik (:yury)]

Attachment: Test case

After bug 1258609, the PDF viewer stopped producing high quality output during printing. STR:

1. Open the attached test case
2. Print it to a PDF file
3. Inspect PDF output

Actual result:

The "Test" text at "... loading print" sections is rasterized

Expected result:

The "Test" text at "... loading print" sections must be vectorized and selectable.

As result any document printed by PDF Viewer with low quality.

20:08.11 LOG: MainThread Bisector INFO Last good revision: c9ff56dbb6fcdb7ce0573b93c520496a8e21f250
20:08.11 LOG: MainThread Bisector INFO First bad revision: c992422247b7b33fa4a89f891d08bfe792fc7d07
20:08.11 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9ff56dbb6fcdb7ce0573b93c520496a8e21f250&tochange=c992422247b7b33fa4a89f891d08bfe792fc7d07

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This sounds Really Not Good.

Comment 2

[Bob Owen (:bobowen)]

Before the fix in bug 1258609, printing via the parent was always resulting in rasterisation because canvas printing wasn't using the correct DrawTarget.
From what I remember the only reason we weren't always getting rasterisation issues before that in non-e10s, was because of some magic in cairo's own recording surface that meant that snapshots were still getting played back as the original draw commands.

I suspect what is happening here is that there are some other cases where canvases are using a DrawTarget/surface that hasn't been created though the printing one.
Although I don't understand why that magic isn't working for 49, when we're not printing via the parent.

I'll see if I can find the problem.

Comment 3

[Bob Owen (:bobowen)]

Is this a Mac only issue?

On Windows the selectable text seems to be the same for me on Nightly and version 45.0.2, which is before any of my printing changes for sandboxing.
Even on 45.0.2 some parts aren't selectable, because of the issues I described in comment 2.

Are none of the "Test" text sections selectable on Mac?

Comment 4

[Yury Delendik (:yury)]

I noticed that on my Mac. AFIAK we had issues before on some platforms/configurations -- I'm not aware on which platformsand when it was broken or non-implemented.

Comment 5

[Bob Owen (:bobowen)]

OK, hopefully I've found the issue.
DrawTargetCairo::CreateSimilarDrawTarget has some specific code for WIN32 surfaces, but not QUARTZ.
Whereas the override that was used before gfxQuartzSurface::CreateSimilarSurface obviously did (because that's why it exists :-) ).

Here a try push with code added to DrawTargetCairo::CreateSimilarDrawTarget for QUARTZ, which will hopefully fix the regression:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00f53ec1258dba07e0adca9149320154d141be3e

Yury - can you try this build, to see if it fixes the issue please, I don't have a Mac to test.

Comment 6

[Yury Delendik (:yury)]

(In reply to Bob Owen (:bobowen) from comment #5)
> Here a try push with code added to DrawTargetCairo::CreateSimilarDrawTarget
> for QUARTZ, which will hopefully fix the regression:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=00f53ec1258dba07e0adca9149320154d141be3e
> 
> Yury - can you try this build, to see if it fixes the issue please, I don't
> have a Mac to test.

It appears to be working in non-e10s mode; and no changes in e10s mode -- rasterized output.

Comment 7

[Bob Owen (:bobowen)]

(In reply to Yury Delendik (:yury) from comment #6)
> (In reply to Bob Owen (:bobowen) from comment #5)
> > Here a try push with code added to DrawTargetCairo::CreateSimilarDrawTarget
> > for QUARTZ, which will hopefully fix the regression:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=00f53ec1258dba07e0adca9149320154d141be3e
> > 
> > Yury - can you try this build, to see if it fixes the issue please, I don't
> > have a Mac to test.
> 
> It appears to be working in non-e10s mode; and no changes in e10s mode --
> rasterized output.

Thanks, would you mind trying with e10s enabled, but with print.print_via_parent=false (it should be true at the moment).
You might need to have a lower setting for the sandbox to print to PDF like this (security.sandbox.content.level=1 or 0 which turns it off)).

Comment 8

[Yury Delendik (:yury)]

(In reply to Bob Owen (:bobowen) from comment #7)
> Thanks, would you mind trying with e10s enabled, but with
> print.print_via_parent=false (it should be true at the moment).
> You might need to have a lower setting for the sandbox to print to PDF like
> this (security.sandbox.content.level=1 or 0 which turns it off)).

Works on Mac as expected on e10s with print.print_via_parent=false and security.sandbox.content.level=0 (doesn't work with 1 or 2).

Comment 9

[Bob Owen (:bobowen)]

(In reply to Yury Delendik (:yury) from comment #8)
> (In reply to Bob Owen (:bobowen) from comment #7)
> > Thanks, would you mind trying with e10s enabled, but with
> > print.print_via_parent=false (it should be true at the moment).
> > You might need to have a lower setting for the sandbox to print to PDF like
> > this (security.sandbox.content.level=1 or 0 which turns it off)).
> 
> Works on Mac as expected on e10s with print.print_via_parent=false and
> security.sandbox.content.level=0 (doesn't work with 1 or 2).

Thanks Yury.
So, it looks like this could be an acceptable fix for release.

I'll file a separate bug for printing via the parent.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Add quartz surface specific code into DrawTargetCairo::CreateSimilarDrawTarget

I also noticed that we weren't destroying the surface in the error case.

MozReview-Commit-ID: F5fMfRBiOW7

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Can you add a test case?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb87ce613c3
Add quartz surface specific code into DrawTargetCairo::CreateSimilarDrawTarget. r=jrmuizel

Comment 13

[Astley Chen (inactive)]

[Tracking Requested - why for this release]:

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4bb87ce613c3

Comment 15

[Astley Chen (inactive)]

[Tracking Requested - why for this release]: Per bug 1310165 comment 4, we may want to uplift this fix to beta and aurora.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Yury, could you please verify this issue is fixed as expected on a latest Nightly build (10-18)? Thanks!

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Bob, should we uplift this fix to Beta50, Aurora51?

Comment 18

[Bob Owen (:bobowen)]

(In reply to Ritu Kothari (:ritu) from comment #17)
> Hello Bob, should we uplift this fix to Beta50, Aurora51?

Yes, I was just waiting for at least one day on Nightly in case there was immediate non-printing related fallout.

Comment 19

[Yury Delendik (:yury)]

(In reply to Ritu Kothari (:ritu) from comment #16)
> Hello Yury, could you please verify this issue is fixed as expected on a
> latest Nightly build (10-18)? Thanks!

Yes, it works for me.

Comment 20

[Bob Owen (:bobowen)]

Comment on attachment 8801063 [details] [diff] [review]
Add quartz surface specific code into DrawTargetCairo::CreateSimilarDrawTarget

jrmuizel - do you think that this change is OK for uplift? (I'm not sure where DrawTargetCairo::CreateSimilarDrawTarget might be used outside of printing).

Approval Request Comment
[Feature/regressing bug #]:
Regressed by bug 1258609.

[User impact if declined]:
On Mac, PDF text will continue to be rasterised when it shouldn't be.

[Describe test coverage new/current, TreeHerder]:
Manual testing for the issue by reporter.
Automated testing, for other things on Mac using DrawTargetCairo::CreateSimilarDrawTarget.


[Risks and why]:
Medium - the change is to a function that could be used for things other than printing. Setting needinfo to jrmuizel to confirm the risk here.

[String/UUID change made/needed]:
None

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8801063 [details] [diff] [review]
Add quartz surface specific code into DrawTargetCairo::CreateSimilarDrawTarget

Jeff said the risk is manageable here and it might be ok to uplift to Beta. Let's stabilize this on Aurora51 before taking this into Beta10 on Monday.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2224ee69d0ac

Comment 23

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ritu Kothari (:ritu) from comment #21)
> Comment on attachment 8801063 [details] [diff] [review]
> Add quartz surface specific code into
> DrawTargetCairo::CreateSimilarDrawTarget
> 
> Jeff said the risk is manageable here and it might be ok to uplift to Beta.
> Let's stabilize this on Aurora51 before taking this into Beta10 on Monday.

Any breakage from this patch is likely going to be rare/hard to find so it's probably worth getting this in as many trees as early as possible.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8801063 [details] [diff] [review]
Add quartz surface specific code into DrawTargetCairo::CreateSimilarDrawTarget

This seems like a core scenario, has stabilize on Nightly52 and Aurora51 for a few days now, let's uplift to Beta50.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f1cca2fddc00