1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1119915 we added a toast that is shown when the user installs an APK with a wrong API range (For example: API 9 APK on Android 5.0). After/while showing the toast we close the application to avoid crashes.

Additionally we want to check the CPU ABI (ARM, x86) to check if this is the correct build.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

More from bug 1119915 comment 34:

> So let's broaden the question: can we measure how often this situation is hit?
> 
> I think we could: assume that users who care (which are the ones we want to measure!) will click through
> a notification, and let's make sure it does something useful _and_ measurable --
> 
> * Load a page in the stock browser. We can measure that with regular web analytics.
> * Load something else (e.g., Google Play) but either go via a bouncer or send some other kind of ping.

Currently we can't use Gecko's telemetry uploader because we do not start Gecko in this situation and even if we did this build of Gecko might not run on this device (CPU ABI / bug 1222538).

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Two things:

* I thought about using AppConstants.MOZ_APP_ABI for that. But it seems to be always set to "arm-eabi-gcc3", even for x86 builds.

* I actually can't install an x86 APK on an ARM device. So I wonder if we even need this check here:

> 0:00.64 make: Entering directory `/Users/sebastian/Projects/Mozilla/fx-team/objdir-droid-x86'
> 0:06.88 7021 KB/s (44674788 bytes in 6.213s)
> 0:07.98 	pkg: /data/local/tmp/fennec-45.0a1.en-US.android-i386.apk
>  0:11.70 Failure [INSTALL_FAILED_NO_MATCHING_ABIS]

Comment 3

[Robert Kaiser]

FWIW, we had those cases where someone linked an ARM APK on a forum and people with x86 devices would install it from there (and then crash on launch) - the toast would hopefully be helpful for cases like that.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

nalexander mentioned in the frontend meeting that (some?) x86 devices try to run arm APKs in a compatibility/emulation and therefore allow to install ARM apks. So we definitely need this.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> * I thought about using AppConstants.MOZ_APP_ABI for that. But it seems to
> be always set to "arm-eabi-gcc3", even for x86 builds.

It seems like this only happened because I've been using disable-compile-environment:

> if not CONFIG['COMPILE_ENVIRONMENT']:
>     # These should really come from the included binaries, but that's not easy.
>     DEFINES['MOZ_APP_ABI'] = 'arm-eabi-gcc3' # Observe quote differences here ...
>     DEFINES['TARGET_XPCOM_ABI'] = '"arm-eabi-gcc3"' # ... and here.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#901-904

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

I'm looking for someone with an x86 device to test the patch.. :)

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Fennec ARM build with x86 escape hatch (for testing):
https://drive.google.com/file/d/0B08p-IjMu0FWQmhzWUhTMkRhNVk/view?usp=sharing

Comment 9

[Richard Newman [:rnewman]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> nalexander mentioned in the frontend meeting that (some?) x86 devices try to
> run arm APKs in a compatibility/emulation and therefore allow to install ARM
> apks. So we definitely need this.

Yup:

http://developer.android.com/reference/android/os/Build.html#SUPPORTED_ABIS

contains both x86 and ARM indicators for these devices.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> Fennec ARM build with x86 escape hatch (for testing):
> https://drive.google.com/file/d/0B08p-IjMu0FWQmhzWUhTMkRhNVk/view?usp=sharing

When I install this APK on my two x86 devices, I am shown the toast:
1. Motorola RAZRi
2. ASUS x86 7" tablet

The toast might fade away too quickly for me to read and understand the issue, but the condition is caught.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

https://reviewboard.mozilla.org/r/24785/#review22407

This looks fine to me, modulo my fear about ARMv8.

::: mobile/android/base/GeckoApp.java:2261
(Diff revision 1)
> +        boolean isSystemARMv7 = "armeabi-v7a".equals(Build.CPU_ABI);

Might want a pointer to

http://developer.android.com/ndk/guides/abis.html

here.

I looked that up because I was concerned about what ARMv8 would appear as. I am still concerned: presumably an ARMv8 device has

  CPU_ABI = "arm64-v8a"
  SUPPORTED_ABIS = ["arm64-v8a", "armeabi-v7a"]
  
and we don't handle that correctly because of our use of Build.CPU_ABI, which is deprecated.

Can we test this on an ARMv8 device (I think the HTC Desire 510 is one?)

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(Finkle got this in comment 10. For the future, I set up STF w/ an x86 device and I'll write up something about how to use it)

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #11)
> I looked that up because I was concerned about what ARMv8 would appear as. I
> am still concerned: presumably an ARMv8 device has
> 
>   CPU_ABI = "arm64-v8a"
>   SUPPORTED_ABIS = ["arm64-v8a", "armeabi-v7a"]

Good point. The current implementation would not do any harm because we'd just ignore this configuration. I think it would be safe to just check the prefix of the primary ABI (arm*, x86*)?. We should have the same problem with x86_64, right?

> and we don't handle that correctly because of our use of Build.CPU_ABI,
> which is deprecated.

I was afraid that those x86 devices with ARM emulation might list ARM in SUPPORTED_ABIS? This might require some testing. Looking at the current implementation Build.CPU_ABI seems to be set to SUPPORTED_32_BIT_ABIS[0] or SUPPORTED_64_BIT_ABIS[0] on API 21+:

https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/os/Build.java#124


> Can we test this on an ARMv8 device (I think the HTC Desire 510 is one?)

Michael, do we have this, or any other ARMv8 (basically a 64 bit ARM?) device in SF? I tried to find a list of devices with ARMv8 but somehow I can't find any.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

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

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

This updated patch just checks the ABI prefix. Additionally I added a log message for unknown app/system ABI combinations.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #13)
> > Can we test this on an ARMv8 device (I think the HTC Desire 510 is one?)
> 
> Michael, do we have this, or any other ARMv8 (basically a 64 bit ARM?)
> device in SF? I tried to find a list of devices with ARMv8 but somehow I
> can't find any.

This is the first I'm hearing of armv8 so I doubt it. :P

I found a list of devices below the tables on this page:
  https://en.wikipedia.org/wiki/List_of_devices_using_Mediatek_SoCs#ARMv8_Quad_Core

Should I order one and throw it up on STF (though I guess it's still unproven), or do you want to order one, or...?

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

I'm updating the patch with the suggestions from bug 1224286.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

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

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

Re-requesting review after several changes.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

https://reviewboard.mozilla.org/r/24785/#review22895

::: mobile/android/base/AppConstants.java.in:356
(Diff revision 3)
> +    public static final String ANDROID_CPU_ARCH = "@ANDROID_CPU_ARCH@";

It'd be nice to include some example values here in a comment.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/c6e983faaffdd9bde8a00cd3f27fdb858fe9cb3c
Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r=rnewman

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

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

Mark, Kevin: While looking at the top crashers on 44.0b2, I came upon bug 1196299 and wondering whether this patch needs to be uplifted to Beta44. Or do you prefer to wait and let this ride the trains? I know that the alert will not fix the startup crash but this has been a top crasher for a few releases now so does it help if we release this in FF44. Just a thought! Please let me know.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't see any need to uplift this. If someone else does, remember that we probably need to uplift bug 1119915 too.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Bug 1119915 has been uplifted. Should this be uplifted now too?

Comment 26

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #25)
> Bug 1119915 has been uplifted. Should this be uplifted now too?

Yes, I think given that we uplifted bug 1119915 we should uplift this, too. Sorry I missed that.

Can you request uplift?

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

Approval Request Comment

[Feature/regressing bug #]: Follow-up uplift from bug 1119915. This will actually make the crash go away we see currently increasingly (Wrong ABI APK installed on device).

[User impact if declined]: App crashes on startup without explanation if wrong APK is installed manually.

[Describe test coverage new/current, TreeHerder]: Manual testing only - no automated tests.

[Risks and why]: Low risk. This is in 45/Aurora already without issues.

[String/UUID change made/needed]: -

Comment 28

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

Comment on attachment 8685444 [details]
MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman

Improves Fennec44 stability, taking it.

Comment 29

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

https://hg.mozilla.org/releases/mozilla-beta/rev/5893c5df7b84

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5893c5df7b84