1533051 - Teach Gecko to packages prefs to architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Nick Alexander :nalexander [he/him]]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1522581#c14, we have that greprefs.js is non-trivially different between armv7 and aarch64 now:

nalexander@roboto ~/M/gecko> diff -U3 /Users/nalexander/Mozilla/objdirs/objdir-droid/gradle/build/mobile/android/geckoview/fat_aar/armeabi-v7a/omnijar/greprefs.js /Users/nalexander/Mozilla/objdirs/objdir-droid/gradle/build/mobile/android/geckoview/fat_aar/arm64-v8a/omnijar/greprefs.js
--- /Users/nalexander/Mozilla/objdirs/objdir-droid/gradle/build/mobile/android/geckoview/fat_aar/armeabi-v7a/omnijar/greprefs.js	2019-03-05 12:35:23.000000000 -0800
+++ /Users/nalexander/Mozilla/objdirs/objdir-droid/gradle/build/mobile/android/geckoview/fat_aar/arm64-v8a/omnijar/greprefs.js	2019-03-05 12:35:24.000000000 -0800
@@ -1487,8 +1487,8 @@
 pref("javascript.options.baselinejit",      true);
 //Duplicated in JitOptions - ensure both match.
 pref("javascript.options.baselinejit.threshold", 10);
-//@line 1465 "$SRCDIR/modules/libpref/init/all.js"
-pref("javascript.options.ion",              true);
+//@line 1463 "$SRCDIR/modules/libpref/init/all.js"
+pref("javascript.options.ion",              false);
 //@line 1467 "$SRCDIR/modules/libpref/init/all.js"
 //Duplicated in JitOptions - ensure both match.
 pref("javascript.options.ion.threshold",    1000);

Such differences are only likely to increase as we invest more into tuning our Android engine settings across a range of target devices.

Somehow someway we need to have some (all?) of these settings be per architecture. This could look like:

1. moving greprefs to an architecture specific location in the omnijar and then gluing all the greprefs.js files into the fat AAR omnijar

2. adding functions to greprefs.js files that are determined at runtime (i.e, isAndroidAarch64())

or ... there are potentially other options. In https://bugzilla.mozilla.org/show_bug.cgi?id=1522581#c16 snorp is fine with option 1.

Comment 1

[Nick Alexander :nalexander [he/him]]

njn: glandium: here's the situation. Right now, we ship a single-architecture APK (Firefox for Android) and AAR (GeckoView library). Internally, it looks like:

/libs/$ARCH/libxul.so
/assets/omni.ja

The dependent bugs of this ticket will produce a multi-architecture AAR (GeckoView library) (but NOT a multi-architecture APK) that will look like:

/libs/$ARCH-1/libxul.so
/libs/$ARCH-2/libxul.so
...
/libs/$ARCH-N/libxul.so
/assets/omni.ja

The enclosing JVM code is not architecture-specific (i.e., tickets like Bug 1485045). So what we need is to make the greprefs.js files inside omni.ja architecture-specific. I see options 1. and 2. in #c0. Do you have thoughts on this? Is there prior art, perhaps with the universal mach binaries we used to produce?

Comment 2

[Mike Hommey [:glandium]]

Pref files with conditions are only a thing with autoconfig. That doesn't apply to greprefs.js, so option 2 is pretty much non-existent.

Comment 3

[Chris Peterson [:cpeterson]]

Not a Fenix release blocker so [geckoview:fenix:p2] like bug 1522581

Comment 5

[Nick Alexander :nalexander [he/him]]

I'm going to reopen this for packaging changes.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1533051 - Package GeckoView prefs to architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds. r?glandium

Bug 1533425 makes Gecko try to load from $ARCH/greprefs.js, etc on
Android. This patch teaches the packager to put GeckoView prefs into
those architecture-specific locations for that code to find.

I conditioned this on MOZ_GECKOVIEW_JAR to limit the scope, and
further limited to just greprefs.js and geckoview-prefs.js to try
to limit any interaction with l10n and/or repacking.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1533051 - Package greprefs.js and geckoview-prefs.js at architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds. r?glandium

Bug 1533425 makes Gecko try to load from $ARCH/greprefs.js, etc on
Android. This patch teaches the packager to put preferences into
those architecture-specific locations for that code to find.

Comment 8

[Nick Alexander :nalexander [he/him]]

Try build bubbling away at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4c2f59830e27ade233838b97712756df8433b05.

Locally, I see:

nalexander@roboto ~/M/gecko> diff -U3 before.txt after.txt
--- before.txt	2019-03-27 10:43:35.000000000 -0700
+++ after.txt	2019-03-27 10:40:23.000000000 -0700
@@ -4,7 +4,7 @@
        72  01-01-2010 00:00   chrome.manifest
      1974  01-01-2010 00:00   chrome/chrome.manifest
      3921  01-01-2010 00:00   components/components.manifest
-      311  01-01-2010 00:00   defaults/pref/mobile-l10n.js
+      311  01-01-2010 00:00   defaults/pref/armeabi-v7a/mobile-l10n.js
         6  01-01-2010 00:00   update.locale
      3074  01-01-2010 00:00   dictionaries/en-US.aff
    583748  01-01-2010 00:00   dictionaries/en-US.dic
@@ -1161,8 +1161,8 @@
      3781  01-01-2010 00:00   chrome/devtools/modules/devtools/shared/worker/helper.js
     18313  01-01-2010 00:00   chrome/devtools/modules/devtools/shared/worker/loader.js
      5883  01-01-2010 00:00   chrome/devtools/modules/devtools/shared/worker/worker.js
