Closed Bug 829954 Opened 12 years ago Closed 11 years ago

OOM crash in mozilla::gfx::AlphaBoxBlur::Blur

Categories

(Core :: Graphics, defect, P1)

Product:
Core
Component:
Graphics
Version:
19 Branch
Platform:
x86
Windows 7
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla24
Tracking Flags:
Tracking Status
firefox18 --- unaffected
firefox19 + wontfix
firefox20 --- wontfix
firefox21 + wontfix
firefox22 + verified
firefox23 + verified
firefox24 --- verified

People

(Reporter: scoobidiver, Assigned: bas.schouten, NeedInfo)

References

Details

(4 keywords, Whiteboard: [MemShrink:P1])

Crash Data

Attachments

(3 files, 1 obsolete file)

Should we check for 2^24 given the alignment and padding instead?
2.29 KB, patch
Details | Diff | Splinter Review
Make AlignedArray use fallible allocation.
806 bytes, patch
jrmuizel
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
Part 2: Check AlignedArray for allocation success.
2.11 KB, patch
jrmuizel
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review 
It's #44 browser crasher in 19.0b1 and #47 in 21.0a1.
It first showed up in 19.0a1/20121109. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=36e99ea02c05&tochange=90cea19e27e2
It might be caused by bug 685012.

Signature 	mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur() More Reports Search
UUID	2f581d2a-9da6-4665-9f46-7215d2130112
Date Processed	2013-01-12 07:44:56
Uptime	2261
Last Crash	18.1 hours before submission
Install Age	21.2 hours since version was first installed.
Install Time	2013-01-11 10:28:58
Product	Firefox
Version	21.0a1
Build ID	20130110030939
Release Channel	nightly
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 8 stepping 3
Crash Reason	EXCEPTION_BREAKPOINT
Crash Address	0xf519a7
App Notes 	
AdapterVendorID: 0x10c8, AdapterDeviceID: 0x0016, AdapterSubsysID: 00000000, AdapterDriverVersion: 5.1.2001.0
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
EMCheckCompatibility	True
Adapter Vendor ID	0x10c8
Adapter Device ID	0x0016
Total Virtual Memory	2147352576
Available Virtual Memory	1688936448
System Memory Use Percentage	90
Available Page File	7081984
Available Physical Memory	13189120
OOMAllocationSize	1277804

Frame 	Module 	Signature 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:30
1 	mozalloc.dll 	mozalloc_handle_oom 	memory/mozalloc/mozalloc_oom.cpp:27
2 	mozalloc.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:56
3 	gkmedias.dll 	mozilla::gfx::AlphaBoxBlur::Blur 	gfx/2d/Blur.cpp:525
4 	xul.dll 	gfxAlphaBoxBlur::Paint 	gfx/thebes/gfxBlur.cpp:86
5 	xul.dll 	nsContextBoxBlur::DoPaint 	layout/base/nsCSSRendering.cpp:4792
6 	xul.dll 	nsCSSRendering::PaintBoxShadowOuter 	layout/base/nsCSSRendering.cpp:1351
7 	xul.dll 	nsDisplayBoxShadowOuter::Paint 	layout/base/nsDisplayList.cpp:2432
8 	xul.dll 	mozilla::FrameLayerBuilder::DrawThebesLayer 	layout/base/FrameLayerBuilder.cpp:3336
9 	xul.dll 	mozilla::layers::BasicShadowableThebesLayer::PaintBuffer 	gfx/layers/basic/BasicThebesLayer.cpp:403
10 	xul.dll 	mozilla::layers::BasicThebesLayer::PaintThebes 	gfx/layers/basic/BasicThebesLayer.cpp:190

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+mozalloc_handle_oom%28unsigned+int%29+|+moz_xmalloc+|+mozilla%3A%3Agfx%3A%3AAlphaBoxBlur%3A%3ABlur%28%29
This report has more stack frames: bp-efb6d972-62e9-4336-b941-7b7022130112
It seems unlikely that bug 685012 (CSS page-break layout code) is the culprit.
Bug 807925 would probably be my first guess in that range since it appears to
have something to do with painting and images.
Here's an interesting crash report(see User Comments and App Notes): bp-25827a02-8569-48e0-bcdf-2349a2130119.

