2012763 - Consider updating NimbusGeckoPrefHandler architecture and follow-ups

Status: RESOLVED FIXED

(Firefox for Android :: Experimentation and Telemetry, task, P1)

Description

[Olivia Hall [:olivia]]

:jonalmeida and I discussed some potential architecture changes for how NimbusGeckoPrefHandler is situated. It might be better for it to be a part of browser-engine-gecko. This bug is to follow-up on if that is a possible and desirable change.

Comment 1

[Olivia Hall [:olivia]]

It might also be necessary to change the abstraction layer from Gecko calls to AC in NimbusGeckoPrefHandler because the observer is likely going to require AC abstractions in bug 2006095. The mixing and matching of abstractions feels like it adds architecture debt.

Comment 2

[Olivia Hall [:olivia]]

This bug might be for concept-engine. Leaving the same title until we do more investigation.

Comment 3

[Olivia Hall [:olivia]]

Also, should look to try and increase testing coverage for this project.

Comment 4

[Olivia Hall [:olivia]]

Attachment: Bug 2012763 - Part 1: Add BrowserPrefType to AC

When refactoring NimbusGeckoPrefHandler, it became more clear having
the explicit Gecko preference type names would be helpful.

The original implementation expected being able to infer the type from the
T type of value. However, this seems brittle due to multiple sources of types
and the fact that T should only ever be one of three values or is else invalid.

Additionally, coordinating between the Kotlin types that can come from Java or Rust also
increased the questionability of how accurate the T type might be when used on
a when statement when attempting to convert into a SetGeckoPreference.

This patch passes the information along on the BrowserPreference object
to be more explicit of the preference type expectations.

Comment 5

[Olivia Hall [:olivia]]

Attachment: Bug 2012763 - Part 2: Refactor NimbusGeckoPrefHandler

This patch:

• Adds experiments/prefhandling directory and moves associated files
• Moves helper/extension functions from NimbusGeckoPrefHandler to NimbusGeckoPrefHandlerExtensions
• Refactors GeckoView method calls to Android Components calls and adds engine dependency
• Some documentation/refactoring to consolidate and name throwables
• Additional test coverage

Comment 6

[Olivia Hall [:olivia]]

Attachment: Bug 2012763 - Part 4: Add constructor to NimbusGeckoPrefHandler

This patch:

• Converts NimbusGeckoPrefHandlerTest away from a singleton to a
  constructed element that takes an engine an nimbusApi.
• Reorganizes how observer registration occurs by adding a start function.

Comment 7

[Olivia Hall [:olivia]]

Attachment: Bug 2012763 - Part 3: Move files to folder

This patch:

• Adds experiments/prefhandling directory and moves associated files

Comment 8

[Olivia Hall [:olivia]]

Attachment: Bug 2012763 - Part 5: Fix issue when preference type isn't known

There is a bug where preference types sometimes haven't been fetched when setting.
Knowing the types is required because Nimbus sends all preferences over as strings.

This patch adds a check in these cases and will fetch the type information,
if it is missing.

Comment 9

[Aaron Train [:aaronmt]]

Just to catch this early, on the Try at (https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=N1jd89nsTlSioYtz8JR1ZQ.0&searchStr=ui-&revision=b77bc47fae0945307556a9e7b9951ecffd7b6c4b) I see some shards in the ui-test-arm jobs crashing in loop with

Matrix: link

java.lang.IllegalThreadStateException: Expected thread 2 ("main"), but running on thread 68 ("DefaultDispatcher-worker-1")
     FATAL EXCEPTION: DefaultDispatcher-worker-1
Process: org.mozilla.fenix.debug:mozilla.components.lib.crash.CrashHandler, PID: 8623
java.lang.IllegalThreadStateException: Expected thread 2 ("main"), but running on thread 68 ("DefaultDispatcher-worker-1")
	at org.mozilla.gecko.util.ThreadUtils.assertOnThreadComparison(ThreadUtils.java:141)
	at org.mozilla.gecko.util.ThreadUtils.assertOnThread(ThreadUtils.java:107)
	at org.mozilla.gecko.util.ThreadUtils.assertOnUiThread(ThreadUtils.java:84)
	at org.mozilla.geckoview.GeckoRuntime.create(GeckoRuntime.java:652)
	at org.mozilla.fenix.gecko.GeckoProvider.createRuntime(GeckoProvider.kt:61)
	at org.mozilla.fenix.gecko.GeckoProvider.getOrCreateRuntime(GeckoProvider.kt:40)
	at org.mozilla.fenix.components.Core.geckoRuntime_delegate$lambda$0(Core.kt:278)
	at org.mozilla.fenix.components.Core.$r8$lambda$VDoFJR2vLCdc212UPXEn2LofB-k(Unknown Source:0)
	at org.mozilla.fenix.components.Core$$ExternalSyntheticLambda28.invoke(D8$$SyntheticClass:0)
	at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:86)
	at org.mozilla.fenix.components.Core.getGeckoRuntime(Core.kt:277)
	at org.mozilla.fenix.components.Core.engine_delegate$lambda$0(Core.kt:248)
	at org.mozilla.fenix.components.Core.$r8$lambda$w-bBz4uVQbTj_b2pNcVVXsc48Ao(Unknown Source:0)
	at org.mozilla.fenix.components.Core$$ExternalSyntheticLambda22.invoke(D8$$SyntheticClass:0)
	at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:86)
	at org.mozilla.fenix.components.Core.getEngine(Core.kt:158)
	at org.mozilla.fenix.components.Components.nimbus_delegate$lambda$0$0(Components.kt:231)
	at org.mozilla.fenix.components.Components.$r8$lambda$pbObrpLwg0ATx4D7F8bKQZyGIsY(Unknown Source:0)
	at org.mozilla.fenix.components.Components$$ExternalSyntheticLambda34.invoke(D8$$SyntheticClass:0)
	at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:86)
	at org.mozilla.fenix.components.NimbusComponents.geckoPrefHandler_delegate$lambda$0(NimbusComponents.kt:48)
	at org.mozilla.fenix.components.NimbusComponents.$r8$lambda$e9HlduSNysmcdey82pGBh8S5DeY(Unknown Source:0)
	at org.mozilla.fenix.components.NimbusComponents$$ExternalSyntheticLambda2.invoke(D8$$SyntheticClass:0)

