Replace unstructured coroutine scopes with applicationScope and lifecycle scopes - #4517
Open
rvandermeulen wants to merge 6 commits into
Open
Replace unstructured coroutine scopes with applicationScope and lifecycle scopes#4517rvandermeulen wants to merge 6 commits into
rvandermeulen wants to merge 6 commits into
Conversation
Bug 1990613 introduced an application-provided CoroutineScope in android-components, and BrowserApplication gained an applicationScope to satisfy it. Switch the remaining GlobalScope.launch call sites over to it, which also drops the @OptIn(DelicateCoroutinesApi::class) annotations they required: - BrowserApplication, two sites. The explicit Dispatchers.Main in restoreBrowserState is redundant because applicationScope already carries it. - CrashIntegration.sendCrashReport, which reaches the scope through context.components. Crash reports should outlive the observer's lifecycle, so the application scope is the appropriate one. BrowserFragment.deleteHistorySuggestion carried the same @OptIn but uses lifecycleScope, so that annotation was already dead and is simply removed. Removing the @OptIn changes sendCrashReport's detekt baseline ID, so its UndocumentedPublicFunction entry is updated to the new signature.
These call sites created a throwaway CoroutineScope or MainScope, launched into it and dropped the reference, leaving the job orphaned with no way to cancel it. That is the same defect as the GlobalScope usage, just spelled differently. All of the work involved is push and account handling that has to survive UI teardown, so the application scope is the correct one rather than any lifecycle scope. Services and both push integrations get it threaded in; BackgroundServices already had it as a constructor parameter.
AccountSettingsFragment launched six coroutines on inline CoroutineScope(Dispatchers.Main) instances that were never cancelled, while touching findPreference, getString and requireContext from inside them. The three driven by click and preference-change listeners now use viewLifecycleOwner.lifecycleScope, matching the call already present in this file. The three SyncStatusObserver callbacks use the fragment's own lifecycleScope instead, because they do not arrive on the main thread: FxaAccountManager defaults to a single-threaded background executor, and ObserverRegistry.notifyObservers invokes observers directly on the calling thread. Fragment.getViewLifecycleOwner is not safe to call from there and throws once the view is gone, whereas Lifecycle.coroutineScope is safe. In both cases Dispatchers.Main stays explicit to preserve the previous non-immediate dispatch. IntentReceiverActivity needed no coroutine at all: IntentProcessor.process is not a suspend function, so the MainScope().launch only served to defer the work by one main-loop pass while leaking an uncancellable job. Inline it.
Three classes held a long-lived CoroutineScope that was never cancelled. ToolbarIntegration launched a BrowserStore collector from init and left it running. Because the store is application-lived, that subscription retained the feature, and with it the fragment's view and context, well past view destruction. Move the collector into start() and cancel it in stop() so it follows the LifecycleAwareFeature contract. Cancelling the scope in stop() alone would not have worked, since nothing would restart the collector. InstalledAddonDetailsActivity and AddonsFragment each hand-rolled a CoroutineScope(Dispatchers.IO) with no cancellation. Both already have a lifecycle to hang off, so use lifecycleScope and viewLifecycleOwner's lifecycleScope respectively, keeping the IO dispatcher where the work is off the main thread. AddonsFragment uses the view lifecycle because its main-thread continuations touch the RecyclerView.
bindAddon in InstalledAddonDetailsActivity threw AddonManagerException from inside a nested lifecycleScope.launch(Dispatchers.Main), a separate coroutine from the try meant to handle it. The catch never saw it, so an add-on missing from the manager's list crashed instead of showing the failure toast. Collapse the nested launches into a single main-dispatched coroutine with withContext around the blocking getAddons call, which puts the throw and the catch in the same coroutine. Both this catch and the one in AddonsFragment also dropped the exception on the floor. Log it so a provider or storage failure leaves a trace beyond the generic toast.
AddonsFragment called bindRecyclerView from both onViewCreated and onStart. Since onStart always follows onViewCreated, every first display issued two concurrent getAddons calls and built two adapters racing to set recyclerView.adapter. Keep the onStart call, which also serves as the refresh after returning from the details activity, and drop the redundant one.
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the applicationScope introduced in #4515. That change added the scope to satisfy Bug 1990613's new android-components constructor parameters; this branch finishes the job by removing the remaining unstructured coroutine usage in the app.
Six commits, each independently buildable:
GlobalScope.launchsites inBrowserApplicationplusCrashIntegration.sendCrashReport, and a stale@OptIninBrowserFragment.CoroutineScope(...)/MainScope()launches acrossBackgroundServices,Services,PushFxaIntegrationandWebPushEngineIntegration. Same defect asGlobalScope, spelled differently.AccountSettingsFragment. The threeSyncStatusObservercallbacks use the fragmentlifecycleScoperather thanviewLifecycleOwnerbecause they arrive on theFxaAccountManagerthread, wheregetViewLifecycleOwneris unsafe; the commit message has the full trace.IntentReceiverActivity's coroutine is removed outright sinceIntentProcessor.processis not suspend.ToolbarIntegrationheld aBrowserStorecollector launched frominitand never cancelled, retaining the fragment view via the application-lived store.AddonsFragmentandInstalledAddonDetailsActivitymove to lifecycle scopes.InstalledAddonDetailsActivitythrew inside a nested coroutine its owncatchcould not see. Both add-on catch blocks now also log the exception.getAddons()on first display.Behavior note for reviewers: everything moved onto
applicationScopenow routes uncaught exceptions to itsCoroutineExceptionHandler(log) instead of crashing. This matches Fenix, including for crash report submission.Verified locally after
clean:compileDebugKotlin,ktfmtCheck,detekt,lintDebug,buildHealth,assembleDebug,assembleAndroidTest.