OOM crash in nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength

VERIFIED FIXED in Firefox 11

Status

()

Core
Internationalization
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Scoobidiver (away), Assigned: smontagu)

Tracking

({crash, regression, topcrash})

11 Branch
mozilla12
x86
All
crash, regression, topcrash
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox10 unaffected, firefox11- verified, firefox12 verified)

Details

(Whiteboard: [qa!], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

6 years ago
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?
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++.
(Reporter)

Comment 5

6 years ago
It's #25 top browser crasher in 11.0a2 and #17 in 12.0a1.
Keywords: topcrash
Version: Trunk → 11 Branch

Comment 6

6 years ago
>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

6 years ago
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.
(Reporter)

Comment 8

6 years ago
I added the Mac crash signature which is #5 top crasher in 11.0a2 on Mac OS X.
Crash Signature: [@ mozalloc_abort(char const* const) | mozalloc_handle_oom() | nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength(nsAString_internal const&, int*, char**) ] → [@ mozalloc_abort(char const* const) | mozalloc_handle_oom() | nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength(nsAString_internal const&, int* char**)] [@ mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | nsScriptableUnicodeConverter…
OS: Windows 7 → All
Whiteboard: [MemShrink]
(Assignee)

Comment 9

6 years ago
Created attachment 589279 [details] [diff] [review]
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/smontagu@mozilla.com-8a469ba9fa7a.
This directory won't be created until the first builds are uploaded, so please be patient.
Whiteboard: [MemShrink]

Comment 10

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #589279 - Flags: review?(benjamin)
Attachment #589279 - Flags: review?(benjamin) → review+
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/993295f8f80b
status-firefox10: --- → unaffected
status-firefox11: --- → affected
tracking-firefox11: --- → ?
Flags: in-testsuite-
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/993295f8f80b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla11 → mozilla12
(Reporter)

Comment 13

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

Comment 14

6 years ago
Would it be more efficient for that 32 byte value to be a member or allocated on the stack?
(Assignee)

Comment 15

6 years ago
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.
Attachment #589279 - Flags: approval-mozilla-aurora?
Comment on attachment 589279 [details] [diff] [review]
Use moz_malloc and friends

[Triage Comment]
Fixes a top crash - approved for Aurora.
Attachment #589279 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6d6545292cfa
status-firefox11: affected → fixed
Target Milestone: mozilla12 → mozilla11
(Reporter)

Comment 18

6 years ago
(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.
Target Milestone: mozilla11 → mozilla12
(Reporter)

Comment 19

6 years ago
There are no crashes in 12.0a1/20120120 and above.
status-firefox12: --- → verified
qa+ for verification using crashstats.
Whiteboard: [qa+]

Updated

6 years ago
tracking-firefox11: ? → -
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.
Status: RESOLVED → VERIFIED
status-firefox11: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.