1209132 - Firefox 43 crash in arena_dalloc_small | je_free | js::jit::IonCompile

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-0d73699a-1b65-4b4c-9cea-749cc2150928.
=============================================================

Stack Trace:
0 		@0x250db051 	
1 	mozglue.dll 	arena_dalloc_small 	memory/mozjemalloc/jemalloc.c
2 	mozglue.dll 	je_free 	memory/mozjemalloc/jemalloc.c
3 	xul.dll 	js::jit::IonCompile 	js/src/jit/Ion.cpp
4 	xul.dll 	js::jit::Compile 	js/src/jit/Ion.cpp

This is the #1 crash since we opened updates to Firefox 43 Developer Edition. The stacks above is how this looks on Win7 and higher, the WinXP stacks look like this one from bp-88592e76-1a91-4fcc-b811-8403b2150928:

0 		@0x81a0f82 	
1 	mozglue.dll 	arena_dalloc_small 	memory/mozjemalloc/jemalloc.c
2 	mozglue.dll 	je_free 	memory/mozjemalloc/jemalloc.c
3 	xul.dll 	js::ScopedJSDeletePtr<js::LifoAlloc>::~ScopedJSDeletePtr<js::LifoAlloc>() 	
4 	xul.dll 	js::jit::IonCompile 	js/src/jit/Ion.cpp
5 	xul.dll 	js::jit::Compile 	js/src/jit/Ion.cpp

Comment 1

[Robert Kaiser]

I wonder if https://crash-stats.mozilla.com/report/list?signature=js%3A%3Ajit%3A%3APrepareOsrTempData is the same thing as well, as it's also EXCEPTION_ILLEGAL_INSTRUCTION crashes with JIT-related stuff on the stacks that end in bare hex addresses.

Comment 2

[(Away)]

The first dump I opened crashed on an instruction "haddpd  xmm0,xmm0"

The xmm register, combined with EXCEPTION_ILLEGAL_INSTRUCTION, plus all these old single-core CPUs, suggests that we're failing to test for SSE support before using it:

Rank 	Cpu info 	Count 	%
1 	GenuineIntel family 15 model 2 stepping 9 | 1 	333 	30.00 %
2 	GenuineIntel family 6 model 13 stepping 8 | 1 	280 	25.23 %
3 	GenuineIntel family 15 model 2 stepping 7 | 1 	162 	14.59 %
4 	GenuineIntel family 15 model 2 stepping 4 | 1 	118 	10.63 %

The code surrounding EIP goes like:

250db043 b8802ce052      mov     eax,offset xul!TO_DOUBLE (52e02c80)
250db048 660f6200        punpckldq xmm0,xmmword ptr [eax]
250db04c 660f5c4010      subpd   xmm0,xmmword ptr [eax+10h]
250db051 660f7cc0        haddpd  xmm0,xmm0
250db055 b868634053      mov     eax,offset xul!RNG_DSCALE_INV (53406368)
250db05a f20f5900        mulsd   xmm0,mmword ptr [eax]
250db05e f20f100db0b30d25 movsd   xmm1,mmword ptr ds:[250DB3B0h]
250db066 f20f59c1        mulsd   xmm0,xmm1

Which looks like CodeGenerator::visitRandom. At first glance I'm going to blame bug 774364, which landed in 43. :arai, does that code need to check for SSE support?

Comment 3

[Tooru Fujisawa [:arai]]

Thank you for pointing that out.  subpd needs SSE2 and haddpd needs SSE3.  We should check them at the beginning of the MacroAssemblerX86::convertUInt64ToDouble, and fallback to the way MacroAssemblerARMCompat::convertUInt64ToDouble does if SSE2 not present.
I'll prepare patch shortly.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Check for SSE3 in MacroAssemblerX86::convertUInt64ToDouble.

Added HasSSE3() in MacroAssemblerX86::convertUInt64ToDouble (HasSSE3()==true implies HasSSE2()==true, right?),
and when HasSSE3()==false, it fallbacks to MacroAssemblerARMCompat::convertUInt64ToDouble's way. it's a bit less performant but works well (bug 774364 comment #31)

Comment 5

[Pulsebot]

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

Comment 6

[(Away)]

Flagging for tracking because this will need to be uplifted to aurora.

[Tracking Requested - why for this release]: Regression in 43, JIT crashes on older CPUs

Comment 7

[(Away)]

And thank you for the fast fix!

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #3)
> haddpd needs SSE3. 

Can we also change this HasSSE2() assert to HasSSE3():

    void vhaddpd(FloatRegister src, FloatRegister dest) {
        MOZ_ASSERT(HasSSE2());

That way jit-tests or fuzzing with --no-sse3 would have caught it :)

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f4ab722222c

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora) Check for SSE3 in MacroAssemblerX86::convertUInt64ToDouble. r=sstangl,jandem

Merged part 2 (only assertion fix).

Approval Request Comment
[Feature/regressing bug #]: bug 774364
[User impact if declined]: Crash with EXCEPTION_ILLEGAL_INSTRUCTION by opening certain webpage on non-SSE2/SSE3 CPU, inside JS JIT code
[Describe test coverage new/current, TreeHerder]: The newly added code path is tested in automated test (basic/math-random.js with --no-sse3 option in jit-test):
  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ef04c494e2ed
[Risks and why]:
* Very low for currently not-crashing environment. This patch changes almost nothing.
* Low for the currently crashing environment.  This patch changes Math.random implementation to a little less-performant way, that is also tested on automated test.  Math.random might be used in some security-related thing on some website.
[String/UUID change made/needed]: None

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8667213 [details] [diff] [review]
(mozilla-aurora) Check for SSE3 in MacroAssemblerX86::convertUInt64ToDouble. r=sstangl,jandem

Tests look ok, crash fix, approved for uplift to aurora.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/3f4ab722222c

Comment 14

[Carsten Book [:Tomcat]]

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