788822 - Assertion failure: [infer failure] Missing type pushed 0: <0x7f7d5aa09060>, at jsinfer.cpp:328

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on ionmonkey revision 0b296feed34c (run with --ion -n -m --ion-eager -a):


function TestCase(n, d, e, a)
  this.name = n;
function reportCompare (expected, actual, description) {
  var testcase = new TestCase("unknown-test-name", description, expected, actual);
}
evaluate("reportCompare(true,true);", { noScriptRval: true });
evaluate("( 2 , TestCase       ) ();", { noScriptRval: true });

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   105607:6cd206b37176
parent:      104959:b63bb39ed1c0
parent:      105606:a0240c1043ee
user:        David Anderson
date:        Wed Aug 29 17:51:24 2012 -0700
summary:     Merge from mozilla-central.

Oops! We didn't test rev b63bb39ed1c0, a parent of the blamed revision! Let's do that now.
Rev b63bb39ed1c0: Found cached shell...    Testing... Exit status: NORMAL (0.194 seconds)
good (not interesting) 
As expected, the parent's label is the opposite of the blamed rev's label.

Oops! We didn't test rev a0240c1043ee, a parent of the blamed revision! Let's do that now.
We did not test rev a0240c1043ee because it is not a descendant of either 4ceb3e9961e4 or 0b296feed34c.
Rev a0240c1043ee: Found cached shell...    Testing... Exit status: ABNORMAL return code 1 (0.111 seconds)
good (not interesting) 
As expected, the parent's label is the opposite of the blamed rev's label.

Comment 2

[Jan de Mooij [:jandem]]

Reduced to:
-----
function TestCase(n) {
    this.name = n;
}
new TestCase(22);
evaluate("(2, TestCase)(2);", {noScriptRval: true});
-----

Interestingly, it also fails with JM (--no-ion -a):

Assertion failure: !fe->isConstant(), at FrameState-inl.h:552
Bus error: 10

Maybe related to  bug 763948, not sure yet.

Comment 3

[Jan de Mooij [:jandem]]

TI problem. When we have a JSOP_CALLGNAME op, we add a PropagateThis constraint:

    if (op == JSOP_CALLGNAME)
        pushed[0].addPropagateThis(cx, script, pc, GetCalleeThisType(pc));

However, if we have for instance:

    (1, TestCase)(2);

JSOP_GETGNAME is emitted instead of JSOP_CALLGNAME, so the constraint is not added and the callee |this| typeset does not contain the right value.

00000:  one
00001:  pop
00002:  getgname "TestCase"
00007:  undefined
00008:  notearg
00009:  int8 2
00011:  notearg
00012:  call 1
00015:  pop

Comment 4

[Jan de Mooij [:jandem]]

Bisecting JM+TI points to bug 778724.

Comment 5

[Jan de Mooij [:jandem]]

Below is another testcase based on bug 763948, here it's JSOP_THIS instead of JSOP_GETGNAME. Bug 763948 was filed in June so the bug was there before bug 778724 landed, but I think bug 778724 made it more likely to fail.

function O() {
    this.x = 10;
}
new O();
O.foo = function() {
    this();
}
O.foo();

Comment 7

[Sean Stangl [:sstangl]]

The test case in Comment 0 currently fails with a different error, not hitting the error originally found. The new error is:

Assertion failure: [infer failure] Missing type pushed 0: <0x7ffff1509060>, at /home/sstangl/dev/mozilla-inbound/js/src/jsinfer.cpp:311

Program received signal SIGSEGV, Segmentation fault.

It's likely that something else regressed this; I'm bisecting.

Comment 8

[Sean Stangl [:sstangl]]

(In reply to Sean Stangl from comment #7)
> The test case in Comment 0 currently fails with a different error, not
> hitting the error originally found. The new error is:

Disregard that -- I mixed up two bugs. All is well.

Comment 9

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 2fae8bd461da).

Comment 10

[Christian Holler (:decoder)]

JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   109825:44079242ee9b
user:        Terrence Cole
date:        Tue Oct 09 17:26:13 2012 -0700
summary:     Bug 799777 - Unravel the threaded interpreter; r=luke

This iteration took 138.373 seconds to run.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: patch

