Merge pull request #319 from bugsnag/android-p-flush-fix

Some unhandled error reports aren't being sent immediately on Android P and in situations with no connectivity.

Internally `FileStore` retains a `Collection` of files which have previously been found for a request, to prevent duplicate reports being sent. This puts the onus on the calling code to cancel or delete the files once the request has been completed. 

This changeset ensures that these files are only requested once when calling `flushOnLaunch`, then cancels all files after delivery has taken place (successfully or not), which resets the `FileStore` state.

After this, `flushAsync` is then called, which will flush any remaining reports that have not yet been delivered.

CHANGELOG.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## 4.4.1 (TBD)
+
+### Bug fixes
+
+* Ensure that unhandled error reports are always sent immediately on launch for Android P and in situations with no connectivity.
+[#319](https://github.com/bugsnag/bugsnag-android/pull/319)
+
 ## 4.4.0 (2018-05-17)
 
 ### Features

features/startup_crash.feature

@@ -0,0 +1,14 @@
+Feature: All errors are flushed if a startup crash is persisted
+
+Scenario: 1 startup crash and 1 regular crash persisted
+    When I configure the app to run in the "CrashOfflineWithDelay" state
+    And I run "StartupCrashFlushScenario" with the defaults
+    And I wait for 10 seconds
+
+    And I configure the app to run in the "CrashOfflineAtStartup" state
+    And I relaunch the app
+
+    And I configure the app to run in the "No crash" state
+    And I relaunch the app
+    And I wait for 5 seconds
+    Then I should receive 2 requests

features/steps/build_steps.rb

@@ -8,3 +8,12 @@
     And I start the "com.bugsnag.android.mazerunner" Android app using the "com.bugsnag.android.mazerunner.MainActivity" activity
   }
 end
+
+When("I relaunch the app") do
+  step('I force stop the "com.bugsnag.android.mazerunner" Android app')
+  step('I start the "com.bugsnag.android.mazerunner" Android app using the "com.bugsnag.android.mazerunner.MainActivity" activity')
+end
+
+When("I configure the app to run in the {string} state") do |event_metadata|
+  step("I set environment variable \"EVENT_METADATA\" to \"#{event_metadata}\"")
+end

mazerunner/src/main/java/com/bugsnag/android/mazerunner/scenarios/Scenario.kt