Which bubbles down to

	at org.mozilla.fenix.crashes.NimbusExperimentDataProvider.getExperimentData(NimbusExperimentDataProvider.kt:23)
	at mozilla.components.lib.crash.runtimetagproviders.ExperimentDataRuntimeTagProvider.invoke(ExperimentDataRuntimeTagProvider.kt:62)
	at mozilla.components.lib.crash.CrashReporter.getRuntimeTags(CrashReporter.kt:154)
	at mozilla.components.lib.crash.CrashReporter.onCrash$lib_crash_debug(CrashReporter.kt:312)
	at mozilla.components.lib.crash.handler.ExceptionHandler.uncaughtException(ExceptionHandler.kt:44)
	at java.lang.ThreadGroup.uncaughtException(ThreadGroup.java:1071)
	at java.lang.ThreadGroup.uncaughtException(ThreadGroup.java:1066)
	at kotlinx.coroutines.internal.CoroutineExceptionHandlerImplKt.propagateExceptionFinalResort(CoroutineExceptionHandlerImpl.kt:31)

Comment 10

[Olivia Hall [:olivia]]

Thanks, yeah! I had noticed they were orange and was working through it today.

I had a T test failure earlier that was truly an intermittent, so I was front-loaded into thinking this one was too since it was green on the second attempt. I had opened up for review while I investigated.

Comment 11

[Olivia Hall [:olivia]]

Attachment: Bug 2012763 - Part 6: Remove Nimbus FML and Improve Null States

• This patch removes the existing test Nimbus FML due to bug 2020683
• Improves null case handling from (!!) to silently continuing as it is
  unrecoverable

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/049fdb2ca9d4
https://hg.mozilla.org/integration/autoland/rev/a99aeff12b2d
Part 1: Add BrowserPrefType to AC r=android-reviewers,nalexander
https://github.com/mozilla-firefox/firefox/commit/60788b78057e
https://hg.mozilla.org/integration/autoland/rev/73699d3e0ee5
Part 2: Refactor NimbusGeckoPrefHandler r=android-reviewers,jonalmeida
https://github.com/mozilla-firefox/firefox/commit/e284fd6c86dc
https://hg.mozilla.org/integration/autoland/rev/709235d2018d
Part 3: Move files to folder r=android-reviewers,jonalmeida
https://github.com/mozilla-firefox/firefox/commit/29bb48a97996
https://hg.mozilla.org/integration/autoland/rev/b9b35e2659d9
Part 4: Add constructor to NimbusGeckoPrefHandler r=android-reviewers,jonalmeida
https://github.com/mozilla-firefox/firefox/commit/53a04c23462b
https://hg.mozilla.org/integration/autoland/rev/dbee7da95bae
Part 5: Fix issue when preference type isn't known r=android-reviewers,jonalmeida
https://github.com/mozilla-firefox/firefox/commit/bdf0a130169b
https://hg.mozilla.org/integration/autoland/rev/e93eace047ad
Part 6: Remove Nimbus FML and Improve Null States r=android-reviewers,jonalmeida

Comment 13

[Silaghi Andreea]

https://hg.mozilla.org/mozilla-central/rev/a99aeff12b2d
https://hg.mozilla.org/mozilla-central/rev/73699d3e0ee5
https://hg.mozilla.org/mozilla-central/rev/709235d2018d
https://hg.mozilla.org/mozilla-central/rev/b9b35e2659d9
https://hg.mozilla.org/mozilla-central/rev/dbee7da95bae
https://hg.mozilla.org/mozilla-central/rev/e93eace047ad

Comment 14

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

Attachment: Bug 2012763 - Part 6: Remove Nimbus FML

• This patch removes the existing test Nimbus FML due to bug 2020683

Comment 15

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: No user impact.

Has CI impact as we are ingesting the FML files from central/beta/release into experimenter and this one has become invalid due to the feature having both "gecko-pref" and "default" as of application-services 150.

• Code covered by automated testing?: no
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: low
• Explanation of risk level: Configuration-only change. Feature is not used.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 16

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/679d1a07b92e
https://hg.mozilla.org/releases/mozilla-release/rev/fa94e161cee3

Comment 17

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/1248f54bbb52
https://hg.mozilla.org/releases/mozilla-release/rev/e6b52736c539

Comment 18

[Pascal Chevrel (relman team) -> :pascalc]

Reverted on release for causing bug 2027633

Comment 19

[Olivia Hall [:olivia]]

Removed test and fml over in bug 2027633.

Comment 20

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9557420 [details]
Bug 2012763 - Part 6: Remove Nimbus FML

This can ride the 150 train.