Adds exception handling for all places rust calls

mozilla-mobile · Jun 9, 2022 · 3199cee · 3199cee
1 parent 90d8bc8
commit 3199cee
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 31 deletions.
diff --git a/...rage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorage.kt b/...rage-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<BookmarkNode> {
         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<BookmarkNode> {
         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/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt b/...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt
@@ -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() }
+            }
         }
     }
 
@@ -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 {
@@ -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/...-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesBookmarksStorageTest.kt b/...-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
@@ -34,6 +34,9 @@ permalink: /changelog/
         Use `listOf` instead of `longArrayOf` or call `.toList`
       * `TimingDistributionMetricType.start` now always returns a valid `TimerId`, `TimingDistributionMetricType.stopAndAccumulate` always requires a `TimerId`.
 
+* **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)