360681 - Assertion failure: offsetInArena < GC_THINGS_SIZE with WAY_TOO_MUCH_GC

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

running js1_5/Array/regress-311515.js in the browser with WAY_TOO_MUCH_GC 

STATUS: Array.sort should skip holes and undefined during sort 
Assertion failure: offsetInArena < GC_THINGS_SIZE, at /work/mozilla/builds/ff/trunk-too-much-gc/mozilla/js/src/jsgc.c:493

I couldn't reproduce on Windows.

marking security sensitive just in case.

Comment 1

[Bob Clary [:bc] (inactive)]

#0  JS_Assert (s=0x28b338 "offsetInArena < GC_THINGS_SIZE", file=0x28b1c8 "mozilla/js/src/jsgc.c", ln=493) at mozilla/js/src/jsutil.c:63

#1  0x001dfd95 in js_GetGCThingFlags (thing=0xa035d90) at mozilla/js/src/jsgc.c:493

#2  0x001e2928 in js_MarkGCThing (cx=0xa035f58, thing=0xa035d90) at mozilla/js/src/jsgc.c:2445

#3  0x001e396e in js_GC (cx=0xa035f58, gckind=GC_LAST_DITCH) at mozilla/js/src/jsgc.c:2924

#4  0x001e12ff in js_NewGCThing (cx=0xa035f58, flags=1, nbytes=8) at mozilla/js/src/jsgc.c:1420

#5  0x00262126 in js_NewString (cx=0xa035f58, chars=0xa213328, length=1, gcflag=0) at mozilla/js/src/jsstr.c:2425

#6  0x001a7fb7 in JS_NewStringCopyZ (cx=0xa035f58, s=0xbfdbe506 "1") at mozilla/js/src/jsapi.c:4465

#7  0x00211fad in js_NumberToString (cx=0xa035f58, d=1) at mozilla/js/src/jsnum.c:719

#8  0x00262d6f in js_ValueToString (cx=0xa035f58, v=3) at mozilla/js/src/jsstr.c:2670

#9  0x001abd24 in sort_compare (arg=0xbfdbe6a4, a=0x99fd930, b=0x99fd934, result=0xbfdbe624) at mozilla/js/src/jsarray.c:997

#10 0x001aba44 in js_MergeSort (src=0x99fd930, nel=2, elsize=4, cmp=0x1abc62 <sort_compare>, arg=0xbfdbe6a4, tmp=0x99fd938) at mozilla/js/src/jsarray.c:912

#11 0x001ac29e in array_sort (cx=0xa035f58, obj=0xa2f6b78, argc=0, argv=0xa31d768, rval=0xbfdbe790) at mozilla/js/src/jsarray.c:1167

#12 0x001e76ff in js_Invoke (cx=0xa035f58, argc=0, flags=0) at mozilla/js/src/jsinterp.c:1396

#13 0x001fb2ea in js_Interpret (cx=0xa035f58, pc=0xa3f051e ":", result=0xbfdbf450) at mozilla/js/src/jsinterp.c:3948

#14 0x001e824f in js_Execute (cx=0xa035f58, chain=0xa1c5de0, script=0xa3f0470, down=0x0, flags=0, result=0xbfdbf55c) at mozilla/js/src/jsinterp.c:1643

#15 0x001a79d1 in JS_EvaluateUCScriptForPrincipals (cx=0xa035f58, obj=0xa1c5de0, principals=0x9a5dcf4, chars=0xa3ef370, length=2170, filename=0xa19a2f0 "http://test.mozilla.com/tests/mozilla.org/js/js1_5/Array/regress-311515.js", lineno=1, 
    rval=0xbfdbf55c) at mozilla/js/src/jsapi.c:4302

#16 0x017b909f in nsJSContext::EvaluateString (this=0xa03e288, aScript=@0xa19b05c, aScopeObject=0xa1c5de0, aPrincipal=0x9a5dcf0, aURL=0xa19a2f0 "http://test.mozilla.com/tests/mozilla.org/js/js1_5/Array/regress-311515.js", aLineNo=1, aVersion=150, 
    aRetValue=0x0, aIsUndefined=0xbfdbf670) at mozilla/dom/src/base/nsJSEnvironment.cpp:1306

#17 0x015c72d1 in nsScriptLoader::EvaluateScript (this=0x9af8100, aRequest=0xa19b048, aScript=@0xa19b05c) at mozilla/content/base/src/nsScriptLoader.cpp:613

#18 0x015c75a6 in nsScriptLoader::ProcessRequest (this=0x9af8100, aRequest=0xa19b048) at mozilla/content/base/src/nsScriptLoader.cpp:527

