Closed Bug 1332557 Opened 8 years ago Closed 7 years ago

Consider enabling sync success logs by default on Nightly


(Firefox :: Sync, defect, P2)




Firefox 55
Tracking Status
firefox55 --- fixed


(Reporter: markh, Assigned: bharatraghunthan9767, Mentored)


(Keywords: good-first-bug)


(3 files)

We regularly get bug reports from Nightly users. It would be helpful, and within the scope of what Nightly is for, to have services.sync.log.appender.file.logOnSuccess be true on Nightly by default - that way we could ask such users for their logs and be more likely to have something useful.

(We could also argue this for Aurora, but one yak at a time ;)
Priority: -- → P2
Mentor: markh
Keywords: good-first-bug
Can I still take this?
Flags: needinfo?(markh)
Of course! Do you already have your Firefox build environment set up? If not, check out

You'll want to add the pref to, and change the `` file in that directory so that it preprocesses the prefs file. Have a look at the DevTools prefs file ( and its build manifest for an example of how to set a Nightly-only pref.

Please feel free to ask questions here, or on IRC in the #sync channel, if you get stuck. Thanks for taking this on!
Flags: needinfo?(markh)
I have taken on this bug as it was unassigned and have submitted on however, it is not appearing as a MozReview Request here, so I'm attaching it separately.
Flags: needinfo?(markh)
Comment on attachment 8843560 [details] [diff] [review]
Enabled sync success logs by default only on Nightly

That looks exactly correct, thanks.
Attachment #8843560 - Attachment is patch: true
Attachment #8843560 - Attachment mime type: application/javascript → text/plain
Flags: needinfo?(markh)
Attachment #8843560 - Flags: feedback?(markh) → review+
Assignee: nobody → bharatraghunthan9767
Keywords: checkin-needed
This needs to be in diff format to be landed.
Keywords: checkin-needed
Should I make obsolete the previous attachment #8843560 [details] [diff] [review] or leave it as it is?
Flags: needinfo?(markh)
Comment on attachment 8843834 [details] [diff] [review]
Enabled sync success logs by default only on Nightly - submitting as diff/patch

Review of attachment 8843834 [details] [diff] [review]:

Thanks - I previously looked at the reviewboard commit - you should probably work out what went wrong there as using reviewboard is much simpler for reviewers.
Attachment #8843834 - Flags: review?(markh)
Attachment #8843834 - Flags: review+
Attachment #8843834 - Flags: checkin?(ryanvm)
Flags: needinfo?(markh)
Keywords: checkin-needed
Pushed by
Enable sync success logs by default only on Nightly r=markh
Keywords: checkin-needed
Comment to attachment #8843834 [details] [diff] [review]
Is there anything more to be done or is the bug fixed?
backed out for test failures like
Flags: needinfo?(bharatraghunthan9767)
Backout by
Backed out changeset c651974bb5ce for test failures in test_service_migratePrefs.js
Should I modify test_service_migratePrefs.js to test for Nightly or not, or change the code in services-sync.js (like changing order of the prefs etc.)?
Flags: needinfo?(bharatraghunthan9767) → needinfo?(markh)
Thanks for your contribution, and my apologies for not noticing the change would cause test failures.

The test should be changed (even though it's now out of date) - if you add near the top of the file:


then change:
do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), false);

do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), LOG_ON_SUCCESS_DEFAULT);

The test will pass. Seeing as it has been backed out once, we really need to run a try build for this, so you will need to work out how to push to mozreview, and we can trigger a test build from that. Please catch us in IRC in the #sync channel if you still have trouble pushing to mozreview.

I also opened bug 1345009 to remove the code that caused the failure, so I guess if you are keen you could take that other bug first, after which no further changes would be needed here :)
Flags: needinfo?(markh)
Pushed to the reviewboard in order for the patch to be landed
Try is failing with an "E" error that indicates your local tree is a week or so old and needs updating - could you please rebase your patches against a current mozilla-central, modify the commit message so it has ". r?markh" at the end, then re-push to reviewboard? You could then push you own try build too (or I'm happy to do that) - I expect try will then pass and I can push it for you. has more info.

Thanks for your patches and your patience!
Please review the MozReview Request of Comment 20 not Comment 19 as it was a mistake.
Flags: needinfo?(markh)
Something strange has gone on with that patch - shows only one .patch file. It's not possible to review an earlier iteration of the patch, so I think you need to push a new patch with only the changes and no .patch files.
Flags: needinfo?(markh)
I'm sorry, it was because my local copy of the mozilla-central repo was in a mess with multiple branches suddenly created that may have caused the problem.
I hope it's fine now?
Flags: needinfo?(markh)
Comment on attachment 8844774 [details]
Bug 1332557 - Enable sync success logs by default on Nightly.

The try run looks good! :)

Can you please change the commit message to "Bug 1332557 - Enable sync success logs by default on Nightly. r?markh" and re-push it, then I'll land it.

Changed the commit message accordingly and re-pushed it. Now please try landing it.
Comment on attachment 8844774 [details]
Bug 1332557 - Enable sync success logs by default on Nightly.

Attachment #8844774 - Flags: review?(markh) → review+
Pushed by
Enable sync success logs by default on Nightly. r=markh
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.