1247312 - crash in EnterBaseline

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

This bug was filed from the Socorro interface and is 
report bp-98997022-97d0-4c77-b637-3c1c72160206.
=============================================================

This is the #9 top crash on 45.0b3 on Android

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Naveed, can you get someone to look at this?

Comment 2

[Naveed Ihsanullah [:naveed]]

Jan can you please take a look

Comment 3

[Jan de Mooij [:jandem]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #0)
> This is the #9 top crash on 45.0b3 on Android

We've always had crashes in JIT code. These are hard to track down, because it's a combination of many bugs (memory corruption elsewhere, bugs in the VM/Gecko/JIT, etc.)

Since Firefox 46, all JIT code is marked non-writable when we're not explicitly patching it. That eliminated the "other code is accidentally corrupting JIT code" crashes. I no longer see EnterBaseline in the top 20 for Fennec 46 and 47 (not sure if that's due to the non-writable JIT code change, but it's possible).

If you notice any reproducible crashes (or spikes) we can investigate them, but there's not a lot we can do for Firefox 45, and the situation looks better for 46 and 47.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

46 and 47 have such small populations (~1000 users) that that sort of analysis may not be valid vs beta's 100k and release in the millions. The users on Aurora and Nightly channels are self selecting and tend to have very modern hardware.

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Margaret, the Nightly Fennec crash rate is at 12.23 %. The maximum on the stability range is 9 so we are well over the limit. It has been trending up steeply since March 19th. https://crash-stats.mozilla.com/home/products/FennecAndroid/versions/48.0a1

Could you please help find an owner for this issue? This is the topmost ranked crash on Fennec 48.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Ritu Kothari (:ritu) from comment #5)
> It has been trending up steeply since March 19th.
> https://crash-stats.mozilla.com/home/products/FennecAndroid/versions/48.0a1

Bug 1248289 landed around that time and introduced a crash spike on desktop as well (bug 1258432). The fix for that one is bug 1258320, let's see if that fixes things.

Comment 7

[Kevin Brosnan [Ex-Mozilla]]

The JS team is working on this. Naveed/Jan would be the people to NI

Comment 8

[Jan de Mooij [:jandem]]

I'll keep an eye on this.

Comment 9

[alex_mayorga]

¡Hola!

Loading facebook.com instance crashes Nightly on Kindle Fire.

¡Gracias!

Submitted Crash Reports
Report ID 	Date Submitted
bp-12376043-340e-4a7f-b790-d0d282160323
	03/23/16	16:29
bp-b0c9d9ba-f9ec-4ef2-b114-1c3172160323
	03/23/16	16:28

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

Those builds should have the fix from bug 1258320.

Comment 11

[alex_mayorga]

¡Hola Kevin!

Shall I file a new bug then?

facebook.com and m.facebook.com always crash =(

¡Gracias!

Report ID 	Date Submitted
bp-47b33ad4-946a-470d-8451-bbdc82160323
	03/23/16	16:43
bp-af9b624c-e743-4b49-b502-4a4e62160323
	03/23/16	16:38
bp-12376043-340e-4a7f-b790-d0d282160323
	03/23/16	16:29

Comment 12

[Jan de Mooij [:jandem]]

We're crashing with SIGILL (illegal instruction). Maybe we're using a newer instruction on older CPUs. 
I'll look into this now.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Patch

All these SIGILL reports are crashing at an LDRD instruction, like this:

  ldrd (%r1,%r3), %r2

It's loading the Value at r1 + r3 into (r2, r3).

It turns out that using r3 as both offset register and destination register invokes undefined behavior. From the ARM manual:

  if t2 == 15 || m == 15 || m == t || m == t2 then UNPREDICTABLE;

Here m is the offset register and t/t2 are the result registers.

The Baseline CacheIR code is likely the first place where we can use loadValue like this.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8734321 [details] [diff] [review]
Patch

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

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +1092,5 @@
>  {
>      MOZ_ASSERT((rt.code() & 1) == 0);
>      MOZ_ASSERT(rt2.value.code() == rt.code() + 1);
> +    MOZ_ASSERT(addr.offsetRegister() != rt);
> +    MOZ_ASSERT(addr.offsetRegister() != rt2);

I wanted to add a comment here but forgot. Added it locally.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89303d46895d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89303d46895d