Closed Bug 1283267 Opened 4 years ago Closed 4 years ago

Automigration undo functionality is broken because we fail to save the start time

Categories

(Firefox :: Migration, defect)

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [migration-needs-uplift])

Attachments

(1 file)

This code: http://searchfox.org/mozilla-central/rev/261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/browser/components/migration/MigrationUtils.jsm#208-274

runs doStartup() before running the migration, but we're saving the timestamp before even calling this:

https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/browser/components/migration/AutoMigrate.jsm#65-66

which results in the 'started' pref never being saved because at that point the prefs are going into no-man's land.

The fix is to just save the value in a local var and then write to a pref at the same time as we write the 'finished' timestamp value.

The reason tests didn't catch this is because the pref service is continually available in xpcshell tests.
Attachment #8766513 - Flags: review?(dolske) → review+
Comment on attachment 8766513 [details]
Bug 1283267 - save start time of automigration when we save the finished time, to ensure both actually get written to prefs,

https://reviewboard.mozilla.org/r/61386/#review58240
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dbcd0b21370b
save start time of automigration when we save the finished time, to ensure both actually get written to prefs, r=Dolske
https://hg.mozilla.org/mozilla-central/rev/dbcd0b21370b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Whiteboard: [migration-needs-uplift]
Comment on attachment 8766513 [details]
Bug 1283267 - save start time of automigration when we save the finished time, to ensure both actually get written to prefs,

Approval Request Comment
[Feature/regressing bug #]: automigration, used in funnelcake builds 88 (bug 1295873) - this particular patch fixes an oversight in the patch for bug 1271799.
[User impact if declined]: no funnelcake. :-(
[Describe test coverage new/current, TreeHerder]: this was manually tested as part of the funnelcake. While the automatic test exercises this code, it didn't originally pick up the issue here. What with the manual testing and the success of the funnelcake we're pretty confident it works now. :-)
[Risks and why]: no risk. Tiny patch, all the code here is behind the main pref.
[String/UUID change made/needed]: no.
Attachment #8766513 - Flags: approval-mozilla-beta?
Attachment #8766513 - Flags: approval-mozilla-aurora?
Comment on attachment 8766513 [details]
Bug 1283267 - save start time of automigration when we save the finished time, to ensure both actually get written to prefs,

Automigration uplift for Funnelcake, preffed off by default in 49.
Attachment #8766513 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
[bugday-20170125]
You need to log in before you can comment on or make changes to this bug.