Closed Bug 1342395 Opened 7 years ago Closed 7 years ago

Crash in the content process when printing a pdf document

Categories

(Core :: Printing: Output, defect)

49 Branch
All
Windows
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 - wontfix
firefox-esr52 53+ fixed
firefox53 --- verified
firefox54 --- verified

People

(Reporter: philipp, Assigned: bobowen)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

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...
Flags: needinfo?(bobowencode)
Attached file test.pdf
attaching the pdf file, in case it gets removed from the homepage
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?
Flags: needinfo?(bugs)
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.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(bugs)
Flags: needinfo?(bobowencode)
I've checked all the other Pattern parameters in the other methods as well and they all use AdjustedPattern.
Attachment #8841543 - Flags: review?(jmuizelaar)
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.
Attachment #8841543 - Flags: review?(jmuizelaar) → review+
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2639190c0e56
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget. r=jrmuizel
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
Attachment #8841543 - Flags: approval-mozilla-beta?
Attachment #8841543 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2639190c0e56
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Too late for 52, though it might be something to keep in mind for esr52.
Attachment #8841543 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(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
(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 on attachment 8841543 [details] [diff] [review]
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget

Fix a crash. Aurora53+.
Attachment #8841543 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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.
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.
Flags: needinfo?(bobowencode)
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.
Flags: needinfo?(bobowencode)
Attachment #8841543 - Flags: approval-mozilla-esr52?
Comment on attachment 8841543 [details] [diff] [review]
Unwrap aPattern in DrawTargetRecording::FillGlyphs when calling wrapped DrawTarget

fix a printing crash in 52.1.0esr
Attachment #8841543 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.