Commit 227c932c authored by Christian Sadilek's avatar Christian Sadilek Committed by Grisha Kruglov
Browse files

Closes #2364: Prevent duplicate ViewHolder IDs in AwesomeBar



- This also fixes the HistoryStorageSuggestionProvider to dedupe
history suggestions based on the ID, picking the one with the
highest score.

- Should other providers still return suggestions with duplicate
IDs we now throw an exception containing the provider class name.
Co-authored-by: default avatarGrisha Kruglov <gkruglov@mozilla.com>
parent 1d529b69
......@@ -5,9 +5,12 @@
package mozilla.components.browser.awesomebar
import android.content.Context
import android.support.annotation.MainThread
import android.support.annotation.VisibleForTesting
import android.support.v7.widget.LinearLayoutManager
import android.support.v7.widget.RecyclerView
import android.util.AttributeSet
import android.util.LruCache
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
......@@ -27,17 +30,24 @@ private const val DEFAULT_DESCRIPTION_TEXT_COLOR = 0xFF737373.toInt()
private const val DEFAULT_CHIP_TEXT_COLOR = 0xFF272727.toInt()
private const val DEFAULT_CHIP_BACKGROUND_COLOR = 0xFFEEEEEE.toInt()
private const val DEFAULT_CHIP_SPACING_DP = 2
internal const val PROVIDER_MAX_SUGGESTIONS = 20
internal const val INITIAL_NUMBER_OF_PROVIDERS = 5
/**
* A customizable [AwesomeBar] implementation.
*/
@Suppress("TooManyFunctions")
class BrowserAwesomeBar @JvmOverloads constructor(
context: Context,
attrs: AttributeSet? = null,
defStyleAttr: Int = 0
) : RecyclerView(context, attrs, defStyleAttr), AwesomeBar {
private val jobDispatcher = Executors.newFixedThreadPool(PROVIDER_QUERY_THREADS).asCoroutineDispatcher()
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal val jobDispatcher = Executors.newFixedThreadPool(PROVIDER_QUERY_THREADS).asCoroutineDispatcher()
private val providers: MutableList<AwesomeBar.SuggestionProvider> = mutableListOf()
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
internal val uniqueSuggestionIds = LruCache<String, Long>(INITIAL_NUMBER_OF_PROVIDERS * PROVIDER_MAX_SUGGESTIONS)
private var lastUsedSuggestionId = 0L
internal var suggestionsAdapter = SuggestionsAdapter(this)
internal var scope = CoroutineScope(Dispatchers.Main)
internal var job: Job? = null
......@@ -76,6 +86,7 @@ class BrowserAwesomeBar @JvmOverloads constructor(
@Synchronized
override fun addProviders(vararg providers: AwesomeBar.SuggestionProvider) {
this.providers.addAll(providers)
this.resizeUniqueSuggestionIdCache(this.providers.size)
scrollToPosition(0)
}
......@@ -86,6 +97,7 @@ class BrowserAwesomeBar @JvmOverloads constructor(
suggestionsAdapter.removeSuggestions(it)
this.providers.remove(it)
}
this.resizeUniqueSuggestionIdCache(this.providers.size)
}
@Synchronized
......@@ -98,6 +110,7 @@ class BrowserAwesomeBar @JvmOverloads constructor(
suggestionsAdapter.removeSuggestions(provider)
providerIterator.remove()
}
this.resizeUniqueSuggestionIdCache(0)
}
@Synchronized
......@@ -124,19 +137,29 @@ class BrowserAwesomeBar @JvmOverloads constructor(
job = scope.launch {
providers.forEach { provider ->
launch {
val suggestions = async(jobDispatcher) {
provider.onInputChanged(text)
}
val suggestions = async(jobDispatcher) { provider.onInputChanged(text) }.await()
val processedSuggestions = processProviderSuggestions(suggestions)
suggestionsAdapter.addSuggestions(
provider,
transformer?.transform(provider, suggestions.await()) ?: suggestions.await()
transformer?.transform(provider, processedSuggestions) ?: processedSuggestions
)
}
}
}
}
internal fun processProviderSuggestions(suggestions: List<AwesomeBar.Suggestion>): List<AwesomeBar.Suggestion> {
// We're generating unique suggestion IDs to guard against collisions
// across providers, but we still require unique IDs for suggestions
// from individual providers.
val idSet = mutableSetOf<String>()
suggestions.forEach {
check(idSet.add(it.id)) { "${it.provider::class.java.simpleName} returned duplicate suggestion IDs" }
}
return suggestions.sortedByDescending { it.score }.take(PROVIDER_MAX_SUGGESTIONS)
}
override fun onDetachedFromWindow() {
super.onDetachedFromWindow()
......@@ -154,6 +177,34 @@ class BrowserAwesomeBar @JvmOverloads constructor(
override fun setOnStopListener(listener: () -> Unit) {
this.listener = listener
}
/**
* Returns a unique suggestion ID to make sure ID's can't collide
* across providers. This method is not thread-safe and must be
* invoked on the main thread.
*
* @param suggestion the suggestion for which a unique ID should be
* generated.
*
* @return the unique ID.
*/
@MainThread
fun getUniqueSuggestionId(suggestion: AwesomeBar.Suggestion): Long {
val key = suggestion.provider.id + "/" + suggestion.id
return uniqueSuggestionIds[key] ?: run {
lastUsedSuggestionId += 1
uniqueSuggestionIds.put(key, lastUsedSuggestionId)
lastUsedSuggestionId
}
}
private fun resizeUniqueSuggestionIdCache(providerCount: Int) {
if (providerCount > 0) {
this.uniqueSuggestionIds.resize((providerCount * 2) * PROVIDER_MAX_SUGGESTIONS)
} else {
this.uniqueSuggestionIds.evictAll()
}
}
}
internal data class BrowserAwesomeBarStyling(
......
......@@ -108,7 +108,7 @@ internal class SuggestionsAdapter(
override fun getItemId(position: Int): Long {
val suggestion = suggestions[position]
return suggestion.generatedUniqueId
return awesomeBar.getUniqueSuggestionId(suggestion)
}
override fun getItemViewType(position: Int): Int = synchronized(suggestions) {
......
......@@ -15,6 +15,7 @@ import mozilla.components.concept.awesomebar.AwesomeBar
import mozilla.components.support.test.mock
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.spy
......@@ -23,6 +24,7 @@ import org.mockito.Mockito.never
import org.mockito.Mockito.verifyNoMoreInteractions
import org.robolectric.RobolectricTestRunner
import org.robolectric.Shadows.shadowOf
import java.lang.IllegalStateException
import java.util.UUID
@RunWith(RobolectricTestRunner::class)
......@@ -145,9 +147,13 @@ class BrowserAwesomeBarTest {
val adapter: SuggestionsAdapter = mock()
awesomeBar.suggestionsAdapter = adapter
assertEquals(PROVIDER_MAX_SUGGESTIONS * INITIAL_NUMBER_OF_PROVIDERS, awesomeBar.uniqueSuggestionIds.maxSize())
awesomeBar.addProviders(provider1, provider2)
assertEquals((PROVIDER_MAX_SUGGESTIONS * 2) * 2, awesomeBar.uniqueSuggestionIds.maxSize())
awesomeBar.removeProviders(provider2)
assertEquals((PROVIDER_MAX_SUGGESTIONS * 1) * 2, awesomeBar.uniqueSuggestionIds.maxSize())
awesomeBar.addProviders(provider3)
assertEquals((PROVIDER_MAX_SUGGESTIONS * 2) * 2, awesomeBar.uniqueSuggestionIds.maxSize())
awesomeBar.onInputStarted()
......@@ -169,8 +175,14 @@ class BrowserAwesomeBarTest {
val provider2 = mockProvider()
val awesomeBar = BrowserAwesomeBar(context)
assertEquals(PROVIDER_MAX_SUGGESTIONS * INITIAL_NUMBER_OF_PROVIDERS, awesomeBar.uniqueSuggestionIds.maxSize())
awesomeBar.addProviders(provider1, provider2)
assertEquals((PROVIDER_MAX_SUGGESTIONS * 2) * 2, awesomeBar.uniqueSuggestionIds.maxSize())
// Verify that all cached suggestion IDs are evicted when all providers are removed
awesomeBar.uniqueSuggestionIds.put("test", 1)
awesomeBar.removeAllProviders()
assertEquals(0, awesomeBar.uniqueSuggestionIds.size())
awesomeBar.onInputStarted()
......@@ -337,6 +349,55 @@ class BrowserAwesomeBarTest {
assertTrue(stopped)
}
@Test
fun `throw exception if provider returns duplicate IDs`() {
val awesomeBar = BrowserAwesomeBar(context)
val suggestions = listOf(
AwesomeBar.Suggestion(id = "dupe", score = 0, provider = BrokenProvider()),
AwesomeBar.Suggestion(id = "dupe", score = 0, provider = BrokenProvider())
)
try {
awesomeBar.processProviderSuggestions(suggestions)
fail("Expected IllegalStateException for duplicate suggestion IDs")
} catch (e: IllegalStateException) {
assertTrue(e.message!!.contains(BrokenProvider::class.java.simpleName))
}
}
@Test
fun `get unique suggestion id`() {
val awesomeBar = BrowserAwesomeBar(context)
val suggestion1 = AwesomeBar.Suggestion(id = "http://mozilla.org/1", score = 0, provider = mock())
assertEquals(1, awesomeBar.getUniqueSuggestionId(suggestion1))
val suggestion2 = AwesomeBar.Suggestion(id = "http://mozilla.org/2", score = 0, provider = mock())
assertEquals(2, awesomeBar.getUniqueSuggestionId(suggestion2))
assertEquals(1, awesomeBar.getUniqueSuggestionId(suggestion1))
val suggestion3 = AwesomeBar.Suggestion(id = "http://mozilla.org/3", score = 0, provider = mock())
assertEquals(3, awesomeBar.getUniqueSuggestionId(suggestion3))
}
@Test
fun `unique suggestion id cache has sufficient space`() {
val awesomeBar = BrowserAwesomeBar(context)
val provider = mockProvider()
awesomeBar.addProviders(provider)
for (i in 1..PROVIDER_MAX_SUGGESTIONS) {
awesomeBar.getUniqueSuggestionId(AwesomeBar.Suggestion(id = "$i", score = 0, provider = provider))
}
awesomeBar.getUniqueSuggestionId(AwesomeBar.Suggestion(id = "21", score = 0, provider = provider))
assertEquals(1, awesomeBar.getUniqueSuggestionId(AwesomeBar.Suggestion(id = "1", score = 0, provider = provider)))
}
private fun mockProvider(): AwesomeBar.SuggestionProvider = spy(object : AwesomeBar.SuggestionProvider {
override val id: String = UUID.randomUUID().toString()
......@@ -344,4 +405,12 @@ class BrowserAwesomeBarTest {
return emptyList()
}
})
class BrokenProvider : AwesomeBar.SuggestionProvider {
override val id: String = "Broken"
override suspend fun onInputChanged(text: String): List<AwesomeBar.Suggestion> {
return emptyList()
}
}
}
......@@ -7,7 +7,6 @@ package mozilla.components.concept.awesomebar
import android.graphics.Bitmap
import android.view.View
import java.util.UUID
import java.util.zip.CRC32
/**
* Interface to be implemented by awesome bar implementations.
......@@ -92,21 +91,6 @@ interface AwesomeBar {
val onChipClicked: ((Chip) -> Unit)? = null,
val score: Int = 0
) {
/**
* A generated unique ID ([Long]), based on the provider and suggestion id (having a reasonable expectation of
* generating mostly-non-colliding IDs).
*/
val generatedUniqueId: Long by lazy {
// A non-cryptographic "hash-suitable" function is used for speed.
// CRC32 is quite fast - several orders of magnitude faster than md5, and likely
// good-enough for our purposes (having a reasonable expectation of generating
// mostly-non-colliding IDs).
val crc32 = CRC32()
crc32.update(provider.id.toByteArray())
crc32.update(id.toByteArray())
crc32.value
}
/**
* Chips are compact actions that are shown as part of a suggestion. For example a [Suggestion] from a search
* engine may offer multiple search suggestion chips for different search terms.
......
......@@ -12,7 +12,7 @@ import mozilla.components.feature.awesomebar.internal.loadLambda
import mozilla.components.feature.session.SessionUseCases
import java.util.UUID
private const val HISTORY_SUGGESTION_LIMIT = 20
internal const val HISTORY_SUGGESTION_LIMIT = 20
/**
* A [AwesomeBar.SuggestionProvider] implementation that provides suggestions based on the browsing
......@@ -30,7 +30,11 @@ class HistoryStorageSuggestionProvider(
if (text.isEmpty()) {
return emptyList()
}
return historyStorage.getSuggestions(text, HISTORY_SUGGESTION_LIMIT).into()
val suggestions = historyStorage.getSuggestions(text, HISTORY_SUGGESTION_LIMIT)
// In case of duplicates we want to pick the suggestion with the highest score.
// See: https://github.com/mozilla/application-services/issues/970
return suggestions.sortedByDescending { it.score }.distinctBy { it.id }.into()
}
override val shouldClearSuggestions: Boolean
......
......@@ -6,12 +6,16 @@ package mozilla.components.feature.awesomebar.provider
import kotlinx.coroutines.runBlocking
import mozilla.components.browser.storage.memory.InMemoryHistoryStorage
import mozilla.components.concept.storage.HistoryStorage
import mozilla.components.concept.storage.SearchResult
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.test.eq
import mozilla.components.support.test.mock
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.mockito.Mockito.`when`
class HistoryStorageSuggestionProviderTest {
@Test
......@@ -52,4 +56,43 @@ class HistoryStorageSuggestionProviderTest {
val provider = HistoryStorageSuggestionProvider(mock(), mock())
assertFalse(provider.shouldClearSuggestions)
}
@Test
fun `Provider dedupes suggestions`() = runBlocking {
val storage: HistoryStorage = mock()
val provider = HistoryStorageSuggestionProvider(storage, mock())
val mozSuggestions = listOf(
SearchResult(id = "http://www.mozilla.com/", url = "http://www.mozilla.com/", score = 1),
SearchResult(id = "http://www.mozilla.com/", url = "http://www.mozilla.com/", score = 2),
SearchResult(id = "http://www.mozilla.com/", url = "http://www.mozilla.com/", score = 3)
)
val pocketSuggestions = listOf(
SearchResult(id = "http://www.getpocket.com/", url = "http://www.getpocket.com/", score = 5)
)
val exampleSuggestions = listOf(
SearchResult(id = "http://www.example.com", url = "http://www.example.com/", score = 2)
)
`when`(storage.getSuggestions(eq("moz"), eq(HISTORY_SUGGESTION_LIMIT))).thenReturn(mozSuggestions)
`when`(storage.getSuggestions(eq("pocket"), eq(HISTORY_SUGGESTION_LIMIT))).thenReturn(pocketSuggestions)
`when`(storage.getSuggestions(eq("www"), eq(HISTORY_SUGGESTION_LIMIT))).thenReturn(pocketSuggestions + mozSuggestions + exampleSuggestions)
var results = provider.onInputChanged("moz")
assertEquals(1, results.size)
assertEquals(3, results[0].score)
results = provider.onInputChanged("pocket")
assertEquals(1, results.size)
assertEquals(5, results[0].score)
results = provider.onInputChanged("www")
assertEquals(3, results.size)
assertEquals(5, results[0].score)
assertEquals(3, results[1].score)
assertEquals(2, results[2].score)
}
}
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment