Backed out changeset 82eb0241b83c (bug 1298752) for test_telemetry.js failures.
authorRyan VanderMeulen <ryanvm@gmail.com>
Fri, 02 Sep 2016 07:49:51 -0400
changeset 350077 b16b9d1eea136bba9d512610e79700192be4e646
parent 350076 fde69933d9cc084bd7bcc70d7304b8ca032b67d8
child 350078 ff1e5019f0c9071165e4a19a65217f807d5267df
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)
bugs1298752
milestone50.0a2
backs out82eb0241b83c35f4031525d50e651008c647633a
Backed out changeset 82eb0241b83c (bug 1298752) for test_telemetry.js failures.
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,30 +17,16 @@ 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,17 +13,16 @@ 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",
@@ -46,24 +45,16 @@ 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;
@@ -77,19 +68,17 @@ 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) {
@@ -105,18 +94,17 @@ function transformError(error, engineNam
   }
 
   if (error.result) {
     return { name: "nserror", code: error.result };
   }
 
   return {
     name: "unexpectederror",
-    // as above, remove the profile dir value.
-    error: String(error).replace(reProfileDir, "[profileDir]")
+    error: String(error),
   }
 }
 
 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,17 +8,16 @@ 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);
@@ -338,52 +337,16 @@ 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"];