Bug 1407082 - Handle more kinds of errors in SyncTelemetry.transformError. r?kitcambridge draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 10 Oct 2017 12:31:42 -0400
changeset 678786 c46e758113e2aff9e4488bdac35b9bc79aacd88b
parent 676930 f8a337cc1e677266d9711e54e6024061460d7e96
child 735437 f951782cbea43a3cdfe1a06bc9684bf78657623a
push id84039
push userbmo:tchiovoloni@mozilla.com
push dateWed, 11 Oct 2017 20:37:31 +0000
reviewerskitcambridge
bugs1407082
milestone58.0a1
Bug 1407082 - Handle more kinds of errors in SyncTelemetry.transformError. r?kitcambridge MozReview-Commit-ID: CjwY7w7vUqs
services/sync/modules/telemetry.js
services/sync/tests/unit/test_telemetry.js
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -411,18 +411,19 @@ function cleanErrorMessage(error) {
   error = error.replace(reProfileDir, "[profileDir]");
   // MSG_INVALID_URL from /dom/bindings/Errors.msg -- no way to access this
   // directly from JS.
   if (error.endsWith("is not a valid URL.")) {
     error = "<URL> is not a valid URL.";
   }
   // Try to filter things that look somewhat like a URL (in that they contain a
   // colon in the middle of non-whitespace), in case anything else is including
-  // these in error messages.
-  error = error.replace(/\S+:\S+/g, "<URL>");
+  // these in error messages. Note that JSON.stringified stuff comes through
+  // here, so we explicitly ignore double-quotes as well.
+  error = error.replace(/[^\s"]+:[^\s"]+/g, "<URL>");
   return error;
 }
 
 class SyncTelemetryImpl {
   constructor(allowedEngines) {
     log.level = Log.Level[Svc.Prefs.get("log.logger.telemetry", "Trace")];
     // This is accessible so we can enable custom engines during tests.
     this.allowedEngines = allowedEngines;
@@ -708,19 +709,31 @@ class SyncTelemetryImpl {
 
     if (httpCode) {
       return { name: "httperror", code: httpCode };
     }
 
     if (error.result) {
       return { name: "nserror", code: error.result };
     }
-
+    // It's probably an Error object, but it also could be some
+    // other object that may or may not override toString to do
+    // something useful.
+    let msg = String(error);
+    if (msg.startsWith("[object")) {
+      // Nothing useful in the default, check for a string "message" property.
+      if (typeof error.message == "string") {
+        msg = String(error.message);
+      } else {
+        // Hopefully it won't come to this...
+        msg = JSON.stringify(error);
+      }
+    }
     return {
       name: "unexpectederror",
-      error: cleanErrorMessage(String(error))
+      error: cleanErrorMessage(msg)
     };
   }
 
 }
 
 /* global SyncTelemetry */
 this.SyncTelemetry = new SyncTelemetryImpl(ENGINES);
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -355,16 +355,46 @@ add_task(async function test_generic_eng
       error: String(e)
     });
   } finally {
     await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
   }
 });
 
+add_task(async function test_engine_fail_weird_errors() {
+  enableValidationPrefs();
+  await Service.engineManager.register(SteamEngine);
+  let engine = Service.engineManager.get("steam");
+  engine.enabled = true;
+  let server = await serverForFoo(engine);
+  await SyncTestingInfrastructure(server);
+  try {
+    let msg = "Bad things happened!"
+    engine._errToThrow = { message: msg };
+    let ping = await sync_and_validate_telem(true);
+    equal(ping.status.service, SYNC_FAILED_PARTIAL);
+    deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
+      name: "unexpectederror",
+      error: "Bad things happened!"
+    });
+    let e = { msg };
+    engine._errToThrow = e;
+    ping = await sync_and_validate_telem(true);
+    deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
+      name: "unexpectederror",
+      error: JSON.stringify(e)
+    });
+
+  } finally {
+    await cleanAndGo(engine, server);
+    Service.engineManager.unregister(engine);
+  }
+});
+
 add_task(async function test_engine_fail_ioerror() {
   enableValidationPrefs();
 
   await Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   let server = await serverForFoo(engine);
   await SyncTestingInfrastructure(server);