TI is mistaken here in assuming that each JSOP_CALL/JSOP_NEW/etc. has its callee pushed by a JSOP_CALL* opcode (did this change recently?).  This behavior is more historical from when the JSOP_CALL* opcodes pushed two values, and isn't really necessary anymore.  This patch changes things so that the callee's this type is determined from constraints pushed at the JSOP_CALL, except for JSOP_CALLPROP which needs to correlate different callees and 'this' types.

Comment 12

[Jan de Mooij [:jandem]]

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

Review of attachment 674087 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinfer.cpp
@@ +4119,5 @@
> +             script->code[calleeValue.pushedOffset()] != JSOP_CALLPROP))
> +        {
> +            HandleScript script_ = script;
> +            calleeTypes->add(cx, cx->analysisLifoAlloc().new_<TypeConstraintPropagateThis>
> +                                   (script_, pc, Type::UndefinedType(), callsite->thisTypes));

You can pass "script" directly and use addPropagateThis:

calleeTypes->addPropagateThis(cx, script, pc, Type::UndefinedType(), callsite->thisTypes);

Comment 13

[Jan de Mooij [:jandem]]

Btw, looking at the code I doubt Firefox 16 is indeed "unaffected". So the least we should do is land this on aurora and beta and add the test cases after beta moves to stable.

Comment 14

[Christian Holler (:decoder)]

Assuming this is sec-critical due to infer failure. Let me know if that isn't right.

Comment 15

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca391c7bceb8

Comment 16

[Alex Keybl [:akeybl]]

(In reply to Brian Hackett (:bhackett) [mostly gone until mid-November] from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ca391c7bceb8

Now that this has landed on m-i, we'll want to get uplift nominations as soon as possible.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ca391c7bceb8

Should this have a test?

Comment 18

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Bob Clary [:bc] (inactive)]

This appears to have fixed a regression on Nightly which still exists on Aurora.

http://gojs.net/latest/samples/flowchart.html

Assertion failure: false (could not find use), at c:/work/mozilla/builds/aurora/mozilla/js/src/ion/MIR.cpp:256

Found regression between 20120910183953-20120911183952
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=96287ad60bef&tochange=6e78c3efd115
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/09/2012-09-11-mozilla-central-debug/firefox-18.0a1.en-US.debug-mac64.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/09/2012-09-12-mozilla-central-debug/firefox-18.0a1.en-US.debug-mac64.dmg

Found fix between 20121022184211-20121024022814
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=48502b61a63e&tochange=b06ee7ea6ba0
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/10/2012-10-23-mozilla-central-debug/firefox-19.0a1.en-US.debug-mac64.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/10/2012-10-24-mozilla-central-debug/firefox-19.0a1.en-US.debug-mac64.dmg

Found regression between 20121008021134-20121008142135
Pushlog: http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=c7c6f2aa37b6&tochange=7588306df29e
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/10/2012-10-08-mozilla-aurora-debug/firefox-17.0a2.en-US.debug-mac64.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/10/2012-10-09-mozilla-aurora-debug/firefox-18.0a2.en-US.debug-mac64.dmg

The first good revision is:
changeset:   111161:ca391c7bceb8
user:        Brian Hackett <[e-mail]>
date:        Tue Oct 23 09:05:27 2012 -0700
summary:     Propagate callee 'this' types more often, bug 788822. r=jandem

Comment 20

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #19)
> This appears to have fixed a regression on Nightly which still exists on
> Aurora.
> 
> http://gojs.net/latest/samples/flowchart.html
> 
> Assertion failure: false (could not find use), at
> c:/work/mozilla/builds/aurora/mozilla/js/src/ion/MIR.cpp:256

The first bad revision is:
changeset:   106790:ca3fa3fbe62a
parent:      106789:01f6ddbb6542
parent:      104782:d042ad078f43
user:        David Anderson <[e-mail]>
date:        Tue Sep 11 10:25:14 2012 -0700
summary:     Merge IonMonkey to mozilla-central. a=arewefastyet

My question is did https://hg.mozilla.org/mozilla-central/rev/ca391c7bceb8 fix Assertion failure: false (could not find use) or did it just hide it?

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 778724
User impact if declined: potential security bug
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): Low risk, this changes some details in TI constraints, designed only to affect very esoteric code. Typical risks apply otherwise (could introduce new bugs or not fully fix the existing bug).
String or UUID changes made by this patch:

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Needs landing for aurora/beta.

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd0363381c3b

Needed some syntactic changes to apply to mozilla-beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/2c15b22a9ed4