Closed Bug 492040 Opened 10 years ago Closed 10 years ago

Static analysis bug: js_Execute calling JS_REQUIRES_STACK js_FreeRawStack


(Core :: JavaScript Engine, defect, blocker)

Not set





(Reporter: benjamin, Assigned: benjamin)



(Keywords: verified1.9.1)


(1 file, 1 obsolete file)

js_Execute is calling js_FreeRawStack which is JS_REQUIRES_STACK. This dates all the way back to

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.
Blocks: 476177
No longer blocks: 476166
Yes, js_Execute should be JS_REQUIRES_STACK. Then what's next? ;-)

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.
Assignee: general → benjamin
Attachment #376415 - Flags: review?(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.
Attachment #376415 - Flags: review?(jorendorff) → review+
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).

LAST_FRAME_CHECKS already calls JS_IsRunning, which does this:

    return JS_ON_TRACE(cx) || cx->fp != NULL;
Ok, common code is best all else equal (and correct!).

Attachment #376415 - Attachment is obsolete: true
Attachment #376431 - Flags: review?(jorendorff)
Attachment #376431 - Flags: review?(jorendorff) → review+

This needs to land in 1.9.1 also: nesting script execution within native methods isn't uncommon.
Closed: 10 years ago
Flags: blocking1.9.1+
Resolution: --- → FIXED
Could this have fixed some of the random tinderbox orange?
I'd be a bit surprised, but it's possible.
Flags: in-testsuite+
Benjamin, which tests are you referring to? There are none included in the patches.
Target Milestone: --- → mozilla1.9.2a1
The static analysis caught the bug originally, so if it regressed the tree would turn red again. It's a build-time test.
Thanks Benjamin. The trees are green so we can safely mark this as verified fixed on trunk and 1.9.1.
OS: Linux → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.