693505 - Sync error muffling causes undesirable lack of logging on error

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

case "weave:service:login: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");
        }
      ...
      case "weave:service:sync:error":
        if (Status.sync == CREDENTIALS_CHANGED) {
          Weave.Service.logout();
        }

        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");
        }

We should always resetFileLog regardless of whether we report the error to the user.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Awful duplication of test code, but it seems to be the fashion these days.

Verified that tests hang (due to lack of reset-file-log event) if I undo the changes in the patch.

I have *not* verified behavior in Real Life® yet. Tomorrow morning before landing…

Comment 2

[Philipp von Weitershausen [:philikon]]

This is somewhat of a regression from bug 659067 I guess, so I'm setting the appropriate flags.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 566127 [details] [diff] [review]
Proposed patch. v1

Yup and yup. Once you've landed and verified, please request approval for Aurora (which is where bug 659067 regressed us)

Comment 4

[Richard Newman [:rnewman]]

Verified in local build by screwing up my clusterURL pref and overriding shouldReportError. error-log written, no error bar.

Will land this morning and post non-hacky STRs.

Comment 5

[Richard Newman [:rnewman]]

Fixed in services:

  https://hg.mozilla.org/services/services-central/rev/a54bc1cfd54f

STR for QA:

* Start with a profile that won't warn on error, as described in Bug 659067. One that's recently synced successfully is great.

* Set up a situation that will result in a (non-displayed, of course) sync error (e.g., unknown host due to DNS issues).

* Provoke an instant sync by adding a bookmark.

* Verify that a new error log has been written in about:sync-log.

Please also perform usual tests around error logging, as described in

  https://bugzilla.mozilla.org/show_bug.cgi?id=659067#c44

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 566127 [details] [diff] [review]
Proposed patch. v1

Verified by both Philipp and myself.

Requesting approval for Aurora: this regression landed as described in Comment 3, and it's a small and safe fix.

Comment 7

[Tracy Walker [:tracy]]

verified on s-c builds from 20111011.

Comment 8

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/a54bc1cfd54f

Comment 9

[Richard Newman [:rnewman]]

Pushed to Aurora; thanks Asa.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ccaf201862a0