1336350 - Some PDF crashes Firefox when using internal PDF Viewer

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[5.5]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20170125094131

Steps to reproduce:

[1] Set the [PDF Document] option to use internal PDF Viewer (default)

[2] Access the following URL.
http://moji.gr.jp/firefox/sample.pdf


Actual results:

Firefox crashed.

Comment 2

[[:philipp]]

i'm reopening this bug since the particular issue with pdfs seems to be regressing more recently than bug 1198921, which goes back further...

the regression range for these STR is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=24c5fbde4488e06ef79905e1c520027cddcd1189&tochange=b942c98f56c4c2926b8b81b98425072a091bbf7b - so likely the switch to msvc 2015 to compile firefox

64bit builds of firefox don't seem to crash in this scenario.

(the following pdf exhibits the same pattern: www.mysoft.hu/docs/Dell/E-PORTPLUS.pdf)

Comment 3

[[:philipp]]

hi, would this bug be better placed in a gfx component?

Comment 4

[Brendan Dahl [:bdahl]]

(In reply to [:philipp] from comment #3)
> hi, would this bug be better placed in a gfx component?

Yes, pdf.js is mostly a regular web page, it shouldn't be able to crash firefox.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

PDFium should make all this obsolete, but in the meantime:

_cairo_dwrite_scaled_font_init_glyph_metrics returns status 100 (CAIRO_INT_STATUS_UNSUPPORTED) and all goes bad after that.

This happens since HRESULT hr = font_face->dwriteface->GetDesignGlyphMetrics(&charIndex, 1, &metrics) returns ERROR_ARITHMETIC_OVERFLOW - (Arithmetic result exceeds 32 bits.)

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1336350: CAIRO_INT_STATUS_UNSUPPORTED is a special case of an error.

Review commit: https://reviewboard.mozilla.org/r/115452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115452/

Comment 7

[Brendan Dahl [:bdahl]]

(In reply to Milan Sreckovic [:milan] from comment #5)
> PDFium should make all this obsolete, but in the meantime:
> 

Like I mentioned above, the majority of PDF.js runs as a regular content page, so it's possible other sites could trigger this issue too.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8841113 [details]
Bug 1336350: CAIRO_INT_STATUS_UNSUPPORTED is a special case of an error.

https://reviewboard.mozilla.org/r/115452/#review117500

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce4d5be6f3fb
CAIRO_INT_STATUS_UNSUPPORTED is a special case of an error.  r=jrmuizel

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8841113 [details]
Bug 1336350: CAIRO_INT_STATUS_UNSUPPORTED is a special case of an error.

Approval Request Comment
[Feature/Bug causing the regression]: Most likely, the switch to the new compiler (bug 1186060) as what triggers the "Arithmetic result exceeds 32 bits" error, which then gets us into this code path.
[User impact if declined]: ~1000 weekly crashes on beta
[Is this code covered by automated tests?]: No, but we have a reproducible test case.
[Has the fix been verified in Nightly?]: The approval should wait until it has been.
[Needs manual test from QE? If yes, steps to reproduce]: Probably a good idea, given the timing of the uplifts - the crashing test case is attached to this bug.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Low risk, other code does similar things.
[Why is the change risky/not risky?]:
[String changes made/needed]:

Comment 11

[[:philipp]]

i can confirm that the crash is no longer reproducible with the tinderbox build from https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-win32/1488304835/ containing the patch.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce4d5be6f3fb

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8841113 [details]
Bug 1336350: CAIRO_INT_STATUS_UNSUPPORTED is a special case of an error.

last minute crash fix for 52 rc2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a89019f9eec3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d1b4c5e38c8f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d1b4c5e38c8f

Comment 18

[Andrei Vaida [:avaida]]

Flagging this for manual testing, str in Comment 0, testcases in Comment 0 and Comment 2.

Comment 19

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52.0-build2 on Win 7 x64, Win 10 x64, OS X 10.11, Ubuntu 16.04 x86.

Comment 20

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52 ESR, 53.0a2 (2017-03-06), 54.0a1 (2017-03-06) Win 10