Bug 1152193 - Ensure sync/readinglist log directory exists. r=rnewman, a=sledru FIREFOX_RELEASE_38_BASE
authorMark Hammond <mhammond@skippinet.com.au>
Mon, 20 Apr 2015 10:00:03 +1000
changeset 260217 fc98815acf5f
parent 260216 5712fefbace8
child 260218 afb64330c5b5
push id721
push userjlund@mozilla.com
push date2015-04-21 23:03 +0000
treeherdermozilla-release@d27c9211ebb3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman, sledru
bugs1152193
milestone38.0
Bug 1152193 - Ensure sync/readinglist log directory exists. r=rnewman, a=sledru
services/common/logmanager.js
services/common/tests/unit/test_logmanager.js
--- a/services/common/logmanager.js
+++ b/services/common/logmanager.js
@@ -137,38 +137,45 @@ LogManager.prototype = {
     }
     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"]);
+  get _logFileSubDirectoryEntries() {
+    // At this point we don't allow a custom directory for the logs, nor allow
+    // it to be outside the profile directory.
+    // This returns an array of the the relative directory entries below the
+    // profile dir, and is the directory about:sync-log uses.
+    return ["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.
+   * outputFileName is a string with the tail of the filename - the file will
+   * be created in the log directory.
+   * Returns a promise that is resolved<undefined> on completion or rejected if
+   * there is an error.
    */
-  _copyStreamToFile: Task.async(function* (inputStream, outputFile) {
+  _copyStreamToFile: Task.async(function* (inputStream, outputFileName) {
     // 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 });
+    // We assume the profile directory exists, but not that the dirs under it do.
+    let profd = FileUtils.getDir("ProfD", []);
+    let outputFile = FileUtils.getDir("ProfD", this._logFileSubDirectoryEntries);
+    yield OS.File.makeDir(outputFile.path, { ignoreExisting: true, from: profd.path });
+    outputFile.append(outputFileName);
     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));
@@ -216,22 +223,20 @@ LogManager.prototype = {
 
       let inStream = this._fileAppender.getInputStream();
       this._fileAppender.reset();
       if (inStream) {
         this._log.debug("Flushing file log");
         // We have reasonPrefix at the start of the filename so all "error"
         // logs are grouped in about:sync-log.
         let filename = reasonPrefix + "-" + this.logFilePrefix + "-" + Date.now() + ".txt";
-        let file = this._logFileDirectory;
-        file.append(filename);
-        this._log.trace("Beginning stream copy to " + file.leafName + ": " +
+        this._log.trace("Beginning stream copy to " + filename + ": " +
                         Date.now());
         try {
-          yield this._copyStreamToFile(inStream, file);
+          yield this._copyStreamToFile(inStream, filename);
           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
@@ -251,17 +256,18 @@ LogManager.prototype = {
     }
   }),
 
   /**
    * 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 logDir = FileUtils.getDir("ProfD", this._logFileSubDirectoryEntries);
+    let iterator = new OS.File.DirectoryIterator(logDir.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("error-" + this.logFilePrefix + "-") &&
           !entry.name.startsWith("success-" + this.logFilePrefix + "-")) {
         return;
--- a/services/common/tests/unit/test_logmanager.js
+++ b/services/common/tests/unit/test_logmanager.js
@@ -1,16 +1,17 @@
 /* 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");
+Cu.import("resource://gre/modules/FileUtils.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) {
@@ -96,8 +97,39 @@ add_task(function* test_SharedLogs() {
   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();
 });
+
+// A little helper to test what log files exist.  We expect exactly zero (if
+// prefix is null) or exactly one with the specified prefix.
+function checkLogFile(prefix) {
+  let logsdir = FileUtils.getDir("ProfD", ["weave", "logs"], true);
+  let entries = logsdir.directoryEntries;
+  if (!prefix) {
+    // expecting no files.
+    ok(!entries.hasMoreElements());
+  } else {
+    // expecting 1 file.
+    ok(entries.hasMoreElements());
+    let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile);
+    equal(logfile.leafName.slice(-4), ".txt");
+    ok(logfile.leafName.startsWith(prefix + "-test-"), logfile.leafName);
+    // and remove it ready for the next check.
+    logfile.remove(false);
+  }
+}
+
+// Test that we correctly write error logs by default
+add_task(function* test_logFileErrorDefault() {
+  let lm = new LogManager("log-manager.test.", ["TestLog2"], "test");
+
+  let log = Log.repository.getLogger("TestLog2");
+  log.error("an error message");
+  yield lm.resetFileLog(lm.REASON_ERROR);
+  // One error log file exists.
+  checkLogFile("error");
+  lm.finalize();
+});