Skip to content

Commit 8b6daf3

Browse files
authored
cache site errors and assign to pages after they load (#5902)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1208671518894266/task/1209947027710700 ### Description Resolves an issue where a web view error delivered while user is reloading the current page would overwrite local state, making it invalid and showing wrong URL and warnings about a lack of protections. ### Steps to test this PR - Open `amazon.com` (either from SERP or manually) - Type in **manually** `amazon.com` and load the page multiple times. - Verify that address bar keeps showing the correct URL (including HTTPS) and privacy shield. - Opening privacy dashboard should always show that protections are active and that the site is secure. You can also try with `abc.com`, `cnn.com` (which always should show `edition.cnn.com`).
1 parent 8dc9f88 commit 8b6daf3

File tree

5 files changed

+339
-11
lines changed

5 files changed

+339
-11
lines changed

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,11 @@ import com.duckduckgo.app.browser.duckplayer.DuckPlayerJSHelper
105105
import com.duckduckgo.app.browser.favicon.FaviconManager
106106
import com.duckduckgo.app.browser.favicon.FaviconSource
107107
import com.duckduckgo.app.browser.history.NavigationHistoryEntry
108+
import com.duckduckgo.app.browser.httperrors.HttpCodeSiteErrorHandler
108109
import com.duckduckgo.app.browser.httperrors.HttpErrorPixelName
109110
import com.duckduckgo.app.browser.httperrors.HttpErrorPixels
111+
import com.duckduckgo.app.browser.httperrors.SiteErrorHandlerKillSwitch
112+
import com.duckduckgo.app.browser.httperrors.StringSiteErrorHandler
110113
import com.duckduckgo.app.browser.logindetection.FireproofDialogsEventHandler
111114
import com.duckduckgo.app.browser.logindetection.LoginDetected
112115
import com.duckduckgo.app.browser.logindetection.NavigationAwareLoginDetector
@@ -540,6 +543,11 @@ class BrowserTabViewModelTest {
540543
private val defaultVisualExperimentStateFlow = MutableStateFlow(FeatureState(isAvailable = true, isEnabled = false))
541544
private val defaultVisualExperimentNavBarStateFlow = MutableStateFlow(FeatureState(isAvailable = true, isEnabled = false))
542545

546+
private val mockSiteErrorHandlerKillSwitch: SiteErrorHandlerKillSwitch = mock()
547+
private val mockSiteErrorHandlerKillSwitchToggle: Toggle = mock { on { it.isEnabled() } doReturn true }
548+
private val mockSiteErrorHandler: StringSiteErrorHandler = mock()
549+
private val mockSiteHttpErrorHandler: HttpCodeSiteErrorHandler = mock()
550+
543551
@Before
544552
fun before() = runTest {
545553
MockitoAnnotations.openMocks(this)
@@ -655,6 +663,8 @@ class BrowserTabViewModelTest {
655663
defaultVisualExperimentNavBarStateFlow,
656664
)
657665

666+
whenever(mockSiteErrorHandlerKillSwitch.self()).thenReturn(mockSiteErrorHandlerKillSwitchToggle)
667+
658668
testee = BrowserTabViewModel(
659669
statisticsUpdater = mockStatisticsUpdater,
660670
queryUrlConverter = mockOmnibarConverter,
@@ -723,6 +733,9 @@ class BrowserTabViewModelTest {
723733
defaultBrowserPromptsExperiment = mockDefaultBrowserPromptsExperiment,
724734
swipingTabsFeature = swipingTabsFeatureProvider,
725735
visualDesignExperimentDataStore = mockVisualDesignExperimentDataStore,
736+
siteErrorHandlerKillSwitch = mockSiteErrorHandlerKillSwitch,
737+
siteErrorHandler = mockSiteErrorHandler,
738+
siteHttpErrorHandler = mockSiteHttpErrorHandler,
726739
)
727740

728741
testee.loadData("abc", null, false, false)
@@ -4917,7 +4930,8 @@ class BrowserTabViewModelTest {
49174930
}
49184931

49194932
@Test
4920-
fun whenPageIsChangedWithHttpErrorThenPrivacyProtectionsPopupManagerIsNotified() = runTest {
4933+
fun whenErrorHandlerKilledAndPageIsChangedWithHttpErrorThenPrivacyProtectionsPopupManagerIsNotified() = runTest {
4934+
whenever(mockSiteErrorHandlerKillSwitchToggle.isEnabled()).thenReturn(false)
49214935
testee.recordHttpErrorCode(statusCode = 404, url = "example2.com")
49224936

49234937
updateUrl(
@@ -4933,6 +4947,26 @@ class BrowserTabViewModelTest {
49334947
)
49344948
}
49354949

4950+
@Test
4951+
fun whenPageIsChangedWithHttpErrorThenPrivacyProtectionsPopupManagerIsNotified() = runTest {
4952+
val siteCaptor = argumentCaptor<Site>()
4953+
whenever(mockSiteHttpErrorHandler.assignErrorsAndClearCache(siteCaptor.capture())).then {
4954+
siteCaptor.lastValue.onHttpErrorDetected(404)
4955+
}
4956+
4957+
updateUrl(
4958+
originalUrl = "example.com",
4959+
currentUrl = "example2.com",
4960+
isBrowserShowing = true,
4961+
)
4962+
4963+
verify(mockPrivacyProtectionsPopupManager).onPageLoaded(
4964+
url = "example2.com",
4965+
httpErrorCodes = listOf(404),
4966+
hasBrowserError = false,
4967+
)
4968+
}
4969+
49364970
@Test
49374971
fun whenPageIsChangedWithHttpError400ThenUpdateCountPixelCalledForWebViewReceivedHttpError400Daily() = runTest {
49384972
testee.recordHttpErrorCode(statusCode = 400, url = "example2.com")
@@ -6085,6 +6119,66 @@ class BrowserTabViewModelTest {
60856119
verify(mockPixel).fire(ONBOARDING_DAX_CTA_DISMISS_BUTTON, mapOf(PixelParameter.CTA_SHOWN to DAX_SERP_CTA))
60866120
}
60876121

6122+
@Test
6123+
fun whenRecordErrorCodeThenProvideValueToErrorHandler() {
6124+
val site = givenCurrentSite("some.domain")
6125+
val siteCaptor = argumentCaptor<Site>()
6126+
val errorUrl = "some.domain"
6127+
val errorValue = "error value"
6128+
6129+
testee.recordErrorCode(url = errorUrl, error = errorValue)
6130+
6131+
verify(mockSiteErrorHandler).handleError(currentSite = siteCaptor.capture(), urlWithError = eq(errorUrl), error = eq(errorValue))
6132+
assertEquals(site.url, siteCaptor.lastValue.url)
6133+
}
6134+
6135+
@Test
6136+
fun whenRecordHttpErrorCodeThenProvideValueToErrorHandler() {
6137+
val site = givenCurrentSite("some.domain")
6138+
val siteCaptor = argumentCaptor<Site>()
6139+
val errorUrl = "some.domain"
6140+
val errorValue = -1
6141+
6142+
testee.recordHttpErrorCode(url = errorUrl, statusCode = errorValue)
6143+
6144+
verify(mockSiteHttpErrorHandler).handleError(currentSite = siteCaptor.capture(), urlWithError = eq(errorUrl), error = eq(errorValue))
6145+
assertEquals(site.url, siteCaptor.lastValue.url)
6146+
}
6147+
6148+
@Test
6149+
fun whenRecordErrorCodeNotMatchingCurrentSiteThenProvideValueToErrorHandler() {
6150+
val site = givenCurrentSite("some.domain")
6151+
val siteCaptor = argumentCaptor<Site>()
6152+
val errorUrl = "error.com"
6153+
val errorValue = "error value"
6154+
6155+
testee.recordErrorCode(url = errorUrl, error = errorValue)
6156+
6157+
verify(mockSiteErrorHandler).handleError(currentSite = siteCaptor.capture(), urlWithError = eq(errorUrl), error = eq(errorValue))
6158+
assertEquals(site.url, siteCaptor.lastValue.url)
6159+
}
6160+
6161+
@Test
6162+
fun whenRecordHttpErrorCodeNotMatchingCurrentSiteThenProvideValueToErrorHandler() {
6163+
val site = givenCurrentSite("some.domain")
6164+
val siteCaptor = argumentCaptor<Site>()
6165+
val errorUrl = "error.com"
6166+
val errorValue = -1
6167+
6168+
testee.recordHttpErrorCode(url = errorUrl, statusCode = errorValue)
6169+
6170+
verify(mockSiteHttpErrorHandler).handleError(currentSite = siteCaptor.capture(), urlWithError = eq(errorUrl), error = eq(errorValue))
6171+
assertEquals(site.url, siteCaptor.lastValue.url)
6172+
}
6173+
6174+
@Test
6175+
fun whenSiteLoadedThenAssignCachedErrors() {
6176+
val site = givenCurrentSite("some.domain")
6177+
6178+
verify(mockSiteErrorHandler).assignErrorsAndClearCache(site)
6179+
verify(mockSiteHttpErrorHandler).assignErrorsAndClearCache(site)
6180+
}
6181+
60886182
private fun aCredential(): LoginCredentials {
60896183
return LoginCredentials(domain = null, username = null, password = null)
60906184
}

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

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,11 @@ import com.duckduckgo.app.browser.favicon.FaviconManager
178178
import com.duckduckgo.app.browser.favicon.FaviconSource.ImageFavicon
179179
import com.duckduckgo.app.browser.favicon.FaviconSource.UrlFavicon
180180
import com.duckduckgo.app.browser.history.NavigationHistoryAdapter.NavigationHistoryListener
181+
import com.duckduckgo.app.browser.httperrors.HttpCodeSiteErrorHandler
181182
import com.duckduckgo.app.browser.httperrors.HttpErrorPixelName
182183
import com.duckduckgo.app.browser.httperrors.HttpErrorPixels
184+
import com.duckduckgo.app.browser.httperrors.SiteErrorHandlerKillSwitch
185+
import com.duckduckgo.app.browser.httperrors.StringSiteErrorHandler
183186
import com.duckduckgo.app.browser.logindetection.FireproofDialogsEventHandler
184187
import com.duckduckgo.app.browser.logindetection.FireproofDialogsEventHandler.Event
185188
import com.duckduckgo.app.browser.logindetection.LoginDetected
@@ -467,6 +470,9 @@ class BrowserTabViewModel @Inject constructor(
467470
private val defaultBrowserPromptsExperiment: DefaultBrowserPromptsExperiment,
468471
private val swipingTabsFeature: SwipingTabsFeatureProvider,
469472
private val visualDesignExperimentDataStore: VisualDesignExperimentDataStore,
473+
private val siteErrorHandlerKillSwitch: SiteErrorHandlerKillSwitch,
474+
private val siteErrorHandler: StringSiteErrorHandler,
475+
private val siteHttpErrorHandler: HttpCodeSiteErrorHandler,
470476
) : WebViewClientListener,
471477
EditSavedSiteListener,
472478
DeleteBookmarkListener,
@@ -541,7 +547,15 @@ class BrowserTabViewModel @Inject constructor(
541547
)
542548

543549
private var autoCompleteJob = ConflatedJob()
550+
544551
private var site: Site? = null
552+
set(value) {
553+
field = value
554+
if (siteErrorHandlerKillSwitch.self().isEnabled()) {
555+
siteErrorHandler.assignErrorsAndClearCache(value)
556+
siteHttpErrorHandler.assignErrorsAndClearCache(value)
557+
}
558+
}
545559
private lateinit var tabId: String
546560
private var webNavigationState: WebNavigationState? = null
547561
private var httpsUpgraded = false
@@ -3391,27 +3405,35 @@ class BrowserTabViewModel @Inject constructor(
33913405
error: String,
33923406
url: String,
33933407
) {
3394-
// when navigating from one page to another it can happen that errors are recorded before pageChanged etc. are
3395-
// called triggering a buildSite.
3396-
if (url != site?.url) {
3397-
site = siteFactory.buildSite(url = url, tabId = tabId)
3408+
if (siteErrorHandlerKillSwitch.self().isEnabled()) {
3409+
siteErrorHandler.handleError(currentSite = site, urlWithError = url, error = error)
3410+
} else {
3411+
// when navigating from one page to another it can happen that errors are recorded before pageChanged etc. are
3412+
// called triggering a buildSite.
3413+
if (url != site?.url) {
3414+
site = siteFactory.buildSite(url = url, tabId = tabId)
3415+
}
3416+
site?.onErrorDetected(error)
33983417
}
33993418
Timber.d("recordErrorCode $error in ${site?.url}")
3400-
site?.onErrorDetected(error)
34013419
}
34023420

34033421
override fun recordHttpErrorCode(
34043422
statusCode: Int,
34053423
url: String,
34063424
) {
3407-
// when navigating from one page to another it can happen that errors are recorded before pageChanged etc. are
3408-
// called triggering a buildSite.
3409-
if (url != site?.url) {
3410-
site = siteFactory.buildSite(url = url, tabId = tabId)
3425+
if (siteErrorHandlerKillSwitch.self().isEnabled()) {
3426+
siteHttpErrorHandler.handleError(currentSite = site, urlWithError = url, error = statusCode)
3427+
} else {
3428+
// when navigating from one page to another it can happen that errors are recorded before pageChanged etc. are
3429+
// called triggering a buildSite.
3430+
if (url != site?.url) {
3431+
site = siteFactory.buildSite(url = url, tabId = tabId)
3432+
}
3433+
site?.onHttpErrorDetected(statusCode)
34113434
}
34123435
Timber.d("recordHttpErrorCode $statusCode in ${site?.url}")
34133436
updateHttpErrorCount(statusCode)
3414-
site?.onHttpErrorDetected(statusCode)
34153437
}
34163438

34173439
fun onAutofillMenuSelected() {

app/src/main/java/com/duckduckgo/app/browser/di/BrowserModule.kt

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@ import com.duckduckgo.app.browser.duckchat.AIChatQueryDetectionFeature
4040
import com.duckduckgo.app.browser.favicon.FaviconPersister
4141
import com.duckduckgo.app.browser.favicon.FileBasedFaviconPersister
4242
import com.duckduckgo.app.browser.httpauth.WebViewHttpAuthStore
43+
import com.duckduckgo.app.browser.httperrors.HttpCodeSiteErrorHandler
44+
import com.duckduckgo.app.browser.httperrors.HttpCodeSiteErrorHandlerImpl
45+
import com.duckduckgo.app.browser.httperrors.StringSiteErrorHandler
46+
import com.duckduckgo.app.browser.httperrors.StringSiteErrorHandlerImpl
4347
import com.duckduckgo.app.browser.logindetection.*
4448
import com.duckduckgo.app.browser.mediaplayback.store.ALL_MIGRATIONS
4549
import com.duckduckgo.app.browser.mediaplayback.store.MediaPlaybackDao
@@ -367,6 +371,16 @@ class BrowserModule {
367371
fun provideRefreshDao(appDatabase: AppDatabase): RefreshDao {
368372
return appDatabase.refreshDao()
369373
}
374+
375+
@Provides
376+
fun provideSiteErrorStringHandler(): StringSiteErrorHandler {
377+
return StringSiteErrorHandlerImpl()
378+
}
379+
380+
@Provides
381+
fun provideSiteErrorCodeHandler(): HttpCodeSiteErrorHandler {
382+
return HttpCodeSiteErrorHandlerImpl()
383+
}
370384
}
371385

372386
@Qualifier
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
/*
2+
* Copyright (c) 2025 DuckDuckGo
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.duckduckgo.app.browser.httperrors
18+
19+
import com.duckduckgo.anvil.annotations.ContributesRemoteFeature
20+
import com.duckduckgo.app.browser.BrowserTabViewModel
21+
import com.duckduckgo.app.global.model.Site
22+
import com.duckduckgo.di.scopes.AppScope
23+
import com.duckduckgo.feature.toggles.api.Toggle
24+
import javax.inject.Inject
25+
26+
interface SiteErrorHandler<T> {
27+
fun handleError(currentSite: Site?, urlWithError: String, error: T)
28+
fun assignErrorsAndClearCache(site: Site?)
29+
}
30+
31+
interface StringSiteErrorHandler : SiteErrorHandler<String>
32+
33+
interface HttpCodeSiteErrorHandler : SiteErrorHandler<Int>
34+
35+
/**
36+
* When navigating from one page to another it can happen that errors are received before [BrowserTabViewModel.pageChanged].
37+
* So, a previous page is still loaded, but we are already receiving potential errors about a page that's about to be loaded.
38+
* For site breakage reports, we want to include these errors as well.
39+
*
40+
* This class provides a [handleError] function that takes the current loaded site and the received error as parameters.
41+
* If the error is generated by the current site, it's immediately assigned.
42+
* If not, a temporary cache is used to hold onto errors and later assign them to a newly loaded [Site].
43+
*
44+
* [assignErrorsAndClearCache] should be called as soon as a new [Site] is built.
45+
* The function will assign previously captured errors that match the [Site.url] and drop everything else.
46+
*
47+
* This means that if we encounter some intermediate errors on redirects that are not part of the target site, they won't be included in the report.
48+
*/
49+
sealed class SiteErrorHandlerImpl<T> : SiteErrorHandler<T> {
50+
private val cache = mutableMapOf<String, MutableList<T>>()
51+
52+
override fun handleError(currentSite: Site?, urlWithError: String, error: T) {
53+
if (currentSite != null && currentSite.url == urlWithError) {
54+
assignError(currentSite, error)
55+
} else {
56+
val cachedErrorsForSite = cache.getOrPut(urlWithError) {
57+
mutableListOf()
58+
}
59+
cachedErrorsForSite.add(error)
60+
}
61+
}
62+
63+
override fun assignErrorsAndClearCache(site: Site?) {
64+
if (site != null) {
65+
cache[site.url]?.forEach {
66+
assignError(site, it)
67+
}
68+
}
69+
cache.clear()
70+
}
71+
72+
protected abstract fun assignError(site: Site, error: T)
73+
}
74+
75+
class StringSiteErrorHandlerImpl @Inject constructor() : StringSiteErrorHandler, SiteErrorHandlerImpl<String>() {
76+
override fun assignError(site: Site, error: String) {
77+
site.onErrorDetected(error)
78+
}
79+
}
80+
81+
class HttpCodeSiteErrorHandlerImpl @Inject constructor() : HttpCodeSiteErrorHandler, SiteErrorHandlerImpl<Int>() {
82+
override fun assignError(site: Site, error: Int) {
83+
site.onHttpErrorDetected(error)
84+
}
85+
}
86+
87+
@ContributesRemoteFeature(
88+
scope = AppScope::class,
89+
featureName = "siteErrorHandlerKillSwitch",
90+
)
91+
interface SiteErrorHandlerKillSwitch {
92+
93+
@Toggle.DefaultValue(true)
94+
fun self(): Toggle
95+
}

0 commit comments

Comments
 (0)