Closed Bug 1263888 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 + verified
firefox-esr45 --- unaffected

People

(Reporter: decoder, Assigned: arai)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

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.
(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.
Keywords: sec-high
(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.
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?
Blocks: 1165052
Flags: needinfo?(arai.unmht)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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?
Flags: needinfo?(jdemooij)
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?
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
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.
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.
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
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
Assignee: nobody → arai.unmht
Flags: needinfo?(jdemooij)
Flags: needinfo?(arai.unmht)
Attachment #8740912 - Flags: review?(jdemooij)
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.
Attachment #8740912 - Flags: review?(jdemooij) → review+
(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.
note: bughunter also found this on real-live websites
https://hg.mozilla.org/mozilla-central/rev/6f330fed2314
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Recent regression, tracking in case it reopens.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.