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.
CCing Till, since I hit this bug from cross-compartment cloning of JSOP_LAMBDA objects from self-hosted code.
Per discussion with Luke: this is a correctness bug. It results in cloned functions having NULL for proto.
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
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
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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.
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.
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.
Oh, I spoke too soon: NULL was being passed to NewObjectWithClassProto, but, in the case of NULL, NewObjectWithClassProto finds the proto using the clasp.
Created attachment 682380 [details] [diff] [review] fix Per discussion with bhackett, just kill the clearParent and clearType calls. It seems to be fairly harmless to existing tests: https://tbpl.mozilla.org/?tree=Try&rev=70d78cf7091d Luke, can you r? or should I bounce this to bhackett?
Attachment #682380 - Flags: review?(luke)
Comment on attachment 682380 [details] [diff] [review] fix 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+
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago → 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.