584650 - TM: "Assertion failure: thing,"

Status: VERIFIED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

x = (evalcx('lazy'))
x.watch("", function () {})
gczeal(1)
for (w in x) {}

asserts js debug shell on JM changeset 6347cf00d3ab with -m at Assertion failure: thing, at ../jsgc.cpp:2177 if passed in as a CLI argument.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

This asserts on tm-tip as well.

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

Locking s-s because gczeal is involved.

I double checked and indeed this seems to be the case, that it's also on TM.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   48355:babf32a45349
user:        Andreas Gal
date:        Wed Jul 28 11:20:19 2010 -0700
summary:     Back out bug 580803.

I'm not sure if it's indeed related to the changeset or not.

Comment 3

[Andreas Gal :gal]

Its a backout. Try to dig a bit further.

Comment 4

[Gary Kwong [:gkw] [:nth10sd]]

(In reply to comment #3)
> Its a backout. Try to dig a bit further.

This is probably more plausible:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   47546:9c869e64ee26
user:        Luke Wagner
date:        Wed Jul 14 23:19:36 2010 -0700
summary:     Bug 549143 - fatvals

Comment 5

[Luke Wagner [:luke]]

Created attachment 463589 [details] [diff] [review]
patch

Awesome find!  The problem is that AutoValueVector/AutoIdVector don't make elements added by resize() GC-safe.

Comment 6

[Brendan Eich [:brendan]]

All hail the Fuzzer! Oh and Gary too ;-).

/be

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 463589 [details] [diff] [review]
patch

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
>+MakeValueRangeGCSafe(Value *vec, size_t len)
>+MakeValueRangeGCSafe(Value *beg, Value *end)
>+MakeIdRangeGCSafe(jsid *beg, jsid *end)
>+MakeIdRangeGCSafe(jsid *vec, size_t len)
>+SetValueRangeToUndefined(Value *beg, Value *end)
>+SetValueRangeToUndefined(Value *vec, size_t len)
>+SetValueRangeToNull(Value *beg, Value *end)
>+SetValueRangeToNull(Value *vec, size_t len)

These would seem more sensible in jsvalue.h.


>+        /* N.B. Value's default ctor leaves the Value udnefined */

Pervasive typo, and put a period at the end of this complete sentence.


>     bool resize(size_t newLength) {
>-        return vector.resize(newLength);
>+        size_t oldLength = vector.length();
>+        if (newLength <= oldLength)
>+            vector.shrinkBy(oldLength - newLength);
>+        /* N.B. Value's default ctor leaves the Value udnefined */
>+        if (!vector.growByUninitialized(newLength - oldLength))
>+            return false;
>+        MakeValueRangeGCSafe(vector.begin() + oldLength, vector.end());
>+        return true;
>     }

Umm, this looks...wrong.  Are you missing a return true in the <= case?


>     bool resize(size_t newLength) {
>-        return vector.resize(newLength);
>+        size_t oldLength = vector.length();
>+        if (newLength <= oldLength)
>+            vector.shrinkBy(oldLength - newLength);
>+        /* N.B. jsid's default ctor leaves the jsid udnefined */
>+        if (!vector.growByUninitialized(newLength - oldLength))
>+            return false;
>+        MakeIdRangeGCSafe(vector.begin() + oldLength, vector.end());
>+        return true;
>     }

Same.

r=me if you agree all that's missing is a return true in the <= cases, otherwise poke me again.

Comment 8

[Luke Wagner [:luke]]

Thanks for pointing out the thinko!
http://hg.mozilla.org/tracemonkey/rev/bd2a104ad837

and, because I didn't notice all the comments at first
http://hg.mozilla.org/tracemonkey/rev/0714e6bd36be

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0714e6bd36be

Comment 10

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug584650.js.