Bug 1298752 - Avoid including the profile directory in sync pings. r=tcsc, a=ritu
☠☠ backed out by b16b9d1eea13 ☠ ☠
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 31 Aug 2016 22:42:58 -0400
changeset 350065 82eb0241b83c35f4031525d50e651008c647633a
parent 350064 1636a87ec69424d84aaa3fc00ff1894c658221a1
child 350066 9731047ebbfa20df44816f7874f57bc8eef9f318
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc, ritu
bugs1298752
milestone50.0a2
Bug 1298752 - Avoid including the profile directory in sync pings. r=tcsc, a=ritu MozReview-Commit-ID: IA8jQ8No1Rv
services/sync/modules-testing/fakeservices.js
services/sync/modules/telemetry.js
services/sync/tests/unit/test_telemetry.js
--- a/services/sync/modules-testing/fakeservices.js
+++ b/services/sync/modules-testing/fakeservices.js
@@ -17,16 +17,30 @@ Cu.import("resource://services-sync/reco
 Cu.import("resource://services-sync/util.js");
 
 var btoa = Cu.import("resource://gre/modules/Log.jsm").btoa;
 
 this.FakeFilesystemService = function FakeFilesystemService(contents) {
   this.fakeContents = contents;
   let self = this;
 
+  // Save away the unmocked versions of the functions we replace here for tests
+  // that really want the originals. As this may be called many times per test,
+  // we must be careful to not replace them with ones we previously replaced.
+  // (And WTF are we bothering with these mocks in the first place? Is the
+  // performance of the filesystem *really* such that it outweighs the downside
+  // of not running our real JSON functions in the tests? Eg, these mocks don't
+  // always throw exceptions when the real ones do. Anyway...)
+  for (let name of ["jsonSave", "jsonLoad", "jsonMove", "jsonRemove"]) {
+    let origName = "_real_" + name;
+    if (!Utils[origName]) {
+      Utils[origName] = Utils[name];
+    }
+  }
+
   Utils.jsonSave = function jsonSave(filePath, that, obj, callback) {
     let json = typeof obj == "function" ? obj.call(that) : obj;
     self.fakeContents["weave/" + filePath + ".json"] = JSON.stringify(json);
     callback.call(that);
   };
 
   Utils.jsonLoad = function jsonLoad(filePath, that, cb) {
     let obj;
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -13,16 +13,17 @@ Cu.import("resource://services-sync/main
 Cu.import("resource://services-sync/status.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-common/observers.js");
 Cu.import("resource://services-common/async.js");
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://gre/modules/TelemetryController.jsm");
 Cu.import("resource://gre/modules/FxAccounts.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+Cu.import("resource://gre/modules/osfile.jsm", this);
 
 let constants = {};
 Cu.import("resource://services-sync/constants.js", constants);
 
 var fxAccountsCommon = {};
 Cu.import("resource://gre/modules/FxAccountsCommon.js", fxAccountsCommon);
 
 XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",
@@ -45,16 +46,24 @@ const TOPICS = [
 ];
 
 const PING_FORMAT_VERSION = 1;
 
 // The set of engines we record telemetry for - any other engines are ignored.
 const ENGINES = new Set(["addons", "bookmarks", "clients", "forms", "history",
                          "passwords", "prefs", "tabs"]);
 
+// A regex we can use to replace the profile dir in error messages. We use a
+// regexp so we can simply replace all case-insensitive occurences.
+// This escaping function is from:
+// https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/Regular_Expressions
+const reProfileDir = new RegExp(
+        OS.Constants.Path.profileDir.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"),
+        "gi");
+
 // is it a wrapped auth error from browserid_identity?
 function isBrowerIdAuthError(error) {
   // I can't think of what could throw on String conversion
   // but we have absolutely no clue about the type, and
   // there's probably some things out there that would
   try {
     if (String(error).startsWith("AuthenticationError")) {
       return true;
@@ -68,17 +77,19 @@ function transformError(error, engineNam
     return { name: "shutdownerror" };
   }
 
   if (typeof error === "string") {
     if (error.startsWith("error.")) {
       // This is hacky, but I can't imagine that it's not also accurate.
       return { name: "othererror", error };
     }
-
+    // There's a chance the profiledir is in the error string which is PII we
+    // want to avoid including in the ping.
+    error = error.replace(reProfileDir, "[profileDir]");
     return { name: "unexpectederror", error };
   }
 
   if (error.failureCode) {
     return { name: "othererror", error: error.failureCode };
   }
 
   if (error instanceof AuthenticationError) {
@@ -94,17 +105,18 @@ function transformError(error, engineNam
   }
 
   if (error.result) {
     return { name: "nserror", code: error.result };
   }
 
   return {
     name: "unexpectederror",
-    error: String(error),
+    // as above, remove the profile dir value.
+    error: String(error).replace(reProfileDir, "[profileDir]")
   }
 }
 
 function tryGetMonotonicTimestamp() {
   try {
     return Telemetry.msSinceProcessStart();
   } catch (e) {
     log.warn("Unable to get a monotonic timestamp!");
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -8,16 +8,17 @@ Cu.import("resource://services-sync/reco
 Cu.import("resource://services-sync/resource.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/engines/clients.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 Cu.import("resource://testing-common/services/sync/fxa_utils.js");
 Cu.import("resource://testing-common/services/sync/rotaryengine.js");
+Cu.import("resource://gre/modules/osfile.jsm", this);
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://services-sync/util.js");
 
 initTestLogging("Trace");
 
 function SteamStore(engine) {
   Store.call(this, "Steam", engine);
@@ -337,16 +338,52 @@ add_task(function* test_generic_engine_f
       error: String(e)
     });
   } finally {
     Service.engineManager.unregister(engine);
     yield cleanAndGo(server);
   }
 });
 
+add_task(function* test_engine_fail_ioerror() {
+  Service.engineManager.register(SteamEngine);
+  let engine = Service.engineManager.get("steam");
+  engine.enabled = true;
+  let store  = engine._store;
+  let server = serverForUsers({"foo": "password"}, {
+    meta: {global: {engines: {steam: {version: engine.version,
+                                      syncID: engine.syncID}}}},
+    steam: {}
+  });
+  new SyncTestingInfrastructure(server.server);
+  // create an IOError to re-throw as part of Sync.
+  try {
+    // (Note that fakeservices.js has replaced Utils.jsonMove etc, but for
+    // this test we need the real one so we get real exceptions from the
+    // filesystem.)
+    yield Utils._real_jsonMove("file-does-not-exist", "anything", {});
+  } catch (ex) {
+    engine._errToThrow = ex;
+  }
+  ok(engine._errToThrow, "expecting exception");
+
+  try {
+    let ping = yield sync_and_validate_telem(true);
+    equal(ping.status.service, SYNC_FAILED_PARTIAL);
+    let failureReason = ping.engines.find(e => e.name === "steam").failureReason;
+    equal(failureReason.name, "unexpectederror");
+    // ensure the profile dir in the exception message has been stripped.
+    ok(!failureReason.error.includes(OS.Constants.Path.profileDir), failureReason.error);
+    ok(failureReason.error.includes("[profileDir]"), failureReason.error);
+  } finally {
+    Service.engineManager.unregister(engine);
+    yield cleanAndGo(server);
+  }
+});
+
 add_task(function* test_initial_sync_engines() {
   Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   let store  = engine._store;
   let engines = {};
   // These are the only ones who actually have things to sync at startup.
   let engineNames = ["clients", "bookmarks", "prefs", "tabs"];