1898012 - Nimbus' JS code is shipped on toolkit (incl. Android, AFAICT) but messaging system + asrouter are not so there are some tricky dependencies

Status: NEW

(Firefox :: Nimbus Desktop Client, defect, P3)

Description

[:Gijs (he/him)]

Apologies for the messy title. We should morph this report as needed. Also this may not be the right component in the end.

For context, I'm trying to make FeatureGates use JEXL bits from Nimbus code for targeting. FeatureGate code is shipped on android and nominally feed into Troubleshoot.sys.mjs here - but then we promptly bail out here. However, after my changes some more of the code gets loaded and things sort of fell apart a bit. So then I investigated and found...

1. toolkit/components/nimbus/ is, AFAICT, shipped on android - cf. toolkit/components/moz.build, and this intriguing comment in nimbus' own moz.build. But, AIUI, Nimbus integration on android is primarily done via the rust component and not via this library, and JS access would need to use a JVM delegate mechanism instead of this code.
2. in practice that code depends on ASRouter here and here and on Messaging System code here and here.
3. but ASRouter lives in browser/ so is "obviously" not available on android
4. and Messaging System code I think is also not available on android, given my backout errors (xpcshell errors not being able to load modules) and this if condition.

I expect that we may want/need to do one or more of the following:

1. make toolkit/components/nimbus/ cope with ASRouter not being available (on non-desktop-Firefox environments like Thunderbird or Fenix or ...), or move the ASRouter targeting bits specifically into Nimbus or a sibling toolkit directory that is shipped in the same circumstances.
2. unship these bits of Nimbus on Android if we're relying solely on the rust component there (but somehow make sure they're in sync about what list of features/experiments/whatever are available, and that the jexl parser behaves the same etc.).
3. move the jexl/targetingcontext bits out of messaging-system such that they are available everywhere toolkit/components/nimbus is.
4. or perhaps instead, move all the toolkit nimbus code into browser/ if we don't think it should be available to toolkit/android/thunderbird/whatever. (unlikely, but want to explicitly call out this is also an option)

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:barret, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Beth Rennie [:beth] (she/her)]

Interestingly, we used to guard against exposing NimbusFeatures.* to android here:

https://searchfox.org/mozilla-central/rev/e42054f5c070191bd73023015576322df556fae8/toolkit/components/nimbus/ExperimentAPI.jsm#294

but then it was immediately removed because it broke thunderbird:

https://hg.mozilla.org/mozilla-central/rev/380d23bfec02eec3f1df9069b1e55bc67ffc8735

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to :Gijs (he/him) from comment #0)

[good times elided :-]
2. in practice that code depends on ASRouter here and here and on Messaging System code here and here.
3. but ASRouter lives in browser/ so is "obviously" not available on android

I'm not sure what the intent of the apparently sarcastic "obviously" is in this sentence. Can you clarify?

[options include]

2. unship these bits of Nimbus on Android if we're relying solely on the rust component there (but somehow make sure they're in sync about what list of features/experiments/whatever are available, and that the jexl parser behaves the same etc.).

The JEXL parsers do not behave the same, and in at least one relatively ugly way: operator precedence is broken in a way that's different from most (all?) other C-style languages.

Barret and I were just talking about this the other day. It's likely been true for the whole time we've been using two parsers.
The simple fix is easy. Unfortunately, it'll take more work to guarantee that we're not breaking any of our existing targeting uses (messaging system in-tree and remote, in live experiments & rollouts, and in experimenter code that generates targeting, users of FilterExpressions, maybe some things I haven't thought of).

As far as we're aware, we haven't yet been bitten by this, so Barret and I were speculating that it was perhaps more trouble to fix than it was worth, even though it is a footgun.

3. move the jexl/targetingcontext bits out of messaging-system such that they are available everywhere toolkit/components/nimbus is.

If we do this, maybe they want to go near FilterExpressions.sys.mjs (which currently lives in toolkit/components/utils).

Comment 4

[:Gijs (he/him)]

(In reply to Dan Mosedale (:dmosedale, :dmose) from comment #3)

(In reply to :Gijs (he/him) from comment #0)

[good times elided :-]
2. in practice that code depends on ASRouter here and here and on Messaging System code here and here.
3. but ASRouter lives in browser/ so is "obviously" not available on android

I'm not sure what the intent of the apparently sarcastic "obviously" is in this sentence. Can you clarify?

I'm sorry - no particular sarcasm intended - but if our source tree was really straightforward for how it is used today it would have 3 toplevel directories - desktop, android and shared (maybe even shared-also-perhaps-with-other-apps-that-arent-here-surprise, but I digress). But it's not (and of course there are many historical reasons for this) so it's the less obvious browser meaning desktop (that is, AFAIK literally nothing under browser/ is shipped on Android - even though what we ship on Android is also, in fact, a browser... - but even here I'm not 100% sure because the build system continues to surprise me). But to people familiar with our source tree the fact that browser really means Desktop is obvious. So I put obvious in quotes...

This is in contrast to some other source dirs (and mapped directories, so resource://messaging-sytem vs resource://app/ and resource:///, where the latter 2 will get you linter failures if used from toolkit, the former will not) where it's less clear where they are/aren't shipped. So how "obvious" it is to a given reader or linter that some file is/isn't reliably available from toolkit... varies.

[options include]

2. unship these bits of Nimbus on Android if we're relying solely on the rust component there (but somehow make sure they're in sync about what list of features/experiments/whatever are available, and that the jexl parser behaves the same etc.).

The JEXL parsers do not behave the same, and in at least one relatively ugly way: operator precedence is broken in a way that's different from most (all?) other C-style languages.

Argh.

Barret and I were just talking about this the other day. It's likely been true for the whole time we've been using two parsers.
The simple fix is easy. Unfortunately, it'll take more work to guarantee that we're not breaking any of our existing targeting uses (messaging system in-tree and remote, in live experiments & rollouts, and in experimenter code that generates targeting, users of FilterExpressions, maybe some things I haven't thought of).

As far as we're aware, we haven't yet been bitten by this, so Barret and I were speculating that it was perhaps more trouble to fix than it was worth, even though it is a footgun.

Do neither parser we use have some kind of AST representation that we could run all our expressions through to check if they're using both operators without any disambiguating parentheses?

But also, can you expand on how/when this matters wrt unshipping toolkit/components/nimbus/ on Android? The actual toolkit jexl impl lives in toolkit/components/utils/mozjexl.sys.mjs anyway, but I expect isn't used at all on Android (is that true?). So how does Android having different jexl bits relate to unshipping the toolkit/-y nimbus bits on Android? Perhaps they're just technically-orthogonal-but-related bits, e.g. if we wanted to make Android rely on the same shared jexl parser?

3. move the jexl/targetingcontext bits out of messaging-system such that they are available everywhere toolkit/components/nimbus is.

If we do this, maybe they want to go near FilterExpressions.sys.mjs (which currently lives in toolkit/components/utils).

That sounds reasonable - though perhaps it all wants its own directory at that point (rather than living in the somewhat cupboard-under-the-stairs-y motley collection that is a folder named utils 🙃)

Comment 5

[Beth Rennie [:beth] (she/her)]

mozjexl is used on Android (by way of FilterExpressions.sys.mjs) in remote-settings.sys.mjs. remote-settings.sys.mjs is used inside GeckoView for all gecko-related RS collections (whereas the rust client in application-services is used for application code in android-components and fenix)