Commit f24449bb authored by Christian Sadilek's avatar Christian Sadilek Committed by Sebastian Kaspari
Browse files

Closes #3524: browser-state: Prevent duplicated tabs (id)

parent 1f54b728
......@@ -37,7 +37,7 @@ class LegacySessionManager(
* Produces a snapshot of this manager's state, suitable for restoring via [SessionManager.restore].
* Only regular sessions are included in the snapshot. Private and Custom Tab sessions are omitted.
*
* @return [Snapshot] of the current session state.
* @return [SessionManager.Snapshot] of the current session state.
*/
fun createSnapshot(): SessionManager.Snapshot = synchronized(values) {
if (values.isEmpty()) {
......@@ -149,6 +149,10 @@ class LegacySessionManager(
parent: Session? = null,
viaRestore: Boolean = false
) = synchronized(values) {
if (values.find { it.id == session.id } != null) {
throw IllegalArgumentException("Session with same ID already exists")
}
if (parent != null) {
val parentIndex = values.indexOf(parent)
......
......@@ -1089,4 +1089,31 @@ class SessionManagerTest {
assertTrue(executed)
assertTrue(runSessionId == "anotherSessionId")
}
@Test(expected = IllegalArgumentException::class)
fun `SessionManager throws exception when adding session that already exists`() {
val session = Session("https://www.firefox.com")
val manager = SessionManager(mock())
manager.add(session)
manager.add(session)
}
@Test(expected = IllegalArgumentException::class)
fun `SessionManager throws exception when restoring session that already exists`() {
val manager = SessionManager(engine = mock())
val session = Session("https://www.mozilla.org")
manager.add(session)
assertEquals("https://www.mozilla.org", manager.selectedSessionOrThrow.url)
val snapshot = SessionManager.Snapshot(
listOf(
SessionManager.Snapshot.Item(session = session)
),
selectedSessionIndex = 1
)
manager.restore(snapshot)
}
}
......@@ -10,6 +10,7 @@ import mozilla.components.support.test.any
import mozilla.components.support.test.eq
import mozilla.components.support.test.mock
import org.junit.Test
import org.mockito.Mockito.`when`
import org.mockito.Mockito.never
import org.mockito.Mockito.spy
import org.mockito.Mockito.verify
......@@ -18,7 +19,9 @@ class AllSessionsObserverTest {
@Test
fun `Observer will be registered on all already existing Sessions`() {
val session1: Session = mock()
`when`(session1.id).thenReturn("1")
val session2: Session = mock()
`when`(session2.id).thenReturn("2")
val sessionManager = SessionManager(engine = mock()).apply {
add(session1)
......@@ -44,7 +47,9 @@ class AllSessionsObserverTest {
.start()
val session1: Session = mock()
`when`(session1.id).thenReturn("1")
val session2: Session = mock()
`when`(session2.id).thenReturn("2")
sessionManager.add(session1)
sessionManager.add(session2)
......
......@@ -18,6 +18,8 @@ internal object TabListReducer {
fun reduce(state: BrowserState, action: TabListAction): BrowserState {
return when (action) {
is TabListAction.AddTabAction -> {
// Verify that tab doesn't already exist
requireUniqueTab(state, action.tab)
val updatedTabList = if (action.tab.parentId != null) {
val parentIndex = state.tabs.indexOfFirst { it.id == action.tab.parentId }
......@@ -77,6 +79,9 @@ internal object TabListReducer {
}
is TabListAction.RestoreAction -> {
// Verify that none of the tabs to restore already exist
action.tabs.forEach { requireUniqueTab(state, it) }
// We are adding the restored tabs first and then the already existing tabs. Since restore can or should
// happen asynchronously we may already have a tab at this point (e.g. from an `Intent`) and so we
// pretend we restored the list of tabs before any tab was added.
......@@ -193,3 +198,16 @@ private fun findNearbyTab(
return null
}
/**
* Checks that the provided tab doesn't already exist and throws an
* [IllegalArgumentException] otherwise.
*
* @param state the current [BrowserState] (including all existing tabs).
* @param tab the [TabSessionState] to check.
*/
private fun requireUniqueTab(state: BrowserState, tab: TabSessionState) {
require(state.tabs.find { it.id == tab.id } == null) {
"Tab with same ID already exists"
}
}
......@@ -21,12 +21,39 @@ class BrowserStoreExceptionTest {
@Test(expected = java.lang.IllegalArgumentException::class)
fun `AddTabAction - Exception is thrown if parent doesn't exist`() {
try {
unwrapStoreExceptionAndRethrow {
val store = BrowserStore()
val parent = createTab("https://www.mozilla.org")
val child = createTab("https://www.firefox.com", parent = parent)
store.dispatch(TabListAction.AddTabAction(child)).joinBlocking()
}
}
@Test(expected = java.lang.IllegalArgumentException::class)
fun `AddTabAction - Exception is thrown if tab already exists`() {
unwrapStoreExceptionAndRethrow {
val store = BrowserStore()
val tab1 = createTab("https://www.mozilla.org")
store.dispatch(TabListAction.AddTabAction(tab1)).joinBlocking()
store.dispatch(TabListAction.AddTabAction(tab1)).joinBlocking()
}
}
@Test(expected = java.lang.IllegalArgumentException::class)
fun `RestoreTabAction - Exception is thrown if tab already exists`() {
unwrapStoreExceptionAndRethrow {
val store = BrowserStore()
val tab1 = createTab("https://www.mozilla.org")
store.dispatch(TabListAction.AddTabAction(tab1)).joinBlocking()
store.dispatch(TabListAction.RestoreAction(listOf(tab1))).joinBlocking()
}
}
private fun unwrapStoreExceptionAndRethrow(block: () -> Unit) {
try {
block()
// Wait for the main looper to process the re-thrown exception.
ShadowLooper.idleMainLooper()
......
......@@ -91,10 +91,11 @@ class SessionSuggestionProviderTest {
fun `Provider only returns non-private Sessions`() = runBlocking {
val sessionManager = SessionManager(mock())
val session = Session("https://www.mozilla.org")
val privateSession = Session("https://mozilla.org/firefox", true)
sessionManager.add(privateSession)
val privateSession1 = Session("https://mozilla.org/firefox", true)
val privateSession2 = Session("https://mozilla.org/projects", true)
sessionManager.add(privateSession1)
sessionManager.add(session)
sessionManager.add(privateSession)
sessionManager.add(privateSession2)
val useCase: TabsUseCases.SelectTabUseCase = mock()
......
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