Sync error muffling causes undesirable lack of logging on error

RESOLVED FIXED in Firefox 9

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Blocks: 1 bug, {regression})

unspecified
mozilla10
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox9 fixed)

Details

(Whiteboard: [verified in services])

Attachments

(1 attachment)

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.
Blocks: 693494
Created attachment 566127 [details] [diff] [review]
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ā€¦
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #566127 - Flags: review?(philipp)
This is somewhat of a regression from bug 659067 I guess, so I'm setting the appropriate flags.
Blocks: 659067
Keywords: regression
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)
Attachment #566127 - Flags: review?(philipp) → review+
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.
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
Whiteboard: [fixed in services]
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.
Attachment #566127 - Flags: approval-mozilla-aurora?
status-firefox9: --- → affected
verified on s-c builds from 20111011.
Whiteboard: [fixed in services] → [verified in services]
https://hg.mozilla.org/mozilla-central/rev/a54bc1cfd54f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Attachment #566127 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to Aurora; thanks Asa.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ccaf201862a0
status-firefox9: affected → fixed
Target Milestone: mozilla10 → mozilla9

Updated

6 years ago
Target Milestone: mozilla9 → mozilla10
You need to log in before you can comment on or make changes to this bug.