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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

50 Branch
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed, firefox51 verified)

Details

(Whiteboard: [migration-needs-uplift])

Attachments

(1 attachment)

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: 3 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.