Closed Bug 1323773 Opened 3 years ago Closed 3 years ago

SIGILL crash in mp4parse_capi::mp4parse_new

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + fixed

People

(Reporter: mccr8, Assigned: rillian)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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.
Ralph, can you take a look? Thanks.
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles)
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
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.
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.
Flags: needinfo?(kbrosnan)
Assignee: nobody → giles
[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.
Tracking 53+ for all the reasons in Comment 7.
I have a device I can reproduce this crash on 100% of the time. If there's anything you need, tell me.
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/rgiles@mozilla.com-78c0929e8344d2ac7f9255b3f64a6b489e1d6ae2/try-android-api-15/fennec-53.0a1.en-US.android-arm.apk
Flags: needinfo?(tobbi.bugs)
(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/rgiles@mozilla.com-
> 78c0929e8344d2ac7f9255b3f64a6b489e1d6ae2/try-android-api-15/fennec-53.0a1.en-
> US.android-arm.apk
Flags: needinfo?(tobbi.bugs)
(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.
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.
If you run `adb logcat` on a machine connected to the device over usb, the stacktrace might show up there.
Here's the stacktrace(s):
http://pastebin.com/Qkwkzs4N
Looks like a different bug at least. What Android version is the device running?
(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
(In reply to Tobias (:Tobbi) Markus from comment #16)
> Here's the stacktrace(s):
> http://pastebin.com/Qkwkzs4N

 This is bug 1326026.
(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.
Flags: needinfo?(kbrosnan)
Attachment #8823824 - Flags: review?(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
Attachment #8823824 - Flags: review?(mshal) → review+
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af17f4212529
Update android builders to rust 1.15.0-beta.2. r=mshal
https://hg.mozilla.org/mozilla-central/rev/af17f4212529
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1332759
You need to log in before you can comment on or make changes to this bug.