Closed Bug 1222925 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 45
Tracking Flags:
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(1 file)

MozReview Request: Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman
40 bytes, text/x-review-board-request
rnewman
: review+
ritu
: approval-mozilla-beta+
Details 
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.
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]
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.
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.
(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
Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r?rnewman
Attachment #8685444 - Flags: review?(rnewman)
I'm looking for someone with an x86 device to test the patch.. :)
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(kbrosnan)
(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.
(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.
Flags: needinfo?(mark.finkle)
Attachment #8685444 - Flags: review?(rnewman) → review+
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?)
(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)
Flags: needinfo?(michael.l.comella)
(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.
Flags: needinfo?(michael.l.comella)
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 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.
Attachment #8685444 - Flags: feedback?(rnewman)
(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...?
Flags: needinfo?(michael.l.comella)
Depends on: 1224286
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.
Attachment #8685444 - Flags: feedback?(rnewman)
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 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.
Attachment #8685444 - Flags: review+ → review?(rnewman)
No longer depends on: 1224286
Attachment #8685444 - Flags: review?(rnewman) → review+
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.
https://hg.mozilla.org/integration/fx-team/rev/c6e983faaffdd9bde8a00cd3f27fdb858fe9cb3c
Bug 1222925 - Escape hatch improvement: Detect ABI mismatches (ARM vs. x86). r=rnewman
https://hg.mozilla.org/mozilla-central/rev/c6e983faaffd
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Flags: needinfo?(kbrosnan)
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.
Flags: needinfo?(mark.finkle)
Flags: needinfo?(kbrosnan)
I don't see any need to uplift this. If someone else does, remember that we probably need to uplift bug 1119915 too.
Flags: needinfo?(mark.finkle)
Bug 1119915 has been uplifted. Should this be uplifted now too?
(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?
Flags: needinfo?(kbrosnan) → needinfo?(s.kaspari)
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]: -
Flags: needinfo?(s.kaspari)
Attachment #8685444 - Flags: approval-mozilla-beta?
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.
Attachment #8685444 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: