Crash [@ JSObject::getParent]

RESOLVED INVALID

Status

()

Core
JavaScript Engine
--
critical
RESOLVED INVALID
7 years ago
7 years ago

People

(Reporter: robarnold, Unassigned)

Tracking

({crash})

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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);
}

Updated

7 years ago
Severity: normal → critical
Keywords: crash
Summary: Crash in JSObject::getParent → Crash [@ JSObject::getParent]
Do you have a stack trace?
(Reporter)

Comment 2

7 years ago
Created attachment 522546 [details]
Stack Trace
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.
(Reporter)

Comment 4

7 years ago
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).
(Reporter)

Comment 5

7 years ago
JS_CallFunctionValue seems to do the trick - thanks! I am not really sure what JS_ValueToFunction is useful for now.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
(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.
(Reporter)

Comment 7

7 years ago
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.
(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

7 years ago
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.
Crash Signature: [@ JSObject::getParent]
You need to log in before you can comment on or make changes to this bug.