Bug 1131410 - Extract sync's log management so it can be reused by the reading-list back-end. r=adw a=readinglist
authorMark Hammond <mhammond@skippinet.com.au>
Tue, 17 Feb 2015 22:06:00 +0100
changeset 248476 214e71c08482583542c79e337b1967f608b494c4
parent 248475 d93c3c3ac3221757ac2982def0fa5abea460f1a2
child 248477 312551e075004fcf9a90aefb0c6935831dc60ee6
push id7837
push userjwein@mozilla.com
push dateFri, 27 Mar 2015 00:27:16 +0000
treeherdermozilla-aurora@cb0db44ce60e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, readinglist
bugs1131410
milestone38.0a2
Bug 1131410 - Extract sync's log management so it can be reused by the reading-list back-end. r=adw a=readinglist
services/common/logmanager.js
services/common/moz.build
services/common/tests/unit/test_load_modules.js
services/common/tests/unit/test_logmanager.js
services/common/tests/unit/xpcshell.ini
services/sync/modules/policies.js
services/sync/tests/unit/test_errorhandler.js
services/sync/tests/unit/test_errorhandler_filelog.js
new file mode 100644
--- /dev/null
+++ b/services/common/logmanager.js
@@ -0,0 +1,281 @@
+/* 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, 'Task',
+  'resource://gre/modules/Task.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, 'OS',
+  'resource://gre/modules/osfile.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, 'CommonUtils',
+  'resource://services-common/utils.js');
+
+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);
+    yield OS.File.makeDir(outputFile.parent.path, { ignoreExisting: true });
+    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);
+    return (yield OS.File.stat(outputFile.path)).lastModificationDate;
+  }),
+
+  /**
+   * 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(reason) {
+    return new Promise((resolve, reject) => {
+      try {
+        let flushToFile;
+        let reasonPrefix;
+        switch (reason) {
+          case this.REASON_SUCCESS:
+            flushToFile = this._prefs.get("log.appender.file.logOnSuccess");
+            reasonPrefix = "success";
+            break;
+          case this.REASON_ERROR:
+            flushToFile = this._prefs.get("log.appender.file.logOnError");
+            reasonPrefix = "error";
+            break;
+          default:
+            return reject(new Error("Invalid reason"));
+        }
+
+        let inStream = this._fileAppender.getInputStream();
+        this._fileAppender.reset();
+        if (flushToFile && 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());
+          this._copyStreamToFile(inStream, file).then(
+            modDate => {
+              this._log.trace("onCopyComplete: " + Date.now());
+              this._log.trace("Output file timestamp: " + modDate + " (" + modDate.getTime() + ")");
+            },
+            err => {
+              this._log.error("Failed to copy log stream to file", err)
+              reject(err)
+            }
+          ).then(
+            () => {
+              // 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 or wait on this promise - it continues
+                // in the background
+                this.cleanupLogs().then(null, err => {
+                  this._log.error("Failed to cleanup logs", err);
+                });
+              }
+              resolve();
+            }
+          );
+        } else {
+          resolve();
+        }
+      } 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);
+  }),
+}
--- a/services/common/moz.build
+++ b/services/common/moz.build
@@ -8,16 +8,17 @@ TEST_DIRS += ['tests']
 
 EXTRA_COMPONENTS += [
     'servicesComponents.manifest',
 ]
 
 EXTRA_JS_MODULES['services-common'] += [
     'hawkclient.js',
     'hawkrequest.js',
+    'logmanager.js',
     'storageservice.js',
     'stringbundle.js',
     'tokenserverclient.js',
     'utils.js',
 ]
 
 EXTRA_PP_JS_MODULES['services-common'] += [
     'async.js',
--- a/services/common/tests/unit/test_load_modules.js
+++ b/services/common/tests/unit/test_load_modules.js
@@ -1,14 +1,15 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 const modules = [
   "async.js",
   "bagheeraclient.js",
+  "logmanager.js",
   "rest.js",
   "storageservice.js",
   "stringbundle.js",
   "tokenserverclient.js",
   "utils.js",
 ];
 
 const test_modules = [
new file mode 100644
--- /dev/null
+++ b/services/common/tests/unit/test_logmanager.js
@@ -0,0 +1,107 @@
+/* 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_test(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();
+  run_next_test();
+});
+
+// Test that changes to the prefs used by the log manager are updated dynamically.
+add_test(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();
+  run_next_test();
+});
+
+// Test that the same log used by multiple log managers does the right thing.
+add_test(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();
+
+  run_next_test();
+});
--- a/services/common/tests/unit/xpcshell.ini
+++ b/services/common/tests/unit/xpcshell.ini
@@ -24,16 +24,17 @@ skip-if = toolkit == 'gonk'
 [test_utils_uuid.js]
 
 [test_async_chain.js]
 [test_async_querySpinningly.js]
 [test_bagheera_server.js]
 [test_bagheera_client.js]
 [test_hawkclient.js]
 [test_hawkrequest.js]
+[test_logmanager.js]
 [test_observers.js]
 [test_restrequest.js]
 [test_tokenauthenticatedrequest.js]
 
 # Storage service APIs
 [test_storageservice_bso.js]
 [test_storageservice_client.js]
 # Bug 969624: Intermittent fail on Android 2.3 emulator
--- a/services/sync/modules/policies.js
+++ b/services/sync/modules/policies.js
@@ -8,18 +8,17 @@ this.EXPORTED_SYMBOLS = [
 ];
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 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");
 
 this.SyncScheduler = function SyncScheduler(service) {
   this.service = service;
   this.init();
 }
@@ -534,19 +533,16 @@ SyncScheduler.prototype = {
       Svc.Prefs.reset("scheduler.blocked-until");
       return false;
     }
     // we remain blocked.
     return true;
   },
 };
 
-const LOG_PREFIX_SUCCESS = "success-";
-const LOG_PREFIX_ERROR   = "error-";
-
 this.ErrorHandler = function ErrorHandler(service) {
   this.service = service;
   this.init();
 }
 ErrorHandler.prototype = {
   MINIMUM_ALERT_INTERVAL_MSEC: 604800000,   // One week.
 
   /**
@@ -569,43 +565,24 @@ ErrorHandler.prototype = {
     Svc.Obs.add("weave:service:sync:finish", this);
 
     this.initLogs();
   },
 
   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 dapp = new Log.DumpAppender(formatter);
-    dapp.level = Log.Level[Svc.Prefs.get("log.appender.dump")];
-    root.addAppender(dapp);
+    let logs = ["Sync", "FirefoxAccounts", "Hawk", "Common.TokenServerClient",
+                "Sync.SyncMigration"];
 
-    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) {
     this._log.trace("Handling " + topic);
     switch(topic) {
       case "weave:engine:sync:applied":
         if (subject.newFailed) {
           // An engine isn't able to apply one or more incoming records.
@@ -620,34 +597,32 @@ ErrorHandler.prototype = {
         let engine_name = data;   // engine name that threw the exception
 
         this.checkServerError(exception);
 
         Status.engines = [engine_name, exception.failureCode || ENGINE_UNKNOWN_FAIL];
         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");
         } else {
           this.notifyOnNextTick("weave:ui:clear-error");
         }
 
         this.dontIgnoreErrors = false;
         break;
       case "weave:service:sync:error":
         if (Status.sync == CREDENTIALS_CHANGED) {
           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");
         } else {
           this.notifyOnNextTick("weave:ui:sync:finish");
         }
 
         this.dontIgnoreErrors = false;
@@ -663,27 +638,25 @@ ErrorHandler.prototype = {
             Status.service == STATUS_OK) {
           // Great. Let's clear our mid-sync 401 note.
           this._log.trace("Clearing lastSyncReassigned.");
           Svc.Prefs.reset("lastSyncReassigned");
         }
 
         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;
             this.notifyOnNextTick("weave:ui:sync:error");
             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");
         break;
     }
   },
 
   notifyOnNextTick: function notifyOnNextTick(topic) {
@@ -701,104 +674,31 @@ ErrorHandler.prototype = {
   syncAndReportErrors: function syncAndReportErrors() {
     this._log.debug("Beginning user-triggered sync.");
 
     this.dontIgnoreErrors = true;
     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);
   },
 
   /**
    * Translates server error codes to meaningful strings.
    *
    * @param code
    *        server error code as an integer
    */
