1342395 - Crash in the content process when printing a pdf document

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-e3c5f05e-d4bf-45d0-ae7d-d790d2170224.
=============================================================

i was going through crash reports for the [@ mozilla::gfx::GfxPatternToCairoPattern] signature, which has increased in volume in firefox 50.
many of the user comments there talked about printing a pdf file. 

this seems to be reproducible (on win7, 32bit version of the browser):
* go to http://www.natural-source.fr/media/wysiwyg/pdf/Orderform_012016/EU/BEL_TA_FR_EUR.pdf (in pdf.js)
* press ctrl+p, select a printer and print
* the content process crashes

regression testing leads me to this pushlog & therefore bug 1258609:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9ff56dbb6fcdb7ce0573b93c520496a8e21f250&tochange=c992422247b7b33fa4a89f891d08bfe792fc7d07

curiously i can reproduce the crash on 51.0.1, 53.0a2 & 54.0a1 but not on 52.0b. these steps also don't generate a crash report in my case - there is a windows appcrash dialog window and then the firefox tab also shows that it has crashed...

Comment 1

[[:philipp]]

Attachment: test.pdf

attaching the pdf file, in case it gets removed from the homepage

Comment 2

[Gerry Chang [:gchang]]

We don't have the plan to have dot release for 51. Mark 51 won't fix.

Hi :jet,
Can you help find an owner for this?

Comment 3

[Bob Owen (:bobowen) PTO until 8th Oct]

The problem here is that we're getting patterns with GradientStopsRecording (wrapping a GradientStopsCairo) and the code assumes it is always GradientStopsCairo and casts to it.

Looks like we're not unwrapping the Pattern in the relevant DrawTargetRecording method.

Comment 4

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget

I've checked all the other Pattern parameters in the other methods as well and they all use AdjustedPattern.

Comment 5

[Bob Owen (:bobowen) PTO until 8th Oct]

Just noticed that 52 is marked as unaffected.
The fact that this doesn't reproduce there is a bit odd, almost makes me think that something else is broken causing us to not go through this code.

Comment 6

[Bob Owen (:bobowen) PTO until 8th Oct]

Happened to push this to try with another patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28a03123b8672b40468546cc98f6f62370d1f4dc

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2639190c0e56
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget. r=jrmuizel

Comment 8

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 8841543 [details] [diff] [review]
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget

Not landed yet, but in case we're taking any more changes into 52, this one is very simple.

Approval Request Comment
[Feature/Bug causing the regression]:
Moz 2D recording, used in printing with e10s.

[User impact if declined]:
Any print that causes a call to DrawTargetRecording::FillGlyphs with a pattern will continue to cause a content process crash.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Not yet, but reproduced and confirmed in local build.

[Needs manual test from QE? If yes, steps to reproduce]: 
Would be good as no automated tests for printing, STR are in Description.
(Although it appears that this test case didn't reproduce on current beta.)

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Very small one line fix and without it we definitely crash, in this situation.

[String changes made/needed]:
No

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2639190c0e56

Comment 10

[Julien Cristau [:jcristau]]

Too late for 52, though it might be something to keep in mind for esr52.

Comment 11

[[:philipp]]

(In reply to Bob Owen (:bobowen) from comment #5)
> Just noticed that 52 is marked as unaffected.
> The fact that this doesn't reproduce there is a bit odd, almost makes me
> think that something else is broken causing us to not go through this code.

in case it helps understanding the issue better: this is what "fixed" the crashes issue on 52.0a1: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4a6cd6dff941517c15ce49226f74e438f83983e8&tochange=afb9ba10005efaddaa9ec736f9c6bf058d4c6362

and after this push the content process crashed again when attempting to print in 53.0a1: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c1f076f55d8c3de786e7e4c96c57567e46297686&tochange=4849ef8c9a349fff273903b9b31ef34ce7f6a5bc

Comment 12

[Bob Owen (:bobowen) PTO until 8th Oct]

(In reply to [:philipp] from comment #11)
> (In reply to Bob Owen (:bobowen) from comment #5)
> > Just noticed that 52 is marked as unaffected.
> > The fact that this doesn't reproduce there is a bit odd, almost makes me
> > think that something else is broken causing us to not go through this code.
> 
> in case it helps understanding the issue better: this is what "fixed" the
> crashes issue on 52.0a1:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=4a6cd6dff941517c15ce49226f74e438f83983e8&tochange=afb9
> ba10005efaddaa9ec736f9c6bf058d4c6362
> 
> and after this push the content process crashed again when attempting to
> print in 53.0a1:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=c1f076f55d8c3de786e7e4c96c57567e46297686&tochange=4849
> ef8c9a349fff273903b9b31ef34ce7f6a5bc

Thanks, looks like maybe bug 1311512 should have been uplifted to 52.

Looking at crash stats, it seems that PDF printing is the only thing that normally triggers this code path and crash. The two Beta52 crashes in the last 14 days are different.
We should recheck once 52 is released and see if this is still occurring then we could reconsider taking it if we have a dot release.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8841543 [details] [diff] [review]
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget

Fix a crash. Aurora53+.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f4e997661be

Comment 15

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Reproduced the content crash on Firefox 51.0.1 under Win 10 64-bit.
The issue no longer occurs with Firefox 53 beta 1 and latest Aurora 54.0a2 2017-03-13.

There are no crash reports in Socorro for these versions either, but there are still three reports for each of 52.0esr and 52.0 release.
https://crash-stats.mozilla.com/report/index/b10cc285-de14-46c8-8b7c-56e2b2170312
https://crash-stats.mozilla.com/report/index/6d7898d7-3cbd-427a-9e7f-959112170311
https://crash-stats.mozilla.com/report/index/834898b6-0da5-4fcd-b1af-a05eb2170310

https://crash-stats.mozilla.com/report/index/c02500ea-ba84-4878-98ca-39ee62170308
https://crash-stats.mozilla.com/report/index/5fbc8743-449d-48d7-8863-9f4632170309
https://crash-stats.mozilla.com/report/index/93306873-81ea-4c70-9fec-bc0f42170308

Although I couldn't reproduce the crash there, I'm marking as affected based on crash-stats.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Bob, can you please request ESR52 approval on this when you get a chance? It's low frequency enough that I don't think we need to keep this on the radar as a possible dot release ride-along candidate.

Comment 17

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 8841543 [details] [diff] [review]
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is a trivial fix for something that will always crash when printing certain pages.

User impact if declined: 
Any print that causes a call to DrawTargetRecording::FillGlyphs with a pattern will continue to cause a content process crash.

Fix Landed on Version:
Fx54 - uplifted to Fx53.

Risk to taking this patch (and alternatives if risky):
None.

String or UUID changes made by this patch:
None.

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8841543 [details] [diff] [review]
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget

fix a printing crash in 52.1.0esr

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/0e0e8abe2153