Last Comment Bug 829954 - OOM crash in mozilla::gfx::AlphaBoxBlur::Blur
: OOM crash in mozilla::gfx::AlphaBoxBlur::Blur
Status: VERIFIED FIXED
[MemShrink:P1]
: crash, regression, steps-wanted, topcrash
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 19 Branch
: x86 Windows 7
: P1 critical with 2 votes (vote)
: mozilla24
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
: 857030 (view as bug list)
Depends on: 845260
Blocks: 834256 509052 837835
  Show dependency treegraph
 
Reported: 2013-01-12 07:27 PST by Scoobidiver (away)
Modified: 2016-04-18 00:33 PDT (History)
38 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
wontfix
wontfix
+
wontfix
+
verified
+
verified
verified


Attachments
Should we check for 2^24 given the alignment and padding instead? (2.29 KB, patch)
2013-02-13 10:54 PST, Milan Sreckovic [:milan]
no flags Details | Diff | Splinter Review
Make AlphaBoxBlur allocation fallible (5.21 KB, patch)
2013-04-16 18:54 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review
Make AlignedArray use fallible allocation. (806 bytes, patch)
2013-05-30 04:09 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Part 2: Check AlignedArray for allocation success. (2.11 KB, patch)
2013-05-30 18:51 PDT, Bas Schouten (:bas.schouten)
jmuizelaar: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2013-01-12 07:27:37 PST
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
Comment 1 Mats Palmgren (:mats) 2013-01-12 20:57:41 PST
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.
Comment 2 Scoobidiver (away) 2013-01-24 09:11:09 PST
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
Comment 3 Robert Kaiser 2013-01-28 09:03:31 PST
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.
Comment 4 Robert Kaiser 2013-01-28 09:05:39 PST
(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",
Comment 5 Robert Kaiser 2013-01-29 06:34:29 PST
Also, this has been rising in 19 data since 19.0b3 was released.
Comment 6 Scoobidiver (away) 2013-01-30 05:45:16 PST
(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.
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-30 12:18:47 PST
Marking tracking so we can watch this in b4 results which should be coming in soon.
Comment 8 Karl Tomlinson (:karlt) 2013-01-30 17:14:25 PST
20121109 was one day after bug 509052 landed.
Comment 9 Karl Tomlinson (:karlt) 2013-01-30 17:20:02 PST
(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.
Comment 10 Robert Kaiser 2013-01-30 18:24:32 PST
(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™.
Comment 11 Bas Schouten (:bas.schouten) 2013-01-31 08:12:56 PST
(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.
Comment 12 Alex Keybl [:akeybl] 2013-02-07 16:18:21 PST
We should grab URLs and get some QA around this, since the engineering investigation hasn't turned much up yet.
Comment 13 Karl Tomlinson (:karlt) 2013-02-07 17:06:30 PST
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?
Comment 14 juan becerra [:juanb] 2013-02-07 17:27:59 PST
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-
Comment 15 Alex Keybl [:akeybl] 2013-02-11 12:57:39 PST
(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?
Comment 16 juan becerra [:juanb] 2013-02-11 13:25:30 PST
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.
Comment 17 Milan Sreckovic [:milan] 2013-02-11 15:01:25 PST
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.
Comment 18 juan becerra [:juanb] 2013-02-11 15:26:51 PST
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.
Comment 19 Milan Sreckovic [:milan] 2013-02-12 12:26:49 PST
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)?
Comment 20 Bas Schouten (:bas.schouten) 2013-02-12 22:55:20 PST
(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 :)
Comment 21 Milan Sreckovic [:milan] 2013-02-13 09:59:39 PST
(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?
Comment 22 Milan Sreckovic [:milan] 2013-02-13 10:54:34 PST
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.
Comment 23 Bas Schouten (:bas.schouten) 2013-02-13 20:08:01 PST
(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.
Comment 24 Milan Sreckovic [:milan] 2013-02-14 07:34:18 PST
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?
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-02-14 13:23:00 PST
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.
Comment 26 Bas Schouten (:bas.schouten) 2013-02-15 07:30:07 PST
(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! :)
Comment 27 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-20 14:27:43 PST
Well 19 is out the door now and this wouldn't drive a chemspill, so wontfixing for 19.
Comment 28 Scoobidiver (away) 2013-02-26 05:19:47 PST
Bug 845260 has STR.
Comment 29 Benjamin Smedberg [:bsmedberg] 2013-04-04 07:57:13 PDT
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.
Comment 30 Scoobidiver (away) 2013-04-04 08:18:45 PDT
*** Bug 857030 has been marked as a duplicate of this bug. ***
Comment 31 Ted Mielczarek [:ted.mielczarek] 2013-04-04 08:49:48 PDT
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.
Comment 32 Alex Keybl [:akeybl] 2013-04-04 09:26:34 PDT
I think we should track this for FF21 given comment 29
Comment 33 Bas Schouten (:bas.schouten) 2013-04-04 21:25:49 PDT
(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.
Comment 34 Benjamin Smedberg [:bsmedberg] 2013-04-05 07:10:06 PDT
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.
Comment 35 Bas Schouten (:bas.schouten) 2013-04-09 14:29:40 PDT
(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.
Comment 36 Benjamin Smedberg [:bsmedberg] 2013-04-10 06:59:34 PDT
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.
Comment 37 Bas Schouten (:bas.schouten) 2013-04-16 18:54:06 PDT
Created attachment 738291 [details] [diff] [review]
Make AlphaBoxBlur allocation fallible
Comment 38 Bas Schouten (:bas.schouten) 2013-04-19 09:48:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/93099b76959f
Comment 40 Scoobidiver (away) 2013-04-24 02:36:03 PDT
It has entered the top-20 crashers in 20.0.1.
Comment 41 Benjamin Smedberg [:bsmedberg] 2013-04-24 11:47:30 PDT
Bas, do you know why both these bugs bounced?
Comment 42 Bas Schouten (:bas.schouten) 2013-04-25 06:29:16 PDT
(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.
Comment 43 Wayne Mery (:wsmwk, NI for questions) 2013-04-30 12:14:36 PDT
me bp-d70e783f-4317-4ad4-a0e0-802a12130430
Comment 44 Wayne Mery (:wsmwk, NI for questions) 2013-05-22 22:51:21 PDT
(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
Comment 45 Alex Keybl [:akeybl] 2013-05-28 10:29:31 PDT
Pinging about this in email.
Comment 46 Bas Schouten (:bas.schouten) 2013-05-30 04:09:10 PDT
Created attachment 755862 [details] [diff] [review]
Make AlignedArray use fallible allocation.
Comment 47 :Ms2ger (⌚ UTC+1/+2) 2013-05-30 06:16:04 PDT
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)];
Comment 48 Bas Schouten (:bas.schouten) 2013-05-30 12:48:37 PDT
(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 :).
Comment 49 Bas Schouten (:bas.schouten) 2013-05-30 18:51:22 PDT
Created attachment 756315 [details] [diff] [review]
Part 2: Check AlignedArray for allocation success.
Comment 50 Alex Keybl [:akeybl] 2013-05-31 09:38:37 PDT
We're going to build with b4 on Tuesday, so would be great to get review/landed prior.
Comment 51 Jeff Muizelaar [:jrmuizel] 2013-06-03 15:13:59 PDT
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.
Comment 54 Robert Kaiser 2013-06-06 06:08:26 PDT
So, this landed on central, and on beta without explicit approval, shouldn't we get it into aurora as well?
Comment 55 Alex Keybl [:akeybl] 2013-06-06 09:41:33 PDT
(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
Comment 56 Alex Keybl [:akeybl] 2013-06-06 09:42:13 PDT
And yes, we should uplift to Aurora.
Comment 58 Bas Schouten (:bas.schouten) 2013-06-06 21:34:16 PDT
(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!
Comment 59 Paul Silaghi, QA [:pauly] 2013-07-11 23:44:11 PDT
No crashes on FF > 21. I guess we can call this bug verified.
Comment 62 negarasaja 2015-10-03 22:37:07 PDT Comment hidden (spam)
Comment 63 ayushi kumar 2016-04-18 00:33:20 PDT Comment hidden (spam)

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