443843 - valgrind errors in sessionstore mochitest chrome tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

These are JS Engine errors.

Comment 1

[Robert Sayre]

Attachment: Valgrind Log of error

Comment 2

[Brendan Eich [:brendan]]

    if ((!js_IdIsIndex(id, &i) &&
         id != ATOM_TO_JSID(cx->runtime->atomState.lengthAtom)) ||
        obj->fslots[JSSLOT_ARRAY_LENGTH] == 0 ||
        i >= ARRAY_DENSE_LENGTH(obj) ||
        obj->dslots[i] == JSVAL_HOLE)

If the first || left-hand side is false, then either js_IdIsIndex(id, &i), in which case i will be set, or id == 'length', in which case i will not be set and the use of i in the fourth line will UMR.

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

That...could be true, yes. *blush*

I'll patch this today, unless someone beats me to it.

sayrer: is there a ready-to-go setup on sm-valgrind for me to test this on?  I don't have a Linux VM or desktop install atm.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I just ran this patch through mochitests and things came out clean [1].

[1] I did leak a bunch of stuff, but in my recollection, I've never *not* leaked when running a full set of MochiTests in a debug build.

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 328325 [details] [diff] [review]
Proposed fix

r=shaver, thanks

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Pushed as changeset 6059b09ca278.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 328325 [details] [diff] [review]
Proposed fix

I'm guessing we want this on the 1.9 branch.

Comment 8

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

Comment on attachment 328325 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.9 branch.