1271799 - Implement 'undo' functionality for automatic startup data import from other browser(s)

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P2)

Description

[:Gijs (he/him)]

Pretty much what it says on the tin. Need to work out bug 1271778 and bug 1271798, and obviously need an actual implementation of the migration in bug 1271775 as well.

Comment 1

[:Gijs (he/him)]

This is more work than I thought it would be, because there are no "sanitize" (clear recent history) implementations for either passwords or bookmarks. Both of them have a way of clearing all bookmarks/logins in one fell swoop, but if we're allowing users to do this some while into their first use of Firefox (e.g. via the add-on folks are working on), I'm sure they wouldn't appreciate us clearing bookmarks/logins they added after the initial migration (e.g. via sync, the removal of which will then also sync to other devices, which would be... bad...).

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1271799 - implement undo functionality in automigration code,

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

Comment 3

[:Gijs (he/him)]

Just so we're clear... this is in no way a final solution - but it's something that is usable for the funnelcake build that folks want to run in July, and landing it on nightly with automated testing is a Good Thing. The whole thing is (and will remain) behind a pref. Things that need addressing before pref flip:

- UI. For the funnelcake, the add-on will take care of this;
- Some solution for bookmarks and logins removal that isn't "remove everything", if we ever expose automatic migration on something other than first run, or let the user undo it (significantly) later;
- Figuring out if it's OK that we remove all previous history, or if we also need a better solution there. Because history is "backdated" to the visit dates in the original browser, it's not really possible to say "remove all the items we added in this timeframe", with the timeframe of the migration in mind.
- potentially adding UI to sync sign in that will warn the users that they can't undo any previous migration once they sign into sync

Note: I also removed the formdata resource from the auto-imported list. AFAICT we don't support importing formdata from any browser other than Firefox itself, and so it's effectively dead code.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8764277 [details]
Bug 1271799 - implement undo functionality in automigration code,

https://reviewboard.mozilla.org/r/60262/#review57654

There are a couple things yet that I'd like to understand

