1310165 - mozPrintCallback stopped producing vector output when printing via the parent on OS X

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

Attachment: Test case

We have a fix for bug 1308259 for non-e10s and e10s without printing via the parent, but there still seems to be a problem for printing via the parent.
It could be to do with the setting up of the dummy off screen surface for recording.
That might be the issue in bug 1301765 as well.

I wonder if we ought to turn off printing via the parent in Fx51 as we only need it for sandboxing.

Original details below.

+++ This bug was initially created as a clone of Bug #1308259 +++

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.

Comment 1

[Haik Aftandilian [:haik]]

(In reply to Bob Owen (:bobowen) from comment #0)
> I wonder if we ought to turn off printing via the parent in Fx51 as we only
> need it for sandboxing.

That might be a good idea until we resolve this, but I'd like to understand the severity of the problem better first. Leaving needinfo for now.

Safari and Chrome both fail to generate the PDF correctly (or rather fail to print it like we do, either with or without the fix for 1308259) omitting the "Test" text entirely.

Comment 2

[Yury Delendik (:yury)]

(In reply to Haik Aftandilian [:haik] from comment #1)
> Safari and Chrome both fail to generate the PDF correctly (or rather fail to
> print it like we do, either with or without the fix for 1308259) omitting
> the "Test" text entirely.

mozPrintCallback is implemented only on Firefox (and was created to replace rasterized canvas output vs vector, current user is PDF.js)

Comment 3

[Astley Chen (inactive)]

Is it a regression of bug 1258609 since FF51 ? I can see the rasterized output on FF50 beta as well.

Comment 4

[Bob Owen (:bobowen)]

(In reply to Astley Chen [:astley] (UTC+8) from comment #3)
> Is it a regression of bug 1258609 since FF51 ? I can see the rasterized
> output on FF50 beta as well.

There was a similar problem caused by bug 1258609, but a fix for that is on inbound from bug 1308259 (I'll request uplift once it has been merged).

I filed this as a separate issue because turning on printing via the parent causes the same problem, even with that fix.

Comment 5

[Astley Chen (inactive)]

(In reply to Bob Owen (:bobowen) from comment #4)
> (In reply to Astley Chen [:astley] (UTC+8) from comment #3)
> > Is it a regression of bug 1258609 since FF51 ? I can see the rasterized
> > output on FF50 beta as well.
> I filed this as a separate issue because turning on printing via the parent
> causes the same problem, even with that fix.

Got it. Thanks for prompt feedback.

Comment 6

[Astley Chen (inactive)]

[Tracking Requested - why for this release]: `print.print_via_parent` had been flipped on on macOS since FF51. The issue would be seen on macOS after merging FF51 to beta.

[1] http://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#1116

Comment 7

[Haik Aftandilian [:haik]]

I filed bug 1310804 (Limit Mac remote printing to Nightly) to workaround this problem. It will have to be uplifted to build 51. We'll still need to resolve this bug so that vector output works properly with remote printing which is still enabled on Nightly. I'll take this bug.

Comment 8

[Julien Cristau [:jcristau]]

Recent regression in 51, tracking+

Comment 9

[Haik Aftandilian [:haik]]

Cancelling the tracking-firefox51 request because bug 1310804 "Limit Mac remote printing to Nightly" prevents this problem on releases other than Nightly and will go into 51. This bug will still be a problem in Nightly where Mac remote printing is enabled.

Comment 10

[Astley Chen (inactive)]

Based on bug 1310804, the regression is limited to nightly only. Update status flag to reflect current result.

Comment 11

[Jim Mathies [:jimm]]

Tracy, please figure out how to reproduce this rasterized print to pdf bug and post some screen shots of the difference.

Comment 12

[Tracy Walker [:tracy]]

Attachment: comparison of pdf saves

Saved to PDF from Release 49.0.2 is on the left.  Saved to PDF from Nightly is on the right. In neither case is the test text selectable as demonstrated by the selection spanning across the Test text.  But there is a difference in font of the Test text.

Comment 13

[Jonathan Watt [:jwatt]]

Taking, as discussed with Haik or IRC.

Comment 14

[Jonathan Watt [:jwatt]]

Here's my analysis of what's going wrong:

Early on during printing we end up under this call stack on the parent process:

  PrintTargetCG::CreateOrNull
  nsDeviceContextSpecX::MakePrintTarget
  nsDeviceContext::InitForPrinting
  RemotePrintJobParent::InitializePrintDevice
  RemotePrintJobParent::RecvInitializePrint
  PRemotePrintJobParent::OnMessageReceived

In nsDeviceContextSpecX::MakePrintTarget the ::PMSessionGetCGGraphicsContext() call fails (because we haven't called ::PMSessionBeginPage() yet, and PMSessionGetCGGraphicsContext will only succeed between PMSessionBeginPage/PMSessionEndPage calls). As a result nsDeviceContextSpecX::MakePrintTarget call PrintTargetCG::CreateOrNull(size, SurfaceFormat::A8R8G8B8_UINT32) which creates a PrintTargetCG that wrapps a raster backed CGContext. This is the PrintTarget that is stored in nsDeviceContext->mPrintTarget for now.

Shortly after this, we create our PrintTranslator under the stack:

  PrintTarget::GetReferenceDrawTarget
  nsDeviceContext::CreateRenderingContextCommon
  nsDeviceContext::CreateReferenceRenderingContext
  PrintTranslator::PrintTranslator
  RemotePrintJobParent::RecvInitializePrint
  PRemotePrintJobParent::OnMessageReceived

PrintTarget::GetReferenceDrawTarget creates a similar cairo_surface_t to the one that it wraps, so it too wraps a raster backed CGContext. This DrawTarget is stored by PrintTranslator's constructor in its mBaseDT.

Shortly after that we end up under nsDeviceContext::BeginPage:

  mozilla::gfx::PrintTargetCG::CreateOrNull
  nsDeviceContextSpecX::MakePrintTarget  <- PMSessionGetCGGraphicsContext works
  nsDeviceContext::BeginPage
  mozilla::layout::RemotePrintJobParent::PrintPage
  mozilla::layout::RemotePrintJobParent::RecvProcessPage

It calls nsDeviceContextSpecX::MakePrintTarget to replace nsDeviceContext->mPrintTarget now that calling ::PMSessionGetCGGraphicsContext() will succeed and provide a vector backed CGContext. Unfortunately we have no mechanism to replace PrintTranslator->mBaseDT.

When we later end up under stacks like:

  RecordedCreateSimilarDrawTarget::PlayEvent
  PrintTranslator::TranslateRecording
  RemotePrintJobParent::PrintPage
  RemotePrintJobParent::RecvProcessPage
  PRemotePrintJobParent::OnMessageReceived

we end up calling things like aTranslator->GetReferenceDrawTarget()->CreateSimilarDrawTarget(mSize, mFormat) and the aTranslator->GetReferenceDrawTarget() call returns aTranslator->mBaseDT. Since that still wraps a raster backed CGContext, we end up creating another raster backed CGContext, and hence why we can fail to vectorize things.

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: non-working patch to fix RecordedCreateSimilarDrawTarget::PlayEvent

I think ideally the way to fix this would be to give RecordedCreateSimilarDrawTarget::PlayEvent access to the DrawTarget on the parent process side that corresponds to the DrawTarget that CreateSimilarDrawTarget was called on on the content process side. Unfortunately this attempt to do that fails because the LookupDrawTarget(mBaseDT) call fails to find that DrawTarget. I'm not familiar enough with the fine details of the record and replay Moz2D stuff to understand why.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: patch - Add a GetReferenceDrawTarget overload to PrintTargetCG to return a PDF CGContext backed DrawTarget

This is another way to fix the issue by making sure that when GetReferenceDrawTarget is called on a PrintTargetCG we always return a PDF CGContext backed DrawTarget. This appears to work, but needs a little cleaning up.

Comment 17

[Haik Aftandilian [:haik]]

(In reply to Jonathan Watt [:jwatt] from comment #16)
> Created attachment 8810895 [details] [diff] [review]
> patch - Add a GetReferenceDrawTarget overload to PrintTargetCG to return a
> PDF CGContext backed DrawTarget
> 
> This is another way to fix the issue by making sure that when
> GetReferenceDrawTarget is called on a PrintTargetCG we always return a PDF
> CGContext backed DrawTarget. This appears to work, but needs a little
> cleaning up.

This works for me. See attached screenshot. The text added for printing was vector and selectable.

Comment 18

[Haik Aftandilian [:haik]]

Attachment: Screenshot generated with the fix.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: patch - add a GetReferenceDrawTarget overload to PrintTargetCG to return a PDF CGContext backed DrawTarget

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8813348 [details] [diff] [review]
patch - add a GetReferenceDrawTarget overload to PrintTargetCG to return a PDF CGContext backed DrawTarget

Review of attachment 8813348 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/PrintTargetCG.cpp
@@ +72,5 @@
> +{
> +  if (!mRefDT) {
> +    const IntSize size(1, 1);
> +
> +    CGRect pageRect;

could just make this
CGRect pageRect = { {0, 0}, {size.width, size.height} };

@@ +82,5 @@
> +    CFMutableDataRef mutableData = CFDataCreateMutable(NULL, 0);
> +    CGDataConsumerRef dataConsumer = CGDataConsumerCreateWithCFData(mutableData);
> +    CFRelease(mutableData);
> +    CGContextRef pdfContext = CGPDFContextCreate(dataConsumer, &pageRect, NULL);
> +    CGDataConsumerRelease(dataConsumer);

Here's code that does something similar:
http://searchfox.org/mozilla-central/source/gfx/2d/PathCG.cpp#338-353

Not sure what's preferable. Either is fine probably.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a01c1474537
Add a GetReferenceDrawTarget overload to PrintTargetCG to return a PDF CGContext backed DrawTarget. r=mstange

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9a01c1474537

Comment 23

[Jonathan Watt [:jwatt]]

Comment on attachment 8813348 [details] [diff] [review]
patch - add a GetReferenceDrawTarget overload to PrintTargetCG to return a PDF CGContext backed DrawTarget

Requesting approval to uplift to v52 in order to get the initial content process sandboxing turned on.

Approval Request Comment
[Feature/regressing bug #]: bug 1314056
[User impact if declined]: less security
[Describe test coverage new/current, TreeHerder]: now merged to m-c
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8813348 [details] [diff] [review]
patch - add a GetReferenceDrawTarget overload to PrintTargetCG to return a PDF CGContext backed DrawTarget

fix printing issue on osx with sandboxing, take in aurora52

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/684cee0b8da4