More reports also at:
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*+const%29+|+moz_xmalloc+|+mozilla%3A%3Agfx%3A%3AAlphaBoxBlur%3A%3ABlur%28%29
Crash Signature: [@ mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur()] → [@ mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur()] [@ mozalloc_abort(char const* const) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur()]
Depends on: 834256
As the STR in bug 834256 report this is happening with viewing PDFs and AFAIK we intend to ship PDF.js with 19, I'm nominating this.
When I run across a site making various things available as PDFs, I'm quite likely to open multiple ones at once in multiple tabs, I guess there's a good collection of other people out there probably doing the same, and we should crash in that case as those STR suggest we do.
tracking-firefox19: --- → ?
(In reply to Scoobidiver from comment #2)
> Here's an interesting crash report(see User Comments and App Notes):
> bp-25827a02-8569-48e0-bcdf-2349a2130119.

The App Notes in the UI has been cut off because of the length. Here's the full field as seen in raw JSON:
Notes": "AdapterVendorID: 0x8086, AdapterDeviceID: 0x2a42, AdapterSubsysID: 3a0217aa, AdapterDriverVersion: 8.15.10.2302\nD2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ Successful print.\nPrint object tree:\n ad41680 = { mFrameType = 0, mHasBeenPrinted = true, mDontPrint = false, mPrintAsIs = false, mInvisible = false, mPrintPreview = false, mDidCreateDocShell = true, mShrinkRatio = 1, mZoomRatio = 0.9, mContent = null }\nSuccessful print.\nPrint object tree:\n 880f940 = { mFrameType = 0, mHasBeenPrinted = true, mDontPrint = false, mPrintAsIs = false, mInvisible = false, mPrintPreview = false, mDidCreateDocShell = true, mShrinkRatio = 1, mZoomRatio = 0.9, mContent = null }\n 1a9a91c0 = { mFrameType = 1, mHasBeenPrinted = false, mDontPrint = true, mPrintAsIs = true, mInvisible = true, mPrintPreview = false, mDidCreateDocShell = false, mShrinkRatio = 1, mZoomRatio = 1, mContent = iframe }\nSuccessful print.\nPrint object tree:\n 363edc80 = { mFrameType = 0, mHasBeenPrinted = true, mDontPrint = false, mPrintAsIs = false, mInvisible = false, mPrintPreview = false, mDidCreateDocShell = true, mShrinkRatio = 1, mZoomRatio = 0.9, mContent = null }\nSuccessful print.\nPrint object tree:\n 20734f00 = { mFrameType = 0, mHasBeenPrinted = true, mDontPrint = false, mPrintAsIs = false, mInvisible = false, mPrintPreview = false, mDidCreateDocShell = true, mShrinkRatio = 1, mZoomRatio = 0.9, mContent = null }\nPrint object tree:\n 1a76d140 = { mFrameType = 0, mHasBeenPrinted = false, mDontPrint = true, mPrintAsIs = false, mInvisible = false, mPrintPreview = false, mDidCreateDocShell = true, mShrinkRatio = 1, mZoomRatio = 1, mContent = null }\n 1d062340 = { mFrameType = 1, mHasBeenPrinted = false, mDontPrint = true, mPrintAsIs = false, mInvisible = false, mPrintPreview = false, mDidCreateDocShell = false, mShrinkRatio = 1, mZoomRatio = 1, mContent = iframe }\n",
Also, this has been rising in 19 data since 19.0b3 was released.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #5)
> Also, this has been rising in 19 data since 19.0b3 was released.
It's indeed #25 top browser crasher in 19.0b3.
Marking tracking so we can watch this in b4 results which should be coming in soon.
tracking-firefox19: ? → +
Assignee: nobody → karlt
20121109 was one day after bug 509052 landed.
Assignee: karlt → bas
Blocks: 509052
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #3)
> As the STR in bug 834256 report this is happening with viewing PDFs and
> AFAIK we intend to ship PDF.js with 19, I'm nominating this.

Bug 748923 implies that PDF.js should not be enabled on beta.
(In reply to Karl Tomlinson (:karlt) from comment #9)
> Bug 748923 implies that PDF.js should not be enabled on beta.

But it is, AFAIK, and as of now, there's (from what I heard) no intent to disable it before release of 19, unless Something Bad Happens™.
(In reply to Karl Tomlinson (:karlt) from comment #8)
> 20121109 was one day after bug 509052 landed.

That bug would've changed the signature so I wonder if there's simply an equivalent crash from before that bug.
We should grab URLs and get some QA around this, since the engineering investigation hasn't turned much up yet.
Keywords: needURLs, qawanted, steps-wanted
bp-25827a02-8569-48e0-bcdf-2349a2130119 had an allocation request of 93 MiB, but not all reports have requests this large.  Many times as much virtual and physical memory is reported as available, but perhaps these numbers are not what I thought they were.

Would checking a fallible allocation request and falling back to the other path use less memory?
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Familly_Regist
http://www.facebook.com/
https://www.facebook.com/
http://web.ebuddy.com/?startsession=1#
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
http://www.detik.com/
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
http://apps.facebook.com/criminalcase/?fb_source=bookmark_apps&ref=bookmarks&cou
http://digsitevalue.net/s/dogma.hr
http://mashughuli.blogspot.com/2013/01/hellen-urio-rahma-kitchen-party.html#more
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
http://www.cartoonclub-th.com/2011/12/berserk-241-250.html
http://9gag.com/hot
http://br46.tribalwars.com.br/game.php?village=305818&screen=main
http://ssc.nic.in/press-release/CHSL_2013_ZEROS.pdf
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
http://www.bien.hu/hazi-fogfeherites-termeszetesen,szepseg,arc,111314
http://9gag.com/
http://www.gardenshop.ru/products/Roses_spring_2013/?arrFilter_cf%5B1%5D%5BLEFT%
http://www.qcdriver.com/jobs/indeed/oo-tank-reg-otr-chicago.html?utm_source=inde
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
https://twitter.com/login
http://www.linhadefensiva.org/forum/topic/148169-chave-de-registro-infectada-rem
http://www.nejm.org/doi/pdf/10.1056/NEJMe1110900
http://www.tumblr.com/dashboard
http://www.betlighting.com.mx/PDF/Beghelli.pdf
http://internet-positif.org/site.block/MThzY2hvb2xnaXJsei5jb20%3D
http://www.berniaga.com/Laptop+Notebook+TOSHIBA+Dynabook-11810700.htm
https://twitter.com/download/?lang=pt&logged_out=1
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
http://hwd.up.nic.in/pensioner%20list/bareilly.pdf
http://www.n-dorphin.wowstead.com/
http://www.bild.de/
http://mashughuli.blogspot.com/2013/02/jamimas-night-pj-hall-kinondoni.html#more
http://magals.tumblr.com/page/16
http://bet-2013.blogspot.com/b/layout-preview?token=aWILuDwBAAA.k-NG7LTJZe0FVjw5
http://www.cartoonclub-th.com/2011/07/toriko-81-90.html
http://www.jasolar.com/uploads/files/201212/20121211153830_KHRi5P.pdf
wyciwyg://73923/http://lax1.ib.adnxs.com/if?enc=AAAAAAAAFEAAAAAAAAAUQAAAAMDMzAhA
http://revistas.inpi.gov.br/pdf/marcas1970.pdf
http://www.facebook.com/ai.php?aed=AQL1Ba2Vv2UHjTF6rPqgHa9qVvY9KKkbnuq_KUjOCAghJ
wyciwyg://35179/http://fanfiction.mugglenet.com/browse.php?type=titles
https://www.facebook.com/ajax/pagelet/generic.php/PhotoViewerInitPagelet?ajaxpip
http://akcontent.ebuddy.com/web_banners/invocation.html?z=895&t=1:1;2:34;3:BR&w=
http://sssm.nic.in/DataManagement/SamagraData/Pages/Rural_NPR_KYC_Family_Member_
https://www.google.co.id/search?hl=id&site=imghp&tbm=isch&source=hp&biw=1152&bih
http://animeid.tv/ver/blood-49
http://www.ncbi.nlm.nih.gov/protein/347840211?report=fasta
http://report.lpse.jabarprov.go.id/home.php?click=s1k04ktie8f0dl44gvbp30b3uds962
http://platform.twitter.com/widgets/tweet_button.html?show_screen_name=false&tex
http://hollyjadepeers.blogspot.com/?zx=19cb447f51d9085e
http://play-therapy.com/playfulpooch/images_resources/APDT.EmotionalDog.pdf
http://oasc12.247realmedia.com/RealMedia/ads/adstream_sx.ads/pu
http://www.ehow.com/how_8415841_plan-home-cctv-camera-system.html
http://aptransport.org/html/form26a.htm
http://view.atdmt.com/MRT/iview/436906037/direct/01/20130206174
http://www.milaiwang.com/2013/02/blog-post_2576.html
http://www.fanfiction.net/s/6099883/3/Now-And-The-Past
https://www.facebook.com/ajax/home/generic.php?ajaxpipe=1&ajaxpipe_token=AXhm6ht
http://www.section508.gov/docs/pdfguidanceforgovernment.pdf
http://motherless.com/V76764F7
http://www.kaskus.co.id/thread/000000000000000011134756/freestyle-casing-section
http://www.rusalarm.ru/assets/files/Starline/sl_a8_red3.pdf
http://www.posterheroes.org/category/blog/
http://www.sonorestaurant.com.au/media/pdfs/DinnerMenuNov.pdf
http://www.havator.com/media/files/news-magazines/havator_wind_eng.pdf
https://ox-social.bidsystem.com/w/1.0/afr?auid=349675&cb=1360241712&ep=tQdN0_58p
http://www.facebook.com/plugins/like.php?api_key=&locale=en_US&sdk=joey&channel_
http://h.cartoonclub-th.com/2013/02/i-friggin-hate-boobs.html
http://www.facebook.com/ajax/pagelet/generic.php/MoreStoriesPagelet?ajaxpipe=1&a
http://edge.omnitwig.com/setImpData.html?p=CPX&i=1&a=Brain&c=471caa2f-32ee-4cb5-
http://www.texa.com/gfx/depliant/pieghevole_KONFORT_600E_en-GB.pdf
https://play.google.com/apps/publish/v2/?dev_acc=08895330968769390461#MarketList
http://9gag.com/trending
https://plusone.google.com/_/+1/fastbutton?bsv&size=medium&annotation=none&hl=en
http://www.google.gr/#hl=el&tbo=d&sclient=psy-ab&q=%CE%98%CE%9F%CE%A1%CE%A5%CE%9
http://www.emmaus-reisen.de/images/pdf/katalog_web.pdf
http://www.homeaway.co.uk/England/holiday-barn-Surrey/p62385.htm#calendar
https://www.google.com.pk/
http://pauli.uni-muenster.de/tp/fileadmin/lehre/vorlesungen/klasen/Physik3/kohl1
http://www.facebook.com/ajax/pagelet/generic.php/MoreStoriesPagelet?ajaxpipe=1&a
http://www.tochigi-agri.or.jp/shunosoudsan/pdf/kinyurei.pdf
http://assets.tumblr.com/analytics.html?459463e736f5e14a6b709883439e6021
https://www.facebook.com/ai.php?aed=AQIj-oRR5-qW0CKx5O1rymTeRdR4KAIevkLmlPO9ia3j
https://customer.onlinelic.in/LICEPS/resources/pdf/PrmPayRcpt-MSBI2912430896.pdf
http://www.orkut.com.br/Main#Conversations
http://www.fold3.com/document/271896294/
http://www.reverbnation.com/naisflavia
http://submit-jxb.oxfordjournals.org/submission/pdf?msid=JEXBOT/2013/094433&role
https://www.facebook.com/ajax/ei.php?aed=AT50F2oH7rkjqu7v5vKmFvLsNogpDukQnfPjxG8
http://educ.lfhk.cuni.cz/file.php/43/G-12-11-20/12_gen-Light_Electron_Micro.pdf
http://www.yophim.com/2012/06/tinh-cam.html
http://www.redtube.com/
http://www.facebook.com/
http://lamasat-smsmia.blogspot.com/2011/04/blog-post_12.html
http://ficheros.esri.es/conferencia2011/conferencia/medioambiente/imperialcolleg
http://www.whitepages.com/name/Carl-D-Reinhardt/North-Sioux-City-
Keywords: needURLs
(In reply to juan becerra [:juanb] from comment #14)
> http://sssm.nic.in/DataManagement/SamagraData/Pages/
> Rural_NPR_KYC_Familly_Regist
> http://www.facebook.com/
> https://www.facebook.com/
> http://web.ebuddy.com/?startsession=1#
> http://sssm.nic.in/DataManagement/SamagraData/Pages/
> Rural_NPR_KYC_Family_Member_
> ...

Hae we attempted to reproduce?
I've been trying to reproduce this using the information in comment #2 and the URLs in comment #14. It looks like people are opening PDFs, and I have been trying to:

- Open within the browser, using Firefox PDF Preview by default
- Open within the browser, using Adobe Reader
- Opening in a separate Adobe Reader window

And trying thins like printing, but so far no luck.
Based on Bug 834256, you can crash Firefox by opening multiple tabs (6-14) with http://people.mozilla.com/~ydelendik/bug834256/web/viewer.html web page and wait.
That might be a different crash. Opening multiple pdf documents in tabs crashes the browser and yields an empty report. This might be something else.
This seems to be in the <= 4kx4k branch of the code.  How hard is the "if larger than 1 << 24, go to a different branch of the code"? Just a magic number assumed to be reasonably small, or is there a hard boundary at 4kx4k (1<<24)?
(In reply to Milan Sreckovic [:milan] from comment #19)
> This seems to be in the <= 4kx4k branch of the code.  How hard is the "if
> larger than 1 << 24, go to a different branch of the code"? Just a magic
> number assumed to be reasonably small, or is there a hard boundary at 4kx4k
> (1<<24)?

There's a hard boundary, beyond that you lack the precision in the integral image fields. After all you can only store 2^24 times 8 bits in a 32-bit number :)
(In reply to Bas Schouten (:bas.schouten) from comment #20)
> There's a hard boundary, beyond that you lack the precision in the integral
> image fields. After all you can only store 2^24 times 8 bits in a 32-bit
> number :)

I have to admit mine was a leading question :-)
If width is 4k, height is 4k, we go into else (as 4k*4k == 1<<24), and ask the AlignedArray to give us back 1<<24 + 12 (stride is 4*4k in this example) in AlphaBoxBlur::Blur():

AlignedArray<uint32_t> integralImage((integralImageStride / 4) * integralImageSize.height + 12);

Is this OK?
Flags: needinfo?(bas)
The worst case scenario is width=1, height=16M.  In the old code, 1*16M == 1<<24, so we would go into else, stride would be 16, we would end up asking for 64M+12, which is definitely > 1<<24.
Attachment #713509 - Flags: feedback?(bas)
(In reply to Milan Sreckovic [:milan] from comment #22)
> Created attachment 713509 [details] [diff] [review]
> Should we check for 2^24 given the alignment and padding instead?
> 
> The worst case scenario is width=1, height=16M.  In the old code, 1*16M ==
> 1<<24, so we would go into else, stride would be 16, we would end up asking
> for 64M+12, which is definitely > 1<<24.

Yes, this is padding we use for being able to read 16 bytes at a time for SSE2 reasons. As these do not result in additional accumulation that's okay.
Flags: needinfo?(bas)
Thanks.  I was misled and confused by the code:
if (something > 16M) {
    special case for large allocations
} else {
    allocate 64M+
}
and it isn't immediately obvious that this is what the author intended.  Is there a good comment we can put in there to stop any further confusion?  Or is it just confusing to me?
Dropping qawanted since attempts to reproduce this have proved to be fruitless. Please re-add qawanted if you have any new leads you'd like us to check.
Keywords: qawanted
(In reply to Milan Sreckovic [:milan] from comment #24)
> Thanks.  I was misled and confused by the code:
> if (something > 16M) {
>     special case for large allocations
> } else {
>     allocate 64M+
> }
> and it isn't immediately obvious that this is what the author intended.  Is
> there a good comment we can put in there to stop any further confusion?  Or
> is it just confusing to me?

I would argue the other (being me), did an extremely poor job at documenting this exception and a comment should definitely be added to clarify this! :)
Well 19 is out the door now and this wouldn't drive a chemspill, so wontfixing for 19.
status-firefox19: affected → wontfix
Depends on: 845260
Bug 845260 has STR.
status-firefox22: --- → affected
Bas, I don't understand what happened to this bug: this is an OOM crash. Are you saying that the math in 

(integralImageStride / 4) * integralImageSize.height + 12);

is incorrect and we're asking for the wrong amount of memory? It seems that the obvious problem here is that the AlignedArray<uint32_t> is using an infallible allocator for large buffer allocation, and this should instead be using a fallible allocator and null-checking.

See bug 857030 for somebody who experiences this. I expect that this crash is the primary cause for the increase of EMPTY DUMP crashes (bug 837835), which are our top stability priority right now.
Blocks: 837835, 834256
No longer depends on: 834256
Flags: needinfo?(bas)
Comment 0 shows mozilla::gfx::AlphaBoxBlur::Blur failing to allocate 1277804 bytes, which isn't really huge but should probably fail gracefully. Sounds like we might have two problems:
1) mozilla::gfx::AlphaBoxBlur::Blur can cause us to crash on OOM, could fail gracefully
2) If this memory doesn't get freed right away then this could cause us to OOM elsewhere, which might show up as an empty-dump OOM crash.
I think we should track this for FF21 given comment 29
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #31)
> Comment 0 shows mozilla::gfx::AlphaBoxBlur::Blur failing to allocate 1277804
> bytes, which isn't really huge but should probably fail gracefully. Sounds
> like we might have two problems:
> 1) mozilla::gfx::AlphaBoxBlur::Blur can cause us to crash on OOM, could fail
> gracefully
> 2) If this memory doesn't get freed right away then this could cause us to
> OOM elsewhere, which might show up as an empty-dump OOM crash.