-    39139  01-01-2010 00:00   defaults/pref/geckoview-prefs.js
-   230502  01-01-2010 00:00   greprefs.js
+    39139  01-01-2010 00:00   defaults/pref/armeabi-v7a/geckoview-prefs.js
+   230502  01-01-2010 00:00   armeabi-v7a/greprefs.js
      6324  01-01-2010 00:00   defaults/autoconfig/prefcalls.js
      9002  01-01-2010 00:00   res/EditorOverride.css
      9410  01-01-2010 00:00   res/contenteditable.css
@@ -1296,7 +1296,7 @@
       316  01-01-2010 00:00   chrome/marionette/content/test_nested_iframe.xul
     16496  01-01-2010 00:00   chrome/marionette/content/transport.js
     18295  01-01-2010 00:00   components/marionette.js
-     1392  01-01-2010 00:00   defaults/pref/marionette.js
+     1392  01-01-2010 00:00   defaults/pref/armeabi-v7a/marionette.js
         6  01-01-2010 00:00   res/multilocale.txt
       367  01-01-2010 00:00   chrome/en-US/locale/branding/brand.dtd
        52  01-01-2010 00:00   chrome/en-US/locale/branding/brand.properties

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5117d2f9311
Package GeckoView prefs at architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds. r=glandium

Comment 10

[Chris Peterson [:cpeterson]]

Nick, do we need to uplift fat AARs to GV 67 Beta for Fenix MVP?

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Peterson [:cpeterson] from comment #10)

Nick, do we need to uplift fat AARs to GV 67 Beta for Fenix MVP?

If we want fat AARs for Fenix MVP, yes. However, they're not a blocker at all -- they're not user visible -- it will merely make the development process smoother. However, we've ironed out most of the kinks at this point anyway so not a pressing need.

I expect to uplift anyway, 'cuz it's relatively low risk.

Keeping the NI for me to circle back to the uplift after this bakes for a few days.

Comment 12

[Noemi Erli[:noemi_erli]]

Backed out changeset e5117d2f9311 (Bug 1533051) for Android xpcshell failures CLOSED TREE

Push that has the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=5e22dbefa98b9bd78c8a4fff69764b443ff6c74f&selectedJob=236492979

Failure examples:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236492978&repo=autoland&lineNumber=1362
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236486021&repo=autoland&lineNumber=1391
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236486026&repo=autoland&lineNumber=1391

Backout: https://hg.mozilla.org/integration/autoland/rev/17b8002251d00d6f0b4e167929c47cfaf25ef355

Comment 13

[Nick Alexander :nalexander [he/him]]

snorp: any thoughts on that xpcshell bustage? I will dig in tomorrow or the next day, but you're better at diagnosing native code crashes than I am.

Comment 14

[Mike Hommey [:glandium]]

Did you change the libpref code to read prefs in those new locations?

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #14)

Did you change the libpref code to read prefs in those new locations?

Yes: that was Bug 1533425. This worked locally and in "most" tests, but xpcshell is such a different vehicle that some additional change must be required.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander [he/him] from comment #13)

snorp: any thoughts on that xpcshell bustage? I will dig in tomorrow or the next day, but you're better at diagnosing native code crashes than I am.

Well, that's easy enough: at https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#4450 we don't accommodate arch-specific prefs.

snorp: clearly this was intentional. I don't care if we only make greprefs.js arch-specific but it doesn't handle mobile.js or geckoview-prefs.js, and those two seem very important for this use case.

Comment 17

[Chris Peterson [:cpeterson]]

(In reply to Nick Alexander :nalexander [he/him] from comment #11)

If we want fat AARs for Fenix MVP, yes. However, they're not a blocker at all -- they're not user visible -- it will merely make the development process smoother. However, we've ironed out most of the kinks at this point anyway so not a pressing need.

I expect to uplift anyway, 'cuz it's relatively low risk.

Setting 67=fix-optional because Fenix is not asking for this. No need to uplift this on their behalf. :)

Comment 18

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

Attachment: Bug 1533051 - Load architecture-specific versions of other default pref files on Android

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c9f52dddd1c
Load architecture-specific versions of other default pref files on Android r=njn

Comment 20

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5c9f52dddd1c

Comment 21

[Chris Peterson [:cpeterson]]

67=wontfix because there's no need to uplift a GV fix to 67 Beta.

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Pulsebot from comment #19)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c9f52dddd1c
Load architecture-specific versions of other default pref files on Android
r=njn

snorp: the xpcshell issue is almost certainly that you don't set MOZ_ANDROID_CPU_ABI for xpcshell (and you don't fail gracefully when it's unset). I will experiment with my pushes for Bug 1533051.

Comment 23

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

(In reply to Nick Alexander :nalexander [he/him] from comment #22)

(In reply to Pulsebot from comment #19)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c9f52dddd1c
Load architecture-specific versions of other default pref files on Android
r=njn

snorp: the xpcshell issue is almost certainly that you don't set MOZ_ANDROID_CPU_ABI for xpcshell (and you don't fail gracefully when it's unset). I will experiment with my pushes for Bug 1533051.

That seems plausible. I'll try to get back to it today.

Comment 24

[Nick Alexander :nalexander [he/him]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #23)

(In reply to Nick Alexander :nalexander [he/him] from comment #22)

(In reply to Pulsebot from comment #19)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c9f52dddd1c
Load architecture-specific versions of other default pref files on Android
r=njn

snorp: the xpcshell issue is almost certainly that you don't set MOZ_ANDROID_CPU_ABI for xpcshell (and you don't fail gracefully when it's unset). I will experiment with my pushes for Bug 1533051.

That seems plausible. I'll try to get back to it today.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f57ebdb0d572831682cceaf0806ac953c0d8d88 has a potential fix percolating away.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb2d189dbf6d
Package greprefs.js and geckoview-prefs.js at architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds. r=glandium

Comment 26

[Daniel Varga [:dvarga]]

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