Closed
Bug 693505
Opened 13 years ago
Closed 13 years ago
Sync error muffling causes undesirable lack of logging on error
Categories
(Firefox :: Sync, defect)
Firefox
Sync
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)
4.69 KB,
patch
|
philikon
:
review+
asa
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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•13 years ago
|
||
This is somewhat of a regression from bug 659067 I guess, so I'm setting the appropriate flags.
Blocks: 659067
Keywords: regression
Comment 3•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
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]
Assignee | ||
Comment 6•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
status-firefox9:
--- → affected
Comment 7•13 years ago
|
||
verified on s-c builds from 20111011.
Whiteboard: [fixed in services] → [verified in services]
Comment 8•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a54bc1cfd54f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Updated•13 years ago
|
Attachment #566127 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 9•13 years ago
|
||
Pushed to Aurora; thanks Asa. https://hg.mozilla.org/releases/mozilla-aurora/rev/ccaf201862a0
Target Milestone: mozilla10 → mozilla9
Updated•6 years ago
|
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.
Description
•