1343541 - Suppress crash dialog for out-of-process decoder service crashes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

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

I see 'Nightly has stopped' dialogs in Nightly sometimes while playing video, and I'm fairly sure what's happening is that he decoder service crashed. We need to find a way to suppress those dialogs.

Comment 1

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

I saw 2 places in Android code that shows the dialog [1] [2]. Will see if we can work around it.

[1] https://android.googlesource.com/platform/frameworks/base/+/master/core/java/com/android/internal/os/RuntimeInit.java#115
[2] https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/am/NativeCrashListener.java#85

Comment 2

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

Following previous comment:
From what I can tell, [1] handles Java crash by assigning uncaught exception handler. Since it's possible to intercept exceptions by try/catch, this should be not a problem.

On the other hand, for native crashes [2] Android uses signal handler to collect and dump relative info and then send them through socket to activity manager, which shows the dialog box for the crashing process (except for 'persistent' processes). It's unlikely for Fennec to be persistent, so what I'm going to do next is refreshing my memory of UNIX signals and see how we can hijack them. Also, we might want to collect the coredump/tombstone by ourselves.

Comment 3

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

John, it should be enough to just use sigaction to make SEGV a noop. There are some examples of hooking up to that in mozglue here: https://dxr.mozilla.org/mozilla-central/source/mozglue/linker/ElfLoader.cpp#1099

In fact maybe we just want to use mozglue for this? Decoder process already uses it for shmem, right?

Comment 4

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

It looks like Android also shows crash dialog box for signals other than SEGV [1] so I guess we need to intercept those too.


[1] http://androidxref.com/7.1.1_r6/xref/bionic/linker/debugger.cpp#312

Comment 5

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

Crash reports [1] show the majority were ABRT(~60%) and SEGV(~40).

[1] https://crash-stats.mozilla.com/search/?signature=~stagefright&signature=~ACodec&product=FennecAndroid&date=%3E%3D2016-04-12T06%3A52%3A00.000Z&date=%3C2017-04-12T06%3A52%3A00.000Z&_sort=-version&_sort=reason&_facets=reason&_columns=signature&_columns=version&_columns=build_id&_columns=reason#facet-reason

Comment 6

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

Attachment: Bug 1343541 - restore signal actions changed by Android debugger.

Android shows a dialog box when it detects app crashing. OOP decoder needs to hide that from user to resume playback gracefully.

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

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

https://reviewboard.mozilla.org/r/129384/#review133252