--- a/services/sync/tests/unit/test_errorhandler.js
+++ b/services/sync/tests/unit/test_errorhandler.js
@@ -9,18 +9,16 @@ Cu.import("resource://services-sync/poli
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/status.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 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;
 
 const NON_PROLONGED_ERROR_DURATION =
   (Svc.Prefs.get('errorhandler.networkFailureReportTimeout') / 2) * 1000;
 
 Service.engineManager.clear();
@@ -1785,18 +1783,17 @@ add_task(function test_sync_engine_gener
       _("Status.engines: " + JSON.stringify(Status.engines));
       do_check_eq(Status.engines["catapult"], ENGINE_UNKNOWN_FAIL);
       do_check_eq(Status.service, SYNC_FAILED_PARTIAL);
 
       // Test Error log was written on SYNC_FAILED_PARTIAL.
       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);
     });
   });
 
   do_check_true(yield setUp(server));
   Service.sync();
@@ -1817,18 +1814,17 @@ add_test(function test_logs_on_sync_erro
 
   Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() {
     Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog);
 
     // Test that error log was written.
     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();
   });
   Svc.Obs.notify("weave:service:sync:error", {});
 });
 
 add_test(function test_logs_on_login_error_despite_shouldReportError() {
@@ -1845,18 +1841,17 @@ add_test(function test_logs_on_login_err
 
   Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() {
     Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog);
 
     // Test that error log was written.
     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();
   });
   Svc.Obs.notify("weave:service:login:error", {});
 });
 
 // This test should be the last one since it monkeypatches the engine object
