From e18fbdfbfcd1f12e8d060ef97c054c037a92ff37 Mon Sep 17 00:00:00 2001 From: Christian Sadilek Date: Mon, 20 Sep 2021 12:30:53 -0400 Subject: [PATCH] [fenix] Make sure to only record view time once for a given tab access Co-authored-by: Grisha Kruglov --- .../historymetadata/HistoryMetadataService.kt | 29 +++++++++++++++++-- .../HistoryMetadataServiceTest.kt | 28 ++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt index 004bbeeed6..5b708a2b07 100644 --- a/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt +++ b/app/src/main/java/org/mozilla/fenix/historymetadata/HistoryMetadataService.kt @@ -5,7 +5,7 @@ package org.mozilla.fenix.historymetadata import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.launch import mozilla.components.browser.state.state.TabSessionState import mozilla.components.concept.storage.DocumentType @@ -13,6 +13,8 @@ import mozilla.components.concept.storage.HistoryMetadataKey import mozilla.components.concept.storage.HistoryMetadataObservation import mozilla.components.concept.storage.HistoryMetadataStorage import mozilla.components.support.base.log.logger.Logger +import mozilla.components.support.base.utils.NamedThreadFactory +import java.util.concurrent.Executors /** * Service for managing (creating, updating, deleting) history metadata. @@ -51,11 +53,18 @@ interface HistoryMetadataService { class DefaultHistoryMetadataService( private val storage: HistoryMetadataStorage, - private val scope: CoroutineScope = CoroutineScope(Dispatchers.IO) + private val scope: CoroutineScope = CoroutineScope( + Executors.newSingleThreadExecutor( + NamedThreadFactory("HistoryMetadataService") + ).asCoroutineDispatcher() + ) ) : HistoryMetadataService { private val logger = Logger("DefaultHistoryMetadataService") + // NB: this map is only accessed from a single-thread executor (dispatcher of `scope`). + private val tabsLastUpdated = mutableMapOf() + override fun createMetadata(tab: TabSessionState, searchTerms: String?, referrerUrl: String?): HistoryMetadataKey { logger.debug("Creating metadata for tab ${tab.id}") @@ -81,6 +90,7 @@ class DefaultHistoryMetadataService( } override fun updateMetadata(key: HistoryMetadataKey, tab: TabSessionState) { + val now = System.currentTimeMillis() val lastAccess = tab.lastAccess if (lastAccess == 0L) { logger.debug("Not updating metadata for tab $tab - lastAccess=0") @@ -89,11 +99,24 @@ class DefaultHistoryMetadataService( logger.debug("Updating metadata for tab $tab") } + // If it's possible that multiple threads overlap and run this block simultaneously, we + // may over-observe, and record when we didn't intend to. + // To make these cases easier to reason through (and likely correct), + // `scope` is a single-threaded dispatcher. Execution of these blocks is thus serialized. scope.launch { + val lastUpdated = tabsLastUpdated[tab.id] ?: 0 + if (lastUpdated > lastAccess) { + logger.debug( + "Failed to update metadata because it was already recorded or lastAccess is incorrect" + ) + return@launch + } + val viewTimeObservation = HistoryMetadataObservation.ViewTimeObservation( - viewTime = (System.currentTimeMillis() - lastAccess).toInt() + viewTime = (now - lastAccess).toInt() ) storage.noteHistoryMetadataObservation(key, viewTimeObservation) + tabsLastUpdated[tab.id] = now } } diff --git a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt index b1f80bb7d5..cbe6f3769b 100644 --- a/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt +++ b/app/src/test/java/org/mozilla/fenix/historymetadata/HistoryMetadataServiceTest.kt @@ -94,6 +94,34 @@ class HistoryMetadataServiceTest { assertTrue(observation.captured.viewTime >= 60 * 1000) } + @Test + fun `WHEN metadata is updated for a tab with no lastAccess THEN view time observation is not recorded`() { + val key = HistoryMetadataKey(url = "https://blog.mozilla.org") + val tab = createTab(key.url, historyMetadata = key, lastAccess = 0) + service.updateMetadata(key, tab) + testDispatcher.advanceUntilIdle() + + coVerify(exactly = 0) { storage.noteHistoryMetadataObservation(key, any()) } + } + + @Test + fun `WHEN metadata is updated for a tab with unchanged lastAccess THEN view time observation is not recorded`() { + val now = System.currentTimeMillis() + val key = HistoryMetadataKey(url = "https://blog.mozilla.org") + val tab = createTab(key.url, historyMetadata = key, lastAccess = now - 60 * 1000) + service.updateMetadata(key, tab) + testDispatcher.advanceUntilIdle() + + val observation = slot() + coVerify(exactly = 1) { storage.noteHistoryMetadataObservation(key, capture(observation)) } + assertTrue(observation.captured.viewTime >= 60 * 1000) + + // Now, call update again with the same lastAccess value. Storage method won't be hit again. + service.updateMetadata(key, tab) + testDispatcher.advanceUntilIdle() + coVerify(exactly = 1) { storage.noteHistoryMetadataObservation(key, any()) } + } + @Test fun `WHEN cleanup is called THEN old metadata is deleted`() { val timestamp = System.currentTimeMillis() - 7 * 24 * 60 * 60 * 1000