485511 - Firefox crashes in nsExtensibleStringBundle::FormatStringFromName

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Firebug is using nsIStringBundleService and its createExtensibleBundle to access localized strings coming from other extensions.

The problem is that localized Firefox crashes when the extensible bundle is used, within: nsExtensibleStringBundle::FormatStringFromName (perhaps because a required string that is not presented in a *.properties file for the current locale)

See more info here:
http://groups.google.com/group/firebug/browse_thread/thread/194d8bfc05398c29

Steps to reproduce:
1) Install Firebug 1.4a14 in localized version of Firefox (e.g. cs-CZ)
http://getfirebug.com/releases/firebug/1.4X/firebug-1.4X.0a14.xpi
(tested with Firefox 3.0.7 cs-CZ).

2) Right click on Firebug's icon in Firefox status bar to open a context menu.

3) Choose "Enable All Panels"

4) Choose "Disable All Panels" -> CRASH

You might repeat #3 and #4 more times to get the crash.


Here is a stack trace of the crash:

xul.dll!BuildArgArray(const wchar_t * fmt=0x00000000, char * ap=0x0012e2c4, int * rv=0x0012e098, NumArgState * nasArray=0x0012e0c0)  Line 592   C++
xul.dll!dosprintf(SprintfStateStr * ss=, const wchar_t * fmt=0x00000000, char * ap=0x00000000)  Line 876    C++
xul.dll!nsTextFormatter::smprintf(const wchar_t * fmt=0x00000000, ...)  Line 1248 + 0x22 bytes  C++
xul.dll!nsStringBundle::FormatString(const wchar_t * aFormatStr=0x00000000, const wchar_t * * aParams=0x03b96fd0, unsigned int aLength=0x00000002, wchar_t * * aResult=0x0012e450)  Line 399 + 0x9a bytes   C++
xul.dll!nsExtensibleStringBundle::FormatStringFromName(const wchar_t * aName=0x01c26490, const wchar_t * * aParams=0x03b96fd0, unsigned int aLength=0x00000002, wchar_t * * aResult=0x0012e450)  Line 529 + 0x18 bytes  C++
xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000006, unsigned int methodIndex=0x00000004, unsigned int paramCount=0x0012e420, nsXPTCVariant * params=0x0259d620)  Line 102 C++
xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=)  Line 2393 + 0x22 bytes  C++
xul.dll!XPC_WN_CallMethod(JSContext * cx=0x01c15250, JSObject * obj=0x0255ec20, unsigned int argc=0x00000003, long * argv=0x06a3e4d0, long * vp=0x0012e6a4)  Line 1473 + 0x11 bytes C++
js3250.dll!js_Invoke(JSContext * cx=0x01c15250, unsigned int argc=0x00000003, long * vp=0x06a3e4c8, unsigned int flags=0x00000002)  Line 1304 + 0x14 bytes  C
js3250.dll!js_Interpret(JSContext * cx=0x01c15250)  Line 4874   C
js3250.dll!js_Invoke(JSContext * cx=0x01c15250, unsigned int argc=0x00000004, long * vp=0x06a3e36c, unsigned int flags=0x00000000)  Line 1321   C
js3250.dll!fun_apply(JSContext * cx=0x01c15250, unsigned int argc=0x00000004, long * vp=0x06a3e354)  Line 1679  C
js3250.dll!js_Interpret(JSContext * cx=)  Line 4847 + 0x10 bytes    C
nspr4.dll!PR_Unlock(PRLock * lock=0x00000000)  Line 356 C
xul.dll!jsd_Unlock(JSDStaticLock * lock=0x00000010)  Line 169 + 0xc bytes   C
nspr4.dll!_MD_CURRENT_THREAD()  Line 300    C
nspr4.dll!PR_Unlock(PRLock * lock=0x015dafb0)  Line 356 C
xul.dll!jsd_Unlock(JSDStaticLock * lock=0x02549520)  Line 169 + 0xc bytes   C
xul.dll!_callHook(JSDContext * jsdc=0x0012eab8, JSContext * cx=0x600cdf82, JSStackFrame * fp=0x0012eaec, int before=0x6085bc83, unsigned int type=0x1f40f95d, int (JSDContext *, JSDThreadState *, unsigned int, void *)* hook=0x01c15250, void * hookData=0x0012eb48)  Line 140 + 0x9 bytes    C
js3250.dll!js_NewObjectWithGivenProto(JSContext * cx=0x00000000, JSClass * clasp=0x00000000, JSObject * proto=0x601a59e0, JSObject * parent=0x00000005, unsigned int objectSize=0x00000000)  + 0x353e5 bytes    C

Here is a crash report
http://crash-stats.mozilla.com/report/index/6fd1a062-ff74-4402-9c0d-3e8cd2090327

Honza

Comment 1

[timeless]

so, the string bundle service has never been tolerant of invalid localizations (there's an old bug I filed about it).

If someone wants to rewrite it to use JS, this could be handled.

Here's a happy testcase:
c="content-sniffing-services"; b=Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);d=b.createExtensibleBundle(c);d.formatStringFromName("stupid", ["a"],0)

Comment 2

[Rob Campbell [:rc] (:robcee)]

Attachment: 485511.diff

added a check to see if GetStringFromName was successful. Return NS_ERROR_FAILURE if not.

Comment 3

[timeless]

Comment on attachment 370277 [details] [diff] [review]
485511.diff

free minuses:
1. ask a module owner or peer for a review (=smontagu)
2. style says we return early for failure.

Comment 4

[timeless]

n.b. i've posted a js port of stringbundle in bug 288400

it wouldn't crash here.

brendan seems to be of the opinion that we should stop plastering over problems and start rooting them out. IMO that means we should replace this c++ impl (which has a number of known crashers and some serious risks) with js.

note that fixing this one crash doesn't mean we still can't crash (there's another crash in the other bug).

Comment 5

[Rob Campbell [:rc] (:robcee)]

I would think that would be a module-owner's decision, though I'm in agreement with a JS replacement, in principle. We'd have to do some perf analysis to make sure there wasn't a significant negative impact. In either case, that's probably outside the scope of this bug.

I'll repost a modified version of the above patch and submit to smontagu (and bsmedberg).

Comment 6

[Rob Campbell [:rc] (:robcee)]

Attachment: 485511-2.diff

Comment 7

[timeless]

Comment on attachment 370387 [details] [diff] [review]
485511-2.diff

i'm also not sure why you're returning FAILURE instead of rv :)

Comment 8

[Rob Campbell [:rc] (:robcee)]

erk.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Attachment: 485511-3.diff [Checkin: Comment 11 & 14]

returning rv instead of NS_ERROR_FAILURE

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 370389 [details] [diff] [review]
485511-3.diff
[Checkin: Comment 11 & 14]

sr=shaver; please get it landed on m-c so we can get it to 1.9.1 in a few days.  Thanks!

Comment 11

[Rob Campbell [:rc] (:robcee)]

pushed to mozilla-central, changeset: 26871:4b7cb0b50483

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 370389 [details] [diff] [review]
485511-3.diff
[Checkin: Comment 11 & 14]

a191=beltzner

Comment 13

[Rob Campbell [:rc] (:robcee)]

thanks for the a+. Can someone check this into branch for me?

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 370389 [details] [diff] [review]
485511-3.diff
[Checkin: Comment 11 & 14]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac4c4f9bcdb6

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I have tested with: 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1-l10n/firefox-3.5b4pre.cs.win32.zip

... and it works for Firebug now (the same scenario crashes for Firefox cs-CZ 3.0.8)

Honza