1289229 - Remove the possibility to 'undo' automigration when passwords/bookmarks are changed

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

See bug 1289172 for rationale.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1289229 - disable automigration undo if people create/change bookmarks/logins,

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

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8774509 [details]
Bug 1289229 - disable automigration undo if people create/change bookmarks/logins,

https://reviewboard.mozilla.org/r/66948/#review63956

I have one big doubt, when we import default bookmarks, we notify and this will prevent undo.
Since every profile imports default bookmarks, won't that make automigration never undoable?

::: browser/components/migration/AutoMigrate.jsm:13
(Diff revision 1)
>  
>  const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;
>  
> +const kAutoMigrateEnabledPref = "browser.migration.automigrate";
> +
>  const kAutoMigrateStartedPref = "browser.migrate.automigrate-started";

Why do we have 2 different roots for the same component, browser.migration. and browser.migrate.?
Couldn't this be browser.migrate.automigrate.enabled?

::: browser/components/migration/AutoMigrate.jsm:37
(Diff revision 1)
>      return BOOKMARKS | HISTORY | PASSWORDS;
>    },
>  
> +  init() {
> +    this.enabled = !!Services.prefs.getPrefType(kAutoMigrateEnabledPref) &&
> +                   Services.prefs.getBoolPref(kAutoMigrateEnabledPref);

Why don't you use Preferences.jsm "get" with a fallback value?

::: browser/components/migration/AutoMigrate.jsm:51
(Diff revision 1)
> +      return;
> +    }
> +    // Now register places and password observers:
> +    this.onItemAdded = this.onItemMoved = this.onItemChanged =
> +      this.dataHasChanged;
> +    PlacesUtils.bookmarks.addObserver(this, true);

This forcibly inits the bookmarks service when maybe it was not needed.
We tried to avoid that as far as possible (maybe we fail at doing this but we should not add further risks).

You can use PlacesUtils.addLazyBookmarkObserver instead, it works the same, but it won't init the service until required.

