Bug 1131410 - Extract sync's log management so it can be reused by the reading-list back-end. r=adw

services/common/logmanager.js

@@ -0,0 +1,277 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
+
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, 'Services',
+  'resource://gre/modules/Services.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, 'Preferences',
+  'resource://gre/modules/Preferences.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, 'FileUtils',
+  'resource://gre/modules/FileUtils.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, 'Log',
+  'resource://gre/modules/Log.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, 'OS',
+  'resource://gre/modules/osfile.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, 'CommonUtils',
+  'resource://services-common/utils.js');
+Cu.import("resource://gre/modules/Task.jsm");
+
+this.EXPORTED_SYMBOLS = [
+  "LogManager",
+];
+
+const DEFAULT_MAX_ERROR_AGE = 20 * 24 * 60 * 60; // 20 days
+
+// "shared" logs (ie, where the same log name is used by multiple LogManager
+// instances) are a fact of life here - eg, FirefoxAccounts logs are used by
+// both Sync and Reading-list.
+// However, different instances have different pref branches, so we need to
+// handle when one pref branch says "Debug" and the other says "Error"
+// So we (a) keep singleton console and dump appenders and (b) keep track
+// of the minimum (ie, most verbose) level and use that.
+// This avoids (a) the most recent setter winning (as that is indeterminate)
+// and (b) multiple dump/console appenders being added to the same log multiple
+// times, which would cause messages to appear twice.
+
+// Singletons used by each instance.
+let formatter;
+let dumpAppender;
+let consoleAppender;
+
+// A set of all preference roots used by all instances.
+let allBranches = new Set();
+
+// The public LogManager object.
+function LogManager(prefRoot, logNames, logFilePrefix) {
+  this.init(prefRoot, logNames, logFilePrefix);
+}
+
+LogManager.prototype = {
+  REASON_SUCCESS: "success",
+  REASON_ERROR: "error",
+
+  _cleaningUpFileLogs: false,
+
+  _prefObservers: [],
+
+  init(prefRoot, logNames, logFilePrefix) {
+    if (prefRoot instanceof Preferences) {
+      this._prefs = prefRoot;
+    } else {
+      this._prefs = new Preferences(prefRoot);
+    }
+
+    this.logFilePrefix = logFilePrefix;
+    if (!formatter) {
+      // Create a formatter and various appenders to attach to the logs.
+      formatter = new Log.BasicFormatter();
+      consoleAppender = new Log.ConsoleAppender(formatter);
+      dumpAppender = new Log.DumpAppender(formatter);
+    }
+
+    allBranches.add(this._prefs._branchStr);
+    // We create a preference observer for all our prefs so they are magically
+    // reflected if the pref changes after creation.
+    let setupAppender = (appender, prefName, defaultLevel, findSmallest = false) => {
+      let observer = newVal => {
+        let level = Log.Level[newVal] || defaultLevel;
+        if (findSmallest) {
+          // We need to find the smallest value from all prefs controlling this appender.
+          for (let branch of allBranches) {
+            let lookPrefBranch = new Preferences(branch);
+            let lookVal = Log.Level[lookPrefBranch.get(prefName)];
+            if (lookVal && lookVal < level) {
+              level = lookVal;
+            }
+          }
+        }
+        appender.level = level;
+      }
+      this._prefs.observe(prefName, observer, this);
+      this._prefObservers.push([prefName, observer]);
+      // and call the observer now with the current pref value.
+      observer(this._prefs.get(prefName));
+      return observer;
+    }
+
+    this._observeConsolePref = setupAppender(consoleAppender, "log.appender.console", Log.Level.Error, true);
+    this._observeDumpPref = setupAppender(dumpAppender, "log.appender.dump", Log.Level.Error, true);
+
+    // The file appender doesn't get the special singleton behaviour.
+    let fapp = this._fileAppender = new Log.StorageStreamAppender(formatter);
+    // the stream gets a default of Debug as the user must go out of there way
+    // to see the stuff spewed to it.
+    this._observeStreamPref = setupAppender(fapp, "log.appender.file.level", Log.Level.Debug);
+
+    // now attach the appenders to all our logs.
+    for (let logName of logNames) {
+      let log = Log.repository.getLogger(logName);
+      for (let appender of [fapp, dumpAppender, consoleAppender]) {
+        log.addAppender(appender);
+      }
+    }
+    // and use the first specified log as a "root" for our log.
+    this._log = Log.repository.getLogger(logNames[0] + ".LogManager");
+  },
+
+  /**
+   * Cleanup this instance
+   */
+  finalize() {
+    for (let [name, pref] of this._prefObservers) {
+      this._prefs.ignore(name, pref, this);
+    }
+    this._prefObservers = [];
+    try {
+      allBranches.delete(this._prefs._branchStr);
+    } catch (e) {}
+    this._prefs = null;
+  },
+
+  get _logFileDirectory() {
+    // At this point we don't allow a custom directory for the logs so
+    // about:sync-log can be used. We could fix this later if necessary.
+    return FileUtils.getDir("ProfD", ["weave", "logs"]);
+  },
+
+  /**
+   * Copy an input stream to the named file, doing everything off the main
+   * thread.
+   * outputFile is an nsIFile, but is used only for the name.
+   * Returns a promise that is resolved with the file modification date on
+   * completion or rejected if there is an error.
+   */
+  _copyStreamToFile: Task.async(function* (inputStream, outputFile) {
+    // The log data could be large, so we don't want to pass it all in a single
+    // message, so use BUFFER_SIZE chunks.
+    const BUFFER_SIZE = 8192;
+
+    // get a binary stream
+    let binaryStream = Cc['@mozilla.org/binaryinputstream;1'].createInstance(Ci.nsIBinaryInputStream);
+    binaryStream.setInputStream(inputStream);
+    // XXX - this somewhat breaks the abstraction of this._logFileDirectory, but
+    // is necessary until bug 1056442 helps.
+    let profileDir = Services.dirsvc.get("ProfD", Ci.nsIFile).path;
+    yield OS.File.makeDir(outputFile.parent.path, { ignoreExisting: true,
+                                                    from: profileDir });
+    let output = yield OS.File.open(outputFile.path, { write: true} );
+    try {
+      while (true) {
+        let available = binaryStream.available();
+        if (!available) {
+          break;
+        }
+        let chunk = binaryStream.readByteArray(Math.min(available, BUFFER_SIZE));
+        yield output.write(new Uint8Array(chunk));
+      }
+    } finally {
+      inputStream.close();
+      binaryStream.close();
+      yield output.close();
+    }
+    this._log.trace("finished copy to", outputFile.path);
+  }),
+
+  /**
+   * Possibly generate a log file for all accumulated log messages and refresh
+   * the input & output streams.
+   * Returns a promise that resolves on completion or rejects if the file could
+   * not be written.
+   */
+  resetFileLog: Task.async(function* (reason) {
+    try {
+      let flushToFile;
+      let reasonPrefix;
+      switch (reason) {
+        case this.REASON_SUCCESS:
+          flushToFile = this._prefs.get("log.appender.file.logOnSuccess", false);
+          reasonPrefix = "success";
+          break;
+        case this.REASON_ERROR:
+          flushToFile = this._prefs.get("log.appender.file.logOnError", true);
+          reasonPrefix = "error";
+          break;
+        default:
+          throw new Error("Invalid reason");
+      }
+
+      // might as well avoid creating an input stream if we aren't going to use it.
+      if (!flushToFile) {
+        this._fileAppender.reset();
+        return;
+      }
+
+      let inStream = this._fileAppender.getInputStream();
+      this._fileAppender.reset();
+      if (inStream) {
+        this._log.debug("Flushing file log");
+        let filename = this.logFilePrefix + "-" + reasonPrefix + "-" + Date.now() + ".txt";
+        let file = this._logFileDirectory;
+        file.append(filename);
+        this._log.trace("Beginning stream copy to " + file.leafName + ": " +
+                        Date.now());
+        try {
+          yield this._copyStreamToFile(inStream, file);
+          this._log.trace("onCopyComplete", Date.now());
+        } catch (ex) {
+          this._log.error("Failed to copy log stream to file", ex);
+          return;
+        }
+        // It's not completely clear to markh why we only do log cleanups
+        // for errors, but for now the Sync semantics have been copied...
+        // (one theory is that only cleaning up on error makes it less
+        // likely old error logs would be removed, but that's not true if
+        // there are occasional errors - let's address this later!)
+        if (reason == this.REASON_ERROR && !this._cleaningUpFileLogs) {
+          this._log.trace("Scheduling cleanup.");
+          // Note we don't return/yield or otherwise wait on this promise - it
+          // continues in the background
+          this.cleanupLogs().catch(err => {
+            this._log.error("Failed to cleanup logs", err);
+          });
+        }
+      }
+    } catch (ex) {
+      this._log.error("Failed to resetFileLog", ex)
+      reject(ex);
+    }
+  }),
+
+  /**
+   * Finds all logs older than maxErrorAge and deletes them using async I/O.
+   */
+  cleanupLogs: Task.async(function* () {
+    this._cleaningUpFileLogs = true;
+    let iterator = new OS.File.DirectoryIterator(this._logFileDirectory.path);
+    let maxAge = this._prefs.get("log.appender.file.maxErrorAge", DEFAULT_MAX_ERROR_AGE);
+    let threshold = Date.now() - 1000 * maxAge;
+
+    this._log.debug("Log cleanup threshold time: " + threshold);
+    yield iterator.forEach(Task.async(function* (entry) {
+      if (!entry.name.startsWith(this.logFilePrefix + "-")) {
+        return;
+      }
+      try {
+        // need to call .stat() as the enumerator doesn't give that to us on *nix.
+        let info = yield OS.File.stat(entry.path);
+        if (info.lastModificationDate.getTime() >= threshold) {
+          return;
+        }
+        this._log.trace(" > Cleanup removing " + entry.name +
+                        " (" + info.lastModificationDate.getTime() + ")");
+        yield OS.File.remove(entry.path);
+        this._log.trace("Deleted " + entry.name);
+      } catch (ex) {
+        this._log.debug("Encountered error trying to clean up old log file "
+                        + entry.name, ex);
+      }
+    }.bind(this)));
+    this._cleaningUpFileLogs = false;
+    this._log.debug("Done deleting files.");
+    // This notification is used only for tests.
+    Services.obs.notifyObservers(null, "services-tests:common:log-manager:cleanup-logs", null);
+  }),
+}