#19 0x015c7637 in nsScriptLoader::ProcessPendingRequests (this=0x9af8100) at mozilla/content/base/src/nsScriptLoader.cpp:649

Comment 2

[Igor Bukanov]

This is a regression from bug 224128.

Comment 3

[Igor Bukanov]

Attachment: Fix

The patch changes "tvr.count = newlen * 2" from "tvr.count += newlen" as after the loop tvr.count is either newlen or newlen + 1 iff the last element was hole or undefined. In that case before the change tvr.count would be 2 * newlen + 1 and GC would read vec[newlen + 1] which is a junk value.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Created an attachment (id=245716) [edit]
> Fix
> 
> The patch changes "tvr.count = newlen * 2" from "tvr.count += newlen" as after
> the loop tvr.count is either newlen or newlen + 1 iff the last element was hole
> or undefined. In that case before the change tvr.count would be 2 * newlen + 1
> and GC would read vec[newlen + 1] which is a junk value.
                     ^^^^^^^^^^^^^^^^
I meant vec[newlen * 2] which is a junk value.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 245716 [details] [diff] [review]
Fix

r=[e-mail]

/be

Comment 6

[Igor Bukanov]

Here is a test case that crashes on Linux without way_too_much_gc:

var a = Array(3);
a[0] = 1;
a[1] = 2;
a.sort(function () { gc(); return 1; });


Let me know if does not crash on Windows.

Comment 7

[Bob Clary [:bc] (inactive)]

crashes trunk debug shell without WAY_TOO_MUCH_GC on winxp at js_GetGCThingFlags(void * thing=0xdadadad8)  Line 492

Comment 8

[Igor Bukanov]

It turned out that the previous test case may not crash on Linux, so here is better one:

var N = 1000;

// Make an array with a hole at the end
var a = Array(N);
for (i = 0; i < N - 1; ++i)
  a[i] = 1;

// array_sort due to the bug for array with N elements with a hole at the end
// allocates a temporary vector with 2*N+1 words. Lets create strings that on
// 32 and 64 bit CPU cause allocation of the same amount of memory for their
// char arrays to cause malloc in array_sort to reuse char arrays memory for
// the temporary vector after we GC strings (assuming a reasonable malloc
// implementation ). Then the bug triggers reinterpretation of 0xFFF0FFF0 as GC
// thing. 

var str1 = Array(2*(2*N + 1) + 1).join(String.fromCharCode(0xFFF0));
var str2 = Array(4*(2*N + 1) + 1).join(String.fromCharCode(0xFFF0));
gc();
str1 = str2 = null;
gc();

var firstCall = true;
a.sort(function (a, b) { 
  if (firstCall) {
    firstCall = false;
    gc();
  }
  return a - b;
});

Comment 9

[Igor Bukanov]

I committed the patch from comment 3 to the trunk:

Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.100; previous revision: 3.99
done

Comment 10

[Igor Bukanov]

Test case with proper explanation in comments:

var N = 1000;

// Make an array with a hole at the end
var a = Array(N);
for (i = 0; i < N - 1; ++i)
  a[i] = 1;

// array_sort due for array with N elements with allocates a temporary vector
// with 2*N. Lets create strings that on 32 and 64 bit CPU cause allocation
// of the same amount of memory + 1 word for their char arrays. After we GC
// strings with a reasonable malloc implementation that memory will be most
// likely reused in array_sort for the temporary vector. Then the bug causes
// accessing the one-beyond-the-aloocation word and re-interpretation of
// 0xFFF0FFF0 as GC thing. 

var str1 = Array(2*(2*N + 1) + 1).join(String.fromCharCode(0xFFF0));
var str2 = Array(4*(2*N + 1) + 1).join(String.fromCharCode(0xFFF0));
gc();
str1 = str2 = null;
gc();

var firstCall = true;
a.sort(function (a, b) { 
  if (firstCall) {
    firstCall = false;
    gc();
  }
  return a - b;
});

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Array/regress-360681-01.js

Comment 12

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Array/regress-360681-02.js

Comment 13

[Bob Clary [:bc] (inactive)]

Note these tests do not require WAY_TOO_MUCH_GC. Thanks Igor!

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061121 windows/linux

Comment 15

[Samuel Sidler (old account; do not CC)]

Given that this was a regression from bug 224128, it's not an issue on the branch.

Comment 16

[Daniel Veditz [:dveditz]]

We usually mark regressions as "blocking" the causing bug rather than "depending on" -- that way should we take the original bug on a branch in the future we will see that a real fix also requires patches from its dependencies

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Array/regress-360681-01.js,v  <--  regress-360681-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/Array/regress-360681-02.js,v  <--  regress-360681-02.js
initial revision: 1.1