If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

TM: "Assertion failure: thing,"

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: gkw, Assigned: luke)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta4+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?][ccbr] fixed-in-tracemonkey [critsmash:patch])

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.
This asserts on tm-tip as well.
No longer blocks: 549412
Summary: JM: "Assertion failure: thing," → TM: "Assertion failure: thing,"
Blocks: 584237
(Reporter)

Comment 2

7 years ago
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.
Group: core-security
blocking2.0: --- → ?

Comment 3

7 years ago
Its a backout. Try to dig a bit further.
(Reporter)

Comment 4

7 years ago
(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
Blocks: 549143
(Assignee)

Comment 5

7 years ago
Created attachment 463589 [details] [diff] [review]
patch

Awesome find!  The problem is that AutoValueVector/AutoIdVector don't make elements added by resize() GC-safe.
Assignee: general → lw
Status: NEW → ASSIGNED
Attachment #463589 - Flags: review?(jwalden+bmo)
(Reporter)

Updated

7 years ago
No longer blocks: 584237
All hail the Fuzzer! Oh and Gary too ;-).

/be

Updated

7 years ago
blocking2.0: ? → beta4+
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Whiteboard: [sg:critical?]
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.
Attachment #463589 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 8

7 years ago
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
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey

Updated

7 years ago
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?][ccbr] fixed-in-tracemonkey [critsmash:patch]

Comment 9

7 years ago
http://hg.mozilla.org/mozilla-central/rev/0714e6bd36be
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Group: core-security
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug584650.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.