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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 29d5a4175c8b (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

Object.extend = function(destination, source) {
    for (property in source) destination[property] = []
}
Enumerable = {
    constructor(method) {}
}
Object.extend(Array.prototype, Enumerable)
formatter = new Intl.NumberFormat
if (formatter.format);



Backtrace:

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007ffff2d69470 in ?? ()
#0  0x00007ffff2d69470 in ?? ()
[...]
rax	0x7ffff3402810	140737274456080
rbx	0x2	2
rcx	0x7ffff7e93c70	140737352645744
rdx	0x7ffff6908878	140737330055288
rsi	0x7ffff6908800	140737330055168
rdi	0x7fffffff6e90	140737488318096
rbp	0x2	2
rsp	0x7fffffff6f28	140737488318248
r8	0x2	2
r9	0x7fffffff6b80	140737488317312
r10	0x7fffffff6bd0	140737488317392
r11	0x7ffff6c27ee0	140737333329632
r12	0x8	8
r13	0x7fffffff7ac8	140737488321224
r14	0x1	1
r15	0x0	0
rip	0x7ffff2d69470	140737267537008
=> 0x7ffff2d69470:	push   %r10
   0x7ffff2d69472:	push   %r9

Marking s-s because I don't know if this assert is security-related or not.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #0)
> Marking s-s because I don't know if this assert is security-related or not.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> (In reply to Christian Holler (:decoder) from comment #0)
> > Marking s-s because I don't know if this assert is security-related or not.

Oops, forgot to comment. It's definitely security-related.

Let's wait for JSBugMon/autoBisect first.

Comment 3

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/830dc8e6e370
user:        Tooru Fujisawa
date:        Sat Mar 05 18:57:53 2016 +0900
summary:     Bug 1165052 - Part 11: Use ArraySpeciesCreate in Array.prototype.slice. r=efaust

arai-san, is bug 1165052 a likely regressor?

Comment 4

[Tooru Fujisawa [:arai]]

Reduced the testcase to following, so it should be a regression from bug 1165052.

Array.prototype.constructor = [];
for (let i = 0; i < 100; i++)
    [].slice();

But I have no idea what's going on there,
jandem, can you help me?

Comment 5

[Tooru Fujisawa [:arai]]

the assertion failure disappears when I remove |setResultTypeSet(obj->resultTypeSet());| there:

>     MArraySlice(CompilerConstraintList* constraints, MDefinition* obj,
>                 MDefinition* begin, MDefinition* end,
>                 JSObject* templateObj, gc::InitialHeap initialHeap, JSValueType unboxedType)
>       : MTernaryInstruction(obj, begin, end),
>         templateObj_(templateObj),
>         initialHeap_(initialHeap),
>         unboxedType_(unboxedType)
>     {
>         setResultType(MIRType_Object);
>         setResultTypeSet(obj->resultTypeSet());
>     }

iiuc, it depends on that js::array_slice returns an object that is allocated with NewFullyAllocatedArrayTryReuseGroup or NewPartlyAllocatedArrayTryReuseGroup, but in newly added path, it allocates an object with std_Array or custom function that is the species of |this| obj.
So we should remove the setResultTypeSet call there.

Is this the reason of the assertion failure?

Comment 6

[Tooru Fujisawa [:arai]]

Just removing setResultTypeSet regresses several Kraken benchmarks:

stanford-crypto-pbkdf2: 108 => 123
stanford-crypto-ccm: 96.7 => 114.8
stanford-crypto-aes: 59.9 => 233.4

Kraken: http://bnjbvr.github.io/simplegraph/#title=Kraken%20result%20comparison&categories=audio-fft,stanford-crypto-pbkdf2,audio-beat-detection,stanford-crypto-ccm,imaging-darkroom,json-parse-financial,audio-oscillator,ai-astar,audio-dft,stanford-crypto-sha256-iterative,json-stringify-tinderbox,imaging-gaussian-blur,stanford-crypto-aes,imaging-desaturate&values=before%2052.8%20108%20100.6%2096.7%2090.1%2039.8%2070.7%2074%20124.4%2045%2043.6%2071.7%2059.9%2060.2%0Aafter%2051.9%20123%2098.3%20114.8%2088.9%2038.9%2069.8%2073%20122.8%2049.1%2042.8%2070.4%20233.4%2058.7

SunSpider: http://bnjbvr.github.io/simplegraph/#title=SunSpider%20result%20comparison&categories=math-spectral-norm,bitops-3bit-bits-in-byte,date-format-xparb,3d-morph,controlflow-recursive,string-base64,3d-raytrace,access-nsieve,crypto-md5,bitops-nsieve-bits,access-binary-trees,bitops-bitwise-and,string-validate-input,access-fannkuch,string-fasta,regexp-dna,math-partial-sums,access-nbody,string-tagcloud,date-format-tofte,string-unpack-code,crypto-aes,crypto-sha1,bitops-bits-in-byte,math-cordic,3d-cube&values=before%202%201.1%2013.1%205.2%202.3%205.6%2013.1%203.1%204.4%203.6%202.9%202%209.9%205.8%206.8%2015.3%206.8%202.8%2023%2018.2%2064.5%2012.4%204%202.4%202.4%2013.3%0Aafter%201.9%201%2013.1%205.1%202.5%205.7%2013.4%202.9%204.3%203.7%202.7%202.1%209.2%205.7%206.9%2015.2%206.8%202.8%2022.5%2018%2063.3%2011.9%203.7%201.8%202.5%2013.2

Octane: http://bnjbvr.github.io/simplegraph/#title=Octane%20result%20comparison&categories=Richards,DeltaBlue,Crypto,RayTrace,EarleyBoyer,RegExp,Splay,SplayLatency,NavierStokes,PdfJS,Mandreel,MandreelLatency,Gameboy,CodeLoad,Box2D,zlib,Typescript,Score%20(version%209)&values=before%2028486%2057786%2027362%20104634%2030283%203260%2015852%2017457%2034426%2011768%2028084%2030588%2045830%2015478%2049327%2074115%2027389%2027556%0Aafter%2028872%2058858%2027504%20103709%2029767%203321%2015892%2017675%2034759%2011876%2028482%2031671%2045938%2015463%2048643%2074213%2027613%2027752

Comment 7

[Tooru Fujisawa [:arai]]

Is there any way to bailout from callVM, or after callVM ?
If so, we could keep the setResultTypeSet for MArraySlice and bailout when IsArraySpecies returns false.

Comment 8

[Tooru Fujisawa [:arai]]

Other option would be, partially self-host Array#slice and implement non-default @@species case in JS code,
so that other cases can use similar JIT code as now, keeping setResultTypeSet.
now I'm trying to implement it to see the performance difference.

Comment 9

[Tooru Fujisawa [:arai]]

With WIP patch for self-hosting Array#slice non-default path, there are 15-30% regression in Kraken,
it's better than comment #6, but still not good...

stanford-crypto-pbkdf2: 99.6 => 115.4
stanford-crypto-ccm:    89.7 => 112.4

http://bnjbvr.github.io/simplegraph/#title=Kraken%20result%20comparison&categories=audio-fft,stanford-crypto-pbkdf2,audio-beat-detection,stanford-crypto-ccm,imaging-darkroom,json-parse-financial,audio-oscillator,ai-astar,audio-dft,stanford-crypto-sha256-iterative,json-stringify-tinderbox,imaging-gaussian-blur,stanford-crypto-aes,imaging-desaturate&values=before%2049.3%2099.6%2092%2089.7%2082.6%2036.4%2065.3%2068%20113.5%2041.2%2039.5%2065.6%2053.9%2054.5%0Aafter%2048%20115.4%2089.8%20112.4%2081.5%2036.1%2064.8%2067.8%20111.6%2045.8%2040.2%2064.7%2056.9%2053.5

http://bnjbvr.github.io/simplegraph/#title=SunSpider%20result%20comparison&categories=math-spectral-norm,bitops-3bit-bits-in-byte,date-format-xparb,3d-morph,controlflow-recursive,string-base64,3d-raytrace,access-nsieve,crypto-md5,bitops-nsieve-bits,access-binary-trees,bitops-bitwise-and,string-validate-input,access-fannkuch,string-fasta,regexp-dna,math-partial-sums,access-nbody,string-tagcloud,date-format-tofte,string-unpack-code,crypto-aes,crypto-sha1,bitops-bits-in-byte,math-cordic,3d-cube&values=before%201.8%202.2%2012.4%206.2%202.1%205.4%2013.7%204.3%204.2%203.4%203.9%201.9%209.3%205.4%207.3%2015.6%206.3%202.7%2022%2017.2%2061.1%2012%204.3%201.6%202.9%2012.5%0Aafter%201.7%201%2012.7%204.8%202.1%205.3%2013%202.7%204.3%203.3%202.5%201.9%208.6%205.3%206.5%2014.4%206.4%202.6%2021.8%2016.4%2059.3%2011.3%203.4%201.7%202.3%2012.5

http://bnjbvr.github.io/simplegraph/#title=Octane%20result%20comparison&categories=Richards,DeltaBlue,Crypto,RayTrace,EarleyBoyer,RegExp,Splay,SplayLatency,NavierStokes,PdfJS,Mandreel,MandreelLatency,Gameboy,CodeLoad,Box2D,zlib,Typescript,Score%20(version%209)&values=before%2031383%2063905%2030177%20113163%2032659%203575%2018279%2018887%2037833%2014181%2030679%2035281%2051317%2016535%2056538%2080772%2029554%2030577%0Aafter%2031537%2064219%2030393%20114402%2032310%203714%2018791%2018894%2037712%2013814%2030517%2033168%2052922%2016534%2056374%2080687%2029743%2030612

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Push TypeBarrier after ArraySlice.

Thank you for your help, jandem!

Added pushTypeBarrier with obj->resultTypeSet() instead of setResultTypeSet(obj->resultTypeSet()), it fixed the issue and it doesn't show any notable regression.

Kraken: http://bnjbvr.github.io/simplegraph/#title=Kraken%20result%20comparison&categories=audio-fft,stanford-crypto-pbkdf2,audio-beat-detection,stanford-crypto-ccm,imaging-darkroom,json-parse-financial,audio-oscillator,ai-astar,audio-dft,stanford-crypto-sha256-iterative,json-stringify-tinderbox,imaging-gaussian-blur,stanford-crypto-aes,imaging-desaturate&values=before%2048.2%2099%2091.1%2088.4%2081.7%2036.3%2064.9%2067.7%20111.7%2040.9%2039.3%2065.3%2054.4%2053.7%0Aafter%2048%2097.9%2089.6%2087.6%2081.4%2035.9%2064.8%2067%20111.5%2040.7%2039.7%2064.8%2053.9%2053.7

SunSpider: http://bnjbvr.github.io/simplegraph/#title=SunSpider%20result%20comparison&categories=math-spectral-norm,bitops-3bit-bits-in-byte,date-format-xparb,3d-morph,controlflow-recursive,string-base64,3d-raytrace,access-nsieve,crypto-md5,bitops-nsieve-bits,access-binary-trees,bitops-bitwise-and,string-validate-input,access-fannkuch,string-fasta,regexp-dna,math-partial-sums,access-nbody,string-tagcloud,date-format-tofte,string-unpack-code,crypto-aes,crypto-sha1,bitops-bits-in-byte,math-cordic,3d-cube&values=before%201.9%201.3%2012.9%204.9%202.1%205.4%2011.9%202.9%204.2%203.3%202.7%201.9%208.6%205.3%206.6%2014.7%206.2%202.6%2021.6%2016.8%2059.5%2011.3%203.6%201.7%202.7%2012.3%0Aafter%201.8%201%2012.2%204.8%202.1%205.4%2013.2%202.6%204.3%203.3%202.6%201.9%208.6%205.3%206.2%2014.6%206.3%202.7%2021.1%2016.8%2059%2011.4%203.5%201.6%202.2%2012.4

Octane: http://bnjbvr.github.io/simplegraph/#title=Octane%20result%20comparison&categories=Richards,DeltaBlue,Crypto,RayTrace,EarleyBoyer,RegExp,Splay,SplayLatency,NavierStokes,PdfJS,Mandreel,MandreelLatency,Gameboy,CodeLoad,Box2D,zlib,Typescript,Score%20(version%209)&values=before%2031558%2064253%2030466%20111942%2032548%203542%2018509%2020762%2037443%2014008%2030535%2032816%2049278%2016267%2056275%2080804%2030204%2030536%0Aafter%2031616%2064131%2030326%20114957%2032680%203781%2019249%2019649%2037702%2014008%2030698%2032226%2052359%2016541%2056466%2081200%2030216%2030791

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8740912 [details] [diff] [review]
Push TypeBarrier after ArraySlice.

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

::: js/src/jit/MCallOptimize.cpp
@@ +890,5 @@
>  
>      if (!resumeAfter(ins))
>          return InliningStatus_Error;
> +
> +    if (!pushTypeBarrier(ins, obj->resultTypeSet(), BarrierKind::TypeSet))

We should use getInlineReturnTypeSet() instead of obj->resultTypeSet(). That way we don't bail out unnecessarily in the unusual case.

Comment 12

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f330fed23146feb54337db0ea94780be788bfc2
Bug 1263888 - Push TypeBarrier after ArraySlice. r=jandem

Comment 13

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> Is there any way to bailout from callVM, or after callVM ?
> If so, we could keep the setResultTypeSet for MArraySlice and bailout when
> IsArraySpecies returns false.

callVM are effectful, so you cannot bailout to any resume point which is before the callVM.  The only way to do bailout after a callVM is to invalidate the code.  As invalidations are extremely costly we should ensure that we do not recompile to the same thing again to avoid invalidation loops.

Comment 14

[Carsten Book [:Tomcat]]

note: bughunter also found this on real-live websites

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f330fed2314

Comment 16

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 17

[Liz Henry (:lizzard)]

Recent regression, tracking in case it reopens.