CloneInterpretedFunction probably shouldn't call JSObject::clearType

RESOLVED FIXED in mozilla19



6 years ago
6 years ago


(Reporter: shu, Assigned: shu)



Firefox Tracking Flags

(Not tracked)



(2 attachments)



6 years ago
JSFunction::setTypeForScriptedFunction is always called, so I'm not sure why clearType is called. Calling clearType actuall results in the cloned function's prototype being set to NULL, which is a discrepancy with cloning functions for JSOP_LAMBDA, for example, causing infer assertions when cloning the objects() array of a function.

Comment 1

6 years ago
CCing Till, since I hit this bug from cross-compartment cloning of JSOP_LAMBDA objects from self-hosted code.

Comment 2

6 years ago
Per discussion with Luke: this is a correctness bug. It results in cloned functions having NULL for proto.

Comment 3

6 years ago
Ahh, I've been confusing js::CloneFunctionObject and js::CloneInterpretedFunction; these names are terribly ambiguous as you pointed out earlier.

The former is what gives us a new object each time we evaluate a function expression (capturing a new environment each time).  The latter is part of the process of cloning a script into a new compartment whereby we make a deep copy of the script which can contain function objects in script->objects.  The important thing is that these functions in script->objects aren't ever executed directly; they are cloned (via CloneFunctionObject) first.  I noticed that there is one CloneFunctionObject overload that uses fun->getProto() as the proto of the new clone, but this has no callers (I'll remove it).  The other CloneFunctionObject paths ignore fun->getProto() and use global.getOrCreateFunctionPrototype().  In fact, the functions in script->objects *initially* have a NULL proto (see the js_NewFunction call in Parser.cpp), so CloneInterpretedFunction is doing exactly the expected thing.  Based on that, resolving invalid.
Last Resolved: 6 years ago
Resolution: --- → INVALID

Comment 4

6 years ago
Created attachment 679328 [details] [diff] [review]
shell crash

Reopening with a crash test.

The test case is given as a diff since it needs to introduce a test builtin function. This trips an assert crash when ran with no options.

The problem is that TI analyzes JSOP_LAMBDA using |script->getObject(index)->type()|. As noted above, when the lambda still resides in the objects array before being cloned, its proto is NULL. During execution, after being cloned, its proto is *no longer NULL*. The problem is that this perturbs the type, and we get an assert error.

This happens due to the cross-compartmental clone from builtins. Note that |script->getObject(index)->getProto() != NULL| for non-cloned objects!
Assignee: general → shu
Resolution: INVALID → ---

Comment 5

6 years ago
Some clarification on "clone" in the above comment. There are 2 clones, in chronological order:

1) |TestBuiltin| is cloned into from the self-hosted global into the current global's intrinsic holder. Let's call it the clone |TestBuiltinClone|. |TestBuiltin->script()->getObject(0)->getProto()| is not NULL, but |TestBuiltinClone->script()->getObject(0)->getProto()| is NULL.
2) TI happens.
3) We execute |TestBuiltinClone|, and when we get to JSOP_LAMBDA, we clone the lambda |closure| into |closureClone|. |closure->getProto()| is NULL, but |closureClone->getProto()| is not NULL. As a result, |closure->type() != closureClone->type()|. TI asserts.

Comment 6

6 years ago
Maybe I am missing something, but wouldn't this also be a problem for the original (pre-clone) function, since the original JSFunction in script->objects (created by the Parser) also has a NULL proto?  If so, then this wouldn't be a bug in CloneInterpretedFunction, but a bug somewhere in TI for assuming a non-null proto.

Comment 7

6 years ago
I don't know enough about the frontend to say what the proto should be, but the original JSFunction in script->objects pre-clone does *not* have a NULL proto.

Comment 8

6 years ago
Oh, I spoke too soon: NULL was being passed to NewObjectWithClassProto, but, in the case of NULL, NewObjectWithClassProto finds the proto using the clasp.


6 years ago
Depends on: 791850

Comment 9

6 years ago
Created attachment 682380 [details] [diff] [review]

Per discussion with bhackett, just kill the clearParent and clearType calls.

It seems to be fairly harmless to existing tests:

Luke, can you r? or should I bounce this to bhackett?
Attachment #682380 - Flags: review?(luke)

Comment 10

6 years ago
Comment on attachment 682380 [details] [diff] [review]

bhackett would be better, even though I suspect this is a trivial r+.
Attachment #682380 - Flags: review?(luke) → review?(bhackett1024)
Attachment #682380 - Flags: review?(bhackett1024) → review+
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.