630735 - NodeVector isn't GC-safe

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

gczeal(2);
var s = '';
for (var i = 0; i < 1000; i++)
    s += i + ',';
Reflect.parse('[' + s + ']', {loc: false});

Assertion failure: addr % sizeof(FreeCell) == 0, at d:\dev\tracemonkey\js\src\jsgc.h:367


jsreflect.cpp defines NodeVector this way:
    typedef Vector<Value, 8> NodeVector;
and it is only used on the stack. If I understand correctly, that will be safe for the first 8 Values. After that, the vector's storage is malloc'd, and the GC does not pursue the pointer from the Vector to the heap.

So I think it needs to be, instead:
    typedef AutoValueVector NodeVector;
and we'll also have to change every place one is declared, to pass the cx to the constructor.

Comment 1

[Andreas Gal :gal]

Any chance of content getting hold of Reflect or content causing chrome to act on it?

Comment 2

[Dave Herman [:dherman]]

Not exposed to chrome. Shell-only. Unless it, like, uses ctypes to extract the functions from the FF binary. But I'll post a patch asap.

Dave

Comment 3

[Andreas Gal :gal]

In that case we don't have to hide this. But yes, please fix.

Comment 4

[Dave Herman [:dherman]]

Switching to AutoValueVector is trivial (Vector already takes cx in its constructor) but it still crashes on

    gczeal(2), Reflect.parse(uneval(new Array(1000)), {loc:false})

Will investigate.

Dave

Comment 5

[Dave Herman [:dherman]]

Attachment: use AutoValueVector for NodeVector, eliminate JS_DestroyScript

This patch includes the suggested fix, and also eliminates the bogus uses of JS_DestroyScript() in shell/js.cpp.

Dave

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 509129 [details] [diff] [review]
use AutoValueVector for NodeVector, eliminate JS_DestroyScript

In shell/js.cpp, Load:
>+        ok = !script
>+             ? JS_FALSE
>+             : !compileOnly
>+             ? JS_ExecuteScript(cx, thisobj, script, NULL)
>+             : JS_TRUE;
>         if (!ok)
>             return JS_FALSE;
>     }

Thanks for getting rid of these JS_DestroyScript calls. Merely as a matter of style, please change these lines to:

        if (!script)
            return JS_FALSE;

        if (!compileOnly && !JS_ExecuteScript(cx, thisobj, script, NULL))
            return JS_FALSE;
    }

r=me with that.

Comment 7

[Brendan Eich [:brendan]]

Or, if you really want to write an expression, avoid ?: expressions with true or false in one arm:

 a ? false : b => a && b
 a ? true : b  => a || b

(with !! as needed to canonicalize ints as bools).

/be

Comment 8

[Dave Herman [:dherman]]

http://hg.mozilla.org/tracemonkey/rev/53403b41c153

Comment 9

[Brendan Eich [:brendan]]

Late for a drive-by, just wanted to say: please use true and false now, not JS_TRUE and JS_FALSE, even with JSBool return type. The C++ bools convert nicely. It's the other direction that requires !!.

/be

Comment 10

[Brendan Eich [:brendan]]

> a ? false : b => a && b

Er, of course:

  a ? b : false => a && b

/be

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/53403b41c153