1323773 - SIGILL crash in mp4parse_capi::mp4parse_new

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-c0d43287-b39c-422c-8609-d63c52161215.
=============================================================

This is only on two installs on Nightly, but it happened a few times for each, all Android. The first crashes are on the 12-12 build.

Comment 1

[Andrew McCreight [:mccr8]]

Ralph, can you take a look? Thanks.

Comment 2

[Ralph Giles (:rillian)]

Both devices are Tegra 2 SoC, which is armv7 (Cortex-A9) without neon. I can't correlate the date with any mp4parse or rust toolchain updates; it may just been when these particular devices updated. We still support no-neon armv7 devices pending bug 1289569 and bug 1305815.

Rust is supposed to not be using neon for armv7; There were some regressions with this in 1.13/1.14 development. (See https://github.com/rust-lang/rust/pull/36933) Maybe they missed something? We moved android from rust 1.12.0 stable to 1.14.0-beta.2 on November 23.

I'll check the code in question and see if it's really a neon issue.

Comment 3

[Ralph Giles (:rillian)]

So it seems I need access to a minidump for easy verification, but I tried building the mp4parse_capi crate with rustc 1.14.0-beta.3 for armv7-linux-androideabi, and I did find neon instructions in the implementation of mp4parse_new:

 198:   e59d3028        ldr     r3, [sp, #40]   ; 0x28
 19c:   e283e058        add     lr, r3, #88     ; 0x58
 1a0:   e28dc068        add     ip, sp, #104    ; 0x68
 1a4:   f46c0acd        vld1.64 {d16-d17}, [ip]!
 1a8:   f44e0acf        vst1.64 {d16-d17}, [lr]
 1ac:   e283e068        add     lr, r3, #104    ; 0x68
 1b0:   f46c0acf        vld1.64 {d16-d17}, [ip]
 1b4:   f44e0acf        vst1.64 {d16-d17}, [lr]
 1b8:   e59dc064        ldr     ip, [sp, #100]  ; 0x64
 1bc:   e58dc150        str     ip, [sp, #336]  ; 0x150
 1c0:   ed9d0b17        vldr    d0, [sp, #92]   ; 0x5c
 1c4:   ed8d0b52        vstr    d0, [sp, #328]  ; 0x148
 1c8:   e28dcf56        add     ip, sp, #344    ; 0x158

Comment 4

[Ralph Giles (:rillian)]

I filed https://github.com/rust-lang/rust/issues/38402 on the rust side.

Comment 5

[Ralph Giles (:rillian)]

With jesup's help I looked at one of the minidumps, where the crash pointed to `lsrs r7, r1, #11` which is a perfectly valid instruction as far as I can tell. No neon in sight. So maybe the minidump is wrong, or maybe I'm missing something.

I was able to confirm rust is generating neon instructions for armv7-android. Upstream is amenable to fixing since google's docs say neon code must be gated on a runtime check. Proceeding with that avenue for now.

Comment 6

[Ralph Giles (:rillian)]

I couldn't reproduce the crash on GT-P7510 (Tegra 2 without neon, one of the two devices showing in the crash reports) with either nightly or my own debug build. Kevin, do you have a device which can reproduce this? Playing YouTube or any mp4 video with fennec nightly should trigger it the stacktrace is correct.

Comment 7

[Ralph Giles (:rillian)]

[Tracking Requested - why for this release]:

Nominating for tracking. Small number of crashes now, but it's nightly-only so could expand significantly as it moves to larger audiences. We should figure out how we want to address this before 53 gets to beta. Options are dropping support for these devices or fixing code generation.

Comment 8

[Marcia Knous [:marcia]]

Tracking 53+ for all the reasons in Comment 7.

Comment 9

[Tobias (:Tobbi) Markus]

I have a device I can reproduce this crash on 100% of the time. If there's anything you need, tell me.

Comment 10

[Ralph Giles (:rillian)]

Attachment: Bug 1323773 - Update android builders to rust 1.15.0-beta.2.

Fix code generation for armv7 devices without neon by switching
official builds to the 1.15 beta rust compiler. This fixes the
target generation (and standard library image) to not assume
neon is available, per Google's NDK guidelines.

We expect to update this to 1.15.0 stable before release.

Review commit: https://reviewboard.mozilla.org/r/102310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102310/

Comment 11

[Ralph Giles (:rillian)]

Tobias, could you please try the test build with the toolchain update and see if it fixes the crash? Much appreciated.

https://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-78c0929e8344d2ac7f9255b3f64a6b489e1d6ae2/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk

Comment 12

[Tobias (:Tobbi) Markus]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #11)
> Tobias, could you please try the test build with the toolchain update and
> see if it fixes the crash? Much appreciated.

It fixes the crash, however, videos won't play for the most part: I am getting a generic "Nightly was quit" message although the main process stays up. I have no way to discern which part crashed and no crash report is getting generated.

> https://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 78c0929e8344d2ac7f9255b3f64a6b489e1d6ae2/try-android-api-15/fennec-53.0a1.en-
> US.android-arm.apk

Comment 13

[Ralph Giles (:rillian)]

(In reply to Tobias (:Tobbi) Markus from comment #12)

> It fixes the crash, however, videos won't play for the most part: I am
> getting a generic "Nightly was quit" message although the main process stays
> up. I have no way to discern which part crashed and no crash report is
> getting generated.

One step forward, two steps back! Ok, thanks for testing. I'll try to investigate on my samsung tablet. In the meantime I've requested mochitests on top of the test build to see if there's any failure there.

Comment 14

[Tobias (:Tobbi) Markus]

If you tell me how, I can try to generate a stack trace. What I mean by quit is: The browser stays up and I can navigate normally, I still get the message though. I wonder whether that's some kind of non-main thread / process that's crashing itself.

Comment 15

[Ralph Giles (:rillian)]

If you run `adb logcat` on a machine connected to the device over usb, the stacktrace might show up there.

Comment 16

[Tobias (:Tobbi) Markus]

Here's the stacktrace(s):
http://pastebin.com/Qkwkzs4N

Comment 17

[Ralph Giles (:rillian)]

Looks like a different bug at least. What Android version is the device running?

Comment 18

[Tobias (:Tobbi) Markus]

(In reply to Ralph Giles (:rillian) | needinfo me from comment #17)
> Looks like a different bug at least. What Android version is the device
> running?

7.1.1

Comment 19

[John Lin [:jhlin][:jolin]]

(In reply to Tobias (:Tobbi) Markus from comment #16)
> Here's the stacktrace(s):
> http://pastebin.com/Qkwkzs4N

 This is bug 1326026.

Comment 20

[Ralph Giles (:rillian)]

(In reply to John Lin [:jolin][:jhlin] from comment #19)

>  This is bug 1326026.

Ah, thanks. Since the patch addresses the crash I'll go ahead and request review for this patch then.

Comment 21

[Michael Shal [:mshal]]

Comment on attachment 8823824 [details]
Bug 1323773 - Update android builders to rust 1.15.0-beta.2.

https://reviewboard.mozilla.org/r/102310/#review103594

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af17f4212529
Update android builders to rust 1.15.0-beta.2. r=mshal

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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