Skip to content

Measure concurrent load requests #6190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ import com.duckduckgo.app.browser.refreshpixels.RefreshPixelSender
import com.duckduckgo.app.browser.remotemessage.RemoteMessagingModel
import com.duckduckgo.app.browser.senseofprotection.SenseOfProtectionExperiment
import com.duckduckgo.app.browser.session.WebViewSessionStorage
import com.duckduckgo.app.browser.tabs.TabManager
import com.duckduckgo.app.browser.trafficquality.AndroidFeaturesHeaderPlugin.Companion.X_DUCKDUCKGO_ANDROID_HEADER
import com.duckduckgo.app.browser.viewstate.BrowserViewState
import com.duckduckgo.app.browser.viewstate.CtaViewState
Expand Down Expand Up @@ -556,6 +557,7 @@ class BrowserTabViewModelTest {
private val mockSiteHttpErrorHandler: HttpCodeSiteErrorHandler = mock()
private val mockSubscriptionsJSHelper: SubscriptionsJSHelper = mock()
private val mockReactivateUsersExperiment: ReactivateUsersExperiment = mock()
private val tabManager: TabManager = mock()

private val fakeOnboardingDesignExperimentToggles: OnboardingDesignExperimentToggles =
FakeFeatureToggleFactory.create(OnboardingDesignExperimentToggles::class.java)
Expand Down Expand Up @@ -763,6 +765,7 @@ class BrowserTabViewModelTest {
senseOfProtectionExperiment = mockSenseOfProtectionExperiment,
subscriptionsJSHelper = mockSubscriptionsJSHelper,
onboardingDesignExperimentToggles = fakeOnboardingDesignExperimentToggles,
tabManager = tabManager,
)

testee.loadData("abc", null, false, false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -977,7 +977,7 @@ class BrowserWebViewClientTest {
fun whenPageFinishesBeforeStartingThenPixelIsNotFired() {
val mockWebView = getImmediatelyInvokedMockWebView()
testee.onPageFinished(mockWebView, EXAMPLE_URL)
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any())
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any(), any())
}

@Test
Expand All @@ -991,7 +991,7 @@ class BrowserWebViewClientTest {
testee.onPageFinished(mockWebView, EXAMPLE_URL)
val startArgumentCaptor = argumentCaptor<Long>()
val endArgumentCaptor = argumentCaptor<Long>()
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture())
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture(), any())
assertEquals(0L, startArgumentCaptor.firstValue)
assertEquals(10L, endArgumentCaptor.firstValue)
}
Expand All @@ -1004,7 +1004,7 @@ class BrowserWebViewClientTest {
whenever(mockWebView.settings).thenReturn(mock())
testee.onPageStarted(mockWebView, "about:blank", null)
testee.onPageFinished(mockWebView, "about:blank")
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any())
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any(), any())
}

@Test
Expand All @@ -1013,7 +1013,7 @@ class BrowserWebViewClientTest {
whenever(mockWebView.settings).thenReturn(mock())
testee.onPageStarted(mockWebView, EXAMPLE_URL, null)
testee.onPageFinished(mockWebView, EXAMPLE_URL)
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any())
verify(pageLoadedHandler, never()).onPageLoaded(any(), any(), any(), any(), any())
}

