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

RESOLVED FIXED in Firefox 19

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Raul Malea, Assigned: milan)

Tracking

({crash, topcrash})

Trunk
mozilla21
x86
Windows 7
crash, topcrash
Points:
---

Firefox Tracking Flags

(firefox19+ verified, firefox20+ verified, firefox21 verified)

Details

(crash signature, URL)

Attachments

(3 attachments, 6 obsolete attachments)

4.40 KB, patch
bas
: review+
bas
: checkin+
Details | Diff | Splinter Review
4.40 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
4.40 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(Reporter)

Comment 1

6 years ago
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
(Reporter)

Updated

6 years ago
Crash Signature: _VEC_memcpy | mozilla::gfx::CopyDataToCairoSurface
See Also: → bug 800523

Comment 2

6 years ago
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)?
Severity: major → critical
Crash Signature: _VEC_memcpy | mozilla::gfx::CopyDataToCairoSurface → [@ _VEC_memcpy | mozilla::gfx::CopyDataToCairoSurface ]
Component: PDF Viewer → Graphics
Keywords: crash
Product: Firefox → Core
Hardware: x86_64 → x86
Version: 19 Branch → Trunk

Comment 3

6 years ago
Forget comment 2, Firefox crashes when toggling full screen and playing with zoom.
(Reporter)

Comment 4

6 years ago
(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

5 years ago
It's #14 top browser crasher in 19.0a2.
tracking-firefox19: --- → ?
Keywords: topcrash
Will track this topcrasher and since PDF.js is something we're looking to ship in 19, Jet can you get someone on this?
Assignee: nobody → bugs
tracking-firefox19: ? → +
(Assignee)

Updated

5 years ago
Assignee: bugs → milan
(Assignee)

Comment 7

5 years ago
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]
(Assignee)

Comment 8

5 years ago
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?
(Assignee)

Comment 9

5 years ago
No need to be full screen, it will crash in the window as well.
(Assignee)

Comment 10

5 years ago
Created attachment 696053 [details] [diff] [review]
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.
Attachment #696053 - Flags: review?(bdahl)
Attachment #696053 - Flags: review?(bas)
see also bug 825037
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.
Attachment #696053 - Flags: review?(bas) → review+
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
If the image is 8192 we're fine, if it's 8193, we're not. See 824981.
Duplicate of this bug: 825037
(Assignee)

Updated

5 years ago
See Also: → bug 824981
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.
Attachment #696053 - Flags: review?(bdahl)
(Assignee)

Comment 17

5 years ago
Created attachment 698787 [details] [diff] [review]
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.
Attachment #696053 - Attachment is obsolete: true
Attachment #698787 - Flags: sec-approval?
Attachment #698787 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 698790 [details] [diff] [review]
Check for Cairo surface creation failure (aurora/19 patch)

Aurora (19) matching patch.  r=me, same change as reviewed.
Attachment #698790 - Flags: review+
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.
Attachment #698787 - Flags: sec-approval?
(Assignee)

Comment 20

5 years ago
Created attachment 700016 [details] [diff] [review]
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?
Attachment #698787 - Attachment is obsolete: true
Attachment #698790 - Attachment is obsolete: true
Attachment #700016 - Flags: review?(bas)
Attachment #700016 - Flags: checkin?(bas)
(Assignee)

Comment 21

5 years ago
Created attachment 700406 [details] [diff] [review]
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
Attachment #700406 - Flags: review?(bas)
Attachment #700406 - Flags: checkin?
Attachment #700406 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

5 years ago
Created attachment 700408 [details] [diff] [review]
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
Attachment #700408 - Flags: review?(bas)
Attachment #700408 - Flags: checkin?
Attachment #700408 - Flags: approval-mozilla-beta?
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.
Attachment #700016 - Flags: review?(bas) → review+
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.
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.
(Assignee)

Comment 26

5 years ago
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).
(Assignee)

Comment 27

5 years ago
Created attachment 702542 [details] [diff] [review]
Check for Cairo surface creation failure (central)

The code comments mention the bug and that this is a band-aid.
Attachment #700016 - Attachment is obsolete: true
Attachment #700016 - Flags: checkin?(bas)
Attachment #702542 - Flags: review?(bas)
Attachment #702542 - Flags: checkin?(bas)
(Assignee)

Comment 28

5 years ago
Created attachment 702544 [details] [diff] [review]
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
Attachment #700406 - Attachment is obsolete: true
Attachment #700406 - Flags: review?(bas)
Attachment #700406 - Flags: checkin?
Attachment #700406 - Flags: approval-mozilla-aurora?
Attachment #702544 - Flags: review?(bas)
Attachment #702544 - Flags: checkin?(bas)
Attachment #702544 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 29

5 years ago
Created attachment 702545 [details] [diff] [review]
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
Attachment #700408 - Attachment is obsolete: true
Attachment #700408 - Flags: review?(bas)
Attachment #700408 - Flags: checkin?
Attachment #700408 - Flags: approval-mozilla-beta?
Attachment #702545 - Flags: review?(bas)
Attachment #702545 - Flags: checkin?(bas)
Attachment #702545 - Flags: approval-mozilla-beta?
Waiting on review and check in to mozilla-central before approving for Aurora/Beta.
Attachment #702542 - Flags: review?(bas) → review+
Comment on attachment 702542 [details] [diff] [review]
Check for Cairo surface creation failure (central)

https://hg.mozilla.org/integration/mozilla-inbound/rev/152d3caaf16a
Attachment #702542 - Flags: checkin?(bas) → checkin+
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
tracking-firefox20: --- → +
https://hg.mozilla.org/mozilla-central/rev/152d3caaf16a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

5 years ago
status-firefox21: affected → fixed
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.
Attachment #702544 - Flags: review?(bas)
Attachment #702544 - Flags: approval-mozilla-aurora?
Attachment #702544 - Flags: approval-mozilla-aurora+

Updated

5 years ago
Attachment #702545 - Flags: review?(bas)
Attachment #702545 - Flags: approval-mozilla-beta?
Attachment #702545 - Flags: approval-mozilla-beta+

Comment 34

5 years ago
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.

Updated

5 years ago
status-firefox19: affected → fixed
status-firefox20: affected → fixed
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/
(Reporter)

Comment 38

5 years ago
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.

Updated

5 years ago
Blocks: 823119

Comment 39

5 years ago
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.
status-firefox19: fixed → verified
Attachment #702544 - Flags: checkin?(bas) → checkin+
Attachment #702545 - Flags: checkin?(bas) → checkin+

Comment 40

5 years ago
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.
status-firefox20: fixed → verified
Keywords: verifyme

Comment 41

5 years ago
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.

Updated

5 years ago
status-firefox21: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.