Fix: Fix various crashes and add bugsnag reporting (#148)

* Fix: Fix crash on empty or non-numeric Manual GPS coord Move code into try-catch fixes #134 * fix: bugsnag notify instead of console.error console.error may have been causing RN crash in production * Fix: App crashes instead of handling error Attempt to fix facebook/react-native#24382 pending facebook/react-native@d7bd6cc which needs update to RN 0.59.5 * fix: Extend server start timeout On some devices server start is taking longer than 10 seconds * fix: catch server start timeout * Fix: Log photo capture error * chore(logging): Add breadcrumbs to mapeo core startup * Update bugsnag breadcrumb text * REVERSE THIS: error test * Add releaseStage to bugsnag reporting * Revert "REVERSE THIS: error test" This reverts commit 9ff0458. * Add bugsnag reporting to photo capture
digidem · Jun 20, 2019 · d8579a9 · d8579a9
1 parent c07c469
commit d8579a9
Show file tree

Hide file tree

Showing 7 changed files with 93 additions and 58 deletions.
diff --git a/package.json b/package.json
@@ -65,7 +65,9 @@
     "react-native-vector-icons": "^6.3.0",
     "react-navigation": "^3.9.0",
     "react-navigation-transitions": "^1.0.10",
+    "semver": "^6.1.1",
     "shallowequal": "^1.1.0",
+    "stacktrace-parser": "^0.1.4",
     "utm": "^1.1.1"
   },
   "devDependencies": {

diff --git a/src/frontend/api.js b/src/frontend/api.js
@@ -12,6 +12,7 @@ import type {
   ObservationValue
 } from "./context/ObservationsContext";
 import { promiseTimeout } from "./lib/utils";
+import bugsnag from "./lib/logger";
 import STATUS from "./../backend/constants";
 
 import type { IconSize, ImageSize } from "./types";
@@ -65,8 +66,12 @@ export { STATUS as Constants };
 
 const log = debug("mapeo-mobile:api");
 const BASE_URL = "http://127.0.0.1:9081/";
+// Timeout between heartbeats from the server. If 10 seconds pass without a
+// heartbeat then we consider the server has errored
 const DEFAULT_TIMEOUT = 10000; // 10 seconds
-const SERVER_START_TIMEOUT = 10000;
+// Timeout for server start. If 20 seconds passes after server starts with no
+// heartbeat then we consider the server has errored
+const SERVER_START_TIMEOUT = 20000;
 
 const pixelRatio = PixelRatio.get();
 
@@ -152,6 +157,7 @@ export function Api({
     startServer: function startServer(): Promise<void> {
       // The server might already be started - request current status
       nodejs.channel.post("request-status");
+      bugsnag.leaveBreadcrumb("Starting Mapeo Core");
       // The server requires read & write permissions for external storage
       const serverStartPromise = PermissionsAndroid.requestMultiple([
         PermissionsAndroid.PERMISSIONS.READ_EXTERNAL_STORAGE,
@@ -163,13 +169,13 @@ export function Api({
           );
           if (!permissionsGranted)
             throw new Error("Storage read and write permissions not granted");
-          log("REQUESTING NODE START");
+          bugsnag.leaveBreadcrumb("Mapeo Core permissions");
           nodejs.start("loader.js");
           // We know the node process has started as soon as we hear a status
           return new Promise(resolve => nodejs.channel.once("status", resolve));
         })
         .then(() => {
-          log("FIRST HEARTBEAT FROM NODE");
+          bugsnag.leaveBreadcrumb("Mapeo Core started");
           // Start monitoring for timeout
           restartTimeout();
           // As soon as we hear from the Node process, send the storagePath so
@@ -178,11 +184,20 @@ export function Api({
           // Resolve once the server reports status as "LISTENING"
           return onReady();
         });
+      serverStartPromise.then(() =>
+        bugsnag.leaveBreadcrumb("Mapeo Core ready")
+      );
       return promiseTimeout(
         serverStartPromise,
         SERVER_START_TIMEOUT,
         "Server start timeout"
-      );
+      ).catch(e => {
+        // We could get here when the timeout timer has not yet started and the
+        // server status is still "STARTING", so we update the status to an
+        // error
+        onStatusChange(status.ERROR);
+        bugsnag.notify(e);
+      });
     },
 
     addServerStateListener: function addServerStateListener(

diff --git a/src/frontend/context/DraftObservationContext.js b/src/frontend/context/DraftObservationContext.js
@@ -8,6 +8,7 @@ import debounce from "lodash/debounce";
 import AsyncStorage from "@react-native-community/async-storage";
 
 import { getDisplayName } from "../lib/utils";
+import bugsnag from "../lib/logger";
 import type {
   ObservationValue,
   ObservationAttachment
@@ -191,7 +192,7 @@ class DraftObservationProvider extends React.Component<
         })
         .then(({ uri }) => {
           if (capturePromise.cancelled) throw new Error("Cancelled");
-          log("Generated thumbnail", uri);
+          bugsnag.leaveBreadcrumb("Generated thumbnail", { type: "process" });
           photo.thumbnailUri = uri;
           return ImageResizer.createResizedImage(
             photo.originalUri,
@@ -204,7 +205,7 @@ class DraftObservationProvider extends React.Component<
         })
         .then(({ uri }) => {
           if (capturePromise.cancelled) throw new Error("Cancelled");
-          log("Generated preview", uri);
+          bugsnag.leaveBreadcrumb("Generated preview", { type: "process" });
           // Remove from pending
           this.pending = this.pending.filter(p => p !== capturePromise);
           photo.previewUri = uri;
@@ -217,8 +218,10 @@ class DraftObservationProvider extends React.Component<
           // Remove from pending
           this.pending = this.pending.filter(p => p !== capturePromise);
           if (capturePromise.cancelled || err.message === "Cancelled")
-            return log("Cancelled!");
-          log("Error capturing image:", err);
+            return bugsnag.leaveBreadcrumb("Cancelled photo");
+          bugsnag.notify(err, report => {
+            report.severity = "error";
+          });
           photo.error = true;
           this.setState(state => ({
             photos: splice(state.photos, index, photo)

diff --git a/src/frontend/lib/logger.js b/src/frontend/lib/logger.js
@@ -1,5 +1,14 @@
-import { Client } from "bugsnag-react-native";
+import { Client, Configuration } from "bugsnag-react-native";
+import semver from "semver";
+import { version } from "../../../package.json";
 
-const bugsnag = new Client("572d472ea9d5a9199777b88ef268da4e");
+const config = new Configuration("572d472ea9d5a9199777b88ef268da4e");
+const prereleaseComponents = semver.prerelease(version);
+config.releaseStage = __DEV__
+  ? "development"
+  : prereleaseComponents
+  ? prereleaseComponents[0]
+  : "production";
+const bugsnag = new Client(config);
 
 export default bugsnag;
diff --git a/src/frontend/screens/ManualGpsScreen.js b/src/frontend/screens/ManualGpsScreen.js
@@ -68,50 +68,50 @@ class ManualGpsScreen extends React.Component<Props, State> {
 
   toLatLon() {
     const { zoneNum, zoneLetter, easting, northing } = this.state;
-    const parsedNorthing = parseNumber(northing);
-    let northern;
-    // There are two conventions for UTM. One uses a letter to refer to latitude
-    // bands from C to X, excluding "I" and "O". The other uses "N" or "S" to
-    // refer to the northern or southern hemisphere. If the user enters "N" or
-    // "S" we do not know which convention they are using, so we guess. We try
-    // to use the latitude band if we can, since it is better for catching
-    // errors in coordinate entry.
-    if (zoneLetter === "S") {
-      // "S" could refer to grid zone "S" (in the northern hemisphere) or it
-      // could mean "Southern" - conventions differ in different places
-      const startOfZoneS = 3544369.909548157;
-      const startOfZoneT = 4432069.057005376;
-      if (
-        parsedNorthing !== undefined &&
-        parsedNorthing >= startOfZoneS &&
-        parsedNorthing < startOfZoneT
-      ) {
-        // Indeterminate, this could be latitude band S, or it could mean
-        // southern hemisphere. The only place in the southern hemisphere that
-        // matches these coordinates is the very southern tip of Chile and
-        // Argentina, so assume that in this case zoneLetter "S" refers to
-        // latitude band "S", in the northern hemisphere.
-        // TODO: Check with the user what they mean, or use last known location
-      } else {
-        // The northing is not within the range of grid zone "S", so we assume
-        // the user meant "Southern" with the letter "S"
-        northern = false;
-      }
-    } else if (zoneLetter === "N") {
-      const startOfZoneN = 0;
-      const startOfZoneP = 885503.7592863895;
-      if (
-        parsedNorthing &&
-        parsedNorthing >= startOfZoneN &&
-        parsedNorthing < startOfZoneP
-      ) {
-        // Definitely in latitude band N, just use the band letter
-      } else {
-        // Outside latitude band "N", so the user probably means "Northern"
-        northern = true;
-      }
-    }
     try {
+      const parsedNorthing = parseNumber(northing);
+      let northern;
+      // There are two conventions for UTM. One uses a letter to refer to latitude
+      // bands from C to X, excluding "I" and "O". The other uses "N" or "S" to
+      // refer to the northern or southern hemisphere. If the user enters "N" or
+      // "S" we do not know which convention they are using, so we guess. We try
+      // to use the latitude band if we can, since it is better for catching
+      // errors in coordinate entry.
+      if (zoneLetter === "S") {
+        // "S" could refer to grid zone "S" (in the northern hemisphere) or it
+        // could mean "Southern" - conventions differ in different places
+        const startOfZoneS = 3544369.909548157;
+        const startOfZoneT = 4432069.057005376;
+        if (
+          parsedNorthing !== undefined &&
+          parsedNorthing >= startOfZoneS &&
+          parsedNorthing < startOfZoneT
+        ) {
+          // Indeterminate, this could be latitude band S, or it could mean
+          // southern hemisphere. The only place in the southern hemisphere that
+          // matches these coordinates is the very southern tip of Chile and
+          // Argentina, so assume that in this case zoneLetter "S" refers to
+          // latitude band "S", in the northern hemisphere.
+          // TODO: Check with the user what they mean, or use last known location
+        } else {
+          // The northing is not within the range of grid zone "S", so we assume
+          // the user meant "Southern" with the letter "S"
+          northern = false;
+        }
+      } else if (zoneLetter === "N") {
+        const startOfZoneN = 0;
+        const startOfZoneP = 885503.7592863895;
+        if (
+          parsedNorthing &&
+          parsedNorthing >= startOfZoneN &&
+          parsedNorthing < startOfZoneP
+        ) {
+          // Definitely in latitude band N, just use the band letter
+        } else {
+          // Outside latitude band "N", so the user probably means "Northern"
+          northern = true;
+        }
+      }
       return toLatLon(
         parseNumber(easting),
         parseNumber(northing),

diff --git a/src/frontend/screens/SyncModal/index.js b/src/frontend/screens/SyncModal/index.js
@@ -14,6 +14,7 @@ import KeepAwake from "react-native-keep-awake";
 
 import SyncView from "./SyncView";
 import api from "../../api";
+import bugsnag from "../../lib/logger";
 import { withObservations } from "../../context/ObservationsContext";
 import { peerStatus } from "./PeerList";
 import type { ObservationsContext } from "../../context/ObservationsContext";
@@ -89,7 +90,7 @@ class SyncModal extends React.Component<Props, State> {
     try {
       ssid = await NetworkInfo.getSSID();
     } catch (e) {
-      console.error(e);
+      bugsnag.notify(e);
     } finally {
       // Even if we don't get the SSID, we still want to show that a wifi
       // network is connected.

diff --git a/src/frontend/sharedComponents/CameraView.js b/src/frontend/sharedComponents/CameraView.js
@@ -9,6 +9,7 @@ import RNFS from "react-native-fs";
 
 import AddButton from "./AddButton";
 import { promiseTimeout } from "../lib/utils";
+import bugsnag from "../lib/logger";
 import type { CapturePromise } from "../context/DraftObservationContext";
 
 const log = debug("CameraView");
@@ -85,6 +86,7 @@ class CameraView extends React.Component<Props, State> {
         setTimeout(this.props.onAddPress, 0, e, capture);
       }
     );
+    capture.catch(bugsnag.notify);
     capture.finally(() => {
       if (!this.mounted) return;
       this.setState({ takingPicture: false });
@@ -127,20 +129,23 @@ function rotatePhoto(acc: Acceleration) {
       rotation
     )
       .then(({ uri }) => {
-        log("Rotated photo");
+        bugsnag.leaveBreadcrumb("Rotate photo", { type: "process" });
         // Image resizer uses `JPEG` as the extension, which gets passed through
         // to mapeo-core media store. Change to `jpg` to match legacy photos and
         // avoid issues on Windows (don't know if it recognizes `JPEG`)
         resizedUri = uri.replace(/\.JPEG$/, ".jpg");
         return RNFS.moveFile(uri, resizedUri);
       })
       .then(() => {
-        log("Renamed captured photo");
+        bugsnag.leaveBreadcrumb("Rename photo", { type: "process" });
         RNFS.unlink(originalUri).then(() => log("Cleaned up un-rotated photo"));
         return { uri: resizedUri };
       })
-      .catch(() => {
-        log("Error rotating photo, returning un-rotated photo");
+      .catch(e => {
+        bugsnag.notify(e, report => {
+          report.errorMessage = "Error rotating photo";
+          report.severity = "warning";
+        });
         // Some devices throw an error trying to rotate the photo, so worst-case
         // we just don't rotate
         return { uri: originalUri, rotate: rotation };