For mozilla-mobile#12569 - Don't report PlacesException$OperationInte…

…rrupted exceptions This will catch OperationInterrupted exceptions from the interrupted operation.
Mugurell · Jul 29, 2022 · 81fc68b · 81fc68b
1 parent 5a510b4
commit 81fc68b
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 12 deletions.
diff --git a/...owser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesStorage.kt b/...owser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesStorage.kt
@@ -95,12 +95,8 @@ abstract class PlacesStorage(
      * Allows immediately dismissing all write operations and clearing the write queue.
      */
     internal fun interruptCurrentWrites() {
-        try {
+        handlePlacesExceptions("interruptCurrentWrites") {
             writer.interrupt()
-        } catch (e: PlacesException.OperationInterrupted) {
-            logger.debug("Ignoring expected OperationInterrupted exception for explicit writer interrupt call", e)
-        } catch (e: PlacesException) {
-            logger.warn("Ignoring PlacesException while interrupting writes", e)
         }
     }
 
@@ -109,12 +105,8 @@ abstract class PlacesStorage(
      * Allows avoiding having to wait for stale queries responses and clears the queries queue.
      */
     internal fun interruptCurrentReads() {
-        try {
+        handlePlacesExceptions("interruptCurrentReads") {
             reader.interrupt()
-        } catch (e: PlacesException.OperationInterrupted) {
-            logger.debug("Ignoring expected OperationInterrupted exception for explicit reader interrupt call", e)
-        } catch (e: PlacesException) {
-            logger.warn("Ignoring PlacesException while interrupting reads", e)
         }
     }
 
@@ -129,6 +121,8 @@ abstract class PlacesStorage(
             block()
         } catch (e: PlacesException.UnexpectedPlacesException) {
             throw e
+        } catch (e: PlacesException.OperationInterrupted) {
+            logger.debug("Ignoring expected OperationInterrupted exception when running $operation", e)
         } catch (e: PlacesException) {
             crashReporter?.submitCaughtException(e)
             logger.warn("Ignoring PlacesException while running $operation", e)
@@ -153,6 +147,9 @@ abstract class PlacesStorage(
             block()
         } catch (e: PlacesException.UnexpectedPlacesException) {
             throw e
+        } catch (e: PlacesException.OperationInterrupted) {
+            logger.debug("Ignoring expected OperationInterrupted exception when running $operation", e)
+            default
         } catch (e: PlacesException) {
             crashReporter?.submitCaughtException(e)
             logger.warn("Ignoring PlacesException while running $operation", e)

diff --git a/...ge-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt b/...ge-sync/src/test/java/mozilla/components/browser/storage/sync/PlacesHistoryStorageTest.kt
@@ -22,6 +22,7 @@ import mozilla.components.concept.storage.PageVisit
 import mozilla.components.concept.storage.VisitType
 import mozilla.components.concept.sync.SyncAuthInfo
 import mozilla.components.concept.sync.SyncStatus
+import mozilla.components.support.test.any
 import mozilla.components.support.test.mock
 import mozilla.components.support.test.robolectric.testContext
 import mozilla.components.support.test.rule.MainCoroutineRule
@@ -38,6 +39,8 @@ import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.Mockito.never
+import org.mockito.Mockito.verify
 import java.io.File
 
 @ExperimentalCoroutinesApi // for runTestOnMain
@@ -50,7 +53,7 @@ class PlacesHistoryStorageTest {
 
     @Before
     fun setup() = runTestOnMain {
-        history = PlacesHistoryStorage(testContext)
+        history = PlacesHistoryStorage(testContext, mock())
         // There's a database on disk which needs to be cleaned up between tests.
         history.deleteEverything()
     }
@@ -1309,6 +1312,16 @@ class PlacesHistoryStorageTest {
         assertEquals(emptyList<HistoryMetadata>(), result)
     }
 
+    @Test
+    fun `interrupted read from places is not reported to crash services and returns the default`() = runTestOnMain {
+        val result = history.handlePlacesExceptions("test", default = emptyList<HistoryMetadata>()) {
+            throw PlacesException.OperationInterrupted("An interrupted in progress query will throw")
+        }
+
+        verify(history.crashReporter!!, never()).submitCaughtException(any())
+        assertEquals(emptyList<HistoryMetadata>(), result)
+    }
+
     @Test
     fun `history delegate's shouldStoreUri works as expected`() {
         // Not an excessive list of allowed schemes.

diff --git a/docs/changelog.md b/docs/changelog.md
@@ -12,7 +12,7 @@ permalink: /changelog/
 * [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml)
 
 * **browser-storage-sync**:
-  * Stop loading to the crash servers the expected `OperationInterrupted` exceptions for when interrupting in progress reads/writes from Application-Services. [#12557](https://github.com/mozilla-mobile/android-components/issues/12557)
+  * Stop reporting to the crash servers the expected `OperationInterrupted` exceptions for when interrupting in progress reads/writes from Application-Services. [#12557](https://github.com/mozilla-mobile/android-components/issues/12557), [#12569](https://github.com/mozilla-mobile/android-components/issues/12569).
 
 # 104.0.0
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v103.0.0...v104.0.0)