If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

JS_ValueToFunction should not chase PRIVATE field

RESOLVED INVALID

Status

()

Core
JavaScript Engine
RESOLVED INVALID
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

9 years ago
Instead it should return a pointer to the JSObject itself, cast to JSFunction.  See bug 476374 comment 15.
(Assignee)

Comment 1

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

Comment 3

9 years ago
> 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

9 years ago
JS_ValueToFunction must chase the PRIVATE field as the object can be a clone, see bug 423874 for the history behind that.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → INVALID
(Assignee)

Comment 5

9 years ago
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

9 years ago
(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.
You need to log in before you can comment on or make changes to this bug.