Bug 1854373 - Move logmanager.sys.mjs from services/common to toolkit/modules. r=Standard8,markh

Differential Revision: https://phabricator.services.mozilla.com/D208815

services/common/moz.build

@@ -22,7 +22,6 @@ EXTRA_JS_MODULES["services-common"] += [
     "kinto-http-client.sys.mjs",
     "kinto-offline-client.sys.mjs",
     "kinto-storage-adapter.sys.mjs",
-    "logmanager.sys.mjs",
     "observers.sys.mjs",
     "rest.sys.mjs",
     "uptake-telemetry.sys.mjs",

services/common/tests/unit/test_load_modules.js

@@ -6,12 +6,7 @@ const { AppConstants } = ChromeUtils.importESModule(
 );
 
 const MODULE_BASE = "resource://services-common/";
-const shared_modules = [
-  "async.sys.mjs",
-  "logmanager.sys.mjs",
-  "rest.sys.mjs",
-  "utils.sys.mjs",
-];
+const shared_modules = ["async.sys.mjs", "rest.sys.mjs", "utils.sys.mjs"];
 
 const non_android_modules = ["tokenserverclient.sys.mjs"];
 
@@ -51,6 +46,7 @@ function expectImportsToFail(mm, base = MODULE_BASE) {
 function run_test() {
   expectImportsToSucceed(shared_modules);
   expectImportsToSucceed(shared_test_modules, TEST_BASE);
+  expectImportsToSucceed(["LogManager.sys.mjs"], "resource://gre/modules/");
 
   if (AppConstants.platform != "android") {
     expectImportsToSucceed(non_android_modules);

services/common/tests/unit/xpcshell.toml

@@ -20,8 +20,6 @@ tags = "blocklist"
 
 ["test_load_modules.js"]
 
-["test_logmanager.js"]
-
 ["test_observers.js"]
 
 ["test_restrequest.js"]

services/fxaccounts/FxAccountsCommon.sys.mjs

@@ -3,7 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 import { Log } from "resource://gre/modules/Log.sys.mjs";
-import { LogManager } from "resource://services-common/logmanager.sys.mjs";
+import { LogManager } from "resource://gre/modules/LogManager.sys.mjs";
 
 // loglevel should be one of "Fatal", "Error", "Warn", "Info", "Config",
 // "Debug", "Trace" or "All". If none is specified, "Debug" will be used by
@@ -29,7 +29,13 @@ let logs = [
 ];
 
 // For legacy reasons, the log manager still thinks it's part of sync.
-export let logManager = new LogManager("services.sync.", logs, "sync");
+export let logManager = new LogManager({
+  prefRoot: "services.sync.",
+  logNames: logs,
+  logFilePrefix: "sync",
+  logFileSubDirectoryEntries: ["weave", "logs"],
+  testTopicPrefix: "services-tests:common:log-manager:",
+});
 
 // A boolean to indicate if personally identifiable information (or anything
 // else sensitive, such as credentials) should be logged.

toolkit/modules/LogManager.sys.mjs

@@ -1,8 +1,15 @@
 /* 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/. */
-"use strict;";
 
+/**
+ * This module provides a file-based, persistent logging facility for scenarios where
+ * retaining those logs over time and across browser restarts is important.
+ * Unless you need this feature specifically, please use console.createInstance.
+ */
+
+// See Bug 1889052
+// eslint-disable-next-line mozilla/use-console-createInstance
 import { Log } from "resource://gre/modules/Log.sys.mjs";
 
 const lazy = {};
@@ -120,10 +127,13 @@ class StorageStreamAppender extends Log.Appender {
   }
 }
 
-// A storage appender that is flushable to a file on disk.  Policies for
-// when to flush, to what file, log rotation etc are up to the consumer
-// (although it does maintain a .sawError property to help the consumer decide
-// based on its policies)
+/**
+ * A storage appender that is flushable to a file on disk.
+ *
+ * Policies for when to flush, to what file, log rotation etc are up to the consumer
+ * (although it does maintain a .sawError property to help the consumer decide
+ *  based on its policies)
+ */
 class FlushableStorageAppender extends StorageStreamAppender {
   constructor(formatter) {
     super(formatter);
@@ -142,8 +152,12 @@ class FlushableStorageAppender extends StorageStreamAppender {
     this.sawError = false;
   }
 
-  // Flush the current stream to a file. Somewhat counter-intuitively, you
-  // must pass a log which will be written to with details of the operation.
+  /**
+   * Flush the current stream to a file.
+   *
+   * Somewhat counter-intuitively, you must pass a log which will be written to
+   * with details of the operation.
+   */
   async flushToFile(subdirArray, filename, log) {
     let inStream = this.getInputStream();
     this.reset();
@@ -194,22 +208,39 @@ class FlushableStorageAppender extends StorageStreamAppender {
   }
 }
 
-// The public LogManager object.
-export function LogManager(prefRoot, logNames, logFilePrefix) {
-  this._prefObservers = [];
-  this.init(prefRoot, logNames, logFilePrefix);
-}
+/**
+ * Each LogManager monitors preferences, resolves log levels and verbosity,
+ * and manages the creation, rotation and clean up of log files in a profile subdirectory.
+ */
+export class LogManager {
+  constructor(options = {}) {
+    this._prefObservers = [];
+    this.#init(options);
+  }
 
-LogManager.StorageStreamAppender = StorageStreamAppender;
+  static StorageStreamAppender = StorageStreamAppender;
 
-LogManager.prototype = {
-  _cleaningUpFileLogs: false,
+  _cleaningUpFileLogs = false;
 
-  init(prefRoot, logNames, logFilePrefix) {
+  #init({
+    prefRoot,
+    logNames,
+    logFilePrefix,
+    logFileSubDirectoryEntries,
+    testTopicPrefix,
+  } = {}) {
     this._prefs = Services.prefs.getBranch(prefRoot);
     this._prefsBranch = prefRoot;
 
     this.logFilePrefix = logFilePrefix;
+    this._testTopicPrefix = testTopicPrefix;
+
+    // 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.
+    this.logFileSubDirectoryEntries = Object.freeze(logFileSubDirectoryEntries);
+
     if (!formatter) {
       // Create a formatter and various appenders to attach to the logs.
       formatter = new Log.BasicFormatter();
@@ -284,7 +315,7 @@ LogManager.prototype = {
     }
     // and use the first specified log as a "root" for our log.
     this._log = Log.repository.getLogger(logNames[0] + ".LogManager");
-  },
+  }
 
   /**
    * Cleanup this instance
@@ -298,23 +329,15 @@ LogManager.prototype = {
       allBranches.delete(this._prefsBranch);
     } catch (e) {}
     this._prefs = null;
-  },
-
-  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"];
-  },
+  }
 
   get sawError() {
     return this._fileAppender.sawError;
-  },
+  }
 
   // Result values for resetFileLog.
-  SUCCESS_LOG_WRITTEN: "success-log-written",
-  ERROR_LOG_WRITTEN: "error-log-written",
+  SUCCESS_LOG_WRITTEN = "success-log-written";
+  ERROR_LOG_WRITTEN = "error-log-written";
 
   /**
    * Possibly generate a log file for all accumulated log messages and refresh
@@ -357,7 +380,7 @@ LogManager.prototype = {
       let filename =
         reasonPrefix + "-" + this.logFilePrefix + "-" + Date.now() + ".txt";
       await this._fileAppender.flushToFile(
-        this._logFileSubDirectoryEntries,
+        this.logFileSubDirectoryEntries,
         filename,
         this._log
       );
@@ -379,7 +402,7 @@ LogManager.prototype = {
       this._log.error("Failed to resetFileLog", ex);
       return null;
     }
-  },
+  }
 
   /**
    * Finds all logs older than maxErrorAge and deletes them using async I/O.
@@ -396,22 +419,24 @@ LogManager.prototype = {
       return fileInfo.lastModified < threshold;
     };
     return this._deleteLogFiles(shouldDelete);
-  },
+  }
 
   /**
    * Finds all logs and removes them.
    */
   removeAllLogs() {
     return this._deleteLogFiles(() => true);
-  },
+  }
 
-  // Delete some log files. A callback is invoked for each found log file to
-  // determine if that file should be removed.
+  /**
+   * Delete some log files. A callback is invoked for each found log file to
+   * determine if that file should be removed.
+   */
   async _deleteLogFiles(cbShouldDelete) {
     this._cleaningUpFileLogs = true;
     let logDir = lazy.FileUtils.getDir(
       "ProfD",
-      this._logFileSubDirectoryEntries
+      this.logFileSubDirectoryEntries
     );
     for (const path of await IOUtils.getChildren(logDir.path)) {
       const name = PathUtils.filename(path);
@@ -439,9 +464,12 @@ LogManager.prototype = {
     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"
-    );
-  },
-};
+    if (this._testTopicPrefix) {
+      Services.obs.notifyObservers(
+        null,
+        `${this._testTopicPrefix}cleanup-logs`
+      );
+      ("cleanup-logs");
+    }
+  }
+}

toolkit/modules/moz.build

@@ -186,6 +186,7 @@ EXTRA_JS_MODULES += [
     "KeywordUtils.sys.mjs",
     "LayoutUtils.sys.mjs",
     "Log.sys.mjs",
+    "LogManager.sys.mjs",
     "NewTabUtils.sys.mjs",
     "NLP.sys.mjs",
     "ObjectUtils.sys.mjs",

toolkit/modules/tests/xpcshell/test_LogManager.js

@@ -5,11 +5,18 @@
 // other aspects of this.
 
 const { LogManager } = ChromeUtils.importESModule(
-  "resource://services-common/logmanager.sys.mjs"
+  "resource://gre/modules/LogManager.sys.mjs"
+);
+const { Log } = ChromeUtils.importESModule(
+  "resource://gre/modules/Log.sys.mjs"
 );
 const { FileUtils } = ChromeUtils.importESModule(
   "resource://gre/modules/FileUtils.sys.mjs"
 );
+const logManagerDefaultOptions = {
+  logFileSubDirectoryEntries: ["weave", "logs"],
+  testTopicPrefix: "services-tests:common:log-manager:",
+};
 
 // Returns an array of [consoleAppender, dumpAppender, [fileAppenders]] for
 // the specified log.  Note that fileAppenders will usually have length=1
@@ -27,7 +34,12 @@ function getAppenders(log) {
 // Test that the correct thing happens when no prefs exist for the log manager.
 add_task(async 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 lm = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "no-such-branch.",
+    logNames: ["TestLog"],
+    logFilePrefix: "test",
+  });
 
   let log = Log.repository.getLogger("TestLog");
   let [capp, dapp, fapps] = getAppenders(log);
@@ -51,7 +63,12 @@ add_task(async function test_PrefChanges() {
     "log-manager.test.log.appender.file.level",
     "Trace"
   );
-  let lm = new LogManager("log-manager.test.", ["TestLog2"], "test");
+  let lm = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "log-manager.test.",
+    logNames: ["TestLog2"],
+    logFilePrefix: "test",
+  });
 
   let log = Log.repository.getLogger("TestLog2");
   let [capp, dapp, [fapp]] = getAppenders(log);
@@ -96,7 +113,12 @@ add_task(async function test_SharedLogs() {
     "log-manager-1.test.log.appender.file.level",
     "Trace"
   );
-  let lm1 = new LogManager("log-manager-1.test.", ["TestLog3"], "test");
+  let lm1 = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "log-manager-1.test.",
+    logNames: ["TestLog3"],
+    logFilePrefix: "test",
+  });
 
   // and the second.
   Services.prefs.setStringPref(
@@ -108,7 +130,12 @@ add_task(async function test_SharedLogs() {
     "log-manager-2.test.log.appender.file.level",
     "Debug"
   );
-  let lm2 = new LogManager("log-manager-2.test.", ["TestLog3"], "test");
+  let lm2 = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "log-manager-2.test.",
+    logNames: ["TestLog3"],
+    logFilePrefix: "test",
+  });
 
   let log = Log.repository.getLogger("TestLog3");
   let [capp, dapp] = getAppenders(log);
@@ -158,7 +185,12 @@ function checkLogFile(prefix) {
 
 // Test that we correctly write error logs by default
 add_task(async function test_logFileErrorDefault() {
-  let lm = new LogManager("log-manager.test.", ["TestLog2"], "test");
+  let lm = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "log-manager.test.",
+    logNames: ["TestLog2"],
+    logFilePrefix: "test",
+  });
 
   let log = Log.repository.getLogger("TestLog2");
   log.error("an error message");
@@ -180,7 +212,12 @@ add_task(async function test_logFileSuccess() {
     false
   );
 
-  let lm = new LogManager("log-manager.test.", ["TestLog2"], "test");
+  let lm = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "log-manager.test.",
+    logNames: ["TestLog2"],
+    logFilePrefix: "test",
+  });
 
   let log = Log.repository.getLogger("TestLog2");
   log.info("an info message");
@@ -230,7 +267,12 @@ add_task(async function test_logFileError() {
     false
   );
 
-  let lm = new LogManager("log-manager.test.", ["TestLog2"], "test");
+  let lm = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "log-manager.test.",
+    logNames: ["TestLog2"],
+    logFilePrefix: "test",
+  });
 
   let log = Log.repository.getLogger("TestLog2");
   log.info("an info message");
@@ -307,7 +349,12 @@ add_task(async function test_logFileError() {
     true
   );
 
-  let lm = new LogManager("log-manager.test.", ["TestLog2"], "test");
+  let lm = new LogManager({
+    ...logManagerDefaultOptions,
+    prefRoot: "log-manager.test.",
+    logNames: ["TestLog2"],
+    logFilePrefix: "test",
+  });
 
   let log = Log.repository.getLogger("TestLog2");
   log.info("an info message");

toolkit/modules/tests/xpcshell/xpcshell.toml

@@ -63,6 +63,8 @@ tags = "remote-settings"
 
 ["test_Log.js"]
 
+["test_LogManager.js"]
+
 ["test_Log_double_ext.js"]
 
 ["test_Log_nsIStackFrame.js"]