645604 - Crash [@ JSObject::getParent]

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Rob Arnold [:robarnold]]

I discovered this crash while trying to port one of the V8 API tests. Reduced testcase is as follows:
void test_fn() {
  const char *text =
    "function Foo() {"
    "  var result = [];"
    "  for (var i = 0; i < arguments.length; i++) {"
    "    result.push(arguments[i]);"
    "  }"
    "  return result;"
    "}";
  JSRuntime *rt = JS_NewRuntime(64 * MB);

  JSContext *ctx(JS_NewContext(rt, 8192));
  JS_SetOptions(ctx, JSOPTION_VAROBJFIX | JSOPTION_JIT | JSOPTION_METHODJIT | JSOPTION_DONT_REPORT_UNCAUGHT);
  JS_SetVersion(ctx, JSVERSION_LATEST);

  JS_BeginRequest(ctx);
  JSObject* global(JS_NewCompartmentAndGlobalObject(ctx, &global_class, NULL));
  JSAutoEnterCompartment compartment;
  compartment.enter(ctx, global);
  JSScript* s = JS_CompileScript(ctx, NULL, text, strlen(text), NULL, NULL);
  JS::Anchor<JSObject*> obj(JS_NewScriptObject(ctx, s));
  jsval r;
  JS_ExecuteScript(ctx, global, s, &r);
  jsval foo;
  JS_GetProperty(ctx, global, "Foo", &foo);
  JSObject *fooObj = JSVAL_TO_OBJECT(foo);
  JS_CallFunction(ctx, fooObj, JS_ValueToFunction(ctx, foo), 0, NULL, &r);
}

Comment 1

[David Mandelin [:dmandelin]]

Do you have a stack trace?

Comment 2

[Rob Arnold [:robarnold]]

Attachment: Stack Trace

Comment 3

[David Mandelin [:dmandelin]]

Your JSAPI looks a bit out of date: the way scripts come back is different now. Anyway, it works for me if I change the last line to:

  JS_CallFunctionValue(ctx, fooObj, foo, 0, NULL, &r);

(I also think fooObj is not the right value for the second parameter, but it probably doesn't matter too much.)

The reason the original version doesn't work is that JS_ValueToFunction(ctx, foo) returns the "canonical" JSFunction for that function. But that value doesn't have a scope chain, so bad things happen when we try to call it.

Adding JSAPI experts in case they have additional commentary.

Comment 4

[Rob Arnold [:robarnold]]

Yes, we're trying to get a version of tracemonkey imported into git so we can merge it in. The process has been running since Saturday morning (sigh).

Comment 5

[Rob Arnold [:robarnold]]

JS_CallFunctionValue seems to do the trick - thanks! I am not really sure what JS_ValueToFunction is useful for now.

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Yes, we're trying to get a version of tracemonkey imported into git so we can
> merge it in. The process has been running since Saturday morning (sigh).
Getting a TM merge into mozilla-central would be a big help too.

Comment 7

[Rob Arnold [:robarnold]]

I'm not sure this should be resolved invalid - given that the program typechecks and I'm not doing anything weird with memory management, I think crashing is not really appropriate. At the very least, an assertion that gives me some clue of what I did wrong would be nice or even returning JS_FALSE as the documentation for JS_CallFunction's documentation implies should happen.

Comment 8

[David Mandelin [:dmandelin]]

(In reply to comment #7)
> I'm not sure this should be resolved invalid - given that the program
> typechecks and I'm not doing anything weird with memory management, I think
> crashing is not really appropriate. At the very least, an assertion that gives
> me some clue of what I did wrong would be nice or even returning JS_FALSE as
> the documentation for JS_CallFunction's documentation implies should happen.

I just checked the docs and they say not to use JS_ValueToFunction [1], so I think it is covered. Maybe we should remove that function from the API, or deprecate it?

https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_ValueToFunction

Comment 9

[timeless]

it's a published api, you can't really remove it. you can make it spit out an error message and return failure. and you definitely should deprecate it if all it could do is crash.