Closed Bug 863159 Opened 7 years ago Closed 6 years ago

Rendering PDF results in huge temporary pixmaps to be created (crashing X)

Categories

(Firefox :: PDF Viewer, defect, P2, critical)

22 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox21 --- unaffected
firefox22 + affected
firefox23 - affected
firefox24 + affected
firefox27 --- verified
firefox28 --- verified
firefox29 --- verified

People

(Reporter: linuxhippy, Assigned: bdahl)

References

(Blocks 1 open bug, )

Details

(4 keywords, Whiteboard: [pdfjs-c-performance][pdfjs-f-regression][pdfjs-f-fixed-upstream] https://github.com/mozilla/pdf.js/issues/3171)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130326150557

Steps to reproduce:

Loaded the PDF located at http://www.informatik.tuwien.ac.at/lehre/services/epilog/poster-mauser.pdf with Firefox internal pdf viewer


Actual results:

Firefox allocated a huge amount of pixmap-data while loading the pdf (over 1.1GB), after resizing the view a few times Firefox caused X to crash (over 4GB of pixmaps allocated)


Expected results:

Firefox should not require such large temporary pixmaps, other viewers are able to render the PDF without problems.
Version: 20 Branch → 23 Branch
Component: Untriaged → PDF Viewer
Seems to be a regression, as I can not reproduce this problem with Firefox-20 but only with a firefox-23 nightly build.
I see the spike in memory on Windows. Aurora is the first version to have the issue and also to render the document without blurry.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, hang, regression
OS: Linux → All
Hardware: x86_64 → All
Version: 23 Branch → 22 Branch
Also found another document which triggers the crash:http://www.informatik.tuwien.ac.at/lehre/services/epilog/2011ws/poster-gruenauer.pdf
I don't get a crash with FF23, only huge hang (Firefox and computer) with important memory use.

Regression range:
m-c
good=2013-03-13
bad=2013-03-14
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c1a5c44ae3d8&tochange=b672877ed046
Before there's one Warning: TODO: TilingType: 1 and after there's a bunch.
I got a different regression range.


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/e6215e0357fa
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130308 Firefox/22.0 ID:20130308135446
Bad:
http://hg.mozilla.org/mozilla-central/rev/9e6232e86000
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130309 Firefox/22.0 ID:20130309160247
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6215e0357fa&tochange=9e6232e86000

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b4bfc1c0829c
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130308 Firefox/22.0 ID:20130308113055
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a19b312bd4f5
Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130308 Firefox/22.0 ID:20130308114346
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b4bfc1c0829c&tochange=a19b312bd4f5


Suspected: 
	8c5d7b67d20d	Ryan VanderMeulen — Bug 848641 - Update pdf.js to version 0.7.337. r=yury
I think it's a regression from bug https://github.com/mozilla/pdf.js/pull/2877.
Priority: -- → P2
Whiteboard: [pdfjs-c-performance][pdfjs-f-regression]
Can you look into this?
Flags: needinfo?(VYV03354)
I saw the huge allocation (>= 2GB) even with locally-built 0.7.311 (the version right before #2877 is merged).
Flags: needinfo?(VYV03354)
The last good commit: d97c5a2b14973051c552775d4248c920e3a6910a (0.7.295)
The first bad commit: 0a7e3645bf9a819561bd0facca7e2353c7c90d89 (0.7.297)
Suspected: #2177 Sharpen the patterns.
https://github.com/mozilla/pdf.js/pull/2177
The PDF in comment #0 has a lot of huge TilingPatterns (11500x11500), so increasing MAX_PATTERN_SIZE badly affects the memory consumption.
Assignee: nobody → VYV03354
Honestly I have no idea how to fix it. I'm not a person who fixed #2177. Maybe @kkujala would know.
Assignee: VYV03354 → nobody
Whiteboard: [pdfjs-c-performance][pdfjs-f-regression] → [pdfjs-c-performance][pdfjs-f-regression][pdfjs-f-fixed-upstream] https://github.com/mozilla/pdf.js/issues/3171
Passing to Brendan for reassignment in that case. We need an owner here on the bugzilla/Firefox side.
Assignee: nobody → bdahl
Attached patch aurora patch (obsolete) — Splinter Review
We're hoping to update mozilla-central in the next few days, so this is only the fix for aurora.
Attachment #745981 - Flags: review?(ydelendik)
Attached patch aurora patchSplinter Review
Attachment #745981 - Attachment is obsolete: true
Attachment #745981 - Flags: review?(ydelendik)
Attachment #749526 - Flags: review?(ydelendik)
Attached patch beta patchSplinter Review
m-c update will be in bug 871530.
Attachment #749527 - Flags: review?(ydelendik)
Attachment #749526 - Flags: review?(ydelendik) → review+
Attachment #749527 - Flags: review?(ydelendik) → review+
But doesn't lead limiting the tiling pattern's size to lower quality? Shouldn't this be handled by implementing some tiling approach?
(In reply to Clemens Eisserer from comment #18)
> But doesn't lead limiting the tiling pattern's size to lower quality?
> Shouldn't this be handled by implementing some tiling approach?

Not sure what do you mean? The max pattern size was increased due to the patches above.
Yury: Sorry, mis-interpreted the patch. Glad to hear that pattern size was increased even more.
Keywords: checkin-needed
(In reply to Brendan Dahl from comment #17)
> Created attachment 749527 [details] [diff] [review]
> beta patch
> 
> m-c update will be in bug 871530.

Are we ready to fix in beta? If so, please nominate for approval.
Flags: needinfo?(bdahl)
Comment on attachment 749526 [details] [diff] [review]
aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 848641
User impact if declined: certain pdfs can consume unnecessary amounts of resources
Testing completed (on m-c, etc.): tested on m-c
Risk to taking this patch (and alternatives if risky): low risk, js only changes to pdf.js 
String or IDL/UUID changes made by this patch: none
Attachment #749526 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bdahl)
Keywords: checkin-needed
Comment on attachment 749527 [details] [diff] [review]
beta patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 848641
User impact if declined: certain pdfs can consume unnecessary amounts of resources
Testing completed (on m-c, etc.): tested on m-c
Risk to taking this patch (and alternatives if risky): low risk, js only changes to pdf.js 
String or IDL/UUID changes made by this patch: none
Attachment #749527 - Flags: approval-mozilla-beta?
Attachment #749526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #749527 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/c56382a7d4a6
https://hg.mozilla.org/releases/mozilla-beta/rev/26a66eb644e0

Fixed on trunk by bug 871530.
No longer blocks: 848641
Status: NEW → RESOLVED
Closed: 7 years ago
Depends on: 871530
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
http://www.informatik.tuwien.ac.at/lehre/services/epilog/poster-mauser.pdf won't load in FF 22b3, latest nightly 2013-05-28. Further more, I even reset my machine once (Intel core 2 duo, 4GB RAM) because it was frozen when trying to load the pdf. So, I would say it's not really fixed?
(In reply to Paul Silaghi [QA] from comment #25)
> http://www.informatik.tuwien.ac.at/lehre/services/epilog/poster-mauser.pdf
> won't load in FF 22b3, latest nightly 2013-05-28. Further more, I even reset
> my machine once (Intel core 2 duo, 4GB RAM) because it was frozen when
> trying to load the pdf. So, I would say it's not really fixed?

Does FF21 work for you? This PDF has some very large images and we just may be running out of resources.
(In reply to Brendan Dahl from comment #26)
> Does FF21 work for you? 
Yes
Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20100101 Firefox/22.0

Everytime I enter fullscreen mode after loading the url from comment 0, FF 22 beta 3 crashes (Build ID: 20130528181031). Here is the crash report: https://crash-stats.mozilla.com/report/index/bp-7c2d4435-4aae-42ba-9050-6d2e22130530
Alexandra: The fix is to be included in FF 24, its not on schedule for FF 22.
Based on comment 24 is fixed on 22 beta, isn't it?
(In reply to Alexandra Lucinet [QA] from comment #28)
> Everytime I enter fullscreen mode after loading the url from comment 0, FF
> 22 beta 3 crashes (Build ID: 20130528181031). Here is the crash report:
> https://crash-stats.mozilla.com/report/index/bp-7c2d4435-4aae-42ba-9050-
> 6d2e22130530
Infinite hang for me in fullscreen on 22.0b3 or Nightly. File a new bug.

(In reply to Clemens Eisserer from comment #29)
> Alexandra: The fix is to be included in FF 24, its not on schedule for FF 22.
What are the changesets of comment 24 for?
Alexandra: Sorry, I missed that.
Reopening based on comment 25.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I has to reboot my computer after displaying the PDF in 22.0b5.
Blocks: 881974
I can confirm opening the referenced PDF file with nightly (FF 24) crashes my system as soon as I switchto slightly higher zoom level. Even for the default zoom-level Firefox allocates about 1Gb of pixmaps.
Brendan - should we back out the patches which did not seem to fix the issue?  Or do you have time for a speculative forward fix in the next couple of weeks before we slow down on taking change to FF23 betas?
Flags: needinfo?(bdahl)
Keywords: reproducible
The patch should still lessen the amount of memory used, so it would be good to leave it in.  We can try to reduce the maximum size of patterns allowed though.  I'll try get a patch together, but it is somewhat low priority at the moment.  If anyone is interested in testing, you'd only need to lower http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/build/pdf.js#33980
Flags: needinfo?(bdahl)
> I'll try get a patch together, but it is somewhat low priority at the moment.  

A browser crashing operating systems when viewing PDFs is "somewhat low priority".
When considering pdfjs has now lived through a few Firefox iterations and still isn't reliable - I wonder if a simple pdf viewer based on poppler wouldn't have been a better idea (or at least pdfjs should not have been integrated in such an early state).
(In reply to Joseph Fitzgerald from comment #38)
> or at least pdfjs should not have been integrated in such an early state
It was scheduled in Firefox 14 and landed in Firefox 19.
(In reply to Joseph Fitzgerald from comment #38)
> > I'll try get a patch together, but it is somewhat low priority at the moment.  
> 
> A browser crashing operating systems when viewing PDFs is "somewhat low
> priority".
> When considering pdfjs has now lived through a few Firefox iterations and
> still isn't reliable - I wonder if a simple pdf viewer based on poppler
> wouldn't have been a better idea (or at least pdfjs should not have been
> integrated in such an early state).

A browser shouldn't be able to crash an operating system. We should work on filing upstream bugs for that.

The real fix for this issue should be outside of pdf.js, since any webpage can do what we're doing currently.  I'll try to create a minimal test case without the pdf.js code.
> The real fix for this issue should be outside of pdf.js, 
> since any webpage can do what we're doing currently.

Yes and no. 

For sure the browser should keep track of allocated resources and take care malicious code can not bring user machines down.
However, wouldn't pdf.js simply stop to work in those OOM cases?
We're too late here for a speculative fix in Beta, if there's a low risk fix later on you can nominate for uplift but we're  not inclined to take major platform change here.
Tracking to see if we can still get this in Fx24 given we are in early beta's.But given this is a regression since FX 22 will not  block the release for this.
Do we have many crash reports for this type of issue?
(In reply to Yury Delendik (:yury) from comment #44)
> Do we have many crash reports for this type of issue?
Crash stats don't collect graphics driver hangs.
> Crash stats don't collect graphics driver hangs.

Just to clarify, this has nothing to do with poorly written or faulty graphic drivers.
Firefox allocates insane amount of resources leading to resource exhaustion - which might result in a killed firefox instance in good cases, or in system crashes in bad ones.
I noticed beach-balling on Mac OS X 10.6.8 when testing urls in bug 880140 with both 24beta5 and Nightly (2013-08-26)
I wonder how little attention this bug receives.
Just open a "bad" PDF (and I bet there are quite a few) and at least firefox or probably even your system crashes.
Bug 912006 includes are fix to reduce size of the temporary pattern canvases. Does this fix your issue?
Flags: needinfo?(linuxhippy)
The original issue is resolved, but quality suffers.

It looks like rendered at low resolution and scaled up, which makes the boxes and arrows look *pretty* bad :/
Flags: needinfo?(linuxhippy)
Thanks!
Can this bug be closed now?
Seems to work fine now on FF 27b5, 28.0a2(2014-01-12), 29.0a1(2014-01-12), Win 7 x64.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.