481144 - JS_ValueToFunction should not chase PRIVATE field

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Instead it should return a pointer to the JSObject itself, cast to JSFunction.  See bug 476374 comment 15.

Comment 1

[Jason Orendorff [:jorendorff]]

I thought this would be easy, but it's not...

Currently we overallocate for Function JSObjects when cloning.  The clone is the size of a JSFunction, but only the JSObject fields are actually populated.  The rest is garbage.

If we really want JSFunction pointers *outside* the JSAPI to point to complete objects, then we should have a new type that means what JSFunction means now, for use *inside* js.

Bug 313370 makes this interesting for another reason.

Comment 2

[Brendan Eich [:brendan]]

JSObject *
js_CloneFunctionObject(JSContext *cx, JSFunction *fun, JSObject *parent)
{
    JSObject *clone;

    /*
     * The cloned function object does not need the extra fields beyond
     * JSObject as it points to fun via the private slot.
     */
    clone = js_NewObject(cx, &js_FunctionClass, NULL, parent,
                         sizeof(JSObject));
 ...

No over-allocation here -- what am I missing?

Cc'ing Igor, who has thought about this a lot. He did the work to fuse JSObject and JSFunction -- make the latter an extension of the former, I mean -- for the compiler-created function objects (not the clones).

"I never clone alone." - Woody Allen, "Sleeper"

/be

Comment 3

[Jason Orendorff [:jorendorff]]

> No over-allocation here -- what am I missing?

You're right. I was thinking of the hack in js_NewObjectWithGivenProto, but that only happens if you pass 0 as the size, which this doesn't.

Comment 4

[Igor Bukanov]

JS_ValueToFunction must chase the PRIVATE field as the object can be a clone, see bug 423874 for the history behind that.

Comment 5

[Jason Orendorff [:jorendorff]]

Igor, the idea was to change the API so that JSFunction* means something different.  It would mean the closure (clone) rather than the function code and flags.

The PRIVATE field would still get chased--but later, when the JSFunction* was used, not when it was produced.  We would have to rename the JSFunction struct we currently have.

This would make a nicer API, but isn't necessarily a wise change to make right now (or ever).

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> Igor, the idea was to change the API so that JSFunction* means something
> different.  It would mean the closure (clone) rather than the function code and
> flags.

This was already tried in bug 423874 and lead IIRC to regressions in the form of apparently unrelated memory leaks.