::: browser/components/migration/AutoMigrate.jsm:139
(Diff revision 1)
> +    }
> +    return [start * 1000, finish * 1000];
> +  },
> +
> +  canUndo() {
> +    // Need to also check whether we're signed into sync!!

leftover todo comment?

::: browser/components/migration/AutoMigrate.jsm:151
(Diff revision 1)
> +    return fxAccounts.getSignedInUser().then(user => !user, () => Promise.resolve(true));
> +  },
> +
> +  undo: Task.async(function* () {
> +    if (!(yield this.canUndo())) {
> +      return Promise.reject(new Error("Can't undo!"));

why don't you just throw new Error since you are in a Task? Here you are resolving to a rejected promise, rather than rejecting directly

::: browser/components/migration/AutoMigrate.jsm:165
(Diff revision 1)
> +    // visit time recorded by the browser from which we imported. As a result,
> +    // a lower bound on this item doesn't really make sense. For form data
> +    // this could be different, but we currently don't support form data
> +    // import from any non-Firefox browser, so it isn't imported from other
> +    // browsers by the automigration code, nor do we remove it here.
> +    sanitizer.range = [0, range[1]];

good that this is in the tree, sooner or later I'd like to land bug 1167238 (when I find enough time to test it and a reviewer).

::: browser/components/migration/AutoMigrate.jsm:177
(Diff revision 1)
> +    } catch (ex) {
> +      // ignore failure.
> +    }
> +    Services.prefs.clearUserPref("browser.migrate.automigrate-started");
> +    Services.prefs.clearUserPref("browser.migrate.automigrate-finished");
> +    return Promise.all(promises);

I fear this may suffer of the same problem we had in migration, where a very long history removal will cause failoure of the eraseEverything command.
You should probably make history and bookmarks removal happen sequentially.

that said, I'm not sure why you use the sanitizer, do you also need to purge session history and the network predictor?
Wouldn't be simpler to directly use PlacesUtils.history.removeVisitsByFilter that is basically what the sanitizer does internally.

::: browser/components/migration/tests/unit/test_automigration.js:141
(Diff revision 1)
> +      return Ci.nsIBrowserProfileMigrator.BOOKMARKS;
> +    },
> +    migrate(types, startup, profileToMigrate) {
> +      this._migrateArgs = [types, startup, profileToMigrate];
> +      TestUtils.executeSoon(function() {
> +        Services.obs.notifyObservers(null, "Migration:Ended", undefined);

I don't know if I'm the right person to review the test changes, since I don't know how it's supposed to work, not having reviewed it originally.
It sounds strange to me that a single migrator is notifying a topic that should happen when all migrators are done (and only the caller can know).
Maybe it's expected, I don't know, it just seems hack-ish.
FYI, Paolo implemented an Integration.jsm module to do on-the-fly replacement of utils in modules (bug 1094201), maybe it could be useful to override part of MigrationUtils.

::: browser/components/migration/tests/unit/test_automigration.js:197
(Diff revision 1)
> +    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
> +    url: "http://www.example.org/",
> +    title: "Some example bookmark",
> +  });
> +
> +  let results = yield PlacesUtils.bookmarks.search({url: "http://www.example.org/"});

Sigh, I must fix the documentation of search, people keep using it when just looking for an url. I knew it was a footgun to add this API just for web extensions :(
I'll file a bug to fix the inline documentation.
please use .fetch() instead of search(), search was intended as a temporary API to query by a partial match, cause the legacy search API is missing some info webextensions need.

::: browser/components/migration/tests/unit/test_automigration.js:205
(Diff revision 1)
> +
> +  // Insert 2 history visits - one in the current migration time, one from before.
> +  let now_uSec = Date.now() * 1000;
> +  let visitedURI = Services.io.newURI("http://www.example.com/", null, null);
> +  yield new Promise(resolve => {
> +    PlacesUtils.asyncHistory.updatePlaces([{

Either use PlacesTestUtils.addVisits or PlacesUtils.history.insert.
updatePlaces has a sucky API for javascript. I should also update the idl to point that out.

::: browser/components/migration/tests/unit/test_automigration.js:228
(Diff revision 1)
> +  results = PlacesUtils.history.executeQuery(query, opts);
> +  results.root.containerOpen = true;
> +  Assert.equal(results.root.childCount, 2, "Should have 2 visits");
> +  // Clean up:
> +  results.root.containerOpen = false;
> +  results = null;

should not be needed, closing the container releases most resources.

::: browser/components/migration/tests/unit/test_automigration.js:243
(Diff revision 1)
> +  // Check that the undo removed the history visits:
> +  results = PlacesUtils.history.executeQuery(query, opts);
> +  results.root.containerOpen = true;
> +  Assert.equal(results.root.childCount, 0, "Should have no more visits");
> +  results.root.containerOpen = false;
> +  results = null;

ditto

::: browser/components/migration/tests/unit/test_automigration.js:246
(Diff revision 1)
> +  Assert.equal(results.root.childCount, 0, "Should have no more visits");
> +  results.root.containerOpen = false;
> +  results = null;
> +
> +  // Check that the undo removed the bookmarks:
> +  results = yield PlacesUtils.bookmarks.search({url: "http://www.example.org/"});

ditto

Comment 5

[:Gijs (he/him)]

Comment on attachment 8764277 [details]
Bug 1271799 - implement undo functionality in automigration code,

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

Comment 6

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/60262/#review57654

> I don't know if I'm the right person to review the test changes, since I don't know how it's supposed to work, not having reviewed it originally.
> It sounds strange to me that a single migrator is notifying a topic that should happen when all migrators are done (and only the caller can know).
> Maybe it's expected, I don't know, it just seems hack-ish.
> FYI, Paolo implemented an Integration.jsm module to do on-the-fly replacement of utils in modules (bug 1094201), maybe it could be useful to override part of MigrationUtils.

This notification is already in the "real" migrator code. It is produced by the XPCOM-ish migrator instance for a browser, not by the individual resources. I think it makes sense as-is, but I guess I could add a comment explaining why I have to fire it manually in the test?

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> https://reviewboard.mozilla.org/r/60262/#review57654
> 
> > I don't know if I'm the right person to review the test changes, since I don't know how it's supposed to work, not having reviewed it originally.
> > It sounds strange to me that a single migrator is notifying a topic that should happen when all migrators are done (and only the caller can know).
> > Maybe it's expected, I don't know, it just seems hack-ish.
> > FYI, Paolo implemented an Integration.jsm module to do on-the-fly replacement of utils in modules (bug 1094201), maybe it could be useful to override part of MigrationUtils.
> 
> This notification is already in the "real" migrator code. It is produced by
> the XPCOM-ish migrator instance for a browser, not by the individual
> resources. I think it makes sense as-is, but I guess I could add a comment
> explaining why I have to fire it manually in the test?

Specifically here: http://searchfox.org/mozilla-central/source/browser/components/migration/MigrationUtils.jsm#245

Comment 8

[Marco Bonardo [:mak]]

Yes I know where it is fired, I was just trying to understand why the test needs to do that manually.

Comment 9

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #8)
> Yes I know where it is fired, I was just trying to understand why the test
> needs to do that manually.

Right, OK. So the idea is that I'm shimming/replacing the instance of nsIBrowserProfileMigrator (based on MigrationUtils' MigratorPrototype). That has the   migrate: function MP_migrate(aItems, aStartup, aProfile) function which is called by the AutoMigrate code. That function is expected to eventually fire that observer notification when the migration is done. Because I'm shimming the migrator so we're sure to have real data and can check the required functions were invoked, I have to fire the observer notification myself. Does that make more sense?

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8764277 [details]
Bug 1271799 - implement undo functionality in automigration code,

https://reviewboard.mozilla.org/r/60262/#review57876

::: browser/components/migration/AutoMigrate.jsm:21
(Diff revision 2)
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Sanitizer",
> +  "resource:///modules/Sanitizer.jsm");

no more needed?

::: browser/components/migration/AutoMigrate.jsm:59
(Diff revision 2)
>          sawErrors = true;
>        } else if (topic == "Migration:Ended") {
>          histogram.add(sawErrors ? "finished-with-errors" : "finished");
>          Services.obs.removeObserver(migrationObserver, "Migration:Ended");
>          Services.obs.removeObserver(migrationObserver, "Migration:ItemError");
> +        Services.prefs.setCharPref(kAutoMigrateFinishedPref, "" + Date.now());

nit: .toString() would look cleaner (imho)

::: browser/components/migration/AutoMigrate.jsm:65
(Diff revision 2)
>        }
>      };
>  
>      Services.obs.addObserver(migrationObserver, "Migration:Ended", false);
>      Services.obs.addObserver(migrationObserver, "Migration:ItemError", false);
> +    Services.prefs.setCharPref(kAutoMigrateStartedPref, "" + Date.now());

ditto

::: browser/components/migration/AutoMigrate.jsm:135
(Diff revision 2)
> +      Cu.reportError(ex);
> +    }
> +    if (!finish || !start) {
> +      return null;
> +    }
> +    return [start * 1000, finish * 1000];

why are you returning microseconds, when later you just divide it by 1000?
I'd return either a timestamp or 2 Date objects.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/101de7dd28aa
implement undo functionality in automigration code, r=mak

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/101de7dd28aa

Comment 13

[Chris More [:cmore]]

:gijs: two questions I want to see if you could help us with:

1) Can you give our onboarding add-on developers guidance on how to call the undo function from an in-product link that the add-on will inject into the footer of about:newtab?

2) Are there any variables that can be read via the add-on after Firefox opens if the auto migration was successful or not?

Comment 14

[:Gijs (he/him)]

(In reply to Chris More [:cmore] from comment #13)
> :gijs: two questions I want to see if you could help us with:
> 
> 1) Can you give our onboarding add-on developers guidance on how to call the
> undo function from an in-product link that the add-on will inject into the
> footer of about:newtab?

Cu.import("resource:///modules/AutoMigrate.jsm");
let canUndoPromise = AutoMigrate.canUndo();

canUndoPromise.then(canUndo => {
  if (canUndo) {
    // insert link to undo
  }

  // on link click:
  AutoMigrate.undo().then(function() { /* undo finished. */ });
});



When are you planning to remove and/or no longer add the link? Note that the "undo" removes all stored passwords and bookmarks, so ideally it should not continue to be possible once the user starts creating new items in those categories.


> 2) Are there any variables that can be read via the add-on after Firefox
> opens if the auto migration was successful or not?

