803568 - Nightly crashed when see pdf file in fullscreen (presentation mode) or zooming

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Raul Nicolae Malea]

1. Opened this pdf file: http://www.primariahd.ro/fisiere/module_fisiere/4191/Dispoz%20ordinara%20SEPTEMBRIE.pdf

2a. Switch to Presentation Mode

 or
2b. Alternate zoom (automatic, page width, 75%, etc.)

3. Nightly crashed

Crash reports:

https://crash-stats.mozilla.com/report/index/bp-37527549-85df-430d-ab85-dd5032121019

https://crash-stats.mozilla.com/report/index/bp-c3397d2f-6204-42d1-9683-18b7c2121019

https://crash-stats.mozilla.com/report/index/bp-8dd9614e-3dd4-484c-adba-222c32121019

Comment 1

[Raul Nicolae Malea]

Version: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121019030551

Extensions:
about:me 0.5
Adblock Plus 2.1.2
Add-on Compatibility Reporter 1.1
Adobe Acrobat - Create PDF 1.2
Adobe Acrobat 10.1.4.38
Adobe Acrobat 11.0.0.379
ANIMATED - Snowflake Village 1320945460 [DISABLED]
British English Dictionary 1.19.1
Bugzilla Tweaks 1.12.1
Cheevos 1.4
Collusion 0.16.3 [DISABLED]
Dicționar românesc de corectare ortografică. 1.12
Download Statusbar 0.9.10
fireblur 1260925626 [DISABLED]
Google Earth Plugin 6.2.0.5788
Google Shortcuts 2.1.7.1
Google Update 1.3.21.123
HP Smart Web Printing 4.5 [DISABLED]
Implicit 19.0a1
Java Deployment Toolkit 7.0.70.11 10.7.2.11
Java(TM) Platform SE 7 U7 10.7.2.11
Microsoft Office 2010 14.0.4730.1010
Microsoft Office 2010 14.0.4761.1000
Mozilla QA Companion 1.2.3
Mozilla Reps Companion 1.1
MozMill 1.5.19
Mozmill Crowd 0.1.5
Nightly Tester Tools 3.3
NoScript 2.5.8
NVIDIA 3D Vision 7.17.13.697
NVIDIA 3D VISION 7.17.13.697
Puppy Dogs... 1291695684 [DISABLED]
Sage 1.4.12
Shockwave Flash 11.4.402.287
Silverlight Plug-In 5.1.10411.0
Submit Word 1.1.0
Test Pilot 1.2.2
VLC Web Plugin 2.0.2.0
Yahoo Application State Plugin 1.0.0.7

Build config:

about:buildconfig
Build Machine

w64-ix-slave98
Source

Built from http://hg.mozilla.org/mozilla-central/rev/0ff60bfb3442
Build platform
target
i686-pc-mingw32
Build tools
Compiler 	Version 	Compiler flags
e:/builds/moz2_slave/m-cen-w32-ntly/build/obj-firefox/_virtualenv/Scripts/python.exe -O e:/builds/moz2_slave/m-cen-w32-ntly/build/build/cl.py cl 	16.00.30319.01 	-TC -nologo -W3 -Gy -Fdgenerated.pdb -we4553 -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1 -Oy-
e:/builds/moz2_slave/m-cen-w32-ntly/build/obj-firefox/_virtualenv/Scripts/python.exe -O e:/builds/moz2_slave/m-cen-w32-ntly/build/build/cl.py cl 	16.00.30319.01 	-TP -nologo -W3 -Gy -Fdgenerated.pdb -wd4345 -wd4351 -wd4800 -we4553 -GR- -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1 -Oy-
Configure arguments

--enable-update-channel=nightly --enable-update-packaging --enable-jemalloc --enable-signmar --enable-profiling --enable-js-diagnostics

Comment 2

[Scoobidiver (away)]

It doesn't crash for me with a new profile. Can you narrow down the extension that causes the issue (probably something similar to ImageTweak)?

Comment 3

[Scoobidiver (away)]

Forget comment 2, Firefox crashes when toggling full screen and playing with zoom.

Comment 4

[Raul Nicolae Malea]