I'm not really sure how graceful we could fail? If we don't have room in our address space for 1277804 bytes doesn't that success we're done for and are better of crashing?

How I'm not sure how using a fallible allocator and a null check would help? We'd just fail in some other part of (possibly GFX code) later shouldn't we? Wasn't the whole point to make out allocations infallible? Maybe I totally misunderstood.
Flags: needinfo?(bas)
Yeah, there's a fundamental misunderstanding of the infallible allocator here.

The point of the infallible allocator was to make small and mostly fixed-size allocations fail. Large allocations (anything which has a content-controlled size) are still supposed to use fallible allocators, null-check, and fail more gracefully. This is especially important for huge allocations (>256k) which can fail do to VM page fragmentation even if there is enough available memory to handle the small allocations from normal browser operation.
Priority: -- → P1
Whiteboard: [MemShrink]
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #34)
> Yeah, there's a fundamental misunderstanding of the infallible allocator
> here.
> 
> The point of the infallible allocator was to make small and mostly
> fixed-size allocations fail. Large allocations (anything which has a
> content-controlled size) are still supposed to use fallible allocators,
> null-check, and fail more gracefully. This is especially important for huge
> allocations (>256k) which can fail do to VM page fragmentation even if there
> is enough available memory to handle the small allocations from normal
> browser operation.

