Last Comment Bug 342960 - String.toSource() on a large string causes crash, probably due to arena bug
: String.toSource() on a large string causes crash, probably due to arena bug
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 336409 344320
  Show dependency treegraph
 
Reported: 2006-06-28 03:09 PDT by georgi - hopefully not receiving bugspam
Modified: 2007-02-08 21:39 PST (History)
9 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.9a1+
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.5+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
String.toSource() (478 bytes, text/html)
2006-06-28 03:10 PDT, georgi - hopefully not receiving bugspam
no flags Details
Fix (1.63 KB, patch)
2006-06-28 18:44 PDT, Blake Kaplan (:mrbkap)
brendan: review+
dveditz: approval1.8.0.5+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
js1_5/GC/regress-342960.js (2.63 KB, text/plain)
2006-07-06 09:10 PDT, Bob Clary [:bc:]
no flags Details
1.0.x version of the patch (1.47 KB, patch)
2006-08-08 08:22 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
js1_5/GC/regress-342960.js (2.64 KB, text/plain)
2006-11-10 16:42 PST, Bob Clary [:bc:]
no flags Details

Description georgi - hopefully not receiving bugspam 2006-06-28 03:09:56 PDT
String.toSource() on a large string causes crash, probably due to arena bug

483                 *bp++ = (char) *s++;
Current language:  auto; currently c
(gdb) x/4x bp
0x8ca6000:       Cannot access memory at address 0x8ca6000
Comment 1 georgi - hopefully not receiving bugspam 2006-06-28 03:10:57 PDT
Created attachment 227392 [details]
String.toSource()
Comment 2 Bob Clary [:bc:] 2006-06-28 08:06:41 PDT
Doesn't crash recent trunk on winxp. Georgi: any reason you didn't file this under JavaScript Engine?
Comment 3 georgi - hopefully not receiving bugspam 2006-06-28 09:23:31 PDT
(In reply to comment #2)
> Doesn't crash recent trunk on winxp. Georgi: any reason you didn't file this
> under JavaScript Engine?
> 

what about linux and mac?
Comment 4 Bob Clary [:bc:] 2006-06-28 09:38:36 PDT
It crashes on linux trunk after clicking the "done" alert and on a second try with winxp trunk it also crashes with an invalid allocation size.
Comment 5 Blake Kaplan (:mrbkap) 2006-06-28 18:44:55 PDT
Created attachment 227486 [details] [diff] [review]
Fix

So, this is almost the same bug as bug 338121, except in the growing case as opposed to the allocating case. With this patch, the testcase runs and ends up reporting out of memory (unsurprisingly, really).
Comment 6 georgi - hopefully not receiving bugspam 2006-06-29 00:27:57 PDT
(In reply to comment #5)
> With this patch, the testcase runs and ends up
> reporting out of memory (unsurprisingly, really).
> 

out of memory kinda surprises me. before toSource() firefox uses 613M memory and after it should use about 512M more, which is not that much virtual memory.

Comment 7 Daniel Veditz [:dveditz] 2006-06-30 15:39:58 PDT
Comment on attachment 227486 [details] [diff] [review]
Fix

approved for 1.8.0 branch pending brendan's review, a=dveditz for drivers
Comment 8 Brendan Eich [:brendan] 2006-06-30 16:43:32 PDT
Comment on attachment 227486 [details] [diff] [review]
Fix

Looks good -- we just read through JS_ArenaRealloc and JS_ArenaGrow and believe they are safe (the former because realloc will fail on any overlarge gross size, the latter because it's layered on JS_ARENA_ALLOCATE).

/be
Comment 9 georgi - hopefully not receiving bugspam 2006-07-01 00:57:56 PDT
-            if ((jsuword)(p) <= _a->limit - _nb) {                            \
+            if (_a->limit >= _nb && (jsuword)(p) <= _a->limit - _nb) {       \
                 _a->avail = (jsuword)(p) + _nb;                               \
                 JS_ArenaCountInplaceGrowth(pool, size, incr);                 \

is the second backslash misaligned on purpose?
Comment 10 Blake Kaplan (:mrbkap) 2006-07-01 14:10:12 PDT
> is the second backslash misaligned on purpose?

Oops, no it isn't. I'll fix it before checking in.
Comment 11 Blake Kaplan (:mrbkap) 2006-07-01 20:06:20 PDT
Fix checked into the 1.8.0 branch and trunk.
Comment 12 Mike Schroepfer 2006-07-05 20:17:28 PDT
Comment on attachment 227486 [details] [diff] [review]
Fix

Approving for branch since this is on trunk and 1.8.0 branch.
Comment 13 Bob Clary [:bc:] 2006-07-05 22:39:02 PDT
This appears to be fixed in the shell on 1.8.0.5 and trunk, but I still get crashes in the browser on 1.8.0.5 and trunk on linux for  js1_5/Regress/regress-336409-[12].js
Comment 14 Bob Clary [:bc:] 2006-07-05 23:03:22 PDT
ditto js1_5/Regress/regress-336410-[12].js
Comment 15 georgi - hopefully not receiving bugspam 2006-07-06 03:18:30 PDT
on linux trunk i get sporadic crashes on reloading String.toSource() several times.

Assertion failure: offsetInArena < GC_THINGS_SIZE, at /opt/joro/firefox/mozilla/js/src/jsgc.c:502
Comment 16 Bob Clary [:bc:] 2006-07-06 09:10:08 PDT
Created attachment 228295 [details]
js1_5/GC/regress-342960.js
Comment 17 Bob Clary [:bc:] 2006-07-06 23:46:52 PDT
verified fixed 1.8.0.5, 1.9a1 windows/macppc/linux.

not fixed 1.8.1a3
crashes shell windows, browser windows/macppc/linux



Comment 18 Bob Clary [:bc:] 2006-07-07 21:57:33 PDT
with 20060707 builds, js1_5/GC/regress-342960.js browser tests no longer crashes windows/macppc 1.8.1 but _does crash linux 1.8.1_.
Comment 19 Brendan Eich [:brendan] 2006-07-11 18:54:07 PDT
(In reply to comment #15)
> on linux trunk i get sporadic crashes on reloading String.toSource() several
> times.
> 
> Assertion failure: offsetInArena < GC_THINGS_SIZE, at
> /opt/joro/firefox/mozilla/js/src/jsgc.c:502

Cc'ing Igor.

(In reply to comment #18)
> with 20060707 builds, js1_5/GC/regress-342960.js browser tests no longer
> crashes windows/macppc 1.8.1 but _does crash linux 1.8.1_.

Between these two, I think we need a new bug or two.  It's important to cite the crash stack or any other skidmarks, though, so we can know whether to file one bug (for the assertion) or two.  Start with one.

/be
Comment 20 Igor Bukanov 2006-07-12 00:47:58 PDT
Comment on attachment 228295 [details]
js1_5/GC/regress-342960.js

>		writeLineToLog("don't interrupt the script. let it go.");
>		for(i=0;i<1024*1024;i++) meg += "v";	
>		for(i=0;i<1024/4;i++) r += meg;
>		var o={f1: r, f2: r, f3: r,f4: r,f5: r, f6: r, f7: r, f8: r,f9: r};
>		writeLineToLog('done obj');
>		var rr=r.toSource();
>		writeLineToLog('done toSource()');

Is the line "var o={f1: r, f2: r, f3: r,f4: r,f5: r, f6: r, f7: r, f8: r,f9: r};" really necessary to trigger the crash on 1.8.1? What happens without it? Or was the test case supposed to use o.toSource() and not r.toSource()?
Comment 21 georgi - hopefully not receiving bugspam 2006-07-12 05:18:46 PDT
> 
> Is the line "var o={f1: r, f2: r, f3: r,f4: r,f5: r, f6: r, f7: r, f8: r,f9:
> r};" really necessary to trigger the crash on 1.8.1? What happens without it?
> Or was the test case supposed to use o.toSource() and not r.toSource()?
> 

in my testcase it was left from the previous bug.

String.toSource() crashed without this line locally, so i believe it is not necessary for this bug, though it may trigger something.
Comment 22 georgi - hopefully not receiving bugspam 2006-07-12 05:21:20 PDT
(In reply to comment #20)

> Or was the test case supposed to use o.toSource() and not r.toSource()?
> 

o.toSource() is different bug (seems fixed).
|o| was just left from the above bug.
Comment 23 Bob Clary [:bc:] 2006-07-25 05:24:46 PDT
mrbkap: can you please land this on 1.8 today? 
Comment 24 Blake Kaplan (:mrbkap) 2006-07-25 17:39:24 PDT
Sorry, this was checked in with js1.7.
Comment 25 Bob Clary [:bc:] 2006-07-27 12:53:31 PDT
verified fixed 1.8 win/mac(ppc|tel)/linux 20060725
Comment 26 Alexander Sack 2006-08-08 08:22:29 PDT
Created attachment 232732 [details] [diff] [review]
1.0.x version of the patch
Comment 27 Bob Clary [:bc:] 2006-11-10 16:42:58 PST
Created attachment 245293 [details]
js1_5/GC/regress-342960.js

modify expected exit code for the shell per bug 358975.
Comment 28 Bob Clary [:bc:] 2007-02-08 21:39:21 PST
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-342960.js,v  <--  regress-342960.js

Note You need to log in before you can comment on or make changes to this bug.