Closed Bug 1247312 Opened 8 years ago Closed 8 years ago

crash in EnterBaseline

Categories

(Core :: JavaScript Engine: JIT, defect)

Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: snorp, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Naveed, can you get someone to look at this?
Flags: needinfo?(nihsanullah)
Jan can you please take a look
Flags: needinfo?(nihsanullah) → needinfo?(jdemooij)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Blocks: SadJit
Flags: needinfo?(jdemooij)
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.
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.
Flags: needinfo?(margaret.leibovic)
(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.
The JS team is working on this. Naveed/Jan would be the people to NI
Flags: needinfo?(margaret.leibovic)
I'll keep an eye on this.
Flags: needinfo?(jdemooij)
¡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
Those builds should have the fix from bug 1258320.
¡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
Flags: needinfo?(kbrosnan)
We're crashing with SIGILL (illegal instruction). Maybe we're using a newer instruction on older CPUs. 
I'll look into this now.
Flags: needinfo?(kbrosnan)
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8734321 - Flags: review?(nicolas.b.pierron)
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.
Attached patch PatchSplinter Review
Attachment #8734321 - Attachment is obsolete: true
Attachment #8734321 - Flags: review?(nicolas.b.pierron)
Attachment #8734327 - Flags: review?(nicolas.b.pierron)
Attachment #8734327 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/89303d46895d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.