683714 - "Assertion failure: fun->getProto() == proto" in CloneFunctionObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: fun->getProto() == proto, at js/src/jsfun.h:474

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I claim this is the same bug we hit on Linux M1: <https://tbpl.mozilla.org/php/getParsedLog.php?id=6263993>

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

Kind of bizarre.

We put a method barrier on a function created in a compileAndGo script, after the script executed XPConnect reparented certain objects so that for the fun in the method barrier, fun->getGlobal() != fun->getProto()->getGlobal() (the parent window of the function changed, but not that of its prototype).  The method barrier broke, and when cloning the object we tried to make a function whose prototype was the Function.prototype of the *new* window.  TI assertions caught this (changing the type/proto of a function when cloning it for a method barrier breaks TI assumptions).  The fix adds a variant of CloneFunctionObject for use in barriers, which always preserves parent/proto.

Comment 4

[Brian Hackett [Laid off!]]

Pushed to JM for further fuzz testing.

http://hg.mozilla.org/projects/jaegermonkey/rev/50d4f6fa00ce

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 558523 [details] [diff] [review]
patch

This worries me for a couple reasons.

Fundamentally, I still don't see exactly how this situation arises.

But also, if XPConnect can reparent an object that is on the scope chain of a compileAndGo function *before* it is cloned, I don't see why it couldn't reparent it *after* it is cloned. That would change the global of a compileAndGo function, which would break engine invariants, right? Maybe not, if only script->global() matters now?

Also, the patch needs a test. I think a jsapi-test would be better than a full-browser test because the asserting test case Jesse found doesn't exactly make it clear what's going on.

Also, I'm worried that SetParent has rules (related to method barriers) that it doesn't assert, even if those rules don't come into play in this testcase. (The patch looks like an OK way to fix this if XPConnect really isn't breaking any engine invariants, but I don't know if that's true.)

I'm a little worried that compile-and-go might have issues in combination with scope chains containing multiple objects.

Comment 6

[Till Schneidereit [:till]]

Did this ever land, or, more likely, was it obsoleted by changes in SpiderMonkey?

Comment 7

[Brian Hackett [Laid off!]]

Yes, in rev 50d4f6fa00ce.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can we land the test?