@Test
Expand Down Expand Up @@ -1046,7 +1046,7 @@ class BrowserWebViewClientTest {

val startArgumentCaptor = argumentCaptor<Long>()
val endArgumentCaptor = argumentCaptor<Long>()
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture())
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture(), any())
assertEquals(0L, startArgumentCaptor.firstValue)
assertEquals(10L, endArgumentCaptor.firstValue)
}
Expand All @@ -1068,7 +1068,7 @@ class BrowserWebViewClientTest {
testee.onPageFinished(mockWebView, EXAMPLE_URL)
val startArgumentCaptor = argumentCaptor<Long>()
val endArgumentCaptor = argumentCaptor<Long>()
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture())
verify(pageLoadedHandler).onPageLoaded(any(), eq(null), startArgumentCaptor.capture(), endArgumentCaptor.capture(), any())
assertEquals(5L, startArgumentCaptor.firstValue)
assertEquals(10L, endArgumentCaptor.firstValue)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ class PageLoadedHandlerTest {

@Test
fun whenInvokingWithValidUrlThenPixelIsAdded() {
testee.onPageLoaded(VALID_URL, "title", 0L, 10L)
testee.onPageLoaded(VALID_URL, "title", 0L, 10L, true)
val argumentCaptor = argumentCaptor<PageLoadedPixelEntity>()
verify(pageLoadedPixelDao).add(argumentCaptor.capture())
Assert.assertEquals(10L, argumentCaptor.firstValue.elapsedTime)
}

@Test
fun whenInvokingWithInvalidUrlThenPixelIsAdded() {
testee.onPageLoaded(INVALID_URL, "title", 0L, 10L)
testee.onPageLoaded(INVALID_URL, "title", 0L, 10L, true)
verify(pageLoadedPixelDao, never()).add(any())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
}

private val tabPagerAdapter by lazy {
TabPagerAdapter(
activity = this,
swipingTabsFeature = swipingTabsFeature,
)
TabPagerAdapter(activity = this)
}

private lateinit var toolbarMockupBinding: IncludeOmnibarToolbarMockupBinding
Expand Down Expand Up @@ -1000,6 +997,7 @@ open class BrowserActivity : DuckDuckGoActivity() {

private fun onMoveToTabRequested(index: Int) {
tabPager.post {
tabPagerAdapter.currentTabIndex = index
tabPager.setCurrentItem(index, false)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ import com.duckduckgo.app.browser.omnibar.model.OmnibarPosition.TOP
import com.duckduckgo.app.browser.refreshpixels.RefreshPixelSender
import com.duckduckgo.app.browser.senseofprotection.SenseOfProtectionExperiment
import com.duckduckgo.app.browser.session.WebViewSessionStorage
import com.duckduckgo.app.browser.tabs.TabManager
import com.duckduckgo.app.browser.urlextraction.UrlExtractionListener
import com.duckduckgo.app.browser.viewstate.AccessibilityViewState
import com.duckduckgo.app.browser.viewstate.AutoCompleteViewState
Expand Down Expand Up @@ -465,6 +466,7 @@ class BrowserTabViewModel @Inject constructor(
private val senseOfProtectionExperiment: SenseOfProtectionExperiment,
private val subscriptionsJSHelper: SubscriptionsJSHelper,
private val onboardingDesignExperimentToggles: OnboardingDesignExperimentToggles,
private val tabManager: TabManager,
) : WebViewClientListener,
EditSavedSiteListener,
DeleteBookmarkListener,
Expand Down Expand Up @@ -1242,6 +1244,14 @@ class BrowserTabViewModel @Inject constructor(

override fun isDesktopSiteEnabled(): Boolean = currentBrowserViewState().isDesktopBrowsingMode

override fun isTabInForeground(): Boolean {
return if (swipingTabsFeature.isEnabled) {
tabId == tabManager.getSelectedTabId()
} else {
true
}
}

override fun closeCurrentTab() {
viewModelScope.launch {
removeCurrentTabFromRepository()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,14 @@ import com.duckduckgo.user.agent.api.ClientBrandHintProvider
import java.net.URI
import javax.inject.Inject
import kotlinx.coroutines.*
import logcat.LogPriority
import logcat.LogPriority.INFO
import logcat.LogPriority.VERBOSE
import logcat.LogPriority.WARN
import logcat.logcat
import java.util.UUID
import java.util.concurrent.ConcurrentHashMap
import java.util.concurrent.atomic.AtomicInteger

private const val ABOUT_BLANK = "about:blank"

Expand Down Expand Up @@ -133,6 +137,9 @@ class BrowserWebViewClient @Inject constructor(

private var shouldOpenDuckPlayerInNewTab: Boolean = true

private var currentLoadOperationId: String? = null
private var parallelRequestsOnStart = 0

init {
appCoroutineScope.launch {
duckPlayer.observeShouldOpenInNewTab().collect {
Expand All @@ -141,6 +148,39 @@ class BrowserWebViewClient @Inject constructor(
}
}

private fun incrementAndTrackLoad(urlForLog: String) {
// a new load operation is starting for this WebView instance.
val loadId = UUID.randomUUID().toString()
this.currentLoadOperationId = loadId

parallelRequestsOnStart = parallelRequestCounter.incrementAndGet() - 1
logcat(LogPriority.DEBUG) { "### Request started (ID: $loadId, URL: $urlForLog). Counter: ${parallelRequestCounter.get()}" }

val job = timeoutScope.launch {
delay(REQUEST_TIMEOUT_MS)
// attempt to remove the job - if successful, it means it hasn't been finished/errored/cancelled yet
if (activeRequestTimeoutJobs.remove(loadId) != null) {
parallelRequestCounter.decrementAndGet()
logcat(LogPriority.WARN) { "### Request timed out (ID: $loadId, URL: $urlForLog). Counter: ${parallelRequestCounter.get()}" }
}
}
activeRequestTimeoutJobs[loadId] = job
}

private fun decrementAndUntrackLoad(reason: String, urlForLog: String?) {
this.currentLoadOperationId?.let { loadId ->
val job = activeRequestTimeoutJobs.remove(loadId)

// if we successfully removed the job (it means it hadn't timed out yet)
if (job != null) {
job.cancel()
parallelRequestCounter.decrementAndGet()
logcat(LogPriority.DEBUG) { "### Request $reason (ID: $loadId, URL: $urlForLog). Counter: ${parallelRequestCounter.get()}" }
}
}
this.currentLoadOperationId = null
}

/**
* This is the method of url overriding available from API 24 onwards
*/
Expand Down Expand Up @@ -422,6 +462,7 @@ class BrowserWebViewClient @Inject constructor(
// See https://app.asana.com/0/0/1206159443951489/f (WebView limitations)
if (it != ABOUT_BLANK && start == null) {
start = currentTimeProvider.elapsedRealtime()
incrementAndTrackLoad(it) // increment the request counter
requestInterceptor.onPageStarted(url)
}
handleMediaPlayback(webView, it)
Expand Down Expand Up @@ -479,7 +520,15 @@ class BrowserWebViewClient @Inject constructor(
if (url != ABOUT_BLANK) {
start?.let { safeStart ->
// TODO (cbarreiro - 22/05/2024): Extract to plugins
pageLoadedHandler.onPageLoaded(it, navigationList.currentItem?.title, safeStart, currentTimeProvider.elapsedRealtime())
pageLoadedHandler.onPageLoaded(
url = it,
title = navigationList.currentItem?.title,
start = safeStart,
end = currentTimeProvider.elapsedRealtime(),
isTabInForeground = webViewClientListener?.isTabInForeground() ?: true,
requestsOnStart = parallelRequestsOnStart,
requestsWhenFinished = parallelRequestCounter.get() - 1
)
shouldSendPagePaintedPixel(webView = webView, url = it)
appCoroutineScope.launch(dispatcherProvider.io()) {
if (duckPlayer.getDuckPlayerState() == ENABLED && duckPlayer.isSimulatedYoutubeNoCookie(uri)) {
Expand All @@ -497,6 +546,10 @@ class BrowserWebViewClient @Inject constructor(
}
}
uriLoadedManager.sendUriLoadedPixel()

// conclude the tracked load operation
decrementAndUntrackLoad("finished", url)

start = null
}
}
Expand Down Expand Up @@ -540,6 +593,13 @@ class BrowserWebViewClient @Inject constructor(
} else {
pixel.fire(WEB_RENDERER_GONE_KILLED)
}

if (this.start != null) {
val currentUrl = view?.url
decrementAndUntrackLoad("render_process_gone", currentUrl)
this.start = null
}

webViewClientListener?.recoverFromRenderProcessGone()
return true
}
Expand Down Expand Up @@ -635,16 +695,19 @@ class BrowserWebViewClient @Inject constructor(
request: WebResourceRequest?,
error: WebResourceError?,
) {
error?.let {
val parsedError = parseErrorResponse(it)
if (parsedError != OMITTED && request?.isForMainFrame == true) {
start = null
webViewClientListener?.onReceivedError(parsedError, request.url.toString())
}
error?.let { webResourceError ->
val parsedError = parseErrorResponse(webResourceError)
if (request?.isForMainFrame == true) {
if (parsedError != OMITTED) {
if (this.start != null) {
decrementAndUntrackLoad("errored", request.url.toString())
this.start = null
}
webViewClientListener?.onReceivedError(parsedError, request.url.toString())
}
logcat { "recordErrorCode for ${request.url}" }
webViewClientListener?.recordErrorCode(
"${it.errorCode.asStringErrorCode()} - ${it.description}",
"${webResourceError.errorCode.asStringErrorCode()} - ${webResourceError.description}",
request.url.toString(),
)
}
Expand Down Expand Up @@ -714,6 +777,15 @@ class BrowserWebViewClient @Inject constructor(
fun addExemptedMaliciousSite(url: Uri, feed: Feed) {
requestInterceptor.addExemptedMaliciousSite(url, feed)
}

companion object {
val parallelRequestCounter = AtomicInteger(0)
private val activeRequestTimeoutJobs = ConcurrentHashMap<String, Job>()
private const val REQUEST_TIMEOUT_MS = 30000L // 30 seconds

// dedicated scope for request count timeout jobs
private val timeoutScope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
}
}

enum class WebViewPixelName(override val pixelName: String) : Pixel.PixelName {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ interface WebViewClientListener {
fun surrogateDetected(surrogate: SurrogateResponse)
fun isDesktopSiteEnabled(): Boolean

fun isTabInForeground(): Boolean

fun loginDetected()
fun dosAttackDetected()
fun iconReceived(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,18 @@ import com.squareup.anvil.annotations.ContributesBinding
import javax.inject.Inject
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import logcat.logcat

interface PageLoadedHandler {
fun onPageLoaded(url: String, title: String?, start: Long, end: Long)
fun onPageLoaded(
url: String,
title: String?,
start: Long,
end: Long,
isTabInForeground: Boolean,
requestsOnStart: Int,
requestsWhenFinished: Int
)
}

@ContributesBinding(AppScope::class)
Expand All @@ -45,7 +54,15 @@ class RealPageLoadedHandler @Inject constructor(
private val optimizeTrackerEvaluationRCWrapper: OptimizeTrackerEvaluationRCWrapper,
) : PageLoadedHandler {

override fun onPageLoaded(url: String, title: String?, start: Long, end: Long) {
override fun onPageLoaded(
url: String,
title: String?,
start: Long,
end: Long,
isTabInForeground: Boolean,
requestsOnStart: Int,
requestsWhenFinished: Int,
) {
appCoroutineScope.launch(dispatcherProvider.io()) {
if (sites.any { UriString.sameOrSubdomain(url, it) }) {
pageLoadedPixelDao.add(
Expand All @@ -58,6 +75,8 @@ class RealPageLoadedHandler @Inject constructor(
),
)
}
logcat { "$$$: Page load time: ${end - start}, foreground: $isTabInForeground, " +
"requestsOnStart: $requestsOnStart, requestsWhenFinished: $requestsWhenFinished" }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import com.duckduckgo.app.tabs.model.TabRepository
import com.duckduckgo.common.utils.DispatcherProvider
import com.duckduckgo.di.scopes.ActivityScope
import com.squareup.anvil.annotations.ContributesBinding
import dagger.SingleInstanceIn
import javax.inject.Inject
import kotlinx.coroutines.withContext
import logcat.LogPriority.INFO
Expand All @@ -50,6 +51,7 @@ interface TabManager {
)
}

@SingleInstanceIn(ActivityScope::class)
@ContributesBinding(ActivityScope::class)
class DefaultTabManager @Inject constructor(
private val tabRepository: TabRepository,
Expand Down
Loading
Loading