At this point we know we're going to actively producing artifacts though, and it's going to be particularly interesting to have higher level code deal gracefully with surfaces not having been created and initialized properly. Making this allocation fallible won't be a big issue, but having that continue into a positive browsing experience is going to be a more interesting challenge.
That's true; but comment data shows that users on low-memory machines pay attention to things like rendering artifacts as a sign of low memory and they will quit/restart Firefox. I think it's worth it in this case at least.
Whiteboard: [MemShrink] → [MemShrink:P1]

        Attachment #738291 -
        Flags: review?(jmuizelaar)

        Attachment #738291 -
        Flags: review?(jmuizelaar) → review+

    
    

    
  
It has entered the top-20 crashers in 20.0.1.
Keywords: topcrash

    
    

    
  
Bas, do you know why both these bugs bounced?
Flags: needinfo?(bas)

    
    

    
  
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> Bas, do you know why both these bugs bounced?

I hadn't noticed! Ugh. I'm on it.
Flags: needinfo?(bas)

    
  
Crash Signature: [@ mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur()]
[@ mozalloc_abort(char const* const) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur()] → [@ mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur()]
[@ mozalloc_abort(char const* const) | moz_xmalloc | mozilla::gfx::AlphaBoxBlur::Blur()]
[@ moz_abort | arena_run_split | arena_…

    
  

          status-firefox23:
          --- → affected

    
    

    
  
(In reply to Wayne Mery (:wsmwk) from comment #43)
> me bp-d70e783f-4317-4ad4-a0e0-802a12130430

(not me, but dee, who has a variety of OOM crash sigs only one of which is AlphaBoxBlur) bp-6429ea66-8650-4b52-8b9a-09b8f2130510
Flags: needinfo?(bas)

    
    

    
  
Pinging about this in email.

    
    

    
  


  

        Attachment #738291 -
        Attachment is obsolete: true
Flags: needinfo?(bas)

    
    

    
  
Comment on attachment 755862 [details] [diff] [review]
Make AlignedArray use fallible allocation.

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

::: gfx/2d/Tools.h
@@ +109,5 @@
>  
>    MOZ_ALWAYS_INLINE void Realloc(size_t aSize)
>    {
>      delete [] mStorage;
> +    mStorage = new (nothrow) T[aSize + (alignment - 1)];

Should be

  static fallible_t fallible = fallible_t();
  mStorage = new (fallible) T[aSize + (alignment - 1)];

    
    

    
  
(In reply to :Ms2ger from comment #47)
> Comment on attachment 755862 [details] [diff] [review]
> Make AlignedArray use fallible allocation.
> 
> Review of attachment 755862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Tools.h
> @@ +109,5 @@
> >  
> >    MOZ_ALWAYS_INLINE void Realloc(size_t aSize)
> >    {
> >      delete [] mStorage;
> > +    mStorage = new (nothrow) T[aSize + (alignment - 1)];
> 
> Should be
> 
>   static fallible_t fallible = fallible_t();
>   mStorage = new (fallible) T[aSize + (alignment - 1)];

Why would we do something so convoluted? Does jemalloc not provide the right overloads? And if so why wouldn't we do:

mStorage = new (fallible_t()) T[aSize + (alignment - 1)];

At least that's a little less ugly. In any case, inside Moz2D we don't have the fallible_t() struct, so that's not going to happen anyway :).

    
  

        Attachment #755862 -
        Flags: review?(jmuizelaar)

    
    

    
  
We're going to build with b4 on Tuesday, so would be great to get review/landed prior.

    
    

    
  
Comment on attachment 756315 [details] [diff] [review]
Part 2: Check AlignedArray for allocation success.

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

::: gfx/2d/Blur.cpp
@@ +469,5 @@
> +      unsigned char* tmpData = new (std::nothrow) uint8_t[szB];
> +
> +      if (!tmpData) {
> +        return;
> +      }

If this works it seems ok.

        Attachment #756315 -
        Flags: review?(jmuizelaar) → review+

        Attachment #755862 -
        Flags: review?(jmuizelaar) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0afc6a9002c5
https://hg.mozilla.org/mozilla-central/rev/ff382b7f221e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

    
    

    
  
So, this landed on central, and on beta without explicit approval, shouldn't we get it into aurora as well?

    
    

    
  
(In reply to Robert Kaiser (:kairo@mozilla.com) [away until early June] from comment #54)
> So, this landed on central, and on beta without explicit approval, shouldn't
> we get it into aurora as well?

Approval was given over email

    
    

    
  
And yes, we should uplift to Aurora.
Flags: needinfo?(bas)

    
  

        Attachment #755862 -
        Flags: approval-mozilla-beta+

        Attachment #755862 -
        Flags: approval-mozilla-aurora+

    
  

        Attachment #756315 -
        Flags: approval-mozilla-beta+

        Attachment #756315 -
        Flags: approval-mozilla-aurora+

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM] from comment #57)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/be6d5d592057
> https://hg.mozilla.org/releases/mozilla-aurora/rev/261bee70e621

Thank you!

Looks good!
Flags: needinfo?(bas)

    
    

    
  
No crashes on FF > 21. I guess we can call this bug verified.
Status: RESOLVED → VERIFIED

          status-firefox22:
          fixed → verified

          status-firefox23:
          fixed → verified

          status-firefox24:
          fixed → verified

    
  

        Attachment #713509 -
        Flags: feedback?(bas)

    
  
Flags: needinfo?(amanda)
Flags: needinfo?(amanda)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  
Restrict Comments: true

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: