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

Commit

Permalink
Adds exception handling for all places rust calls
Browse files Browse the repository at this point in the history
  • Loading branch information
Tarik Eshaq committed Jun 14, 2022
1 parent ac1a2df commit 30e312a
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun getTree(guid: String, recursive: Boolean): BookmarkNode? {
return withContext(readScope.coroutineContext) {
reader.getBookmarksTree(guid, recursive)?.asBookmarkNode()
handlePlacesExceptions("getTree", default = null) {
reader.getBookmarksTree(guid, recursive)?.asBookmarkNode()
}
}
}

Expand All @@ -46,7 +48,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun getBookmark(guid: String): BookmarkNode? {
return withContext(readScope.coroutineContext) {
reader.getBookmark(guid)?.asBookmarkNode()
handlePlacesExceptions("getBookmark", default = null) {
reader.getBookmark(guid)?.asBookmarkNode()
}
}
}

Expand All @@ -58,7 +62,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun getBookmarksWithUrl(url: String): List<BookmarkNode> {
return withContext(readScope.coroutineContext) {
reader.getBookmarksWithURL(url).map { it.asBookmarkNode() }
handlePlacesExceptions("getBookmarkWithUrl", default = emptyList()) {
reader.getBookmarksWithURL(url).map { it.asBookmarkNode() }
}
}
}

Expand All @@ -71,7 +77,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun searchBookmarks(query: String, limit: Int): List<BookmarkNode> {
return withContext(readScope.coroutineContext) {
reader.searchBookmarks(query, limit).map { it.asBookmarkNode() }
handlePlacesExceptions("searchBookmarks", default = emptyList()) {
reader.searchBookmarks(query, limit).map { it.asBookmarkNode() }
}
}
}

Expand All @@ -94,9 +102,11 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
} else {
0
}
reader.getRecentBookmarks(limit)
.map { it.asBookmarkNode() }
.filter { it.dateAdded >= threshold }
handlePlacesExceptions("getRecentBookmarks", default = emptyList()) {
reader.getRecentBookmarks(limit)
.map { it.asBookmarkNode() }
.filter { it.dateAdded >= threshold }
}
}
}

Expand All @@ -113,7 +123,21 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun addItem(parentGuid: String, url: String, title: String, position: UInt?): String {
return withContext(writeScope.coroutineContext) {
writer.createBookmarkItem(parentGuid, url, title, position)
try {
writer.createBookmarkItem(parentGuid, url, title, position)
} catch (e: PlacesException.UrlParseFailed) {
// We re-throw this exception, it should be handled by the caller
throw e
} catch (e: PlacesException.UnexpectedPlacesException) {
// this is a fatal error, and should be rethrown
throw e
} catch (e: PlacesException) {
crashReporter?.submitCaughtException(e)
logger.warn("Ignoring PlacesException while running addItem", e)
// Should not return an empty string here. The function should be nullable
// however, it is better than the app crashing.
""
}
}
}

Expand All @@ -129,7 +153,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun addFolder(parentGuid: String, title: String, position: UInt?): String {
return withContext(writeScope.coroutineContext) {
writer.createFolder(parentGuid, title, position)
handlePlacesExceptions("addFolder", default = "") {
writer.createFolder(parentGuid, title, position)
}
}
}

Expand All @@ -144,7 +170,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun addSeparator(parentGuid: String, position: UInt?): String {
return withContext(writeScope.coroutineContext) {
writer.createSeparator(parentGuid, position)
handlePlacesExceptions("addSeparator", default = "") {
writer.createSeparator(parentGuid, position)
}
}
}

Expand All @@ -158,7 +186,18 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
*/
override suspend fun updateNode(guid: String, info: BookmarkInfo) {
return withContext(writeScope.coroutineContext) {
writer.updateBookmark(guid, info.parentGuid, info.position, info.title, info.url)
try {
writer.updateBookmark(guid, info.parentGuid, info.position, info.title, info.url)
} catch (e: PlacesException.CannotUpdateRoot) {
// We re-throw this exception, it should be handled by the caller
throw e
} catch (e: PlacesException.UnexpectedPlacesException) {
// this is a fatal error, and should be rethrown
throw e
} catch (e: PlacesException) {
crashReporter?.submitCaughtException(e)
logger.warn("Ignoring PlacesException while running updateNode", e)
}
}
}