Right now the fallback to the automigration is to show the importer dialog as we do now. More generally, AutoMigrate.canUndo() returns a promise that resolves to |true| iff there was an automigration that hasn't been undone yet and the user is not signed into sync.

Comment 15

[Chris More [:cmore]]

> When are you planning to remove and/or no longer add the link? Note that the "undo" removes all stored
> passwords and bookmarks, so ideally it should not continue to be possible once the user starts creating 
> new items in those categories.

Here's a screenshot of the experience:

https://cloud.githubusercontent.com/assets/9794516/16434876/1bed79ca-3d47-11e6-9165-c1502ce4d4d9.png

You can see the undo link at the bottom.

The undo link will only show during the first session of Firefox and about:newtab will be loaded instead of about:home on the first session only. After that, the modified about:newtab will revert back to normal and the undo link disappears. 

Does that answer your question?

Comment 16

[:Gijs (he/him)]

(In reply to Chris More [:cmore] from comment #15)
> > When are you planning to remove and/or no longer add the link? Note that the "undo" removes all stored
> > passwords and bookmarks, so ideally it should not continue to be possible once the user starts creating 
> > new items in those categories.
> 
> Here's a screenshot of the experience:
> 
> https://cloud.githubusercontent.com/assets/9794516/16434876/1bed79ca-3d47-
> 11e6-9165-c1502ce4d4d9.png
> 
> You can see the undo link at the bottom.
> 
> The undo link will only show during the first session of Firefox and
> about:newtab will be loaded instead of about:home on the first session only.
> After that, the modified about:newtab will revert back to normal and the
> undo link disappears. 
> 
> Does that answer your question?

Yes, though I'd note that "preferences" is IMO a poor choice of words - we don't import meaningful user settings at all, just bookmarks/passwords/history.

Comment 17

[Chris More [:cmore]]

(In reply to :Gijs Kruitbosch from comment #16)
> (In reply to Chris More [:cmore] from comment #15)
> > > When are you planning to remove and/or no longer add the link? Note that the "undo" removes all stored
> > > passwords and bookmarks, so ideally it should not continue to be possible once the user starts creating 
> > > new items in those categories.
> > 
> > Here's a screenshot of the experience:
> > 
> > https://cloud.githubusercontent.com/assets/9794516/16434876/1bed79ca-3d47-
> > 11e6-9165-c1502ce4d4d9.png
> > 
> > You can see the undo link at the bottom.
> > 
> > The undo link will only show during the first session of Firefox and
> > about:newtab will be loaded instead of about:home on the first session only.
> > After that, the modified about:newtab will revert back to normal and the
> > undo link disappears. 
> > 
> > Does that answer your question?
> 
> Yes, though I'd note that "preferences" is IMO a poor choice of words - we
> don't import meaningful user settings at all, just
> bookmarks/passwords/history.

We have changed it from preferences to data a few hours ago. thanks!

Comment 18

[:Gijs (he/him)]

Comment on attachment 8764277 [details]
Bug 1271799 - implement undo functionality in automigration code,

Approval Request Comment
[Feature/regressing bug #]: automigration, used in funnelcake builds 88 (bug 1295873)
[User impact if declined]: no funnelcake. :-(
[Describe test coverage new/current, TreeHerder]: comes with additions to the automated xpcshell test
[Risks and why]: low risk. All the code here is behind the pref (so won't run on general release), and there's no telemetry involved. The main goal is to provide an undo API for the funnelcake add-on to use to offer the user the ability to undo the automatic migration (ie remove the stuff we automatically imported from another browser).
[String/UUID change made/needed]: no.

Comment 19

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

Comment on attachment 8764277 [details]
Bug 1271799 - implement undo functionality in automigration code,

Automigration undo code, will be preffed off on 49 except for funnelcake users.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/51f3ece19e7b