Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Support for tracking external sources #10711

Merged
merged 2 commits into from
Aug 5, 2021
Merged

Support for tracking external sources #10711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7613c2a
Support for tracking external sources
Jul 13, 2021
358785f
Introduced 'restored' content state for tracking tab restoration
Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions .../src/main/java/mozilla/components/browser/session/storage/serialize/BrowserStateReader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ import mozilla.components.browser.session.storage.RecoverableBrowserState
import mozilla.components.browser.state.state.BrowserState
import mozilla.components.browser.state.state.LastMediaAccessState
import mozilla.components.browser.state.state.ReaderState
import mozilla.components.browser.state.state.SessionState
import mozilla.components.browser.state.state.TabSessionState
import mozilla.components.browser.state.state.recover.RecoverableTab
import mozilla.components.concept.engine.Engine
import mozilla.components.concept.engine.EngineSessionState
import mozilla.components.concept.storage.HistoryMetadataKey
import mozilla.components.support.ktx.android.util.nextBooleanOrNull
import mozilla.components.support.ktx.android.util.nextIntOrNull
import mozilla.components.support.ktx.android.util.nextStringOrNull
import mozilla.components.support.ktx.util.readJSON
import java.util.UUID
Expand Down Expand Up @@ -159,8 +161,8 @@ private fun JsonReader.tab(
)
}

@Suppress("ComplexMethod")
private fun JsonReader.tabSession(): RecoverableTab? {
@Suppress("ComplexMethod", "LongMethod")
private fun JsonReader.tabSession(): RecoverableTab {
var id: String? = null
var parentId: String? = null
var url: String? = null
Expand All @@ -179,6 +181,10 @@ private fun JsonReader.tabSession(): RecoverableTab? {
var historyMetadataSearchTerm: String? = null
var historyMetadataReferrerUrl: String? = null

var sourceId: Int? = null
var externalSourcePackageId: String? = null
var externalSourceCategory: Int? = null

beginObject()

while (hasNext()) {
Expand All @@ -198,7 +204,10 @@ private fun JsonReader.tabSession(): RecoverableTab? {
Keys.SESSION_LAST_MEDIA_URL -> lastMediaUrl = nextString()
Keys.SESSION_LAST_MEDIA_SESSION_ACTIVE -> mediaSessionActive = nextBoolean()
Keys.SESSION_LAST_MEDIA_ACCESS -> lastMediaAccess = nextLong()
Keys.SESSION_SOURCE_KEY -> nextString()
Keys.SESSION_SOURCE_ID -> sourceId = nextInt()
Keys.SESSION_EXTERNAL_SOURCE_PACKAGE_ID -> externalSourcePackageId = nextStringOrNull()
Keys.SESSION_EXTERNAL_SOURCE_PACKAGE_CATEGORY -> externalSourceCategory = nextIntOrNull()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit dangerous and we need to check the existing behaviors: We did not restore the source on purpose. There have been some parts of the code that look at new tabs with specific sources and log telemetry etc. If we now restore them they may show up as things like "new external app" to observers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A safer approach may be to introduce a new property - and potentially deprecate the existing one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pocmo yeah we were concerned about this too, but I couldn't find a case where this would be a problem? Possible I missed it. Do you have any hints where to look? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was basically looking for readers of SessionState.Source which make decisions based on it, but didn't find much other than what's covered in: https://github.com/mozilla-mobile/fenix/pull/20578/files?

Copy link
Contributor

@csadilek csadilek Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK made another pass and couldn't find anything except: https://github.com/mozilla-mobile/fenix/pull/20578/files#r682729976 which we could handle, but not sure if worth it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I would vote for going ahead and maybe handling the one case. Keeping both the old deprecated source and new one might cause other issues and extra work. Since we don't really know a case that could break maybe we should find out and worst case back out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's double check though in case I missed anything :).

Copy link
Contributor

@csadilek csadilek Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pocmo we found the old ticket that prompted us to not restore the source field: mozilla-mobile/fenix#9937

Basically to make sure the back button behaviour is accurate for tabs originally opened from a VIEW intent where back should close the app, but when restored it shouldn't. We can introduce a separate state (as part of Source) for this because technically restored is not a source, but additional information.

Keys.SESSION_DEPRECATED_SOURCE_KEY -> nextString()
else -> throw IllegalArgumentException("Unknown session key: $name")
}
}
Expand Down Expand Up @@ -232,6 +241,7 @@ private fun JsonReader.tabSession(): RecoverableTab? {
lastMediaUrl ?: "",
lastMediaAccess = lastMediaAccess ?: 0,
mediaSessionActive = mediaSessionActive ?: false
)
),
source = SessionState.Source.restore(sourceId, externalSourcePackageId, externalSourceCategory)
)
}
14 changes: 14 additions & 0 deletions .../src/main/java/mozilla/components/browser/session/storage/serialize/BrowserStateWriter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package mozilla.components.browser.session.storage.serialize
import android.util.AtomicFile
import android.util.JsonWriter
import mozilla.components.browser.state.state.BrowserState
import mozilla.components.browser.state.state.SessionState
import mozilla.components.browser.state.state.TabSessionState
import mozilla.components.concept.engine.EngineSessionState
import mozilla.components.support.ktx.util.streamJSON
Expand Down Expand Up @@ -120,6 +121,19 @@ private fun JsonWriter.tab(
value(metadata.referrerUrl)
}

