From 1b490c833c9935a250b7b4c1e29eb7f272d3e75f Mon Sep 17 00:00:00 2001 From: Tarik Eshaq Date: Tue, 7 Jun 2022 14:53:08 -0700 Subject: [PATCH] Adds exception handling for all places rust calls --- .../storage/sync/PlacesBookmarksStorage.kt | 75 ++++++++++++++++--- .../storage/sync/PlacesHistoryStorage.kt | 62 ++++++++++----- .../sync/PlacesBookmarksStorageTest.kt | 2 +- docs/changelog.md | 3 + 4 files changed, 111 insertions(+), 31 deletions(-) diff --git a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt b/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt index f33312e696d..85bbdda352f 100644 --- a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt +++ b/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt @@ -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() + } } } @@ -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() + } } } @@ -58,7 +62,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo */ override suspend fun getBookmarksWithUrl(url: String): List { return withContext(readScope.coroutineContext) { - reader.getBookmarksWithURL(url).map { it.asBookmarkNode() } + handlePlacesExceptions("getBookmarkWithUrl", default = emptyList()) { + reader.getBookmarksWithURL(url).map { it.asBookmarkNode() } + } } } @@ -71,7 +77,9 @@ open class PlacesBookmarksStorage(context: Context) : PlacesStorage(context), Bo */ override suspend fun searchBookmarks(query: String, limit: Int): List { return withContext(readScope.coroutineContext) { - reader.searchBookmarks(query, limit).map { it.asBookmarkNode() } + handlePlacesExceptions("searchBookmarks", default = emptyList()) { + reader.searchBookmarks(query, limit).map { it.asBookmarkNode() } + } } } @@ -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 } + } } } @@ -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. + "" + } } } @@ -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) + } } } @@ -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) + } } } @@ -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) + } } } @@ -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 + } } /** diff --git a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt b/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt index 98521f3a5b3..4fd201ced11 100644 --- a/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt +++ b/components/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt @@ -89,28 +89,38 @@ open class PlacesHistoryStorage( } override suspend fun getVisited(uris: List): List { - 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 { 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): List { 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): List { 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() } + } } } @@ -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 { 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 { @@ -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) + } } } @@ -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) + } } } @@ -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) + } } } @@ -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) + } } } @@ -208,7 +232,9 @@ open class PlacesHistoryStorage( */ override suspend fun prune() { withContext(writeScope.coroutineContext) { - places.writer().pruneDestructively() + handlePlacesExceptions("prune") { + places.writer().pruneDestructively() + } } } diff --git a/components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt b/components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt index 9d793cf5452..4f312347ebd 100644 --- a/components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt +++ b/components/browser/storage-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt @@ -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")) { diff --git a/docs/changelog.md b/docs/changelog.md index 9b323966188..a55fd4c150f 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -14,6 +14,9 @@ permalink: /changelog/ * **feature-top-sites** * Replaced `frecencyConfig` from `TopSitesConfig` with `TopSitesFrecencyConfig`, which specifies the `FrecencyTresholdOption` and the frecency filter, an optional function used to filter the top frecent sites. [#12384] (https://github.com/mozilla-mobile/android-components/issues/12384) +* **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) + # 103.0.0 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v102.0.0...v103.0.0) * [Milestone](https://github.com/mozilla-mobile/android-components/milestone/150?closed=1)