Expand All @@ -170,7 +209,19 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo
* @return Whether the bookmark existed or not.
*/
override suspend fun deleteNode(guid: String): Boolean = withContext(writeScope.coroutineContext) {
writer.deleteBookmarkNode(guid)
try {
writer.deleteBookmarkNode(guid)
} catch (e: PlacesException.CannotUpdateRoot) {
// We re-throw this exception, it should be handled by the caller
throw e
} catch (e: PlacesException.UnexpectedPlacesException) {
// this is a fatal error, and should be rethrown
throw e
} catch (e: PlacesException) {
crashReporter?.submitCaughtException(e)
logger.warn("Ignoring PlacesException while running deleteNode", e)
false
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,28 +89,38 @@ open class PlacesHistoryStorage(
}

override suspend fun getVisited(uris: List<String>): List<Boolean> {
return withContext(readScope.coroutineContext) { places.reader().getVisited(uris) }
return withContext(readScope.coroutineContext) {
handlePlacesExceptions("getVisited", default = uris.map { false }) {
places.reader().getVisited(uris)
}
}
}

override suspend fun getVisited(): List<String> {
return withContext(readScope.coroutineContext) {
places.reader().getVisitedUrlsInRange(
start = 0,
end = System.currentTimeMillis(),
includeRemote = true
)
handlePlacesExceptions("getVisited", default = emptyList()) {
places.reader().getVisitedUrlsInRange(
start = 0,
end = System.currentTimeMillis(),
includeRemote = true
)
}
}
}

override suspend fun getDetailedVisits(start: Long, end: Long, excludeTypes: List<VisitType>): List<VisitInfo> {
return withContext(readScope.coroutineContext) {
places.reader().getVisitInfos(start, end, excludeTypes.map { it.into() }).map { it.into() }
handlePlacesExceptions("getDetailedVisits", default = emptyList()) {
places.reader().getVisitInfos(start, end, excludeTypes.map { it.into() }).map { it.into() }
}
}
}

override suspend fun getVisitsPaginated(offset: Long, count: Long, excludeTypes: List<VisitType>): List<VisitInfo> {
return withContext(readScope.coroutineContext) {
places.reader().getVisitPage(offset, count, excludeTypes.map { it.into() }).map { it.into() }
handlePlacesExceptions("getVisitsPaginated", default = emptyList()) {
places.reader().getVisitPage(offset, count, excludeTypes.map { it.into() }).map { it.into() }
}
}
}

Expand All @@ -123,20 +133,26 @@ open class PlacesHistoryStorage(
}

return withContext(readScope.coroutineContext) {
places.reader().getTopFrecentSiteInfos(numItems, frecencyThreshold.into())
.map { it.into() }
handlePlacesExceptions("getTopFrecentSites", default = emptyList()) {
places.reader().getTopFrecentSiteInfos(numItems, frecencyThreshold.into())
.map { it.into() }
}
}
}

override fun getSuggestions(query: String, limit: Int): List<SearchResult> {
require(limit >= 0) { "Limit must be a positive integer" }
return places.reader().queryAutocomplete(query, limit = limit).map {
SearchResult(it.url, it.url, it.frecency.toInt(), it.title)
return handlePlacesExceptions("getSuggestions", default = emptyList()) {
places.reader().queryAutocomplete(query, limit = limit).map {
SearchResult(it.url, it.url, it.frecency.toInt(), it.title)
}
}
}

override fun getAutocompleteSuggestion(query: String): HistoryAutocompleteResult? {
val url = places.reader().matchUrl(query) ?: return null
val url = handlePlacesExceptions("getAutoCompleteSuggestions", default = null) {
places.reader().matchUrl(query)
} ?: return null

val resultText = segmentAwareDomainMatch(query, arrayListOf(url))
return resultText?.let {
Expand Down Expand Up @@ -168,7 +184,9 @@ open class PlacesHistoryStorage(
*/
override suspend fun deleteVisitsSince(since: Long) {
withContext(writeScope.coroutineContext) {
places.writer().deleteVisitsSince(since)
handlePlacesExceptions("deleteVisitsSince") {
places.writer().deleteVisitsSince(since)
}
}
}

Expand All @@ -178,7 +196,9 @@ open class PlacesHistoryStorage(
*/
override suspend fun deleteVisitsBetween(startTime: Long, endTime: Long) {
withContext(writeScope.coroutineContext) {
places.writer().deleteVisitsBetween(startTime, endTime)
handlePlacesExceptions("deleteVisitsBetween") {
places.writer().deleteVisitsBetween(startTime, endTime)
}
}
}

Expand All @@ -187,7 +207,9 @@ open class PlacesHistoryStorage(
*/
override suspend fun deleteVisitsFor(url: String) {
withContext(writeScope.coroutineContext) {
places.writer().deleteVisitsFor(url)
handlePlacesExceptions("deleteVisitsFor") {
places.writer().deleteVisitsFor(url)
}
}
}

Expand All @@ -197,7 +219,9 @@ open class PlacesHistoryStorage(
*/
override suspend fun deleteVisit(url: String, timestamp: Long) {
withContext(writeScope.coroutineContext) {
places.writer().deleteVisit(url, timestamp)
handlePlacesExceptions("deleteVisit") {
places.writer().deleteVisit(url, timestamp)
}
}
}

Expand All @@ -208,7 +232,9 @@ open class PlacesHistoryStorage(
*/
override suspend fun prune() {
withContext(writeScope.coroutineContext) {
places.writer().pruneDestructively()
handlePlacesExceptions("prune") {
places.writer().pruneDestructively()
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ class PlacesBookmarksStorageTest {
try {
bookmarks.deleteNode(root.id)
fail("Expected root deletion for ${root.id} to fail")
} catch (e: PlacesException) {}
} catch (e: PlacesException.CannotUpdateRoot) {}
}

with(bookmarks.searchBookmarks("mozilla")) {
Expand Down
3 changes: 3 additions & 0 deletions docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ permalink: /changelog/
* **support-rusterrors**
* 🆕 New component to report Rust errors

* **browser-storage-sync**:
* Added exception handling for all places history and bookmark calls. This prevents the app from crashing on SQL errors. [#12300](https://github.com/mozilla-mobile/android-components/pull/12300)

# 102.0.0
* [Commits](https://github.com/mozilla-mobile/android-components/compare/v101.0.0...v102.0.1)
* [Milestone](https://github.com/mozilla-mobile/android-components/milestone/149?closed=1)
Expand Down

0 comments on commit 30e312a

Please sign in to comment.