@@ -1880,18 +1875,17 @@ add_task(function test_engine_applyFaile
 
     do_check_eq(Status.engines["catapult"], ENGINE_APPLY_FAIL);
     do_check_eq(Status.service, SYNC_FAILED_PARTIAL);
 
     // Test Error log was written on SYNC_FAILED_PARTIAL.
     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);
   });
 
   do_check_eq(Status.engines["catapult"], undefined);
   do_check_true(yield setUp(server));
   Service.sync();
--- a/services/sync/tests/unit/test_errorhandler_filelog.js
+++ b/services/sync/tests/unit/test_errorhandler_filelog.js
@@ -5,18 +5,16 @@ Cu.import("resource://gre/modules/Log.js
 Cu.import("resource://services-common/utils.js");
 Cu.import("resource://services-sync/service.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://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
 // so otherwise we can end up with all of our files -- the ones we want to
 // keep, and the ones we want to clean up -- having the same modified time.
 const CLEANUP_DELAY      = 2000;
 const DELAY_BUFFER       = 500;  // Buffer for timers on different OS platforms.
 
@@ -27,38 +25,39 @@ let errorHandler = Service.errorHandler;
 
 function setLastSync(lastSyncValue) {
   Svc.Prefs.set("lastSync", (new Date(Date.now() - lastSyncValue)).toString());
 }
 
 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;
 
   run_next_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);
   Svc.Obs.notify("weave:service:sync:finish");
 
   // Clear again without having issued any output.
   Svc.Prefs.set("log.appender.file.logOnSuccess", true);
 
   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();
   });
 
   // Fake a successful sync.
   Svc.Obs.notify("weave:service:sync:finish");
 });
 
@@ -104,18 +103,17 @@ add_test(function test_logOnSuccess_true
   Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() {
     Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog);
 
     // Exactly one log file was written.
     let entries = logsdir.directoryEntries;
     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.
     readFile(logfile, function (error, data) {
       do_check_true(Components.isSuccessCode(error));
       do_check_neq(data.indexOf(MESSAGE), -1);
 
       // Clean up.
@@ -157,43 +155,48 @@ add_test(function test_sync_error_logOnE
 
 add_test(function test_sync_error_logOnError_true() {
   Svc.Prefs.set("log.appender.file.logOnError", true);
 
   let log = Log.repository.getLogger("Sync.Test.FileLog");
   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);
 
     // Exactly one log file was written.
     let entries = logsdir.directoryEntries;
     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.
     readFile(logfile, function (error, data) {
       do_check_true(Components.isSuccessCode(error));
       do_check_neq(data.indexOf(MESSAGE), -1);
 
       // Clean up.
       try {
         logfile.remove(false);
       } catch(ex) {
         dump("Couldn't delete file: " + ex + "\n");
         // Stupid Windows box.
       }
 
       Svc.Prefs.resetBranch("");
-      run_next_test();
     });
   });
 
   // Fake an unsuccessful sync due to prolonged failure.
   setLastSync(PROLONGED_ERROR_DURATION);
   Svc.Obs.notify("weave:service:sync:error");
 });
 
@@ -219,43 +222,48 @@ add_test(function test_login_error_logOn
 
 add_test(function test_login_error_logOnError_true() {
   Svc.Prefs.set("log.appender.file.logOnError", true);
 
   let log = Log.repository.getLogger("Sync.Test.FileLog");
   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);
 
     // Exactly one log file was written.
     let entries = logsdir.directoryEntries;
     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.
     readFile(logfile, function (error, data) {
       do_check_true(Components.isSuccessCode(error));
       do_check_neq(data.indexOf(MESSAGE), -1);
 
       // Clean up.
       try {
         logfile.remove(false);
       } catch(ex) {
         dump("Couldn't delete file: " + ex + "\n");
         // Stupid Windows box.
       }
 
       Svc.Prefs.resetBranch("");
-      run_next_test();
     });
   });
 
   // Fake an unsuccessful login due to prolonged failure.
   setLastSync(PROLONGED_ERROR_DURATION);
   Svc.Obs.notify("weave:service:login:error");
 });
 
@@ -268,27 +276,27 @@ add_test(function test_logErrorCleanup_a
   let errString = "some error log\n";
 
   Svc.Prefs.set("log.appender.file.logOnError", true);
   Svc.Prefs.set("log.appender.file.maxErrorAge", maxAge);
 
   _("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);
     foStream.close();
     _("  > Created " + filename);
     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;
     do_check_true(entries.hasMoreElements());
     let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
     do_check_true(oldLogs.every(function (e) {
       return e != logfile.leafName;
     }));