(In reply to Scoobidiver from comment #3)
> Forget comment 2, Firefox crashes when toggling full screen and playing with
> zoom.

Yes, confirmed this. In Safe Mode Firefox crashed when zooming in pdf files . Try + / - zoom more times.

New crash reports:

http://crash-stats.mozilla.com/report/index/bp-c37ab190-5d84-420d-8aed-11b8e2121019

http://crash-stats.mozilla.com/report/index/bp-5d2447a9-86e2-467b-acb6-030d32121019

In Safe mode fullscreen working fine .

Comment 5

[Scoobidiver (away)]

It's #14 top browser crasher in 19.0a2.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Will track this topcrasher and since PDF.js is something we're looking to ship in 19, Jet can you get someone on this?

Comment 7

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

Are these errors in any way useful:
it JavaScript "debugger" keyword. JS call stack...
0 CanvasGraphics_putBinaryImageData() ["resource://pdf.js/build/pdf.js":3237]
    this = [object Object]
1 CanvasGraphics_paintImageXObject() ["resource://pdf.js/build/pdf.js":3216]
    this = [object Object]
2 CanvasGraphics_executeOperatorList() ["resource://pdf.js/build/pdf.js":2254]
    this = [object Object]
3 next() ["resource://pdf.js/build/pdf.js":1651]
    <failed to get 'this' value>
4 pdfViewcContinueCallback() ["resource://pdf.js/web/viewer.js":2079]
    <failed to get 'this' value>
5 anonymous() ["resource://pdf.js/build/pdf.js":1644]
    <failed to get 'this' value>
6 PDFPageProxy_display() ["resource://pdf.js/build/pdf.js":1659]
    this = [object Object]
7 pageDisplayReadyPromise() ["resource://pdf.js/build/pdf.js":1560]
    this = [object Object]
8 Promise_resolve() ["resource://pdf.js/build/pdf.js":1171]
    this = [object Object]
9 pageSetTimeout() ["resource://pdf.js/build/pdf.js":1585]
    this = [object Window]

Comment 8

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

Seems that CanvasRenderingContext2D is bad.  Takes a lot longer (for me at least) to crash in the debugger, which could be us releasing the memory while we still need it?

Comment 9

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

No need to be full screen, it will crash in the window as well.

Comment 10

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

Attachment: Check for Cairo surface creation failure

When we fall far enough behind (because of repeated +/- zoom requests), Cairo seems to fail to give us back a valid surface, at least on Windows.  Just check for null pointers.  Brendan, I've added you because of previous association with PDF, but I may be way off base.

I will open another bug for the same page actually not rendering under windows.

Comment 11

[Bob Clary [:bc] (inactive)]

see also bug 825037

Comment 12

[Bas Schouten (:bas.schouten)]

Comment on attachment 696053 [details] [diff] [review]
Check for Cairo surface creation failure

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

I'd like to have a better understanding of why this happens. I'd like to see a little more inline comments as to what we've tested so far. Having said that, as a band-aid this patch seems fine to me.

Comment 13

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

I imagine it has to do with us trying to allocate >4k x > 8k texture repeatedly, but I don't know Cairo.  I'm looking at the fact that it never displays, even if it does not hit this crash.  This document is made of 1-bit/1-channel textures.

Comment 14

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

If the image is 8192 we're fine, if it's 8193, we're not. See 824981.

Comment 16

[Brendan Dahl [:bdahl]]

Comment on attachment 696053 [details] [diff] [review]
Check for Cairo surface creation failure

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

I can review code in the pdf.js extension, but this isn't my area so I'm removing the review request.

Comment 17

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

Attachment: Check for Cairo surface creation failure (removed code comment)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
A graphics expert could figure out how to have the Cairo image surface creation fail if they know about image size limits.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
There are no code comments for the null pointer checks, no check-in comments beyond what the code does - check for the failure.

Which older supported branches are affected by this flaw?
19 has this problem.

If not all supported branches, which bug introduced the flaw?
This is a new feature (PDF support).

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes, same, simple change, no additional risk, look for the attachment.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.  All we are doing is making sure null pointers don't get used.

Comment 18

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

Attachment: Check for Cairo surface creation failure (aurora/19 patch)

Aurora (19) matching patch.  r=me, same change as reviewed.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 698787 [details] [diff] [review]
Check for Cairo surface creation failure (removed code comment)

This doesn't need sec approval as it is not a rated security bug.

Comment 20

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

Attachment: Check for Cairo surface creation failure (with better code comments)

Since this wasn't a security bug (I mixed up a couple), here are more code comments as per original review.  Bas, if this is OK, can you also check it in?  Also, this is tracked for 19, does that mean I should produce aurora and beta patches and ask for the approval?

Comment 21

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

Attachment: Check for Cairo surface creation failure (with better code comments, aurora 20)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Introduction of the pdf.js viewer as the default
User impact if declined: crash
Testing completed (on m-c, etc.): m-c, m-a, m-b
Risk to taking this patch (and alternatives if risky): low; we are stopping the code from accessing a nullptr
String or UUID changes made by this patch: none

Comment 22

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

Attachment: Check for Cairo surface creation failure (with better code comments, beta 19)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Introduction of the pdf.js viewer as the default
User impact if declined: crash
Testing completed (on m-c, etc.): m-c, m-a, m-b
Risk to taking this patch (and alternatives if risky): low; we are stopping the code from accessing a nullptr
String or UUID changes made by this patch: none

Comment 23

[Bas Schouten (:bas.schouten)]

Comment on attachment 700016 [details] [diff] [review]
Check for Cairo surface creation failure (with better code comments)

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

It's not entirely clear to be how this could happen the way it's described in the comments. Having said that the comments are descriptive enough that they allow further investigation and refer to this bug, so it should be easily enough to investigate further later.

What I think is happening is that with a smaller size than 8193, we're not using Cairo drawtargets and -that-'s why we're not hitting the bug there. In any case if these checks help this bug we should just land them, as they can't hurt.

::: gfx/2d/DrawTargetCairo.cpp
@@ +178,5 @@
> +  // dimension (4k on older cards) could fail, so we have to be ready for the above
> +  // operation failing.
> +  if (cairo_surface_status(surf)) {
> +    return nullptr;
> +  }

As mentioned above, I believe this could fail, but probably for different reasons.

Comment 24

[Alex Keybl [:akeybl]]

Since this has r+, can we land this on mozilla-central as soon as possible? We'd like to uplift to Aurora/Beta given the fact that we've only got a handful of betas for FF19 before release.

Comment 25

[bhavana bajaj [:bajaj]]

Reached out to Bas/Milan on email & Bas confirmed he will land this by EOD today as there was some pending review on the comments with the code here.

Comment 26

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

To speed things up, I will submit new patches, with Bas' more detailed comments, so that he can land them tomorrow (time difference kicking in).

Comment 27

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

Attachment: Check for Cairo surface creation failure (central)

The code comments mention the bug and that this is a band-aid.

Comment 28

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

Attachment: Check for Cairo surface creation failure (aurora)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Introduction of the pdf.js viewer as the default
User impact if declined: crash
Testing completed (on m-c, etc.): m-c, m-a, m-b
Risk to taking this patch (and alternatives if risky): low; we are stopping the code from accessing a nullptr
String or UUID changes made by this patch: none

Comment 29

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

Attachment: Check for Cairo surface creation failure (beta)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Introduction of the pdf.js viewer as the default
User impact if declined: crash
Testing completed (on m-c, etc.): m-c, m-a, m-b
Risk to taking this patch (and alternatives if risky): low; we are stopping the code from accessing a nullptr
String or UUID changes made by this patch: none

Comment 30

[Alex Keybl [:akeybl]]

Waiting on review and check in to mozilla-central before approving for Aurora/Beta.

Comment 31

[Bas Schouten (:bas.schouten)]

Comment on attachment 702542 [details] [diff] [review]
Check for Cairo surface creation failure (central)

https://hg.mozilla.org/integration/mozilla-inbound/rev/152d3caaf16a

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/152d3caaf16a

Comment 33

[Alex Keybl [:akeybl]]

Comment on attachment 702544 [details] [diff] [review]
Check for Cairo surface creation failure (aurora)

This patch looks identical to the m-c checkin. Approving for Aurora/Beta.

Comment 34

[Robert Kaiser]

FWIW, it looks so far like the fix has helped on trunk and no crashes of this signature have been seen yet with builds after the checkin.

Comment 35

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ec9a94061f9
https://hg.mozilla.org/releases/mozilla-aurora/rev/1a2b008b524b

Comment 36

[Bas Schouten (:bas.schouten)]

Correction:
https://hg.mozilla.org/releases/mozilla-beta/rev/cb91e0df071e

Comment 37

[Cornel Ionce [:noni] [Hubs QA]]

The pdf isn't loading correctly due to https://bugzilla.mozilla.org/show_bug.cgi?id=728571

Raul, can you please verify if it's still reproducing on FF 19.0b3?
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/19.0b3-candidates/build1/

Comment 38

[Raul Nicolae Malea]

In Nightly Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130123 Firefox/21.0 ID:20130123052720 pdf loading correctly (e.g. http://www.primariahd.ro/fisiere/module_fisiere/4506/PROCES%20VERBAL%20EXTRA%2018.01.2013.pdf).

No crash.

Comment 39

[Ioana (away)]

I can reproduce this issue quite easily on Firefox 19 beta 1, but no matter how much I tried it didn't reproduce on beta 4. Marking bug as verified on 19 considering this and the fact that there are no post-fix crashes in Socorro https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=_VEC_memcpy%20%7C%20mozilla%3A%3Agfx%3A%3ACopyDataToCairoSurface.

Comment 40

[Ioana (away)]

Verified as fixed on Friefox 20 Beta 1 (20130220104816):
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0

No new crashes in Socorro either.

Comment 41

[Ioana (away)]

Verified as fixed on Friefox 21 Beta 1 (20130401192816):
Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:21.0) Gecko/20100101 Firefox/21.0

No new crashes in Socorro either.