306788 - Sorting array of arrays can crash due to GC

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  Current trunk debug build

STEPS TO REPRODUCE:

1)  Load http://www.cloud9sales.com/test/raining_NSMozFail.htm
2)  Click the "All Products" link

NOTE:  What the page is doing is:

prodRainTapThr[0] = new
Array("Airedale","","ATPAIR.jpg",125,151,"Multi","RcP","$59.95",339);
...
prodRainTapThr[58] = new
Array("Yorkie","","ATPYOR.jpg",150,114,"Multi","RcL","$59.95",396);
prodRainTapThr.sort();

We crash inside Array.sort like so:

(gdb) frame 0
#0  0xb7e6fb09 in js_CompareStrings (str1=0xb32460e0, str2=0x83ae120)
    at ../../../mozilla/js/src/jsstr.c:2800
2800        s1 = JSSTRING_CHARS(str1),  s2 = JSSTRING_CHARS(str2);
(gdb) p *str1
$9 = {length = 3671775962, chars = 0xdadadada}

The problem is the code at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsarray.c&rev=3.60&mark=816,817#812
-- astr can get GCed when we create bstr.  I've verified that if I lock astr
around the creation of bstr the crash goes away.

Comment 1

[Boris Zbarsky [:bzbarsky]]

At brendan's prodding, I broke in js_GC -- it's being called of a DOM branch
callback with this stack:

#4  0xb6884975 in nsJSContext::DOMBranchCallback (cx=0x88010a0, script=0x0)
    at ../../../../mozilla/dom/src/base/nsJSEnvironment.cpp:516
#5  0xb7e35636 in js_EnterSharpObject (cx=0x88010a0, obj=0xb29bcbd0, idap=0x0, 
    sp=0xbfffd9d0) at ../../../mozilla/js/src/jsobj.c:507
#6  0xb7de2c69 in array_join_sub (cx=0x88010a0, obj=0xb29bcbd0, sep=0xb7e9ce44, 
    literalize=0, rval=0xbfffdb40, localeString=0)
    at ../../../mozilla/js/src/jsarray.c:369
#7  0xb7de3319 in array_toString (cx=0x88010a0, obj=0xb29bcbd0, argc=0,
argv=0xb2949d2c, 
    rval=0xbfffdb40) at ../../../mozilla/js/src/jsarray.c:545
#8  0xb7e14d4f in js_Invoke (cx=0x88010a0, argc=0, flags=2)
    at ../../../mozilla/js/src/jsinterp.c:1174
#9  0xb7e15182 in js_InternalInvoke (cx=0x88010a0, obj=0xb29bcbd0,
fval=-1259988056, 
    flags=0, argc=0, argv=0x0, rval=0xbfffdd10)
    at ../../../mozilla/js/src/jsinterp.c:1271
#10 0xb7e3f5e4 in js_TryMethod (cx=0x88010a0, obj=0xb29bcbd0, atom=0x81f6620,
argc=0, 
    argv=0x0, rval=0xbfffdd10) at ../../../mozilla/js/src/jsobj.c:3890
#11 0xb7e3d58e in js_DefaultValue (cx=0x88010a0, obj=0xb29bcbd0,
hint=JSTYPE_STRING, 
    vp=0xbfffdd54) at ../../../mozilla/js/src/jsobj.c:3217
#12 0xb7e6f78a in js_ValueToString (cx=0x88010a0, v=-1298412592)
    at ../../../mozilla/js/src/jsstr.c:2739
#13 0xb7de3bda in sort_compare (a=0xb29c2b98, b=0x

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Fixes the bug bz identified, plus an old bug where failure from
js_ValueToNumber was not propagated.

/be

Comment 3

[Brendan Eich [:brendan]]

Easy crash bug to fix.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 194628 [details] [diff] [review]
fix

r=mrbkap

Comment 5

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

Comment on attachment 194628 [details] [diff] [review]
fix

sr=shaver

Comment 6

[Brendan Eich [:brendan]]

Fixed on trunk and 1.8 branch.

/be

Comment 7

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using trunk build 2005-09-04-06 on Windows XP SeaMonkey trunk by
loading http://www.cloud9sales.com/test/raining_NSMozFail.htm and clicking "All
Products" a few times.  No crash.

Comment 8

[Bob Clary [:bc] (inactive)]

Checking in regress-306788.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-306788.js,v  <--  regress-306788.js
initial revision: 1.1
done

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.7 branch as part of my bug 311497 checkin.

Comment 10

[Tracy Walker [:tracy]]

hmmm, not crashing. But "all products" page never finishes loading *endless throbber spinning*. Is that expected?

Comment 11

[Jay Patel [:jay]]

v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215 Firefox/1.0.8, seeing the same as Tracy, but no crash.  Looks like that is a test page, so not sure how to answer Tracy's question, but if anyone knows, please share.

Comment 12

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-306788.js 
verified on winxp/linux/mac, firefox 1.7.13, 1.8.0.1, 1.8, 1.9a1
verified on winxp, mozilla 1.7.13

note that I don't show a crash in the tests for firefox 1.0 or 1.0.7 on any platform for this test and don't crash in either 1.0 or 1.0.7 for the testcase when clicking the all products link.