Closed
Bug 829954
Opened 12 years ago
Closed 11 years ago
OOM crash in mozilla::gfx::AlphaBoxBlur::Blur
Categories
(Core :: Graphics, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla24
People
(Reporter: scoobidiver, Assigned: bas.schouten, NeedInfo)
References
Details
(4 keywords, Whiteboard: [MemShrink:P1])
Crash Data
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jrmuizel
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
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
Comment 1•12 years ago
|
||
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.
Reporter | ||
Comment 2•12 years ago
|
||
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()]
Comment 3•12 years ago
|
||
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:
--- → ?
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
Also, this has been rising in 19 data since 19.0b3 was released.
Reporter | ||
Comment 6•12 years ago
|
||
(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•12 years ago
|
||
Marking tracking so we can watch this in b4 results which should be coming in soon.
Updated•12 years ago
|
Assignee: nobody → karlt
Comment 8•12 years ago
|
||
20121109 was one day after bug 509052 landed.
Assignee: karlt → bas
Blocks: 509052
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
(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™.
Assignee | ||
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
We should grab URLs and get some QA around this, since the engineering investigation hasn't turned much up yet.
Comment 13•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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)?
Assignee | ||
Comment 20•12 years ago
|
||
(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•12 years ago
|
||
(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)
Comment 22•12 years ago
|
||
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)
Assignee | ||
Comment 23•12 years ago
|
||
(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)
Comment 24•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Comment 26•12 years ago
|
||
(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•12 years ago
|
||
Well 19 is out the door now and this wouldn't drive a chemspill, so wontfixing for 19.
Reporter | ||
Comment 28•12 years ago
|
||
Bug 845260 has STR.
Reporter | ||
Updated•12 years ago
|
status-firefox22:
--- → affected
Comment 29•12 years ago
|
||
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 31•12 years ago
|
||
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•12 years ago
|
||
I think we should track this for FF21 given comment 29
tracking-firefox21:
--- → ?
tracking-firefox22:
--- → ?
Assignee | ||
Comment 33•12 years ago
|
||
(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)
Comment 34•12 years ago
|
||
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
Updated•12 years ago
|
Updated•12 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 35•12 years ago
|
||
(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•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee | ||
Comment 37•12 years ago
|
||
Attachment #738291 -
Flags: review?(jmuizelaar)
Updated•12 years ago
|
Attachment #738291 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 38•12 years ago
|
||
Comment 39•12 years ago
|
||
Assignee | ||
Comment 42•12 years ago
|
||
(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)
Comment 43•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
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_…
Reporter | ||
Updated•12 years ago
|
status-firefox23:
--- → affected
Comment 44•11 years ago
|
||
(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)
Comment 45•11 years ago
|
||
Pinging about this in email.
Assignee | ||
Comment 46•11 years ago
|
||
Attachment #738291 -
Attachment is obsolete: true
Flags: needinfo?(bas)
Comment 47•11 years ago
|
||
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)];
Assignee | ||
Comment 48•11 years ago
|
||
(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 :).
Assignee | ||
Comment 49•11 years ago
|
||
Attachment #756315 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•11 years ago
|
Attachment #755862 -
Flags: review?(jmuizelaar)
Comment 50•11 years ago
|
||
We're going to build with b4 on Tuesday, so would be great to get review/landed prior.
Comment 51•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #755862 -
Flags: review?(jmuizelaar) → review+
Comment 52•11 years ago
|
||
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
Assignee | ||
Comment 53•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Comment 54•11 years ago
|
||
So, this landed on central, and on beta without explicit approval, shouldn't we get it into aurora as well?
Comment 55•11 years ago
|
||
(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
Updated•11 years ago
|
Attachment #755862 -
Flags: approval-mozilla-beta+
Attachment #755862 -
Flags: approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #756315 -
Flags: approval-mozilla-beta+
Attachment #756315 -
Flags: approval-mozilla-aurora+
Updated•11 years ago
|
tracking-firefox23:
--- → +
Comment 57•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/be6d5d592057
https://hg.mozilla.org/releases/mozilla-aurora/rev/261bee70e621
status-firefox24:
--- → fixed
Assignee | ||
Comment 58•11 years ago
|
||
(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)
Comment 59•11 years ago
|
||
No crashes on FF > 21. I guess we can call this bug verified.
Status: RESOLVED → VERIFIED
Assignee | ||
Updated•11 years ago
|
Attachment #713509 -
Flags: feedback?(bas)
Updated•10 years ago
|
Flags: needinfo?(amanda)
Updated•10 years ago
|
Flags: needinfo?(amanda)
Updated•5 years ago
|
Restrict Comments: true
You need to log in
before you can comment on or make changes to this bug.
Description
•