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

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

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.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1283267 - save start time of automigration when we save the finished time, to ensure both actually get written to prefs,

Review commit: https://reviewboard.mozilla.org/r/61386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61386/

Comment 2

[Justin Dolske [:Dolske]]

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
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

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbcd0b21370b

Comment 5

[:Gijs (he/him)]

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.

Comment 6

[:Gijs (he/him)]

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.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/49071ecbfedb

Comment 9

[Surentharan]

[bugday-20170125]