services/common/moz.build

@@ -13,6 +13,7 @@ EXTRA_COMPONENTS += [
 EXTRA_JS_MODULES['services-common'] += [
     'hawkclient.js',
     'hawkrequest.js',
+    'logmanager.js',
     'storageservice.js',
     'stringbundle.js',
     'tokenserverclient.js',

services/common/tests/unit/test_load_modules.js

@@ -4,6 +4,7 @@
 const modules = [
   "async.js",
   "bagheeraclient.js",
+  "logmanager.js",
   "rest.js",
   "storageservice.js",
   "stringbundle.js",

services/common/tests/unit/test_logmanager.js

@@ -0,0 +1,103 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// NOTE: The sync test_errorhandler_* tests have quite good coverage for
+// other aspects of this.
+
+Cu.import("resource://services-common/logmanager.js");
+Cu.import("resource://gre/modules/Log.jsm");
+
+function run_test() {
+  run_next_test();
+}
+
+// Returns an array of [consoleAppender, dumpAppender, [fileAppenders]] for
+// the specified log.  Note that fileAppenders will usually have length=1
+function getAppenders(log) {
+  let capps = log.appenders.filter(app => app instanceof Log.ConsoleAppender);
+  equal(capps.length, 1, "should only have one console appender");
+  let dapps = log.appenders.filter(app => app instanceof Log.DumpAppender);
+  equal(dapps.length, 1, "should only have one dump appender");
+  let fapps = log.appenders.filter(app => app instanceof Log.StorageStreamAppender);
+  return [capps[0], dapps[0], fapps];
+}
+
+// Test that the correct thing happens when no prefs exist for the log manager.
+add_task(function* test_noPrefs() {
+  // tell the log manager to init with a pref branch that doesn't exist.
+  let lm = new LogManager("no-such-branch.", ["TestLog"], "test");
+
+  let log = Log.repository.getLogger("TestLog");
+  let [capp, dapp, fapps] = getAppenders(log);
+  // the "dump" and "console" appenders should get Error level
+  equal(capp.level, Log.Level.Error);
+  equal(dapp.level, Log.Level.Error);
+  // and the file (stream) appender gets Dump by default
+  equal(fapps.length, 1, "only 1 file appender");
+  equal(fapps[0].level, Log.Level.Debug);
+  lm.finalize();
+});
+
+// Test that changes to the prefs used by the log manager are updated dynamically.
+add_task(function* test_PrefChanges() {
+  Services.prefs.setCharPref("log-manager.test.log.appender.console", "Trace");
+  Services.prefs.setCharPref("log-manager.test.log.appender.dump", "Trace");
+  Services.prefs.setCharPref("log-manager.test.log.appender.file.level", "Trace");
+  let lm = new LogManager("log-manager.test.", ["TestLog2"], "test");
+
+  let log = Log.repository.getLogger("TestLog2");
+  let [capp, dapp, [fapp]] = getAppenders(log);
+  equal(capp.level, Log.Level.Trace);
+  equal(dapp.level, Log.Level.Trace);
+  equal(fapp.level, Log.Level.Trace);
+  // adjust the prefs and they should magically be reflected in the appenders.
+  Services.prefs.setCharPref("log-manager.test.log.appender.console", "Debug");
+  Services.prefs.setCharPref("log-manager.test.log.appender.dump", "Debug");
+  Services.prefs.setCharPref("log-manager.test.log.appender.file.level", "Debug");
+  equal(capp.level, Log.Level.Debug);
+  equal(dapp.level, Log.Level.Debug);
+  equal(fapp.level, Log.Level.Debug);
+  // and invalid values should cause them to fallback to their defaults.
+  Services.prefs.setCharPref("log-manager.test.log.appender.console", "xxx");
+  Services.prefs.setCharPref("log-manager.test.log.appender.dump", "xxx");
+  Services.prefs.setCharPref("log-manager.test.log.appender.file.level", "xxx");
+  equal(capp.level, Log.Level.Error);
+  equal(dapp.level, Log.Level.Error);
+  equal(fapp.level, Log.Level.Debug);
+  lm.finalize();
+});
+
+// Test that the same log used by multiple log managers does the right thing.
+add_task(function* test_SharedLogs() {
+  // create the prefs for the first instance.
+  Services.prefs.setCharPref("log-manager-1.test.log.appender.console", "Trace");
+  Services.prefs.setCharPref("log-manager-1.test.log.appender.dump", "Trace");
+  Services.prefs.setCharPref("log-manager-1.test.log.appender.file.level", "Trace");
+  let lm1 = new LogManager("log-manager-1.test.", ["TestLog3"], "test");
+
+  // and the second.
+  Services.prefs.setCharPref("log-manager-2.test.log.appender.console", "Debug");
+  Services.prefs.setCharPref("log-manager-2.test.log.appender.dump", "Debug");
+  Services.prefs.setCharPref("log-manager-2.test.log.appender.file.level", "Debug");
+  let lm2 = new LogManager("log-manager-2.test.", ["TestLog3"], "test");
+
+  let log = Log.repository.getLogger("TestLog3");
+  let [capp, dapp, fapps] = getAppenders(log);
+
+  // console and dump appenders should be "trace" as it is more verbose than
+  // "debug"
+  equal(capp.level, Log.Level.Trace);
+  equal(dapp.level, Log.Level.Trace);
+
+  // Set the prefs on the -1 branch to "Error" - it should then end up with
+  // "Debug" from the -2 branch.
+  Services.prefs.setCharPref("log-manager-1.test.log.appender.console", "Error");
+  Services.prefs.setCharPref("log-manager-1.test.log.appender.dump", "Error");
+  Services.prefs.setCharPref("log-manager-1.test.log.appender.file.level", "Error");
+
+  equal(capp.level, Log.Level.Debug);
+  equal(dapp.level, Log.Level.Debug);
+
+  lm1.finalize();
+  lm2.finalize();
+});

services/common/tests/unit/xpcshell.ini

@@ -29,6 +29,7 @@ skip-if = toolkit == 'gonk'
 [test_bagheera_client.js]
 [test_hawkclient.js]
 [test_hawkrequest.js]
+[test_logmanager.js]
 [test_observers.js]
 [test_restrequest.js]
 [test_tokenauthenticatedrequest.js]

services/sync/modules/policies.js

@@ -13,8 +13,7 @@ Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/util.js");
-Cu.import("resource://gre/modules/FileUtils.jsm");
-Cu.import("resource://gre/modules/NetUtil.jsm");
+Cu.import("resource://services-common/logmanager.js");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Status",
                                   "resource://services-sync/status.js");
@@ -539,9 +538,6 @@ SyncScheduler.prototype = {
   },
 };
 
-const LOG_PREFIX_SUCCESS = "success-";
-const LOG_PREFIX_ERROR   = "error-";
-
 this.ErrorHandler = function ErrorHandler(service) {
   this.service = service;
   this.init();
@@ -574,33 +570,14 @@ ErrorHandler.prototype = {
   initLogs: function initLogs() {
     this._log = Log.repository.getLogger("Sync.ErrorHandler");
     this._log.level = Log.Level[Svc.Prefs.get("log.logger.service.main")];
-    this._cleaningUpFileLogs = false;
 
     let root = Log.repository.getLogger("Sync");
     root.level = Log.Level[Svc.Prefs.get("log.rootLogger")];
 
-    let formatter = new Log.BasicFormatter();
-    let capp = new Log.ConsoleAppender(formatter);
-    capp.level = Log.Level[Svc.Prefs.get("log.appender.console")];
-    root.addAppender(capp);
+    let logs = ["Sync", "FirefoxAccounts", "Hawk", "Common.TokenServerClient",
+                "Sync.SyncMigration"];
 
-    let dapp = new Log.DumpAppender(formatter);
-    dapp.level = Log.Level[Svc.Prefs.get("log.appender.dump")];
-    root.addAppender(dapp);
-
-    let fapp = this._logAppender = new Log.StorageStreamAppender(formatter);
-    fapp.level = Log.Level[Svc.Prefs.get("log.appender.file.level")];
-    root.addAppender(fapp);
-
-    // Arrange for a number of other sync-related logs to also go to our
-    // appenders.
-    for (let extra of ["FirefoxAccounts", "Hawk", "Common.TokenServerClient",
-                       "Sync.SyncMigration"]) {
-      let log = Log.repository.getLogger(extra);
-      for (let appender of [fapp, dapp, capp]) {
-        log.addAppender(appender);
-      }
-    }
+    this._logManager = new LogManager(Svc.Prefs, logs, "sync");
   },
 
   observe: function observe(subject, topic, data) {
@@ -625,8 +602,7 @@ ErrorHandler.prototype = {
         this._log.debug(engine_name + " failed: " + Utils.exceptionStr(exception));
         break;
       case "weave:service:login:error":
-        this.resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"),
-                          LOG_PREFIX_ERROR);
+        this.resetFileLog(this._logManager.REASON_ERROR);
 
         if (this.shouldReportError()) {
           this.notifyOnNextTick("weave:ui:login:error");
@@ -641,8 +617,7 @@ ErrorHandler.prototype = {
           this.service.logout();
         }
 
-        this.resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"),
-                          LOG_PREFIX_ERROR);
+        this.resetFileLog(this._logManager.REASON_ERROR);
 
         if (this.shouldReportError()) {
           this.notifyOnNextTick("weave:ui:sync:error");
@@ -668,8 +643,7 @@ ErrorHandler.prototype = {
 
         if (Status.service == SYNC_FAILED_PARTIAL) {
           this._log.debug("Some engines did not sync correctly.");
-          this.resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"),
-                            LOG_PREFIX_ERROR);
+          this.resetFileLog(this._logManager.REASON_ERROR);
 
           if (this.shouldReportError()) {
             this.dontIgnoreErrors = false;
@@ -677,8 +651,7 @@ ErrorHandler.prototype = {
             break;
           }
         } else {
-          this.resetFileLog(Svc.Prefs.get("log.appender.file.logOnSuccess"),
-                            LOG_PREFIX_SUCCESS);
+          this.resetFileLog(this._logManager.REASON_SUCCESS);
         }
         this.dontIgnoreErrors = false;
         this.notifyOnNextTick("weave:ui:sync:finish");
@@ -705,95 +678,22 @@ ErrorHandler.prototype = {
     Utils.nextTick(this.service.sync, this.service);
   },
 
-  /**
-   * Finds all logs older than maxErrorAge and deletes them without tying up I/O.
-   */
-  cleanupLogs: function cleanupLogs() {
-    let direntries = FileUtils.getDir("ProfD", ["weave", "logs"]).directoryEntries;
-    let oldLogs = [];
-    let index = 0;
-    let threshold = Date.now() - 1000 * Svc.Prefs.get("log.appender.file.maxErrorAge");
-
-    this._log.debug("Log cleanup threshold time: " + threshold);
-    while (direntries.hasMoreElements()) {
-      let logFile = direntries.getNext().QueryInterface(Ci.nsIFile);
-      if (logFile.lastModifiedTime < threshold) {
-        this._log.trace(" > Noting " + logFile.leafName +
-                        " for cleanup (" + logFile.lastModifiedTime + ")");
-        oldLogs.push(logFile);
-      }
-    }
-
-    // Deletes a file from oldLogs each tick until there are none left.
-    let errorHandler = this;
-    function deleteFile() {
-      if (index >= oldLogs.length) {
-        errorHandler._log.debug("Done deleting files.");
-        errorHandler._cleaningUpFileLogs = false;
-        Svc.Obs.notify("weave:service:cleanup-logs");
-        return;
-      }
-      try {
-        let file = oldLogs[index];
-        file.remove(false);
-        errorHandler._log.trace("Deleted " + file.leafName + ".");
-      } catch (ex) {
-        errorHandler._log._debug("Encountered error trying to clean up old log file '"
-                                 + oldLogs[index].leafName + "':"
-                                 + Utils.exceptionStr(ex));
-      }
-      index++;
-      Utils.nextTick(deleteFile);
-    }
-
-    if (oldLogs.length > 0) {
-      this._cleaningUpFileLogs = true;
-      Utils.nextTick(deleteFile);
-    } else {
-      this._log.debug("No logs to clean up.");
-    }
-  },
-
   /**
    * Generate a log file for the sync that just completed
    * and refresh the input & output streams.
    *
-   * @param flushToFile
-   *        the log file to be flushed/reset
-   *
-   * @param filenamePrefix
-   *        a value of either LOG_PREFIX_SUCCESS or LOG_PREFIX_ERROR
-   *        to be used as the log filename prefix
+   * @param reason
+   *        A constant from the LogManager that indicates the reason for the
+   *        reset.
    */
-  resetFileLog: function resetFileLog(flushToFile, filenamePrefix) {
-    let inStream = this._logAppender.getInputStream();
-    this._logAppender.reset();
-    if (flushToFile && inStream) {
-      this._log.debug("Flushing file log.");
-      try {
-        let filename = filenamePrefix + Date.now() + ".txt";
-        let file = FileUtils.getFile("ProfD", ["weave", "logs", filename]);
-        let outStream = FileUtils.openFileOutputStream(file);
-
-        this._log.trace("Beginning stream copy to " + file.leafName + ": " +
-                        Date.now());
-        NetUtil.asyncCopy(inStream, outStream, function onCopyComplete() {
-          this._log.trace("onCopyComplete: " + Date.now());
-          this._log.trace("Output file timestamp: " + file.lastModifiedTime);
-          Svc.Obs.notify("weave:service:reset-file-log");
-          this._log.trace("Notified: " + Date.now());
-          if (filenamePrefix == LOG_PREFIX_ERROR &&
-              !this._cleaningUpFileLogs) {
-            this._log.trace("Scheduling cleanup.");
-            Utils.nextTick(this.cleanupLogs, this);
-          }
-        }.bind(this));
-      } catch (ex) {
-        Svc.Obs.notify("weave:service:reset-file-log");
-      }
-    } else {
+  resetFileLog: function resetFileLog(reason) {
+    let onComplete = () => {
       Svc.Obs.notify("weave:service:reset-file-log");
-    }
+      this._log.trace("Notified: " + Date.now());
+    };
+    // Note we do not return the promise here - the caller doesn't need to wait
+    // for this to complete.
+    this._logManager.resetFileLog(reason).then(onComplete, onComplete);
   },
 
   /**

services/sync/tests/unit/test_errorhandler.js

@@ -14,8 +14,6 @@ Cu.import("resource://gre/modules/FileUtils.jsm");
 
 const FAKE_SERVER_URL = "http://dummy:9000/";
 const logsdir = FileUtils.getDir("ProfD", ["weave", "logs"], true);
-const LOG_PREFIX_SUCCESS = "success-";
-const LOG_PREFIX_ERROR   = "error-";
 
 const PROLONGED_ERROR_DURATION =
   (Svc.Prefs.get('errorhandler.networkFailureReportTimeout') * 2) * 1000;
@@ -1772,8 +1770,7 @@ add_task(function test_sync_engine_generic_fail() {
       let entries = logsdir.directoryEntries;
       do_check_true(entries.hasMoreElements());
       let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
-      do_check_eq(logfile.leafName.slice(0, LOG_PREFIX_ERROR.length),
-                  LOG_PREFIX_ERROR);
+      do_check_true(logfile.leafName.startsWith("sync-error-"), logfile.leafName);
 
       clean();
       server.stop(deferred.resolve);
@@ -1804,8 +1801,7 @@ add_test(function test_logs_on_sync_error_despite_shouldReportError() {
     let entries = logsdir.directoryEntries;
     do_check_true(entries.hasMoreElements());
     let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
-    do_check_eq(logfile.leafName.slice(0, LOG_PREFIX_ERROR.length),
-                LOG_PREFIX_ERROR);
+    do_check_true(logfile.leafName.startsWith("sync-error-"), logfile.leafName);
 
     clean();
     run_next_test();
@@ -1832,8 +1828,7 @@ add_test(function test_logs_on_login_error_despite_shouldReportError() {
     let entries = logsdir.directoryEntries;
     do_check_true(entries.hasMoreElements());
     let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
-    do_check_eq(logfile.leafName.slice(0, LOG_PREFIX_ERROR.length),
-                LOG_PREFIX_ERROR);
+    do_check_true(logfile.leafName.startsWith("sync-error-"), logfile.leafName);
 
     clean();
     run_next_test();
@@ -1867,8 +1862,7 @@ add_task(function test_engine_applyFailed() {
     let entries = logsdir.directoryEntries;
     do_check_true(entries.hasMoreElements());
     let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
-    do_check_eq(logfile.leafName.slice(0, LOG_PREFIX_ERROR.length),
-                LOG_PREFIX_ERROR);
+    do_check_true(logfile.leafName.startsWith("sync-error-"), logfile.leafName);
 
     clean();
     server.stop(deferred.resolve);

services/sync/tests/unit/test_errorhandler_filelog.js

@@ -10,8 +10,6 @@ Cu.import("resource://gre/modules/NetUtil.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 
 const logsdir            = FileUtils.getDir("ProfD", ["weave", "logs"], true);
-const LOG_PREFIX_SUCCESS = "success-";
-const LOG_PREFIX_ERROR   = "error-";
 
 // Delay to wait before cleanup, to allow files to age.
 // This is so large because the file timestamp granularity is per-second, and
@@ -32,6 +30,7 @@ function setLastSync(lastSyncValue) {
 function run_test() {
   initTestLogging("Trace");
 
+  Log.repository.getLogger("Sync.LogManager").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.Service").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.SyncScheduler").level = Log.Level.Trace;
   Log.repository.getLogger("Sync.ErrorHandler").level = Log.Level.Trace;
@@ -41,7 +40,7 @@ function run_test() {
 
 add_test(function test_noOutput() {
   // Ensure that the log appender won't print anything.
-  errorHandler._logAppender.level = Log.Level.Fatal + 1;
+  errorHandler._logManager._fileAppender.level = Log.Level.Fatal + 1;
 
   // Clear log output from startup.
   Svc.Prefs.set("log.appender.file.logOnSuccess", false);
@@ -53,7 +52,7 @@ add_test(function test_noOutput() {
   Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() {
     Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog);
 
-    errorHandler._logAppender.level = Log.Level.Trace;
+    errorHandler._logManager._fileAppender.level = Log.Level.Trace;
     Svc.Prefs.resetBranch("");
     run_next_test();
   });
@@ -109,8 +108,7 @@ add_test(function test_logOnSuccess_true() {
     do_check_true(entries.hasMoreElements());
     let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
     do_check_eq(logfile.leafName.slice(-4), ".txt");
-    do_check_eq(logfile.leafName.slice(0, LOG_PREFIX_SUCCESS.length),
-                LOG_PREFIX_SUCCESS);
+    do_check_true(logfile.leafName.startsWith("sync-success-"), logfile.leafName);
     do_check_false(entries.hasMoreElements());
 
     // Ensure the log message was actually written to file.
@@ -162,6 +160,13 @@ add_test(function test_sync_error_logOnError_true() {
   const MESSAGE = "this WILL show up";
   log.info(MESSAGE);
 
+  // We need to wait until the log cleanup started by this test is complete
+  // or the next test will fail as it is ongoing.
+  Svc.Obs.add("services-tests:common:log-manager:cleanup-logs", function onCleanupLogs() {
+    Svc.Obs.remove("services-tests:common:log-manager:cleanup-logs", onCleanupLogs);
+    run_next_test();
+  });
+
   Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() {
     Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog);
 
@@ -170,8 +175,7 @@ add_test(function test_sync_error_logOnError_true() {
     do_check_true(entries.hasMoreElements());
     let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
     do_check_eq(logfile.leafName.slice(-4), ".txt");
-    do_check_eq(logfile.leafName.slice(0, LOG_PREFIX_ERROR.length),
-                LOG_PREFIX_ERROR);
+    do_check_true(logfile.leafName.startsWith("sync-error-"), logfile.leafName);
     do_check_false(entries.hasMoreElements());
 
     // Ensure the log message was actually written to file.
@@ -188,7 +192,6 @@ add_test(function test_sync_error_logOnError_true() {
       }
 
       Svc.Prefs.resetBranch("");
-      run_next_test();
     });
   });
 
@@ -224,6 +227,13 @@ add_test(function test_login_error_logOnError_true() {
   const MESSAGE = "this WILL show up";
   log.info(MESSAGE);
 
+  // We need to wait until the log cleanup started by this test is complete
+  // or the next test will fail as it is ongoing.
+  Svc.Obs.add("services-tests:common:log-manager:cleanup-logs", function onCleanupLogs() {
+    Svc.Obs.remove("services-tests:common:log-manager:cleanup-logs", onCleanupLogs);
+    run_next_test();
+  });
+
   Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() {
     Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog);
 
@@ -232,8 +242,7 @@ add_test(function test_login_error_logOnError_true() {
     do_check_true(entries.hasMoreElements());
     let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
     do_check_eq(logfile.leafName.slice(-4), ".txt");
-    do_check_eq(logfile.leafName.slice(0, LOG_PREFIX_ERROR.length),
-                LOG_PREFIX_ERROR);
+    do_check_true(logfile.leafName.startsWith("sync-error-"), logfile.leafName);
     do_check_false(entries.hasMoreElements());
 
     // Ensure the log message was actually written to file.
@@ -250,7 +259,6 @@ add_test(function test_login_error_logOnError_true() {
       }
 
       Svc.Prefs.resetBranch("");
-      run_next_test();
     });
   });
 
@@ -273,7 +281,7 @@ add_test(function test_logErrorCleanup_age() {
   _("Making some files.");
   for (let i = 0; i < numLogs; i++) {
     let now = Date.now();
-    let filename = LOG_PREFIX_ERROR + now + "" + i + ".txt";
+    let filename = "sync-error-" + now + "" + i + ".txt";
     let newLog = FileUtils.getFile("ProfD", ["weave", "logs", filename]);
     let foStream = FileUtils.openFileOutputStream(newLog);
     foStream.write(errString, errString.length);
@@ -282,8 +290,8 @@ add_test(function test_logErrorCleanup_age() {
     oldLogs.push(newLog.leafName);
   }
 
-  Svc.Obs.add("weave:service:cleanup-logs", function onCleanupLogs() {
-    Svc.Obs.remove("weave:service:cleanup-logs", onCleanupLogs);
+  Svc.Obs.add("services-tests:common:log-manager:cleanup-logs", function onCleanupLogs() {
+    Svc.Obs.remove("services-tests:common:log-manager:cleanup-logs", onCleanupLogs);
 
     // Only the newest created log file remains.
     let entries = logsdir.directoryEntries;