::: mozglue/android/APKOpen.cpp:490
(Diff revision 1)
>  }
> +
> +extern "C" APKOPEN_EXPORT void MOZ_JNICALL
> +Java_org_mozilla_gecko_mozglue_GeckoLoader_suppressCrashDialog(JNIEnv *jenv, jclass jc)
> +{
> +  // Restore all singal actions changed by Android debugger.

Please assert that this doesn't run on Gecko processes. Please also fix the typos for signal here and in the commit message.

Comment 8

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

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129384/diff/1-2/

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

https://reviewboard.mozilla.org/r/129384/#review134060

::: mozglue/android/APKOpen.cpp:488
(Diff revision 2)
> +static bool
> +IsMediaProcess()

I guess that's good enough, but maybe we could instead check whether Gecko's crash reporter was initialized (which if that happened, would have called the getLibraryMapping function in this file.

::: mozglue/android/APKOpen.cpp:499
(Diff revision 2)
> +  FILE* f = fopen(str, "r");
> +  if (f) {
> +    fgets(str, sizeof(str), f);
> +    fclose(f);
> +    const size_t strLen = strlen(str);
> +    const char* suffix = ":media";

const char suffix[] = ...

::: mozglue/android/APKOpen.cpp:500
(Diff revision 2)
> +  if (f) {
> +    fgets(str, sizeof(str), f);
> +    fclose(f);
> +    const size_t strLen = strlen(str);
> +    const char* suffix = ":media";
> +    const size_t suffixLen = strlen(suffix);

sizeof(suffix) + 1

Comment 10

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

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

https://reviewboard.mozilla.org/r/129384/#review134060

> sizeof(suffix) + 1

I suppose you mean `sizeof(suffix) - 1`?

Comment 11

[Mike Hommey [:glandium]]

err, yeah.

Comment 12

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

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129384/diff/2-3/

Comment 13

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

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

https://reviewboard.mozilla.org/r/129384/#review134060

> I guess that's good enough, but maybe we could instead check whether Gecko's crash reporter was initialized (which if that happened, would have called the getLibraryMapping function in this file.

Thanks for the suggestion but I will keep this media process only for now and change the assertion if there is another process wants to suppress crash dialog in the future.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/103dceccaf84
restore signal actions changed by Android debugger. r=glandium

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/103dceccaf84

Comment 16

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

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: It shows 'Fennec has stopped' dialog when playing video.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes.
 1. Watch a video
 2. Open CMD or PowerShell use the command line "adb shell ps |FINDSTR :media” for CMD or "adb shell ps |sls :media” for PowerShell
    There will be one process that will be displayed. For example:
    u0_a89 11086 2410 993952 52964 ffffffff 00000000 S org.mozilla.firefox:media  
 3. Use commandline "adb shell kill -ABRT <PID>", where <PID> is the PID of above process (11086 in the example)
 4. There will a short pause (may show loading icon) then the video will resume.
    Before the fix, there will be a dialog box pop up saying "Firefox has stopped". After the fix you won't see the dialog. 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: it makes crashing OOP decoder process behave as default.
[String changes made/needed]: none

Comment 17

[Gerry Chang [:gchang]]

Hi Ioana, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 18

[Ioana Chiorean]

Yes, I will let Bogdan look over it as he did some work on this side.

Comment 19

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

 @John could this be verified without needing to root your device? Following the instructions you provided I can't use the kill command without su access. I would need to rood a device in order to verify this. Thank you.

Comment 20

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

(In reply to Bogdan Surd, QA [:bogdan] from comment #19)
> Hello,
> 
>  @John could this be verified without needing to root your device? Following
> the instructions you provided I can't use the kill command without su
> access. I would need to rood a device in order to verify this. Thank you.

Unfortunately super user access is needed to use the 'kill' command to send signal to other user's processes. So it can only be verified on rooted devices.

Comment 21

[Gerry Chang [:gchang]]

Hi :Bogdan,
Any thoughts on comment #20?

Comment 22

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

 Sorry for the late response, I've been out of office. Was and still am trying to root a device in order to verify this, I will give it another try this Thursday after we finish with this Beta release. If I am not able to successfully root a device we will not be able to properly verify this. I will get back to you this Thursday with a response.

Thank you for your patience.

Comment 23

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

 I was not able to rood a device but I have tried the steps in Comment 16 on a OnePlus One (Android 6.0.1) rooted device from a colleague of mine and I kept getting the "Operation not permitted" error as on a non rooted device. I did check the root access permissions for both apps and ADB in the developer option menu.

Comment 24

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

(In reply to Bogdan Surd, QA [:BogdanS] from comment #23)
> Hello,
> 
>  I was not able to rood a device but I have tried the steps in Comment 16 on
> a OnePlus One (Android 6.0.1) rooted device from a colleague of mine and I
> kept getting the "Operation not permitted" error as on a non rooted device.
> I did check the root access permissions for both apps and ADB in the
> developer option menu.

Hi Bodgan, what's the last char of 'adb shell' prompt? If it's '$', please try the following steps:
1. 'adb root'. It should say 'restarting adbd as root' or 'adbd is already running as root'
2. 'adb shell'. It should start a shell prompt
3. 'su' in the shell prompt. If the prompt changes to '#' then you can use the 'kill' command. If it says something like '/system/bin/sh: su: not found' or other error, then I'm sorry that you need another device. :(

Comment 25

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

 I am sorry to inform you that we are not able to verify this, we don't have any other device that is rooted and we were unable to properly root a device.

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8857388 [details]
Bug 1343541 - restore signal actions changed by Android debugger.

Suppress the crash dialog to avoid confusion. Beta 54+. Should be in 54 beta 6.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/71ee4cad7943

Comment 28

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

 I've managed to successfully root a device and verified this in both nightly and beta. After killing the process the video would stop and resume from where it left off. No other dialog box would open. 
 Sorry for not being able to verify this faster.