Closed Bug 693505 Opened 8 years ago Closed 8 years ago

Sync error muffling causes undesirable lack of logging on error

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
firefox9 --- fixed

People

(Reporter: rnewman, Assigned: rnewman)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [verified in services])

Attachments

(1 file)

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
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?
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Attachment #566127 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to Aurora; thanks Asa.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ccaf201862a0
Target Milestone: mozilla10 → mozilla9
Target Milestone: mozilla9 → mozilla10
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.