Firefox crashes in nsExtensibleStringBundle::FormatStringFromName

VERIFIED FIXED in mozilla1.9.2a1

Status

()

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Honza, Assigned: rcampbell)

Tracking

({fixed1.9.1, intl})

Trunk
mozilla1.9.2a1
x86
Windows XP
fixed1.9.1, intl
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1b4] [firebug-p2])

Attachments

(1 attachment, 2 obsolete attachments)

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
Whiteboard: [firebug-p2]
(Assignee)

Updated

10 years ago
Keywords: intl

Comment 1

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

Comment 2

10 years ago
Created attachment 370277 [details] [diff] [review]
485511.diff

added a check to see if GetStringFromName was successful. Return NS_ERROR_FAILURE if not.
Assignee: smontagu → rcampbell
Status: NEW → ASSIGNED
Attachment #370277 - Flags: review?(benjamin)

Comment 3

10 years ago
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.
Attachment #370277 - Flags: review?(benjamin) → review-

Comment 4

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

Comment 5

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

Comment 6

10 years ago
Created attachment 370387 [details] [diff] [review]
485511-2.diff
Attachment #370277 - Attachment is obsolete: true
Attachment #370387 - Flags: review?(smontagu)

Comment 7

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

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

Updated

10 years ago
Attachment #370387 - Flags: review?(benjamin)
(Assignee)

Comment 8

10 years ago
erk.
(Assignee)

Comment 9

10 years ago
Created attachment 370389 [details] [diff] [review]
485511-3.diff
[Checkin: Comment 11 & 14]

returning rv instead of NS_ERROR_FAILURE
Attachment #370387 - Attachment is obsolete: true
Attachment #370387 - Flags: review?(smontagu)
Attachment #370387 - Flags: review?(benjamin)
(Assignee)

Updated

10 years ago
Attachment #370389 - Flags: review?(smontagu)
Attachment #370389 - Flags: review?(smontagu) → review+
(Assignee)

Updated

10 years ago
Attachment #370389 - Flags: superreview?(shaver)
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!
Attachment #370389 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 11

10 years ago
pushed to mozilla-central, changeset: 26871:4b7cb0b50483
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #370389 - Flags: approval1.9.1?
(Assignee)

Updated

10 years ago
Flags: wanted1.9.1?
Comment on attachment 370389 [details] [diff] [review]
485511-3.diff
[Checkin: Comment 11 & 14]

a191=beltzner
Attachment #370389 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 13

10 years ago
thanks for the a+. Can someone check this into branch for me?
Whiteboard: [firebug-p2] → [firebug-p2][needs-branch-checkin-1.9.1]
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
Attachment #370389 - Attachment description: 485511-3.diff → 485511-3.diff [Checkin: Comment 11 & 14]
Flags: wanted1.9.1?
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [firebug-p2][needs-branch-checkin-1.9.1] → [fixed1.9.1b4] [firebug-p2]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
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
(Assignee)

Updated

10 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.