492040 - Static analysis bug: js_Execute calling JS_REQUIRES_STACK js_FreeRawStack

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Smedberg]

js_Execute is calling js_FreeRawStack which is JS_REQUIRES_STACK. This dates all the way back to http://hg.mozilla.org/mozilla-central/rev/932126be5356

I think this was only recently exposed due to a bug fix in lazy_types.js

I don't know the JS engine enough to know where the bug is here: should js_Execute be marked JS_REQUIRES_STACK? It gets called from JS_ExecuteScript which, I think, needs to bail off trace, but I'd like confirmation of that from somebody who knows better.

Comment 1

[Brendan Eich [:brendan]]

Yes, js_Execute should be JS_REQUIRES_STACK. Then what's next? ;-)

/be

Comment 2

[Benjamin Smedberg]

Attachment: js_Execute is JS_REQUIRES_STACK, and implications thereof, rev. 1

It only unravels a bit... but that seems to be an important bit!

This fixes the js_FreeRawStack error, but it does *not* fix another set of errors in js_CheckUndeclaredVarAssignment which appear to be more recent, and seem to be an analysis problem. I'll look at that separately.

Comment 3

[Jason Orendorff [:jorendorff]]

I think it would be better (less code) to make js_Execute JS_FORCES_STACK and put the js_LeaveTrace call in there.

r=me with that.  Thanks for looking into these.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 376415 [details] [diff] [review]
js_Execute is JS_REQUIRES_STACK, and implications thereof, rev. 1

(In reply to comment #3)
> I think it would be better (less code) to make js_Execute JS_FORCES_STACK and
> put the js_LeaveTrace call in there.

That works for this case:

> JS_ExecuteScript(JSContext *cx, JSObject *obj, JSScript *script, jsval *rval)
> {
>     JSBool ok;
> 
>     CHECK_REQUEST(cx);
>+    js_LeaveTrace(cx);
>     ok = js_Execute(cx, obj, script, NULL, 0, rval);
>     LAST_FRAME_CHECKS(cx, ok);
>     return ok;
> }

But what about this one, if there's a compilation error?

>@@ -5227,16 +5228,17 @@ JS_EvaluateUCScriptForPrincipals(JSConte
>                                  const jschar *chars, uintN length,
>                                  const char *filename, uintN lineno,
>                                  jsval *rval)
> {
>     JSScript *script;
>     JSBool ok;
> 
>     CHECK_REQUEST(cx);
>+    js_LeaveTrace(cx);
>     script = js_CompileScript(cx, obj, NULL, principals,
>                               !rval
>                               ? TCF_COMPILE_N_GO | TCF_NO_SCRIPT_RVAL
>                               : TCF_COMPILE_N_GO,
>                               chars, length, NULL, filename, lineno);
>     if (!script) {
>         LAST_FRAME_CHECKS(cx, script);
>         return JS_FALSE;

LAST_FRAME_CHECKS is tricky, it wants to do stuff if !cx->fp. Could we trigger a trace with cx->fp null, then from the trace call one of these APIs and end up misjudging cx->fp's null-ness? This isn't an issue now with traces triggered only from the interpreter, but it could be if we trigger from native code calling an event handler (event loop tracing).

/be

Comment 5

[Jason Orendorff [:jorendorff]]

LAST_FRAME_CHECKS already calls JS_IsRunning, which does this:

    return JS_ON_TRACE(cx) || cx->fp != NULL;

Comment 6

[Brendan Eich [:brendan]]

Ok, common code is best all else equal (and correct!).

/be

Comment 7

[Benjamin Smedberg]

Attachment: js_Execute should be JS_FORCES_STACK (and actually do it), rev. 2

Comment 8

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/98dab8153e42
http://hg.mozilla.org/tracemonkey/rev/250bd212fd59

This needs to land in 1.9.1 also: nesting script execution within native methods isn't uncommon.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could this have fixed some of the random tinderbox orange?

Comment 10

[Benjamin Smedberg]

I'd be a bit surprised, but it's possible.

Comment 11

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c27e4c8b850f

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Benjamin, which tests are you referring to? There are none included in the patches.

Comment 13

[Benjamin Smedberg]

The static analysis caught the bug originally, so if it regressed the tree would turn red again. It's a build-time test.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Benjamin. The trees are green so we can safely mark this as verified fixed on trunk and 1.9.1.