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)
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)
2.27 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
(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
Comment 2•8 years ago
|
||
(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.
tracking-firefox48:
--- → ?
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?
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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?
Assignee | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f330fed23146feb54337db0ea94780be788bfc2 Bug 1263888 - Push TypeBarrier after ArraySlice. r=jandem
Comment 13•8 years ago
|
||
(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•8 years ago
|
||
note: bughunter also found this on real-live websites
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6f330fed2314
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 16•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•