Bug 693505 - Sync error muffling causes undesirable lack of logging on error. r=philikon

services/sync/modules/policies.js

@@ -520,9 +520,10 @@ let ErrorHandler = {
         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);
+
         if (this.shouldReportError()) {
-          this.resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"),
-                            LOG_PREFIX_ERROR);
           this.notifyOnNextTick("weave:ui:login:error");
         } else {
           this.notifyOnNextTick("weave:ui:clear-error");
@@ -535,9 +536,10 @@ let ErrorHandler = {
           Weave.Service.logout();
         }
 
+        this.resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"),
+                          LOG_PREFIX_ERROR);
+
         if (this.shouldReportError()) {
-          this.resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"),
-                            LOG_PREFIX_ERROR);
           this.notifyOnNextTick("weave:ui:sync:error");
         } else {
           this.notifyOnNextTick("weave:ui:sync:finish");

services/sync/tests/unit/test_errorhandler.js

@@ -1264,6 +1264,62 @@ add_test(function test_sync_engine_generic_fail() {
   Service.sync();
 });
 
+add_test(function test_logs_on_sync_error_despite_shouldReportError() {
+  _("Ensure that an error is still logged when weave:service:sync:error " +
+    "is notified, despite shouldReportError returning false.");
+
+  let log = Log4Moz.repository.getLogger("Sync.ErrorHandler");
+  Svc.Prefs.set("log.appender.file.logOnError", true);
+  log.info("TESTING");
+
+  // Ensure that we report no error.
+  Status.login = MASTER_PASSWORD_LOCKED;
+  do_check_false(ErrorHandler.shouldReportError());
+
+  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);
+
+    clean();
+    run_next_test();
+  });
+  Svc.Obs.notify("weave:service:sync:error", {});
+});
+
+add_test(function test_logs_on_login_error_despite_shouldReportError() {
+  _("Ensure that an error is still logged when weave:service:login:error " +
+    "is notified, despite shouldReportError returning false.");
+
+  let log = Log4Moz.repository.getLogger("Sync.ErrorHandler");
+  Svc.Prefs.set("log.appender.file.logOnError", true);
+  log.info("TESTING");
+
+  // Ensure that we report no error.
+  Status.login = MASTER_PASSWORD_LOCKED;
+  do_check_false(ErrorHandler.shouldReportError());
+
+  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);
+
+    clean();
+    run_next_test();
+  });
+  Svc.Obs.notify("weave:service:login:error", {});
+});
+
 // This test should be the last one since it monkeypatches the engine object
 // and we should only have one engine object throughout the file (bug 629664).
 add_test(function test_engine_applyFailed() {