Closed Bug 803568 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 + verified
firefox20 + verified
firefox21 --- verified

People

(Reporter: raul.malea, Assigned: milan)

References

()

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(3 files, 6 obsolete files)

4.40 KB, patch
bas.schouten
: review+
bas.schouten
: 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
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
Crash Signature: _VEC_memcpy | mozilla::gfx::CopyDataToCairoSurface
See Also: → 800523
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
Forget comment 2, Firefox crashes when toggling full screen and playing with zoom.
(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 .
It's #14 top browser crasher in 19.0a2.
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
Assignee: bugs → milan
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]
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?
No need to be full screen, it will crash in the window as well.
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+
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.
If the image is 8192 we're fine, if it's 8193, we're not. See 824981.
Duplicate of this bug: 825037
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)
[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+
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?
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)
[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?
[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.
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).
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)
[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?
[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+
https://hg.mozilla.org/mozilla-central/rev/152d3caaf16a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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+
Attachment #702545 - Flags: review?(bas)
Attachment #702545 - Flags: approval-mozilla-beta?
Attachment #702545 - Flags: approval-mozilla-beta+
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.
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.
Blocks: 823119
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.
Attachment #702544 - Flags: checkin?(bas) → checkin+
Attachment #702545 - Flags: checkin?(bas) → checkin+
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.
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.
You need to log in before you can comment on or make changes to this bug.