Remove main RC checks in BrowserTabFragment/VM #7095

CrisBarreiro · 2025-11-10T15:13:40Z

Task/Issue URL: https://app.asana.com/1/137249556945/project/488551667048375/task/1211897095283266?focus=true

Description

Steps to test this PR

Update override fun isEnabled(): Boolean in FeatureToggles adding this snippet at the beginning of the method

        val threadName = Thread.currentThread().name
        if (threadName == "main" || threadName == "Main") {
            val stackTrace = Thread.currentThread().stackTrace
                .drop(1) // Skip getStackTrace itself
                .take(10) // Limit to first 10 frames to avoid too much output
                .joinToString("\n  ") { "at ${it.className}.${it.methodName}(${it.fileName}:${it.lineNumber})" }
            System.err.println("FeatureToggles: isEnabled() called on main thread for feature: ${featureName()}\nCall stack:\n  $stackTrace")
        }

Feature 1

Smoke test app

Feature 2

UI changes

n/a

CrisBarreiro · 2025-11-10T15:13:58Z

Remove main RC checks in BrowserTabFragment/VM #7095 👈 (View in Graphite)
develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

CrisBarreiro · 2025-11-10T15:56:51Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

+            configureNavigationBar()
+            configureOmnibar()
+
+            configureObservers()


We need to call configureObservers after configuring the views as, otherwise, the app would crash because webView, popupMenu, omnibar, etc. aren't initialized yet. It used to work because we were blocking the main thread, and therefore no values were emitted before they could be processed

Copilot

Pull Request Overview

This PR removes main thread checks from BrowserTabFragment and BrowserTabViewModel by moving Remote Config (feature toggle) checks off the main thread using coroutines and dispatchers.

Wraps Remote Config isEnabled() calls in withContext(dispatchers.io()) to execute them on background threads
Updates CustomHeadersProvider.getCustomHeaders() to be a suspend function that runs on IO dispatcher
Refactors initialization code in BrowserTabFragment.onActivityCreated() to run within a coroutine scope

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
CustomHeadersProvider.kt	Changed `getCustomHeaders()` to suspend function with IO dispatcher execution
OmnibarLayoutViewModel.kt	Moved feature toggle check for `serpEasterEggLogosToggles` to IO thread within coroutine
OmnibarLayout.kt	Wrapped feature toggle check in coroutine with IO dispatcher
LegacyOmnibarLayout.kt	Wrapped feature toggle check in coroutine with IO dispatcher
BrowserTabViewModel.kt	Added IO dispatcher wrapping for multiple feature toggle checks and updated calls to `getUrlHeaders()`
BrowserTabFragment.kt	Wrapped initialization logic in coroutine and updated multiple methods to suspend functions
BrowserTabViewModelTest.kt	Updated test fake to match new suspend function signature

Copilot · 2025-11-12T12:00:55Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

