1373094 - Assertion failure: MIR instruction returned object with unexpected type, at js/src/jit/MacroAssembler.cpp:1695

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: windows assertion crash report @ js::jit::IonCompileScriptForBaseline with ebx = 0xdeadbeef

1. https://manishearth.github.io/rust-internals-docs/rustc/ty/adjustment/enum.Adjust.html?search=JJ

2. Assertion failure: MIR instruction returned object with unexpected type, at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jit/MacroAssembler.cpp:1695

Thread 0 (crashed)
 0  0x149ac691
    eip = 0x149ac691   esp = 0x006fd2ec   ebp = 0xffffff8c   ebx = 0xdeadbeef
    esi = 0x0c631378   edi = 0x1e9c3010   eax = 0x1e6a6ec0   ecx = 0x1e6b2670
    edx = 0x00000003   efl = 0x00200202
    Found by: given as instruction pointer in context
 1  xul.dll!js::jit::IonCompileScriptForBaseline(JSContext *,js::jit::BaselineFrame *,unsigned char *) [Ion.cpp:da66c4a05fda : 2705 + 0xc]
    eip = 0x54029c01   esp = 0x006fd2f8   ebp = 0x006fd318
    Found by: stack scanning

Even though this is an assert, I'm marking s-s since ebx = 0xdeadbeef.

reproduced with today's builds on Windows and Linux in Bughunter and also a local Linux build from 2017-06-07.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: Windows assertion crash report @ EnterBaseline

Assertion failure: MIR instruction returned object with unexpected type, at c:/builds/moz2_slave/m-cen-w64-d-000000000000000000/build/src/js/src/jit/MacroAssembler.cpp:1695

Linux debug builds also assert with this same stack and Windows 7 opt builds has also crashed with this stack at least once.

Comment 2

[Andrew McCreight [:mccr8]]

Bug 1368362 is hitting the same assertion. Maybe this is a dupe with better STR?

Comment 3

[Jan de Mooij [:jandem]]

I have a shell testcase, reducing it now (it's pretty huge because it contains the Rust documentation search index of > 1.5 MB). Fortunately we can discard most of it.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Shell test

This fails with --no-threads.

Comment 5

[Jan de Mooij [:jandem]]

Gary maybe you can run autoBisect on the attached shell test? It should repro with --no-threads.

Comment 6

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

I couldn't reproduce with the shell test in comment 4 using debug builds from m-c rev 464b2a3c25aa. What rev did you use, and what are the compilation/runtime parameters?

Comment 7

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

I managed to reproduce this with the testcase in comment 4 after all, with a build that does not have --enable-more-deterministic set.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ba5cee986655
user:        Jan de Mooij
date:        Wed May 31 09:43:16 2017 +0200
summary:     Bug 1368509 part 2 - Generalize Baseline unknown/unknownObject type update stubs. r=tcampbell

Jan, is bug 1368509 a likely regressor?

Comment 8

[Jan de Mooij [:jandem]]

Thanks Gary! That's very useful because these TI failures are not trivial to debug. I'll investigate soon.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

Blegh, in addUpdateStubForValue we look at obj->group() but that's not guaranteed to be the group we care about if obj is an unboxed expando object - see DoTypeUpdateFallback. The unboxed expando object has unknown properties, so we'd incorrectly attach an AnyValue stub.

We can just pass the group to addUpdateStubForValue and use it.

I'm going to hide in a corner now because I wrote DoTypeUpdateFallback and should have realized this.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Test + comment

Comment 11

[Ted Campbell [:tcampbell]]

Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

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

Ah dang, I also should have noticed this too when we had to fix the last issue of mismatch with DoTypeUpdateFallback.

Looks good now.

::: js/src/jit/SharedIC.cpp
@@ +2557,1 @@
>          AddTypePropertyId(cx, obj, id, val);

Is there any material difference between this and the variant that DoTypeUpdateFallback uses?

Comment 12

[Ted Campbell [:tcampbell]]

Comment on attachment 8880309 [details] [diff] [review]
Part 2 - Test + comment

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

Looks good. Testcase seems a little fragile, so many a comment to help someone who trips over it later.

::: js/src/jit-test/tests/baseline/bug1373094.js
@@ +7,5 @@
> +    for (var i = 0; i < 100; i++) {
> +        arr.push({x: i});
> +    }
> +    for (var i = 0; i < 1600; i++) {
> +        assertEq(inIon() === true, false);

I imagine this test sneaking up on someone in the future when our optimizers change. Can we have a comment about how to address this? (eg. remove assertion)

Comment 13

[Carsten Book [:Tomcat]]

seems http://static.echonest.com/autocanonizer/go.html?trid=TRJQDYU14404EDA900 is another live url for this assertion, will check this url after this bug here is fixed

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's not trivial but possible with some work.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
55+.

> If not all supported branches, which bug introduced the flaw?
Bug 1368509.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

sec-approval+.
If it is 55+ only, please nominate a patch for Beta as well.

Comment 16

[Carsten Book [:Tomcat]]

jan, still need to land and also need beta approval

Comment 17

[Jan de Mooij [:jandem]]

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

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Ted Campbell [:tcampbell] [pto until July 11] from comment #11)
> Is there any material difference between this and the variant that
> DoTypeUpdateFallback uses?

Not really but I think we have to do this here after the EnsureTrackPropertyTypes call.

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 8880306 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1368509.
[User impact if declined]: Security bugs, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Patch is pretty straight-forward.
[String changes made/needed]: None.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ebda0666d66f