mozilla-mobile / android-components

⚠️ This project moved to a new repository. It is now developed and maintained at: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
2.02k stars 474 forks source link

FxaDeviceConstellation.registerDeviceObserver: java.lang.IllegalStateException: Method addObserver must be called on the main thread #9972

Closed pocmo closed 3 years ago

pocmo commented 3 years ago

Using a newer Android X version, I see this startup crash. Looks like the enforceMainThreadIfNeeded() is new.

         AndroidRuntime  E  FATAL EXCEPTION: main
                         E  Process: org.mozilla.reference.browser.debug, PID: 16312
                         E  java.lang.IllegalStateException: Method addObserver must be called on the main thread
                         E      at androidx.lifecycle.LifecycleRegistry.enforceMainThreadIfNeeded(LifecycleRegistry.java:317)
                         E      at androidx.lifecycle.LifecycleRegistry.addObserver(LifecycleRegistry.java:172)
                         E      at mozilla.components.support.base.observer.ObserverRegistry.register(ObserverRegistry.kt:66)
                         E      at mozilla.components.service.fxa.FxaDeviceConstellation.registerDeviceObserver(FxaDeviceConstellation.kt:126)
                         E      at mozilla.components.feature.accounts.push.AccountObserver.onAuthenticated(FxaPushSupportFeature.kt:160)
                         E      at mozilla.components.service.fxa.manager.FxaAccountManager$accountStateSideEffects$6.invoke(FxaAccountManager.kt:535)
                         E      at mozilla.components.service.fxa.manager.FxaAccountManager$accountStateSideEffects$6.invoke(FxaAccountManager.kt:123)
                         E      at mozilla.components.support.base.observer.ObserverRegistry.notifyObservers(ObserverRegistry.kt:136)
                         E      at mozilla.components.service.fxa.manager.FxaAccountManager.notifyObservers(Unknown Source:7)
                         E      at mozilla.components.service.fxa.manager.FxaAccountManager.accountStateSideEffects(FxaAccountManager.kt:535)
                         E      at mozilla.components.service.fxa.manager.FxaAccountManager.stateActions(FxaAccountManager.kt:755)
                         E      at mozilla.components.service.fxa.manager.FxaAccountManager$processQueue$2.invokeSuspend(FxaAccountManager.kt:506)
                         E      at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
                         E      at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
                         E      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
                         E      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
                         E      at java.lang.Thread.run(Thread.java:923)

┆Issue is synchronized with this Jira Task

grigoryk commented 3 years ago

@pocmo what androidx.lifecycle version is this running against? I've tried 2.3.1 and 2.4.0-alpha01, both worked. We're currently on 2.2.0 in both a-c and Fenix.

grigoryk commented 3 years ago

From the commit that added this check:

RelNote: "LifecycleRegistry now verifies that its methods are called
on main thread. It was always a requirement for lifecycles of
activities, fragments etc. An addition of observers from non-main threads
resulted in hard to catch crashes in runtime. For LifecycleRegistry
objects that owned by your own components, you can explicitly opt out
from checks by using LifecycleRegistry.createUnsafe(...), but then you
have to ensure that a proper synchronization is in place when
this LifecycleRegistry is accessed from different threads"

So, in theory we could use LifecycleRegistry.createUnsafe for the deviceObserverRegistry in FxaDeviceConstellation as a quick upgrade path - it won't make things any worse. In this case their check isn't really about the main thread, but about being thread safe - and enforcing a single caller thread is one simple way to do that. I'll investigate a bit more. Never mind, I didn't read our ObserverRegistry code, which is what's actually used in FxaDeviceConstellation :) LifecycleRegistry in question here is owned by ProcessLifecycleOwner, so we can't change how it's initialized.

So the simplest approach is to see if just switching to the main thread for the registration in FxaPushSupportFeature is feasible.

A more useful approach that's still relatively quick (as it'll benefit other observers, and remove the existing foot-gun of "you'll be notified about account events on a random background thread") is to make sure we always notify our observers of account state on the main thread.

And a longer, perhaps-what-we-want-to-do-anyway-in-the-long-term approach is to start moving all this code toward using browser-state.

pocmo commented 3 years ago

@pocmo what androidx.lifecycle version is this running against? I've tried 2.3.1 and 2.4.0-alpha01, both worked. We're currently on 2.2.0 in both a-c and Fenix.

I was experimenting with Jetpack Compose in Reference Browser and it was updated transitively by using Jetpack Compose Beta versions. Using the dependencies command, I only see 2.3.+?

+--- androidx.lifecycle:lifecycle-runtime-ktx:2.3.1@aar
+--- androidx.lifecycle:lifecycle-viewmodel-ktx:2.3.1@aar
+--- androidx.lifecycle:lifecycle-livedata-ktx:2.2.0@aar
+--- androidx.lifecycle:lifecycle-livedata-core-ktx:2.2.0@aar
+--- androidx.lifecycle:lifecycle-extensions:2.2.0@aar
+--- androidx.lifecycle:lifecycle-viewmodel-savedstate:2.3.1@aar
+--- androidx.lifecycle:lifecycle-process:2.2.0@aar
+--- androidx.lifecycle:lifecycle-service:2.2.0@aar
+--- androidx.lifecycle:lifecycle-runtime:2.3.1@aar
+--- androidx.lifecycle:lifecycle-common-java8:2.0.0@jar
+--- androidx.lifecycle:lifecycle-viewmodel:2.3.1@aar
[..]

It would be nice if we could find a short-term solution just to unblock upgrading Android X. :)