OOM in JS_ArenaAllocate generates assertion in js_GetDependentStringChars

VERIFIED FIXED in mozilla1.0

Status

()

Core
JavaScript Engine
VERIFIED FIXED
16 years ago
13 years ago

People

(Reporter: Steven Cole, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.0
x86
Other
js1.5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

16 years ago
While running mozilla/js/tests/ecma/Array/15.4.2.1-2.js in the js shell:

Malloc failure at:

JS_ArenaAllocate(JSArenaPool * 0x0042007c, unsigned int 9216) line 189 + 9 bytes
gc_new_arena(JSArenaPool * 0x0042007c) line 181 + 14 bytes
js_AllocGCThing(JSContext * 0x00520c94, unsigned int 1) line 496 + 9 bytes
js_NewString(JSContext * 0x00520c94, unsigned short * 0x00437938, unsigned int
2, unsigned int 0) line 2346 + 16 bytes
JS_NewStringCopyZ(JSContext * 0x00520c94, const char * 0x0064e0a2) line 3523 +
19 bytes
js_NumberToString(JSContext * 0x00520c94, double 82.000000000000) line 625 + 13
bytes
js_ValueToString(JSContext * 0x00520c94, long 165) line 2558 + 26 bytes
js_Interpret(JSContext * 0x00520c94, long * 0x0064fc6c) line 2190 + 16 bytes
js_Execute(JSContext * 0x00520c94, JSObject * 0x00423740, JSScript * 0x0043713c,
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0064fc6c) line 968 + 13 bytes
JS_ExecuteScript(JSContext * 0x00520c94, JSObject * 0x00423740, JSScript *
0x0043713c, long * 0x0064fc6c) line 3258 + 25 bytes
Process(JSContext * 0x00520c94, JSObject * 0x00423740, char * 0x0041764c
`string') line 335 + 22 bytes
ProcessArgs(JSContext * 0x00520c94, JSObject * 0x00423740, char * * 0x00419c6c,
int 4) line 486 + 17 bytes
orig_main(int 4, char * * 0x00419c6c) line 2134 + 21 bytes
main(int 5, char * * 0x00419c68 _replacementArgs) line 2199 + 13 bytes
mainCRTStartup() line 338 + 17 bytes

Results in assertion failure (much later) at

js_GetDependentStringChars(JSString * 0x004506a0) line 111 + 36 bytes
js_FinalizeStringRT(JSRuntime * 0x00420078, JSString * 0x004506a0) line 2505 +
27 bytes
js_FinalizeString(JSContext * 0x00520c94, JSString * 0x004506a0) line 2491 + 16
bytes
js_GC(JSContext * 0x00520c94, unsigned int 0) line 1272 + 11 bytes
js_ForceGC(JSContext * 0x00520c94) line 979 + 11 bytes
js_DestroyContext(JSContext * 0x00520c94, int 2) line 210 + 9 bytes
JS_DestroyContext(JSContext * 0x00520c94) line 892 + 11 bytes
orig_main(int 4, char * * 0x00419c6c) line 2145 + 10 bytes
main(int 5, char * * 0x00419c68 _replacementArgs) line 2199 + 13 bytes
mainCRTStartup() line 338 + 17 bytes

The assertion is "start < base->length", both of the values are zero. 
(base->chars is also null).

An extra null check in js_GetDependentStringChars fixes the problem (patch
attached shortly) but I'm not sure that it's valid.  Should a string with
base->chars == null exist to begin with?

--scole
(Reporter)

Comment 1

16 years ago
Created attachment 75018 [details] [diff] [review]
proposed fix

Doh! Don't need to add a null check; just change the assertion...

--scole
(Assignee)

Comment 2

16 years ago
How did base become null?  If you search for JSSTRDEP_SET_BASE calls, it's hard
to see how a null base could be set.  The only one I see where base comes in as
a parameter is js_NewDependentString, but its callers all seem to me to
null-check well before the call, and bail.

scole, can you find out how that null base got set?

/be
(Reporter)

Comment 3

16 years ago
Well, I instrumented the JSSTRDEP_SET_BASE macro, and nobody uses that to get
the base set to zero.  There's some error situation that's leaving the "base"
wiped to zero without filling in any values.  Still looking for it though.

(The error absolutely depends on the memory failure, though.  It causes a
last-ditch GC to run, and then the allocator returns success, since I only cause
the first allocation to fail.  I'm going to look more carefully at that code to
see what I can see.)

--scole
(Reporter)

Comment 4

16 years ago
OK, base->chars gets set to null at jsstr.c:2519, in the routine
js_FinalizeStringRT.  

--scole
(Assignee)

Comment 5

16 years ago
Oh, but the JSSTRFLAG_DEPENDENT bit gets left set!  Ok, patch in a trice.

/be
Assignee: khanson → brendan
Keywords: js1.5, mozilla1.0
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 6

16 years ago
Created attachment 75115 [details] [diff] [review]
proposed fix

That assertion change should not be needed with this patch.  This patch fixes
js_FinalizeStringRT so it doesn't depend on the base string of a dependent
string being finalized after the dependent string.

All the string finalizer cares about, really, is whether the string has a
deflated string cache entry indexed by the JSString *, and it avoids probing
the deflated string cache by testing for a non-null chars pointer.  But in the
dependent string case, the string must be valid (must have a non-null base
pointer that overlays chars) if it has the JSSTRFLAG_DEPENDENT flag set in its
length overlay.

/be
(Assignee)

Updated

16 years ago
Attachment #75018 - Attachment is obsolete: true
(Reporter)

Comment 7

16 years ago
Comment on attachment 75115 [details] [diff] [review]
proposed fix

Makes sense and fixes my testcase.  r=scole@planetweb.com
Attachment #75115 - Flags: review+
Comment on attachment 75115 [details] [diff] [review]
proposed fix

sr=shaver.
Attachment #75115 - Flags: superreview+

Comment 9

16 years ago
Comment on attachment 75115 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #75115 - Flags: approval+
(Assignee)

Comment 10

16 years ago
Fixed.

/be
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 11

16 years ago
Marking Verified per Comment #7
Status: RESOLVED → VERIFIED

Updated

13 years ago
Flags: testcase+
You need to log in before you can comment on or make changes to this bug.