::: browser/components/migration/AutoMigrate.jsm:71
(Diff revision 1)
> +   * which will silently eliminate the possibility of undoing the
> +   * changes.
> +   */
> +  dataHasChanged() {
> +    Services.obs.removeObserver(this, kPasswordManagerTopic);
> +    PlacesUtils.bookmarks.removeObserver(this);

and there's removeLazyBookmarkObserver, too.

::: browser/components/migration/AutoMigrate.jsm:183
(Diff revision 1)
>  
>    getUndoRange() {
>      let start, finish;
>      try {
> +      if (Services.prefs.getPrefType(kAutoMigrateStartedPref) &&
> +          Services.prefs.getPrefType(kAutoMigrateFinishedPref)) {

ditto for Preferences.jsm

Comment 3

[:Gijs (he/him)]

Comment on attachment 8774509 [details]
Bug 1289229 - disable automigration undo if people create/change bookmarks/logins,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66948/diff/1-2/

Comment 4

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/66948/#review63956

On the migration run, we need to init the observers after the migration has happened. They won't init before because the migration at that stage won't have happened and so there's nothing to undo they need to watch for. So I added code that inits the observers once the migration has finished. That is guaranteed to run after the default bookmarks have been imported, so I think that covers this issue.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8774509 [details]
Bug 1289229 - disable automigration undo if people create/change bookmarks/logins,

https://reviewboard.mozilla.org/r/66948/#review63984

::: browser/components/migration/AutoMigrate.jsm:44
(Diff revision 2)
> +      this.maybeInitUndoObserver();
> +    }
> +  },
> +
> +  maybeInitUndoObserver() {
> +    // Check sync if we really need to do this:

nit: "sync" abbr. is quite confusing since we have a product named like that :) (UC/lc is imo too subtle of a difference). "Synchronously check" works better.

::: browser/components/migration/AutoMigrate.jsm:48
(Diff revision 2)
> +  maybeInitUndoObserver() {
> +    // Check sync if we really need to do this:
> +    if (!this.getUndoRange()) {
> +      return;
> +    }
> +    // Now register places and password observers:

you see, Places is lowercase here :D

::: browser/components/migration/AutoMigrate.jsm:102
(Diff revision 2)
>          Services.obs.removeObserver(migrationObserver, "Migration:ItemError");
>          Services.prefs.setCharPref(kAutoMigrateStartedPref, startTime.toString());
>          Services.prefs.setCharPref(kAutoMigrateFinishedPref, Date.now().toString());
> +        Services.prefs.setCharPref(kAutoMigrateBrowserPref, pickedKey);
> +        // Need to manually start listening to new bookmarks/logins being created,
> +        // because when we were initialized there wasn't the possibility to

please add commas around "when we were initialized"

::: browser/components/migration/tests/unit/test_automigration.js:164
(Diff revision 2)
>    let expectedTypes = BOOKMARKS | HISTORY | PASSWORDS;
>    Assert.deepEqual(gShimmedMigrator._migrateArgs, [expectedTypes, "startup", null],
>                     "migrate called with 'null' as a profile");
>  
>    yield migrationFinishedPromise;
> -  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate-started"),
> +  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate.started"),

Preferences.has()

::: browser/components/migration/tests/unit/test_automigration.js:166
(Diff revision 2)
>                     "migrate called with 'null' as a profile");
>  
>    yield migrationFinishedPromise;
> -  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate-started"),
> +  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate.started"),
>              "Should have set start time pref");
> -  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate-finished"),
> +  Assert.ok(Services.prefs.getPrefType("browser.migrate.automigrate.finished"),

ditto

::: browser/components/migration/tests/unit/test_automigration.js:248
(Diff revision 2)
>    storedLogins = Services.logins.findLogins({}, "www.mozilla.org",
>                                              "http://www.mozilla.org", null);
>    Assert.equal(storedLogins.length, 0, "Should have no logins");
>  
>    // Finally check prefs got cleared:
> -  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate-started"),
> +  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate.started"),

ditto

::: browser/components/migration/tests/unit/test_automigration.js:250
(Diff revision 2)
>    Assert.equal(storedLogins.length, 0, "Should have no logins");
>  
>    // Finally check prefs got cleared:
> -  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate-started"),
> +  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate.started"),
>              "Should no longer have pref for migration start time.");
> -  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate-finished"),
> +  Assert.ok(!Services.prefs.getPrefType("browser.migrate.automigrate.finished"),

ditto

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/80afb86a5a17
disable automigration undo if people create/change bookmarks/logins, r=mak

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80afb86a5a17

Comment 8

[:Gijs (he/him)]

Comment on attachment 8774509 [details]
Bug 1289229 - disable automigration undo if people create/change bookmarks/logins,

Approval Request Comment
[Feature/regressing bug #]: automigration, used in funnelcake builds 88 (bug 1295873)
[User impact if declined]: this patch fixes the situation where it would be possible for the user to undo a migration and have the undo process accidentally delete non-imported (ie new) user data. In funnelcake v1 this was avoided mostly by the add-on itself (which provides the UI for the user to undo). This patch builds in more safeguards on the Firefox side.
[Describe test coverage new/current, TreeHerder]: again comes with more changes to its own automated xpcshell test to make sure this code is sane.
[Risks and why]: low risk. Although the pref name changes in this patch (for consistency) and the pref reading is moved into the JSM itself, the net result is still the same - none of this code (beyond reading the pref) will run on "normal" 49 release. Some utility methods are added to the pre-existing MigrationUtils.jsm, but at the moment those are only called from the JSM, behind the pref, so those too would only run if the pref was turned on.
[String/UUID change made/needed]: no.

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> [Risks and why]: Some utility methods are added to the
> pre-existing MigrationUtils.jsm, but at the moment those are only called
> from the JSM, behind the pref, so those too would only run if the pref was
> turned on.

Egh, hate to correct myself here, but that code is actually effectively being *moved* from migration.js to MigrationUtils.jsm, and still called from the existing dialog (so it *does* run on normal beta/release) but because we're just moving existing code, I still think the risk in this patch is very low.

Comment 10

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

Comment on attachment 8774509 [details]
Bug 1289229 - disable automigration undo if people create/change bookmarks/logins,

Automigration uplift for Funnelcake, preffed off by default in 49.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bf1e41bcedc0