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

RESOLVED FIXED in Firefox 68

Status

defect
P1
normal
RESOLVED FIXED
3 months ago
6 days ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

(Whiteboard: [geckoview:fenix:p2])

Attachments

(2 attachments, 1 obsolete attachment)

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.

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?

Flags: needinfo?(n.nethercote)
Flags: needinfo?(mh+mozilla)

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.

Flags: needinfo?(mh+mozilla)
Flags: needinfo?(n.nethercote)

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

OS: All → Android
Priority: -- → P1
Whiteboard: [geckoview:fenix:p2]
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1533425

I'm going to reopen this for packaging changes.

Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Teach Gecko to load prefs from architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds → Teach Gecko to packages prefs to architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds
Depends on: 1533425

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.

Assignee: nobody → nalexander
Status: REOPENED → ASSIGNED

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.

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
Attachment #9051445 - Attachment is obsolete: true

Comment 9

2 months ago
Pushed by nalexander@mozilla.com:
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

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

Flags: needinfo?(nalexander)

(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.

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.

Flags: needinfo?(nalexander) → needinfo?(snorp)

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

(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.

(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.

(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 19

21 days ago
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c9f52dddd1c
Load architecture-specific versions of other default pref files on Android r=njn
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago20 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

(In reply to Pulsebot from comment #19)

Pushed by jwillcox@mozilla.com:
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.

Flags: needinfo?(snorp)

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

(In reply to Pulsebot from comment #19)

Pushed by jwillcox@mozilla.com:
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.

Flags: needinfo?(snorp)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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 jwillcox@mozilla.com:
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.

Attachment #9053755 - Attachment description: Bug 1533051 - Package GeckoView prefs at architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds. r?glandium → Bug 1533051 - Package greprefs.js and geckoview-prefs.js at architecture-specific paths for Android fat AAR/GeckoView multi-architecture builds. r?glandium
You need to log in before you can comment on or make changes to this bug.