705407 - OOM crash in nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Scoobidiver (away)]

It's #64 top crasher in build 11.0a1/20111124.
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3c8147998124&tochange=de483d897af4

Signature	mozalloc_abort(char const* const) | mozalloc_handle_oom() | nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength(nsAString_internal const&, int*, char**)
UUID	d0507530-f4f4-4e4a-8293-6372f2111125
Date Processed	2011-11-25 19:27:26.554462
Uptime	386
Last Crash	10.3 hours before submission
Install Age	12.5 hours since version was first installed.
Install Time	2011-11-25 14:53:14
Product	Firefox
Version	11.0a1
Build ID	20111125031016
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7600
Build Architecture	x86
Build Architecture Info	AuthenticAMD family 16 model 4 stepping 3
Crash Reason	EXCEPTION_BREAKPOINT
Crash Address	0x6de4193d
App Notes 	AdapterVendorID: 1002, AdapterDeviceID: 9710, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.872.0.0
D2D? D2D+
DWrite? DWrite+
D3D10 Layers? D3D10 Layers+
EMCheckCompatibility	False

Frame 	Module 	Signature [Expand] 	Source
0 	mozalloc.dll 	mozalloc_abort 	memory/mozalloc/mozalloc_abort.cpp:77
1 	mozalloc.dll 	mozalloc_handle_oom 	memory/mozalloc/mozalloc_oom.cpp:54
2 	xul.dll 	nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength 	intl/uconv/src/nsScriptableUConv.cpp:80
3 	xul.dll 	nsScriptableUnicodeConverter::ConvertToByteArray 	intl/uconv/src/nsScriptableUConv.cpp:207
4 	xul.dll 	nsScriptableUnicodeConverter::ConvertToInputStream 	intl/uconv/src/nsScriptableUConv.cpp:241
5 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
6 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1554
7 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:625
8 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:3751
9 	mozjs.dll 	js::ContextStack::pushInvokeFrame 	js/src/vm/Stack.cpp:690
10 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:643
11 	mozjs.dll 	mozjs.dll@0x3198f 	
12 		@0x132e5db1 	
13 	mozjs.dll 	js::StackFrame::functionEpilogue 	js/src/vm/Stack-inl.h:410
14 	mozjs.dll 	Detecting 	js/src/jsobj.cpp:3070
15 	mozjs.dll 	js_InferFlags 	js/src/jsobj.cpp:3146
16 	mozjs.dll 	js::StackFrame::pcQuadratic 	js/src/vm/Stack.cpp:196
17 	mozjs.dll 	js::Shape::get 	js/src/jsscopeinlines.h:294
18 	mozjs.dll 	js::types::TypeMonitorResult 	js/src/jsinfer.cpp:5151
19 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:5891
20 		@0xffffff81 

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char%20const*%20const%29%20|%20mozalloc_handle_oom%28%29%20|%20nsScriptableUnicodeConverter%3A%3AConvertFromUnicodeWithLength%28nsAString_internal%20const%26%2C%20int*%2C%20char**%29

Comment 1

[Simon Montagu :smontagu]

Bug 680556 seems to me the likeliest thing in that regression range, especially considering bug 680556 comment 13.

Comment 2

[Robert Claypool]

I have a windbg dump of what I am told is likely this bug uploaded to Dropbox. how should it be shared?

Comment 3

[Robert Claypool]

nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength intl/uconv/src/nsScriptableUConv.cpp
and probably every other instance of code that requests memory expects a null pointer when out of memory, so why doesn't the memory allocator return a null pointer instead of forcing a crash? Also, is this memory garbage collected? The comment at mozalloc_handle_oom() suggests not, but if not, why not?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The simplest fix here is to have nsScriptableUnicodeConverter use moz_malloc() explicitly.  (Or |new (fallible)|.)

(In reply to Robert Claypool from comment #3)
> nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength
> intl/uconv/src/nsScriptableUConv.cpp
> and probably every other instance of code that requests memory expects a
> null pointer when out of memory, so why doesn't the memory allocator return
> a null pointer instead of forcing a crash?

It's a cost/benefit tradeoff.  We have security bugs that arise when code doesn't check for allocation failure properly, and we can't test those checks exhaustively, so we only use fallible allocators where failed allocation is high probability.  "High probability" is partly computed based on crash reports like this one.

> Also, is this memory garbage
> collected? The comment at mozalloc_handle_oom() suggests not, but if not,
> why not?

No.  GC has an unacceptable performance overhead in gecko C/C++.

Comment 5

[Scoobidiver (away)]

It's #25 top browser crasher in 11.0a2 and #17 in 12.0a1.

Comment 6

[Robert Claypool]

>No.  GC has an unacceptable performance overhead in gecko C/C++.

Still? For text? On modern systems with multiple cores and gobs of memory and low-level caches? On current designs of webpages?

When was that statement last evaluated? Has an analysis been posted anywhere?

Comment 7

[Robert Claypool]

Also, with the way Firefox has been eating up memory, it may be worth it to garbage collect in areas outside of JavaScript. The amount of processor computation garbage collection takes may be fixed, but the percentage of time it takes decreases as the processor gets more powerful. Also, not garbage collecting can cause memory to be paged out to disk, trading one performance overhead for another.

Comment 8

[Scoobidiver (away)]

I added the Mac crash signature which is #5 top crasher in 11.0a2 on Mac OS X.

Comment 9

[Simon Montagu :smontagu]

Attachment: Use moz_malloc and friends

I made the change suggested in comment 4, but I'm flying blind because I haven't been able to reproduce the bug. Robert, are you willing to test a try build with the patch?

Builds and logs will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8a469ba9fa7a.
This directory won't be created until the first builds are uploaded, so please be patient.

Comment 10

[Robert Claypool]

The changes you've made appears to have improved things. I did things that should have caused a crash, and it didn't. It eventually crashed, but it appears to be at a different place in Firefox's code, which would be a different bug.

Comment 11

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/993295f8f80b

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/993295f8f80b

Comment 13

[Scoobidiver (away)]

It's #26 top browser crasher in 11.0a2, so I think the patch should land in Aurora.

Comment 14

[Robert Claypool]

Would it be more efficient for that 32 byte value to be a member or allocated on the stack?

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 589279 [details] [diff] [review]
Use moz_malloc and friends

[Approval Request Comment]
Regression caused by (bug #): 680556
User impact if declined: Frequent crashes
Testing completed (on m-c, etc.): Baked 4 days on m-c, see also comment 10
Risk to taking this patch (and alternatives if risky): None, AFAIK.

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 589279 [details] [diff] [review]
Use moz_malloc and friends

[Triage Comment]
Fixes a top crash - approved for Aurora.

Comment 17

[Simon Montagu :smontagu]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d6545292cfa

Comment 18

[Scoobidiver (away)]

(In reply to Simon Montagu from comment #17)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/6d6545292cfa
Changing the target milestone means it's not fixed in Fx 12.

Comment 19

[Scoobidiver (away)]

There are no crashes in 12.0a1/20120120 and above.

Comment 20

[u279076]

qa+ for verification using crashstats.

Comment 21

[Virgil Dicu [:virgil] [QA]]

No crashes in Firefox 11 beta Last crash in 2012012304 before the fix landed in 11 (link to crash stats in comment 0). Marking as verified.