name(Keys.SESSION_SOURCE_ID)
value(tab.source.id)

(tab.source as? SessionState.Source.External)?.let { externalSource ->
externalSource.caller?.let { caller ->
name(Keys.SESSION_EXTERNAL_SOURCE_PACKAGE_ID)
value(caller.packageId)

name(Keys.SESSION_EXTERNAL_SOURCE_PACKAGE_CATEGORY)
value(caller.category.id)
}
}

endObject()
}

Expand Down
7 changes: 6 additions & 1 deletion ...ession-storage/src/main/java/mozilla/components/browser/session/storage/serialize/Keys.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,17 @@ internal object Keys {
const val SESSION_LAST_MEDIA_URL = "lastMediaUrl"
const val SESSION_LAST_MEDIA_ACCESS = "lastMediaAccess"
const val SESSION_LAST_MEDIA_SESSION_ACTIVE = "mediaSessionActive"
const val SESSION_SOURCE_KEY = "source"
// Deprecated for SESSION_SOURCE_ID, kept around for backwards compatibility.
const val SESSION_DEPRECATED_SOURCE_KEY = "source"

const val SESSION_HISTORY_METADATA_URL = "historyMetadataUrl"
const val SESSION_HISTORY_METADATA_SEARCH_TERM = "historyMetadataSearchTerm"
const val SESSION_HISTORY_METADATA_REFERRER_URL = "historyMetadataReferrerUrl"

const val SESSION_SOURCE_ID = "sourceId"
const val SESSION_EXTERNAL_SOURCE_PACKAGE_ID = "externalPackageId"
const val SESSION_EXTERNAL_SOURCE_PACKAGE_CATEGORY = "externalPackageCategory"

const val SESSION_KEY = "session"
const val ENGINE_SESSION_KEY = "engineSession"

Expand Down
126 changes: 115 additions & 11 deletions ...java/mozilla/components/browser/session/storage/serialize/BrowserStateWriterReaderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ import android.util.JsonReader
import android.util.JsonWriter
import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.browser.state.state.EngineState
import mozilla.components.browser.state.state.ExternalPackage
import mozilla.components.browser.state.state.LastMediaAccessState
import mozilla.components.browser.state.state.PackageCategory
import mozilla.components.browser.state.state.ReaderState
import mozilla.components.browser.state.state.SessionState
import mozilla.components.browser.state.state.TabSessionState
import mozilla.components.browser.state.state.createTab
import mozilla.components.concept.engine.Engine
Expand Down Expand Up @@ -94,22 +97,24 @@ class BrowserStateWriterReaderTest {
}

@Test
fun `Read tab with session source`() {
// We don't write tabs with session source anymore but need to be tolerant to
// session source being in the JSON to remain backward compatible.
fun `Read tab with deprecated session source`() {
// We don't persist session source of tabs unless it's an external source.
// However, in older versions we did persist other types of sources so need to be tolerant
// if these older cases are encountered in JSON to remain backward compatible.
val engineState = createFakeEngineState()
val engine = createFakeEngine(engineState)
val tab = createTab(url = "https://www.mozilla.org", title = "Mozilla")
val file = AtomicFile(
File.createTempFile(UUID.randomUUID().toString(), UUID.randomUUID().toString())
)
writeTabWithSource(tab, file)
writeTabWithDeprecatedSource(tab, file)

// When reading we don't care about the source either as we will just set
// it to RESTORED. So we just need to make sure we de-serialized successfully.
// When reading a tab that didn't have a source persisted, we just need to make sure
// it is deserialized correctly. In this case, source defaults to `Internal.Restored`.
val reader = BrowserStateReader()
val restoredTab = reader.readTab(engine, file)
assertNotNull(restoredTab!!)
assertEquals(SessionState.Source.Internal.None, restoredTab.source)

assertEquals("https://www.mozilla.org", restoredTab.url)
assertEquals("Mozilla", restoredTab.title)
Expand Down Expand Up @@ -147,6 +152,105 @@ class BrowserStateWriterReaderTest {
assertEquals(tab.content.url, restoredTab.historyMetadata!!.url)
}

@Test
fun `Read and write tab with external custom tab source and full caller`() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this is in Fenix it would be nice to add a new test case to RestoringBrowsingSessionsTest, which tests actual snapshots from actual installs. :)

val engineState = createFakeEngineState()
val engine = createFakeEngine(engineState)

val tab = createTab(
url = "https://www.mozilla.org",
title = "Mozilla",
contextId = "work",
source = SessionState.Source.External.CustomTab(
caller = ExternalPackage("com.mozilla.test", PackageCategory.PRODUCTIVITY)
)
)

val writer = BrowserStateWriter()
val reader = BrowserStateReader()

val file = AtomicFile(
File.createTempFile(UUID.randomUUID().toString(), UUID.randomUUID().toString())
)

assertTrue(writer.writeTab(tab, file))

val restoredTab = reader.readTab(engine, file)
assertNotNull(restoredTab!!)

assertNotNull(restoredTab.source)
assertTrue(restoredTab.source is SessionState.Source.External.CustomTab)
with(restoredTab.source as SessionState.Source.External.CustomTab) {
assertEquals("com.mozilla.test", this.caller!!.packageId)
assertEquals(PackageCategory.PRODUCTIVITY, this.caller!!.category)
}
}

@Test
fun `Read and write tab with external action view source and partial caller`() {
val engineState = createFakeEngineState()
val engine = createFakeEngine(engineState)

val tab = createTab(
url = "https://www.mozilla.org",
title = "Mozilla",
contextId = "work",
source = SessionState.Source.External.ActionView(
caller = ExternalPackage("com.mozilla.test", category = PackageCategory.UNKNOWN)
)
)

val writer = BrowserStateWriter()
val reader = BrowserStateReader()

val file = AtomicFile(
File.createTempFile(UUID.randomUUID().toString(), UUID.randomUUID().toString())
)

assertTrue(writer.writeTab(tab, file))

val restoredTab = reader.readTab(engine, file)
assertNotNull(restoredTab!!)

assertNotNull(restoredTab.source)
assertTrue(restoredTab.source is SessionState.Source.External.ActionView)
with(restoredTab.source as SessionState.Source.External.ActionView) {
assertEquals("com.mozilla.test", this.caller!!.packageId)
assertEquals(PackageCategory.UNKNOWN, this.caller!!.category)
}
}

@Test
fun `Read and write tab with external action send source and missing caller`() {
val engineState = createFakeEngineState()
val engine = createFakeEngine(engineState)

val tab = createTab(
url = "https://www.mozilla.org",
title = "Mozilla",
contextId = "work",
source = SessionState.Source.External.ActionSend(caller = null)
)

val writer = BrowserStateWriter()
val reader = BrowserStateReader()

val file = AtomicFile(
File.createTempFile(UUID.randomUUID().toString(), UUID.randomUUID().toString())
)

assertTrue(writer.writeTab(tab, file))

val restoredTab = reader.readTab(engine, file)
assertNotNull(restoredTab!!)

assertNotNull(restoredTab.source)
assertTrue(restoredTab.source is SessionState.Source.External.ActionSend)
with(restoredTab.source as SessionState.Source.External.ActionSend) {
assertNull(this.caller)
}
}

@Test
fun `Read and write tab with LastMediaAccessState`() {
val engineState = createFakeEngineState()
Expand Down Expand Up @@ -252,11 +356,11 @@ private fun createFakeEngine(engineState: EngineSessionState): Engine {
return engine
}

private fun writeTabWithSource(tab: TabSessionState, file: AtomicFile) {
file.streamJSON { tabWithSource(tab) }
private fun writeTabWithDeprecatedSource(tab: TabSessionState, file: AtomicFile) {
file.streamJSON { tabWithDeprecatedSource(tab) }
}

private fun JsonWriter.tabWithSource(
private fun JsonWriter.tabWithDeprecatedSource(
tab: TabSessionState
) {
beginObject()
Expand All @@ -272,8 +376,8 @@ private fun JsonWriter.tabWithSource(
name(Keys.SESSION_TITLE)
value(tab.content.title)

name(Keys.SESSION_SOURCE_KEY)
value(tab.source.name)
name(Keys.SESSION_DEPRECATED_SOURCE_KEY)
value(tab.source.toString())

endObject()
}
Expand Down
6 changes: 4 additions & 2 deletions ...owser/state/src/main/java/mozilla/components/browser/state/state/CustomTabSessionState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import java.util.UUID
* values for this tab.
* @property mediaSessionState the [MediaSessionState] of this session.
* @property contextId the session context ID of this custom tab.
* @property source the [SessionState.Source] of this session.
*/
data class CustomTabSessionState(
override val id: String = UUID.randomUUID().toString(),
Expand All @@ -29,7 +30,8 @@ data class CustomTabSessionState(
override val extensionState: Map<String, WebExtensionState> = emptyMap(),
override val mediaSessionState: MediaSessionState? = null,
override val contextId: String? = null,
override val source: SessionState.Source = SessionState.Source.CUSTOM_TAB
override val source: SessionState.Source = SessionState.Source.Internal.CustomTab,
override val restored: Boolean = false
) : SessionState {

override fun createCopy(
Expand Down Expand Up @@ -64,7 +66,7 @@ fun createCustomTab(
engineSession: EngineSession? = null,
mediaSessionState: MediaSessionState? = null,
crashed: Boolean = false,
source: SessionState.Source = SessionState.Source.CUSTOM_TAB,
source: SessionState.Source = SessionState.Source.Internal.CustomTab,
private: Boolean = false,
webAppManifest: WebAppManifest? = null,
initialLoadFlags: EngineSession.LoadUrlFlags = EngineSession.LoadUrlFlags.none()
Expand Down
Loading