+        lifecycleScope.launch {
+            omnibar = Omnibar(


Wrapping the entire onActivityCreated initialization in lifecycleScope.launch introduces a race condition. The function returns immediately before initialization completes, but observers and other components may expect the view to be fully configured. Consider using runBlocking for critical initialization or restructuring to ensure proper sequencing.

Copilot · 2025-11-12T12:00:55Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

+            configureNavigationBar()
+            configureOmnibar()
+
+            configureObservers()


Moving configureObservers() after the omnibar and webview configuration changes the initialization order. Observers should be configured before other setup to ensure they can respond to state changes during initialization. This was previously called earlier in the sequence.

Copilot · 2025-11-12T12:00:55Z

app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt

+        viewModelScope.launch {
+            if (withContext(dispatcherProvider.io()) { serpEasterEggLogosToggles.feature().isEnabled() }) {


Launching a new coroutine within onOmnibarViewStateUpdated() breaks synchronous UI update expectations. The calling code may expect state updates to complete immediately, but now they happen asynchronously, potentially causing UI inconsistencies or timing issues.

Copilot · 2025-11-12T12:00:56Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

+                        viewModelScope.launch {
+                            command.value = NavigationCommand.Navigate(urlToNavigate, getUrlHeaders(urlToNavigate))
+                        }


Wrapping navigation commands in new coroutines introduces timing issues. The navigation command may execute after subsequent code that expects it to have already fired, potentially breaking navigation flow or causing race conditions with other navigation logic.

Copilot · 2025-11-12T12:00:56Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt


            it.settings.apply {
-                clientBrandHintProvider.setDefault(this)
+                withContext(dispatchers.io()) { clientBrandHintProvider.setDefault(this@apply) }


Calling clientBrandHintProvider.setDefault() on IO dispatcher while configuring WebView settings is problematic. WebView configuration must happen on the main thread, and switching contexts mid-configuration may cause threading violations or incomplete setup.

Suggested change

withContext(dispatchers.io()) { clientBrandHintProvider.setDefault(this@apply) }

clientBrandHintProvider.setDefault(this@apply)

CDRussell

Still reviewing, but releasing pending comments along with some things you should consider before we merge this (if you haven't already), since it's a riskier change with no ability to FF:

ensure you've tested on both play and internal variants
run as many of the Maestro UI tests against this change as you can

CDRussell · 2025-11-12T11:53:40Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

+                if (siteErrorHandlerKillSwitch.self().isEnabled()) {
+                    siteErrorHandler.assignErrorsAndClearCache(value)
+                    siteHttpErrorHandler.assignErrorsAndClearCache(value)


is it just the FF check that needs to be done on io / does calling the other methods on io change existing behaviour?

The FF check needs to happen in io, and I thought it would be nice to have the other ones execute on io as well. It's not a super complex logic, so it might not hurt to have it in main, but I think io is better

override fun assignErrorsAndClearCache(site: Site?) { if (site != null) { cache[site.url]?.forEach { assignError(site, it) } } cache.clear() } ```

CDRussell · 2025-11-12T12:01:47Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt


            it.settings.apply {
-                clientBrandHintProvider.setDefault(this)
+                withContext(dispatchers.io()) { clientBrandHintProvider.setDefault(this@apply) }


is it safe to do all of clientBrandHintProvider.setDefault on the io thread now, whereas it was all main before?

if you're fully confident it's a safe change, what you have is fine.

but if you have any doubt or want to de-risk further, can consider: should it be suspend instead, and inside you could just do the FF check on io leaving all the other logic to run as is?

CDRussell · 2025-11-12T12:04:48Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

            webViewCapabilityChecker.isSupported(WebViewCapability.DocumentStartJavaScript)

-    private fun configureWebViewForAutofill(it: DuckDuckGoWebView) {
+    private suspend fun configureWebViewForAutofill(it: DuckDuckGoWebView) {


is adding suspend necessary, nothing else seems to use it?

CDRussell · 2025-11-12T12:09:42Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

+            )
+            webViewContainer = binding.webViewContainer
+            viewModel.registerWebViewListener(webViewClient, webChromeClient)
+            configureWebView()


nit: in here it calls swipingTabsFeature.isEnabled which might sometimes check the FF (first time). it's possible it's already cached at this point (BrowserActivity maybe) but if configureWebView is suspend you could also add a withContext(dispatchers.io()) around swipingTabsFeature.isEnabled check; might help to prevent it appearing as a subtle issue later if the thing that is causing it to already be cached at this point is changed.

CDRussell · 2025-11-12T12:23:14Z

@CrisBarreiro , note I see a few other cases where FFs are checked on main thread

CrisBarreiro · 2025-11-12T13:14:49Z

@CrisBarreiro , note I see a few other cases where FFs are checked on main thread

Discussed on MM. These are not necessarily happening in BrowserTabFragment or BrowserTabViewModel, and the intent of this PR is not to eliminate all RC checks in main, just address the ones in these classes for now

CrisBarreiro marked this pull request as ready for review November 10, 2025 15:54

CrisBarreiro commented Nov 10, 2025

View reviewed changes

CrisBarreiro assigned CDRussell Nov 10, 2025

CrisBarreiro requested a review from CDRussell November 10, 2025 15:57

CrisBarreiro added 6 commits November 12, 2025 10:27

Make onActivityCreated non-blocking

a3a18a4

Check siteErrorHandlerKillSwitch.self from io

b7cb299

Check serpSettingsSync from io

f2b9eaa

Don't check featuresRequestHeader from main in renderOmnibarViewState

e4e4e16

Check serpEasterEggLogosToggles from io

1201c8a

Fix tests

a69da00

CrisBarreiro force-pushed the feature/cris/on-activity-created-non-blocking branch from fe9135f to a69da00 Compare November 12, 2025 09:29

Fix CI issue after resolving conflicts

76e8e7c

CrisBarreiro force-pushed the feature/cris/on-activity-created-non-blocking branch from 96dcdd2 to 76e8e7c Compare November 12, 2025 10:00

CDRussell requested a review from Copilot November 12, 2025 11:57

Copilot AI reviewed Nov 12, 2025

View reviewed changes

CDRussell reviewed Nov 12, 2025

View reviewed changes

CrisBarreiro added 2 commits November 12, 2025 14:41

Address PR comments

b9df6f2

Fix onboarding bug

8830159

		viewModelScope.launch {
		if (withContext(dispatcherProvider.io()) { serpEasterEggLogosToggles.feature().isEnabled() }) {

	withContext(dispatchers.io()) { clientBrandHintProvider.setDefault(this@apply) }
	clientBrandHintProvider.setDefault(this@apply)

Remove main RC checks in BrowserTabFragment/VM #7095

Are you sure you want to change the base?

Remove main RC checks in BrowserTabFragment/VM #7095

Conversation

CrisBarreiro commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to test this PR

UI changes

Uh oh!

CrisBarreiro commented Nov 10, 2025

Uh oh!

CrisBarreiro Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

CDRussell left a comment

Choose a reason for hiding this comment

Uh oh!

CDRussell Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

CrisBarreiro Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

CDRussell Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CDRussell Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

CDRussell Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

CDRussell commented Nov 12, 2025

Uh oh!

CrisBarreiro commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CrisBarreiro commented Nov 10, 2025 •

edited

Loading

CDRussell Nov 12, 2025 •

edited

Loading