632544 - tracemonkey crashes with jsdScript with NULL function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

I sometimes get a crash in the Firebug test suite in jsdScript::GetParameterNames because JSD_GetJSFunction returns NULL. In the most recent example, the script was 

http://getfirebug.com/tests/content/commandLine/dom.html line 87

That file is 54 lines long. Disassembling the script, I get

(gdb) p js_Disassemble(cx, mScript->script, 0, stdout)
main:
00000:  name "_FirebugCommandLine"
00003:  enterwith
00004:  nullblockchain
00005:  name "blah"
00008:  popv
00009:  leavewith
00010:  stop

which just looks like

  with (_FirebugCommandLine) {
    blah;
  }

or something. I'm guessing that results from dom.js, sitting next to that dom.html:

            taskList.push(executeAndVerify, "blah",
                /\s*ReferenceError: blah is not defined/,
                "div", "logRow logRow-errorMessage");

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Handle jsdScript with NULL functions

Perhaps it's a real error that should be tracked down where jsdScript can get a NULL function, but it still seems like we should be defensive.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 510763 [details] [diff] [review]
Handle jsdScript with NULL functions

Steve, a jsdScript mirrors a JSScript, it can be global, eval, or function code (eval can be direct from function, direct from global, or indirect -- as if from global). If a script does not represent function code, as your with-using script does not, then it will have a null function.

This is a safe null-defense fix. Plussing now to get it going for Firefox 4, welcome timeless's thoughts.

/be

Comment 3

[timeless]

http://hg.mozilla.org/mozilla-central/file/default/js/jsd/jsd_xpc.cpp
currently points to

http://hg.mozilla.org/mozilla-central/file/62601280d25d/js/jsd/jsd_xpc.cpp

  1280 jsdScript::GetParameterNames(PRUint32* count, PRUnichar*** paramNames)
  1281 {
  1282     ASSERT_VALID_EPHEMERAL;
  1283     JSContext *cx = JSD_GetDefaultJSContext (mCx);
  1284     if (!cx) {
  1285         NS_WARNING("No default context !?");
  1286         return NS_ERROR_FAILURE;
  1287     }
  1288     JSFunction *fun = JSD_GetJSFunction (mCx, mScript);
  1289 
  1290     JSAutoRequest ar(cx);
  1291 
  1292     uintN nargs;
  1293     if (!fun ||
  1294         !JS_FunctionHasLocalNames(cx, fun) ||
  1295         (nargs = JS_GetFunctionArgumentCount(cx, fun)) == 0) {
  1296         *count = 0;
  1297         *paramNames = nsnull;
  1298         return NS_OK;
  1299     }
  1300 
  1301     PRUnichar **ret =

...

the code is careful, i think this is based on a local changeset which i'd hope would not be r+'d.

afaict from checking w/ mxr we're currently properly handling his function returning null.

Comment 4

[timeless]

ok, this is a regression caused by bug 628758

http://hg.mozilla.org/tracemonkey/rev/87dc60c12d24

the patch is more or less wrong. as is actually visible given the context of the patch:

>     JSFunction *fun = JSD_GetJSFunction (mCx, mScript);
>+    if (!fun)
>+        return NS_ERROR_NOT_AVAILABLE;
> 
>     JSAutoRequest ar(cx);
>     JSAutoEnterCompartment ac;
>     if (!ac.enter(cx, JS_GetFunctionObject(fun)))
>         return NS_ERROR_FAILURE;
> 
>     uintN nargs;
here is the check which was expecting to be hit:
>     if (!fun ||

Please properly split out that if instead of adding a second if and leaving a useless condition.

Comment 5

[timeless]

Comment on attachment 510763 [details] [diff] [review]
Handle jsdScript with NULL functions

actually, this is also an api change.

the old path yielded:
1299 *count = 0;
1300 *paramNames = nsnull;
1301 return NS_OK;

i.e. an empty list, the new behavior would be to throw, which is something i'm assuming consumers don't expect and certainly don't want.

so, r-.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Handle jsdScript with NULL functions

timeless is right. I broke the NULL handling in bug 628758.

This is one possible fix, which unfortunately duplicates the return-empty-list code.

There is an alternative fix which would move the compartment entry to below the if statement that tests JS_FunctionHasLocalNames and JS_GetFunctionArgumentCount. This appears to be safe, since neither of those routines actually needs to be called in the correct compartment since they're just field accessors (one of them goes through several layers, but still). I'd just rather be able to verify at a glance that the compartment juggling is correct without needing to know which JS_* calls require a matching compartment and which don't. (I can't dispense with the cross-compartment call altogether because JS_GetFunctionLocalNameArray could set an exception via js_ReportOutOfScriptQuota(cx).)

Comment 7

[Steve Fink [:sfink] [:s:]]

Re-nominating for hardblocker.

It causes a null pointer dereference in the Firebug test suite, and the fix is trivial.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 511069 [details] [diff] [review]
Handle jsdScript with NULL functions

This is holding up the merge and the review doesn't require any timeless-specific expertise, so switching to Andreas.

Comment 9

[David Mandelin [:dmandelin]]

Comment on attachment 511069 [details] [diff] [review]
Handle jsdScript with NULL functions

Drive-by r+. Looks simple and correct, let's get it in.

Comment 10

[timeless]

i don't like this.

i claim the caller doesn't expect this method to fail *period*. unless someone can give me a good reason to violate that expectation, i claim the following is correct:

     JSFunction *fun = JSD_GetJSFunction (mCx, mScript);
     JSAutoRequest ar(cx);
     JSAutoEnterCompartment ac;
 
     uintN nargs;
     if (!fun ||
         !ac.enter(cx, JS_GetFunctionObject(fun))
         !JS_FunctionHasLocalNames(cx, fun) ||
         (nargs = JS_GetFunctionArgumentCount(cx, fun)) == 0) {
         *count = 0;
         *paramNames = nsnull;
         return NS_OK;
     }

Comment 11

[timeless]

Comment on attachment 511069 [details] [diff] [review]
Handle jsdScript with NULL functions

i'm stamping a feedback-, although perhaps i should claim moa- instead.

note: if someone can provide a valid justification for triggering the exception (regression from the other bug), then my feedback can be ignored, otherwise i request that the patch (which is basically just the code i inlined in my previous comment) is what should be landed, take any reviewer for that.

Comment 12

[Steve Fink [:sfink] [:s:]]

It will only fail in out-of-memory conditions.

But why would it be better to succeed and say there are no parameters? That just seems wrong.

Comment 13

[Steve Fink [:sfink] [:s:]]

Further, that method already returns errors in other cases (NS_ERROR_OUT_OF_MEMORY, which is similar to this case except we can't really assume that the only way enter() can fail is by running oom, and NS_ERROR_FAILURE if the context is null.) So we're not going from something that never fails to something that does.

Comment 14

[timeless]

if the only reason we know of that it can fail is oom, i'd rather we say that.

go ahead and land this.

Comment 15

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/2d60ba7e95a1

Comment 16

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

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2d60ba7e95a1