@@ -50,11 +50,24 @@ abstract internal class Scenario(protected val config: Configuration,
      * Sets a NOP implementation for the Error Tracking API and the Session Tracking API,
      * preventing delivery
      */
+    @Deprecated("Disable via config instead, using the new delivery API")
     protected fun disableAllDelivery() {
         disableSessionDelivery()
         disableReportDelivery()
     }
 
+    protected fun disableAllDelivery(config: Configuration) {
+        config.delivery = object: Delivery {
+            override fun deliver(payload: SessionTrackingPayload?, config: Configuration?) {
+                throw DeliveryFailureException("Error Delivery NOP", RuntimeException("NOP"))
+            }
+
+            override fun deliver(report: Report?, config: Configuration?) {
+                throw DeliveryFailureException("Session Delivery NOP", RuntimeException("NOP"))
+            }
+        }
+    }
+
     /**
      * Returns a throwable with the message as the current classname
      */

mazerunner/src/main/java/com/bugsnag/android/mazerunner/scenarios/StartupCrashFlushScenario.kt

@@ -0,0 +1,42 @@
+package com.bugsnag.android.mazerunner.scenarios
+
+import android.content.Context
+import android.os.Handler
+import com.bugsnag.android.*
+
+/**
+ * Generates an uncaught exception, catches it, and persists it to disc, preventing any delivery.
+ *
+ * To generate a startup crash, set "eventMetaData" to "StartupCrash", otherwise the default
+ * behaviour is to report a normal crash.
+ *
+ * The mazerunner scenario that tests this works in 3 parts:
+ *
+ * 1. Generate and persist a normal crash on disk, by waiting longer than the threshold for
+ *    a startup crash
+ * 2. Generate and persist a startup crash on disk, by crashing as soon as the scenario starts
+ * 3. Send the stored reports. The startup crash should be delivered synchronously on the main thread,
+ * and the normal crash asynchronously.
+ */
+internal class StartupCrashFlushScenario(config: Configuration,
+                                         context: Context) : Scenario(config, context) {
+
+    override fun run() {
+        if ("CrashOfflineWithDelay" == eventMetaData) {
+            // Part 1 - Persist a regular crash to disk
+            disableAllDelivery(config)
+            super.run()
+            Handler().postDelayed({
+            	throw RuntimeException("Regular crash")
+			}, 6000)
+        } else if ("CrashOfflineAtStartup" == eventMetaData) {
+            // Part 2 - Persist a startup crash to disk
+            disableAllDelivery(config)
+            super.run()
+            throw RuntimeException("Startup crash")
+        } else {
+            // Part 3 - Online, no crashes: send any cached reports
+            super.run()
+        }
+    }
+}

sdk/src/main/java/com/bugsnag/android/ErrorStore.java

@@ -51,38 +51,42 @@ public int compare(File lhs, File rhs) {
     }
 
     void flushOnLaunch() {
-        final List<File> crashReports = findLaunchCrashReports();
-
-        if (crashReports.isEmpty() || config.getLaunchCrashThresholdMs() == 0) {
-            flushAsync(); // if disabled or no startup crash, flush async
-        } else {
-
-            // Block the main thread for a 2 second interval as the app may crash very soon.
-            // The request itself will run in a background thread and will continue after the 2
-            // second period until the request completes, or the app crashes.
-            flushOnLaunchCompleted = false;
-            Logger.info("Attempting to send launch crash reports");
-
-            Async.run(new Runnable() {
-                @Override
-                public void run() {
-                    flushReports(crashReports);
-                    flushOnLaunchCompleted = true;
-                }
-            });
-
-            long waitMs = 0;
-
-            while (!flushOnLaunchCompleted && waitMs < LAUNCH_CRASH_TIMEOUT_MS) {
-                try {
-                    Thread.sleep(LAUNCH_CRASH_POLL_MS);
-                    waitMs += LAUNCH_CRASH_POLL_MS;
-                } catch (InterruptedException exception) {
-                    Logger.warn("Interrupted while waiting for launch crash report request");
+        if (config.getLaunchCrashThresholdMs() != 0) {
+            List<File> storedFiles = findStoredFiles();
+            final List<File> crashReports = findLaunchCrashReports(storedFiles);
+
+            if (!crashReports.isEmpty()) {
+
+                // Block the main thread for a 2 second interval as the app may crash very soon.
+                // The request itself will run in a background thread and will continue after the 2
+                // second period until the request completes, or the app crashes.
+                flushOnLaunchCompleted = false;
+                Logger.info("Attempting to send launch crash reports");
+
+                Async.run(new Runnable() {
+                    @Override
+                    public void run() {
+                        flushReports(crashReports);
+                        flushOnLaunchCompleted = true;
+                    }
+                });
+
+                long waitMs = 0;
+
+                while (!flushOnLaunchCompleted && waitMs < LAUNCH_CRASH_TIMEOUT_MS) {
+                    try {
+                        Thread.sleep(LAUNCH_CRASH_POLL_MS);
+                        waitMs += LAUNCH_CRASH_POLL_MS;
+                    } catch (InterruptedException exception) {
+                        Logger.warn("Interrupted while waiting for launch crash report request");
+                    }
                 }
+                Logger.info("Continuing with Bugsnag initialisation");
             }
-            Logger.info("Continuing with Bugsnag initialisation");
+            cancelQueuedFiles(storedFiles); // cancel all previously found files
         }
+
+        flushAsync(); // flush any remaining errors async that weren't delivered
     }
 
     /**
@@ -130,7 +134,7 @@ private void flushErrorReport(File errorFile) {
         } catch (DeliveryFailureException exception) {
             cancelQueuedFiles(Collections.singleton(errorFile));
             Logger.warn("Could not send previously saved error(s)"
-                        + " to Bugsnag, will try again later", exception);
+                + " to Bugsnag, will try again later", exception);
         } catch (Exception exception) {
             deleteStoredFiles(Collections.singleton(errorFile));
             Logger.warn("Problem sending unsent error from disk", exception);
@@ -141,8 +145,7 @@ boolean isLaunchCrashReport(File file) {
         return file.getName().endsWith("_startupcrash.json");
     }
 
-    private List<File> findLaunchCrashReports() {
-        Collection<File> storedFiles = findStoredFiles();
+    private List<File> findLaunchCrashReports(Collection<File> storedFiles) {
         List<File> launchCrashes = new ArrayList<>();
 
         for (File file : storedFiles) {