Fixes #24823: remove recent synced tab when logged out

[MatthewTighe]

Depends on mozilla-mobile/android-components#12449

This adds SyncStore observation to the recent synced tab feature, in order to successfully determine when the Sync state becomes logged out. I'll have a follow-up PR immediately after this to address #25955. I initially had that issue addressed as part of this patch but wanted to slim it down as it includes quite a bit more refactoring.

To download an APK when reviewing a PR:

1. click on Show All Checks,
2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
3. click on the "Fenix - assemble" task, then click "Run Artifacts".
4. the APK links should be on the left side of the screen, named for each CPU architecture

[MozillaNoah]

looks good so far! I'll wait to officially approve it until the AC changes land

[MozillaNoah]

flowScoped is based off of MainScoped, so you might not need to keep the coroutine as a local variable in this instance, since it'll cancel the job in onDestroy, if that lifecycle works as a substitute for stop.

[MatthewTighe]

I actually ran into some trouble with these coroutines, and so did a fair bit of investigation that led to this implementation. I'm not sure it's necessarily the best or in style with other examples in the codebase, but I'll break some of it down and if you have suggestions or examples I'm happy to update it.

• Just as a point of clarity: MainScope is not tied to lifecycles at all. It just creates a new CoroutineScope on the main thread. This could even cause objects to survive longer than anticipated, as I think any running coroutines in the created scope would keep an object from being collected.

• flowScoped exposes an optional owner: LifecycleOwner parameter which can achieve the behavior you described in your comment. There's additional code in the Store.flow extension that it uses to cancel the flow when a lifecycle moves into the Destroyed state.

• However, RecentSyncedTabFeature is currently set using ViewBoundFeatureWrapper, which was in keeping with existing code in the HomeFragment. The whole purpose of ViewBoundFeatureWrapper is to keep a strong reference to objects across destructive view lifecycle events. This can prevent expensive operations like reloading a bunch of synced tabs. That said, the purpose of ViewBoundFeatureWrapper is the thing I understand least here so maybe it's not even necessary in this case.

• Normally in Fragments I've seen it suggested to use viewLifecycleOwner for automatic cancellation and re-collection of flows.

All of the above combines to mean that the normal lifecycle owner I'd use here for collecting these flows (viewLifecycleOwner) is destroyed but the Feature outlives it. The flow collection will stop as soon as view is destroyed and not started again. This way of manually handling the scope lifecycles prevents all that.

[MozillaNoah]

gotcha gotcha. I appreciate the thorough write-up for all of the context! I'm sure you've gone through several iterations of stuff with this overall effort.

[Mugurell]

Regarding the ViewBoundFeatureWrapper it was added in mozilla-mobile/android-components#1943 to fix leaks or even crashes from features that updates various Fragment Views and for that receive them as parameters.

It's a simple convenience to ensure that when the Fragment that creates a certain feature referencing a View is added to the backstack - and so the layout and Views are destroyed - the feature is also destroyed.

As a general rule of thumb if a feature needs a reference to a View then it should be wrapped in a ViewBoundFeatureWrapper.

[MatthewTighe]

@Mugurell thanks for the clarity around ViewBoundFeatureWrapper. RecentSyncedTabFeature doesn't need a reference to a view, so it should be safe to stop wrapping it based on your rule of thumb. I'm still not sure I fully understand the case where ViewBoundFeatureWrapper prevents leaks, however. Would it be a scenario like this?

class MyFragment : Fragment() {
    val feature = MyFeature()

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        feature.view = view
    }
}

So is ViewBoundFeatureWrapper basically equivalent to the following?

class MyFragment : Fragment() {
    val feature: MyFeature? = null

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        feature = MyFeature(view)
    }

    override fun onDestroyView() {
        feature = null
    }
}

I've added a separate commit that simplifies the feature's logic in handling the coroutine scopes. To my understanding these updates aren't at risk of creating leaks, but I would be interesting in getting a second opinion there 😂. I'll squash the second commit if it looks safe.

@MozillaNoah as an aside, the A-C patch has landed!

[Mugurell]

Yes. That's exactly the scenario.
When the fragment is put to backstack the Views are destroyed (more so for performance reasons, Views take a large amount of memory) but the fragment instance still exists.
If the fragment which got onDestroyView still references a feature that references a View then the fragment by the feature as proxy will leak it's View which should otherwise be destroyed.
The wrapper could be useful in other slightly more exotic scenarios like replacing/removing Views in a foreground fragment, not sure we're doing that but it's a nice safety net.

[Mugurell]

Seeing many features in HomeFragment being used inside of the said ViewBoundFeatureWrapper though they don't need a View and have injected a reference of the root layout for the required View argument.
While not optimal (based on the above) it should't cause any issues with them being initialized in onCreateView and then destroyed in onDestroyView (by the wrapper) (other than wasting a few CPU cycles and a bit of memory).
Them being a LifecycleAwareFeature that only works while the fragment is in foreground would probably be enough.

[MatthewTighe]

Gotcha, I appreciate the detailed walkthrough! Think I have a better grasp on all this now. Based on that, I think the simplified implementation I've added should be safe. It looks like viewLifecycleOwner is managed by the underlying Fragment in much the same way - it is reinitialized to a new instance in onCreateView.

[MozillaNoah]

can I get a TLDR with what SyncStoreSupport does, since it looks like it's on the AC side?

[MatthewTighe]

Sure!

The SyncStore in general is meant as an abstraction over the FxaAccountManager and some other Sync components. We had two options here:

• Dig into the internals of FxaAccountManager and dispatch store updates from there.
• Create a sidecar implementation that could follow the existing patterns of observation and dispatch store updates from there.

We went with the second because it seemed more easy, understandable, and extensible. That implementation is SyncStoreSupport

[MozillaNoah]

sounds good, thank you!

[MozillaNoah]

🚢 🚢 🚢