1285577 - Ensure we can distinguish auto-imported bookmarks / bookmark folders from others, and remove only the auto-imported ones when the user chooses 'undo' for the automatic migration

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P2)

Description

[:Gijs (he/him)]

Right now we will just remove everything, which also removes the default bookmarks and means we're not comfortable with allowing the user the 'undo' option for any length of time, because we'll also delete bookmarks they've created since the import.

When I talked about this with Marco, it seemed our best bet was going to be using the annotations API.

There are two issues with this:
1) the annotations API is sync, which is potentially problematic for perf, and IIRC requires bookmark ids which we might not always keep/have from the insertion, requiring a separate lookup. So it's slightly painful to integrate at the moment.
2) similar to bug 1285575, we'd need to teach all the migrators to annotate their bookmarks. However, unlike bug 1285575, almost all the migrators support bookmarks (only a few know about passwords). So this is time-consuming to write, and error-prone (what if we get a new browser migrator and we forget to do it there, etc.). It's likely a good idea to unify the "turn this JS-based data into actual bookmarks" part of the migrators in some way, which will avoid having to write (and/or potentially update/rewrite) this code N times for N migrators.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

This patch also addresses logic issues in LoginHelper.maybeImportLogin, renames
a parameter of maybeImportLogin to be consistent and adds a test.

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

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

This patch adds a `fetchMultipleByGuids` method to Bookmarks.jsm that retrieves
multiple bookmarks.  It also allows History.removeVisitsByFilter to take
`limit` and `url` options restricting which visits are removed.

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

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them,

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

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1285577 - part 2: keep track of added logins in an import and allow removing them,

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

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

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

Comment 6

[:Gijs (he/him)]

Note that this is a partial (but hopefully carefully unit-tested) patch series. I'm working on the final part, which basically stores the "undo history" into a JSON file and reads it back out when trying to "undo" the import.

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1285577 - part 4: save, use and delete implementations for import undo state,

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

Comment 8

[:Gijs (he/him)]

Comment on attachment 8816213 [details]
Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

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

Comment 9

[:Gijs (he/him)]

Comment on attachment 8816214 [details]
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8816215 [details]
Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them,

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

Comment 11

[:Gijs (he/him)]

Comment on attachment 8816216 [details]
Bug 1285577 - part 2: keep track of added logins in an import and allow removing them,

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

Comment 12

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

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

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8816214 [details]
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

https://reviewboard.mozilla.org/r/96982/#review98036

::: toolkit/components/places/Bookmarks.jsm:635
(Diff revision 2)
> +   * @resolves to an array of objects representing the found items. If no item
> +   *           is found, the returned promise is resolved to null.
> +   * @rejects if an error happens while fetching.
> +   * @throws if the arguments are invalid.
> +   */
> +  fetchMultipleByGuids(guids) {

as discussed, please measure if this makes a big perf difference compared to a for loop of "fetch" calls.
The reason is that I'm trying to keep the API as simple as possible, for maintenance and error-prevention reasons.

If it ends up being useful, then for coherency with the other APIs, this should be named fetchMany (coherent with History.jsm) and should accept an array of guids OR infos (coherent with fetch).

Finally, we should decide in case of no results, if it's better to return an empty array (so the result type is always the same) or null (that may be better test than .length == 0). Both approaches have their pro and cons and I think we didn't stick to one yet.

::: toolkit/components/places/Bookmarks.jsm:639
(Diff revision 2)
> +   */
> +  fetchMultipleByGuids(guids) {
> +    if (!Array.isArray(guids)) {
> +      throw new Error("Should have an array of guids");
> +    }
> +    if (!guids.length) {

this should also check each entry is a proper GUID or object (there are helpers for that)

::: toolkit/components/places/Bookmarks.jsm:1207
(Diff revision 2)
> +                (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount,
> +                p.parent AS _grandParentId, b.syncStatus AS _syncStatus
> +         FROM moz_bookmarks b
> +         LEFT JOIN moz_bookmarks p ON p.id = b.parent
> +         LEFT JOIN moz_places h ON h.id = b.fk
> +         WHERE b.guid IN (${guidReplacer})

There is a limit of variables you can bind, IIRC about 1000, so this should chunk at 999.
But actually, there is no risk in directly building the query (without binding) if you previously check that all the values are valid guids in the API main method, those have no risk. Not binding may also be faster.
On the other side, there is also a limit to the length of a sql query, 1000000 bytes, it's unlikely we hit it, but we may have to protect from that.

::: toolkit/components/places/Bookmarks.jsm:1210
(Diff revision 2)
> +         LEFT JOIN moz_bookmarks p ON p.id = b.parent
> +         LEFT JOIN moz_places h ON h.id = b.fk
> +         WHERE b.guid IN (${guidReplacer})
> +        `, guids);
> +
> +      let itemMap = new Map(rowsToItemsArray(rows).map(bm => { return [bm.guid, bm] }));

{ return... } could be avoided with arrow function

::: toolkit/components/places/History.jsm:373
(Diff revision 2)
>      }
> -    if (!hasBeginDate && !hasEndDate) {
> +    if (!hasBeginDate && !hasEndDate && !hasURL && !hasLimit) {
>        throw new TypeError("Expected a non-empty filter");
>      }
>  
> +    if (hasURL && !(filter.url instanceof URL)) {

to simplify compatibility with the old API and simpler usage, all the new Places APIs when taking urls accept either an nsIURI, URL or a plain href string.
in all the cases you can convert at the beginning to a URL (invoking URL on an URL is fine so the only case to handle apart is nsIURI)

::: toolkit/components/places/History.jsm:813
(Diff revision 2)
>    }
> +  if ("limit" in filter) {
> +    args.limit = Number(filter.limit);
> +  }
> +
> +  if ("url" in filter && filter.url instanceof URL) {

if there's url in filter, is should be a URL at this point, otherwise something went wrong in the API call. Basically if the value is invalid we should throw, if it's valid here we should always have a URL and not need to check it.

::: toolkit/components/places/History.jsm:814
(Diff revision 2)
> +  if ("limit" in filter) {
> +    args.limit = Number(filter.limit);
> +  }
> +
> +  if ("url" in filter && filter.url instanceof URL) {
> +    let rows = yield db.executeCached(`SELECT id FROM moz_places WHERE url = :url`, {url: filter.url.href});

we don't have an index on url, so this is quite slow, we instead have an url on a url hash.
Basically you should do WHERE url_hash = hash(:url) AND url = :url

Couldn't this be put directly into the final query as a condition like place_id = (SELECT id FROM moz_places ...)? Running a separate query seems to add pointless overhead in this case.

::: toolkit/components/places/History.jsm:825
(Diff revision 2)
>    let visitsToRemove = [];
>    let pagesToInspect = new Set();
>    let onResultData = onResult ? [] : null;
>  
> +  let sql = `SELECT id, place_id, visit_date / 1000 AS date, visit_type FROM moz_historyvisits
> +             WHERE ${ conditions.join(" AND ") }${ args.limit ? " LIMIT :limit" : "" }`

nit: I don't see a reason to move this out to a temp var

::: toolkit/components/places/tests/bookmarks/test_bookmarks_fetchMultipleByGuids.js:10
(Diff revision 2)
> +                /Should have an array of guids/);
> +  Assert.throws(() => PlacesUtils.bookmarks.fetchMultipleByGuids({}),
> +                /Should have an array of guids/);
> +  Assert.throws(() => PlacesUtils.bookmarks.fetchMultipleByGuids(""),
> +                /Should have an array of guids/);
> +});

please add tests of arrays containing invalid data

::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js:32
(Diff revision 2)
> -    let bookmarks = new Set(options.bookmarks);
> +    let bookmarkIndices = new Set(options.bookmarks);
>      let visits = [];
> -    let visitByURI = new Map();
> +    let frecencyChangePromises = new Map();
> +    let uriDeletePromises = new Map();
> +    let getURL = options.url ?
> +      i => "http://mozilla.com/test_browserhistory/test_removeVisitsByFilter/removeme/byurl/" + Math.floor(i / (SAMPLE_SIZE / 5)) + "/" :

please add a test for nsIURI and plain href.
Also tests for passing invalid limit and url values.

::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js:107
(Diff revision 2)
> +      filter.url = new URL(removedItems[0].uri.spec);
> +      endIndex = Math.min(endIndex, removedItems.findIndex((v, index) => v.uri.spec != filter.url.href) - 1);
> +    }
> +    removedItems.splice(endIndex + 1);
> +    let remainingItems = visits.filter(v => !removedItems.includes(v));
> +    Cu.reportError(remainingItems.map(v => v.uri.spec).join(';\n'));

leftover?

::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js:116
(Diff revision 2)
>        test.toRemove = true;
> -      if (test.hasBookmark) {
> -        test.onFrecencyChanged = PromiseUtils.defer();
> -      } else {
> -        test.onDeleteURI = PromiseUtils.defer();
> +      if (test.hasBookmark ||
> +          (options.url && remainingItems.some(v => v.uri.spec == removedItems[i].uri.spec))) {
> +        frecencyChangePromises.set(removedItems[i].uri.spec, PromiseUtils.defer());
> +      } else if (!options.url || i == 0) {
> +        Cu.reportError(`Setting onDeleteURI for ${i} (${removedItems[i].uri.spec})`);

ditto

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

https://reviewboard.mozilla.org/r/96988/#review98060

::: browser/components/migration/tests/unit/test_automigration.js:47
(Diff revision 2)
> +    Task.async(function* (db) {
> +      rv = yield db.execute(
> +        `SELECT count(*) FROM moz_historyvisits v
> +         JOIN moz_places h ON h.id = v.place_id
> +         WHERE url_hash = hash(:url) AND url = :url`,
> +         {url});

looks like you would need history.fetch with a fetchVisits option... unfortunately we never found resources to write it :(
If you want to give it a first kick-off I'd not complain, otherwise please add a comment here that this should be replaced by history.fetch once available.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

https://reviewboard.mozilla.org/r/97514/#review98062

::: browser/components/migration/AutoMigrate.jsm:19
(Diff revision 1)
>  const kAutoMigrateBrowserPref = "browser.migrate.automigrate.browser";
>  
>  const kAutoMigrateLastUndoPromptDateMsPref = "browser.migrate.automigrate.lastUndoPromptDateMs";
>  const kAutoMigrateDaysToOfferUndoPref = "browser.migrate.automigrate.daysToOfferUndo";
>  
> -const kPasswordManagerTopic = "passwordmgr-storage-changed";
> +const kUndoStateFile = "initialMigrationMetadata.json";

May I suggest using jsonlz4?
2 obvious reasons:
1. less I/O on startup
2. history and bookmarks for some users could be "large"

::: browser/components/migration/AutoMigrate.jsm:111
(Diff revision 1)
> -        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
> -        // 'undo' anything.
> +        // Save the undo history and block shutdown on that save completing.
> +        AsyncShutdown.profileBeforeChange.addBlocker(
> +          "AutoMigrate Undo saving", this.saveUndoState());

I suggest passing the third argument with a fetchState. Without it you may get shutdown crashes but you will have an hard time figuring out where the issue happened. The downside is that you'll need a state object and to keep it up-to-date along the operation

Comment 16

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

https://reviewboard.mozilla.org/r/96988/#review98060

> looks like you would need history.fetch with a fetchVisits option... unfortunately we never found resources to write it :(
> If you want to give it a first kick-off I'd not complain, otherwise please add a comment here that this should be replaced by history.fetch once available.

This was copy-pasted from existing places tests, fwiw...

Comment 17

[:Gijs (he/him)]

Comment on attachment 8816214 [details]
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

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

Comment 18

[:Gijs (he/him)]

Comment on attachment 8816215 [details]
Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them,

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

Comment 19

[:Gijs (he/him)]

Comment on attachment 8816216 [details]
Bug 1285577 - part 2: keep track of added logins in an import and allow removing them,

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

Comment 20

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

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

Comment 21

[:Gijs (he/him)]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

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

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8816213 [details]
Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

https://reviewboard.mozilla.org/r/96980/#review98268

::: toolkit/components/passwordmgr/LoginHelper.jsm:579
(Diff revision 2)
>      // Add the login only if it doesn't already exist
>      // if the login is not already available, it's going to be added or merged with other
>      // logins
> -    if (existingLogins.some(l => login.matches(l, true))) {
> +    if (existingLogins.some(l => login.matches(l, false))) {

The comment is now inaccurate

::: toolkit/components/passwordmgr/LoginHelper.jsm:589
(Diff revision 2)
>          // Bug 1187190: Password changes should be propagated depending on timestamps.
>          // this an old login or a just an update, so make sure not to add it

Please update/remove this.

::: toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js:16
(Diff revision 2)
> +add_task(function test_new_logins() {
> +  LoginHelper.maybeImportLogin({

Please add some tests for the maybeImportLogin return value.

::: toolkit/components/passwordmgr/test/unit/xpcshell.ini:32
(Diff revision 2)
>  [test_logins_decrypt_failure.js]
>  skip-if = os == "android" # Bug 1171687: Needs fixing on Android
> +[test_maybeImportLogin.js]
>  [test_user_autocomplete_result.js]
>  skip-if = os == "android"
>  [test_logins_metainfo.js]

Please move your test in the correct alphabetical order ignoring test_user_autocomplete_result.js

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8816216 [details]
Bug 1285577 - part 2: keep track of added logins in an import and allow removing them,

https://reviewboard.mozilla.org/r/96986/#review98272

::: browser/components/migration/tests/unit/test_automigration.js:394
(Diff revision 3)
> +add_task(function* checkUndoLoginsState() {
> +  MigrationUtils.startUndoHistory();
> +  MigrationUtils.insertLoginWrapper({
> +    username: "foo",
> +    password: "bar",
> +    hostname: "example.com",

This should be an origin. Not sure why it's not already throwing

::: browser/components/migration/tests/unit/test_automigration.js:398
(Diff revision 3)
> +    password: "bar",
> +    hostname: "example.com",
> +    formSubmitURL: "https://example.com/",
> +    timeCreated: new Date(),
> +  });
> +  let storedLogins = Services.logins.findLogins({}, "example.com", "", "");

"example.com" is not a valid origin. I know it's confusing that `hostname` is actually an origin.

::: browser/components/migration/tests/unit/test_automigration.js:413
(Diff revision 3)
> +  MigrationUtils.startUndoHistory();
> +  MigrationUtils.insertLoginWrapper({
> +    username: "foo",
> +    password: "bar",
> +    hostname: "example.com",
> +    submitURL: "https://example.com/",

`formSubmitURL` here and elsewhere

::: browser/components/migration/tests/unit/test_automigration.js:431
(Diff revision 3)
> +    password: "bazzy",
> +    hostname: "example.org",
> +    submitURL: "https://example.org/",
> +    timePasswordChanged: new Date(),
> +  });
> +  Assert.equal(1, LoginHelper.searchLoginsWithObject({hostname: "example.org", formSubmitURL: "https://example.org/"}).length,

Ditto

::: browser/components/migration/tests/unit/test_automigration.js:436
(Diff revision 3)
> +  Assert.equal(0, LoginHelper.searchLoginsWithObject({hostname: "example.com", formSubmitURL: "https://example.com/"}).length,
> +                   "unchanged example.com entry should have been removed.");
> +  Assert.equal(1, LoginHelper.searchLoginsWithObject({hostname: "example.org", formSubmitURL: "https://example.org/"}).length,

Ditto

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 8816214 [details]
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

https://reviewboard.mozilla.org/r/96982/#review99074

::: toolkit/components/places/History.jsm:822
(Diff revision 3)
> +    if (url instanceof Ci.nsIURI) {
> +      url = filter.url.spec;
> +    } else {
> +      url = new URL(url).href;
> +    }
> +    optionalJoin = `INNER JOIN moz_places ON moz_places.id = v.place_id`;

nit: just JOIN, we usually omit INNER cause it doesn't make any difference. we usually just use LEFT JOIN or JOIN, in some rare case you may need an explicit CROSS JOIN but it's rare.

::: toolkit/components/places/History.jsm:823
(Diff revision 3)
> +      url = filter.url.spec;
> +    } else {
> +      url = new URL(url).href;
> +    }
> +    optionalJoin = `INNER JOIN moz_places ON moz_places.id = v.place_id`;
> +    conditions.push("moz_places.url_hash = hash(:url)", "moz_places.url = :url");

please assign an alias to moz_places (we usually use "h" for historical reasons) and use h. instead of moz_places. all around (JOIN moz_places h ON ...)

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8816215 [details]
Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them,

https://reviewboard.mozilla.org/r/96984/#review99388

::: browser/components/migration/AutoMigrate.jsm:399
(Diff revision 3)
> +  UNDO_SUCCESSFUL: 0,
> +  UNDO_MISSING_OR_CORRUPT: 1,
> +  UNDO_BOOKMARKS_FAILED: 2,
> +  UNDO_VISITS_FAILED: 3,
> +  UNDO_LOGINS_FAILED: 4,
> +  _removeUnchangedBookmarks: Task.async(function* __removeUnchangedBookmarks(bookmarks) {

nit: add newline above?
question: does naming the generators in Task.async give better stacks? We usually don't name those, but if stacks are better we probably should.

::: browser/components/migration/AutoMigrate.jsm:405
(Diff revision 3)
> +    if (!bookmarks.length) {
> +      return;
> +    }
> +
> +    let guidToLMMap = new Map(
> +        bookmarks.map(b => { return [b.guid, b.lastModified] })

nit: you don't need the { return }, and then you can likely go oneline, if it's readable.

::: browser/components/migration/AutoMigrate.jsm:439
(Diff revision 3)
> +      bm._ancestorCount = myCount;
> +      return myCount;
> +    }
> +    for (let bm of unchangedBookmarks) {
> +      determineAncestorCount(bm);
> +    }

can you use .forEach here?

::: browser/components/migration/AutoMigrate.jsm:440
(Diff revision 3)
> +      return myCount;
> +    }
> +    for (let bm of unchangedBookmarks) {
> +      determineAncestorCount(bm);
> +    }
> +    unchangedBookmarks.sort((a, b) => b._ancestorCount - a._ancestorCount);

...and then just chain .sort

::: browser/components/migration/AutoMigrate.jsm:444
(Diff revision 3)
> +    }
> +    unchangedBookmarks.sort((a, b) => b._ancestorCount - a._ancestorCount);
> +    for (let {guid} of unchangedBookmarks) {
> +      yield PlacesUtils.bookmarks.remove(guid, {preventRemovalOfNonEmptyFolders: true}).catch(err => {
> +        if (err && err.message != "Cannot remove a non-empty folder.") {
> +          throw this.UNDO_BOOKMARKS_FAILED;

So, the chosen strategy is to give up at the first error, rather than trying to continue and then report eventual errors later?
I'm asking just to be sure this is wanted rather than a miss.

::: browser/components/migration/MigrationUtils.jsm:41
(Diff revision 3)
>  var gMigrators = null;
>  var gProfileStartup = null;
>  var gMigrationBundle = null;
>  var gPreviousDefaultBrowserKey = "";
>  
> +let gKeepUndoHistory = false;

It may be confusing to use the word History here, when we also are undoing browser History!
What about s/History/Data/ all around?

::: browser/components/migration/MigrationUtils.jsm:954
(Diff revision 3)
>      history: 0,
>    },
>  
>    insertBookmarkWrapper(bookmark) {
>      this._importQuantities.bookmarks++;
> -    return PlacesUtils.bookmarks.insert(bookmark);
> +    let insertHandler = val => val;

Could you please add a very brief comment explaining insertHandler usage for future ref?

::: browser/components/migration/MigrationUtils.jsm:978
(Diff revision 3)
>    insertLoginWrapper(login) {
>      this._importQuantities.logins++;
>      return LoginHelper.maybeImportLogin(login);
>    },
>  
> +  startUndoHistory() {

s/start/reset/ (or initialize)?

::: browser/components/migration/MigrationUtils.jsm:998
(Diff revision 3)
> +      return PlacesUtils.bookmarks.fetch(guid).then(bm => bm && bookmarkFolderData.push(bm), () => {});
> +    });
> +
> +    yield Promise.all(bmPromises);
> +    let folderLMMap = new Map(
> +        bookmarkFolderData.map(b => { return [b.guid, b.lastModified] })

ditto on { return }

::: browser/components/migration/MigrationUtils.jsm:1001
(Diff revision 3)
> +    yield Promise.all(bmPromises);
> +    let folderLMMap = new Map(
> +        bookmarkFolderData.map(b => { return [b.guid, b.lastModified] })
> +    );
> +    for (let bookmark of bookmarkFolders) {
> +      if (bookmark.type == PlacesUtils.bookmarks.TYPE_FOLDER) {

How can this not be true?
If there's really a care, please comment about it.

::: browser/components/migration/MigrationUtils.jsm:1004
(Diff revision 3)
> +    );
> +    for (let bookmark of bookmarkFolders) {
> +      if (bookmark.type == PlacesUtils.bookmarks.TYPE_FOLDER) {
> +        let lastModified = folderLMMap.get(bookmark.guid);
> +        // If the bookmark was deleted, the map will be returning null, so
> +        // check:

nit: no reason to go newline, we accept going slightly over 80 when there's no good reason to not

::: browser/components/migration/MigrationUtils.jsm:1014
(Diff revision 3)
> +    }
> +    return state;
> +  }),
> +
> +  stopAndRetrieveUndoHistory() {
> +    let rv = gUndoHistory;

I sort-of expect rv to be boolean/nserror, for historical reasons. better name please, like undoData?

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

https://reviewboard.mozilla.org/r/96988/#review99410

I'd probably change some of the storing logic to reduce size of the stored data and simplify the removal, but nothing blocking.

::: browser/components/migration/AutoMigrate.jsm:475
(Diff revision 3)
> +      let urlObj;
> +      try {
> +        urlObj = new URL(url);
> +      } catch (ex) {
> +        continue;
> +      }

Is there a reason to do this here, rather than doing the check when we actually add the visits to the store?
If I read correctly, you add the visits to the store when you create the place objects, and those are nsIURI (or URL once you'll move to History.insertMany) so they are already verified.
Personally, I'd convert urls to URL() when adding them to the store and just trust the store contents here, for simplicity.

::: browser/components/migration/AutoMigrate.jsm:480
(Diff revision 3)
> +      }
> +      yield PlacesUtils.history.removeVisitsByFilter({
> +        url: urlObj,
> +        beginDate: PlacesUtils.toDate(urlVisits.first),
> +        endDate: PlacesUtils.toDate(urlVisits.last),
> +        limit: urlVisits.visitCount,

Based on this, I don't see why you store all the visits rather than just first, last and count.
Supposing you don't have duped urls, you could just store a more memory/store efficient array of
[ { url, first, last, count } ... ]
that would only require a tiny additional amount of logic in the storing part.

::: browser/components/migration/MigrationUtils.jsm:1047
(Diff revision 3)
> +        currentData.visitCount += visitCount;
> +        currentData.first = Math.min(currentData.first, first);
> +        currentData.last = Math.max(currentData.last, last);
> +      }
> +    }
> +  },

...So here you could makea temp map, and in the end "normalize" it to a simple array in visits.
This would also have the advantage to make all the stores consistent (all arrays).

::: browser/components/migration/tests/unit/test_automigration.js:8
(Diff revision 3)
>  Cu.import("resource:///modules/MigrationUtils.jsm");
>  Cu.import("resource://gre/modules/LoginHelper.jsm");
>  Cu.import("resource://gre/modules/PlacesUtils.jsm");
>  Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/PromiseUtils.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");

if there's an head file here, I'd suggest to move some of these imports there rather than repeating them in all the migration tests.

::: browser/components/migration/tests/unit/test_automigration.js:42
(Diff revision 3)
>  
> +// This should be replaced by using History.fetch with a fetchVisits option,
> +// once that becomes available
> +function* visitsForURL(url)
> +{
> +  let rv = 0;

s/rv/visitCount/?

::: browser/components/migration/tests/unit/test_automigration.js:43
(Diff revision 3)
> +// This should be replaced by using History.fetch with a fetchVisits option,
> +// once that becomes available
> +function* visitsForURL(url)
> +{
> +  let rv = 0;
> +  yield PlacesUtils.withConnectionWrapper("test_automigration visitsForURL:",

You could use simple promiseDBConnection here, it's a select and you don't care about adding a shutdown blocker (that is what withConnectionWrapper does).

::: browser/components/migration/tests/unit/test_automigration.js:466
(Diff revision 3)
>  });
>  
> +add_task(function* checkUndoVisitsState() {
> +  MigrationUtils.startUndoHistory();
> +  yield MigrationUtils.insertVisitsWrapper([{
> +    uri: Services.io.newURI("http://www.example.com/", null, null),

nit: NetUtil.newURI?

::: browser/components/migration/tests/unit/test_automigration.js:470
(Diff revision 3)
> +  yield MigrationUtils.insertVisitsWrapper([{
> +    uri: Services.io.newURI("http://www.example.com/", null, null),
> +    title: "Example",
> +    visits: [{
> +      visitDate: new Date("2015-07-10").getTime() * 1000,
> +      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,

Using insertMany would simplify this since you could use an href and transitionType would be optional :(

::: browser/components/migration/tests/unit/test_automigration.js:500
(Diff revision 3)
> +  }], {
> +    handleResult: function() {
> +    },
> +    handleError: function() {},
> +    handleCompletion: function() {
> +    }

Is the callback mandatory?
at least the 3 methods could use consistent coding style.

::: browser/components/migration/tests/unit/test_automigration.js:581
(Diff revision 3)
> +  yield PlacesUtils.history.insertMany([{
> +    url: "http://www.example.com/",
> +    title: "Example",
> +    visits: [{
> +      date: new Date("2015-08-16"),
> +      transition: Ci.nsINavHistoryService.TRANSITION_LINK,

with insertMany TRANSITION_LINK should be optional and the default.

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

https://reviewboard.mozilla.org/r/97514/#review99420

not marking yet cause I have a bit more comments here than the previous parts. And mostly I'm worried about a couple issues with memory usage and startup perf.

::: browser/components/migration/AutoMigrate.jsm:41
(Diff revision 2)
>  
>  Cu.importGlobalProperties(["URL"]);
>  
> +/* globals kUndoStateFullPath */
> +XPCOMUtils.defineLazyGetter(this, "kUndoStateFullPath", function() {
> +  return OS.Path.join(OS.Constants.Path.profileDir, kUndoStateFile);

considered it's only used here, I'm not sure how much it's useful to keep kUndoStateFile rather than directly hardcoding the name here.

::: browser/components/migration/AutoMigrate.jsm:184
(Diff revision 2)
> -  getUndoRange() {
> -    let start, finish;
> +  canUndo: Task.async(function* () {
> +    if (this._savingPromise) {
> +      yield this._savingPromise;
> +    }
>      try {
> -      start = parseInt(Preferences.get(kAutoMigrateStartedPref, "0"), 10);
> +      let readPromise = OS.File.read(kUndoStateFullPath, {

there is a pitfall here, if one uses canUndo anyway, we load the undo file, causing I/O and memory usage, likely in the startup path.

While this is not a big deal when we know we want to undo, if someone in the future uses canUndo to figure out whether he should show some kind of UI to the user, that would cause every session to pay the load cost.
I don't know if that's already the case with maybeShowUndoNotification, but if this stays like this, it deserves a large WARNING above it.

A stat to check file existence would be cheaper, and then maybe you could have a lazy getter for the cache?

::: browser/components/migration/AutoMigrate.jsm:188
(Diff revision 2)
>      try {
> -      start = parseInt(Preferences.get(kAutoMigrateStartedPref, "0"), 10);
> -      finish = parseInt(Preferences.get(kAutoMigrateFinishedPref, "0"), 10);
> +      let readPromise = OS.File.read(kUndoStateFullPath, {
> +        encoding: "utf-8",
> +        compression: "lz4",
> +      });
> +      this._cachedUndoState = this._dejsonifyUndoState(yield readPromise);

is this nullified somewhere, after an undo completes? It seems pointless to keep using this memory after that point.

::: browser/components/migration/AutoMigrate.jsm:210
(Diff revision 2)
>  
> -    yield PlacesUtils.bookmarks.eraseEverything();
> +    let stateData = this._cachedUndoState;
> +    yield this._removeUnchangedBookmarks(stateData.get("bookmarks"));
>      histogram.add(15);
>  
> -    // NB: we drop the start time of the migration for now. This is because
> +    yield this._removeSomeVisits(stateData.get("visits"));

do we want try/catch around single removals? if bookmarks fail, nothing else will succeed.

::: browser/components/migration/AutoMigrate.jsm:346
(Diff revision 2)
> +      return "null";
> +    }
> +    // Deal with date serialization.
> +    let bookmarks = state.get("bookmarks");
> +    for (let bm of bookmarks) {
> +      bm.lastModified = bm.lastModified.getTime();

Is this really needed? I was expecting stringify to already convert Date to timestamps :(

::: browser/components/migration/AutoMigrate.jsm:351
(Diff revision 2)
> +      bm.lastModified = bm.lastModified.getTime();
> +    }
> +    let serializableState = {
> +      bookmarks,
> +      logins: state.get("logins"),
> +      visits: Array.from(state.get("visits").entries()),

If visits would already be an array, this would be simpler :)

::: browser/components/migration/AutoMigrate.jsm:364
(Diff revision 2)
> +      bm.lastModified = new Date(bm.lastModified);
> +    }
> +    return new Map([
> +      ["bookmarks", state.bookmarks],
> +      ["logins", state.logins],
> +      ["visits", new Map(state.visits)],

ditto

Comment 29

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

https://reviewboard.mozilla.org/r/96988/#review99410

> Is there a reason to do this here, rather than doing the check when we actually add the visits to the store?
> If I read correctly, you add the visits to the store when you create the place objects, and those are nsIURI (or URL once you'll move to History.insertMany) so they are already verified.
> Personally, I'd convert urls to URL() when adding them to the store and just trust the store contents here, for simplicity.

In many cases we'll be reading the URLs from disk, and JSON.stringify(new URL(...)) returns "{}", so storing as strings seems more sensible anyway. I can add verification that the URL constructor succeeds, but it seems like we should keep that check here anyway, as we'll be reading data from disk that might be busted.

Comment 30

[:Gijs (he/him)]

Comment on attachment 8816213 [details]
Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

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

Comment 31

[:Gijs (he/him)]

Comment on attachment 8816214 [details]
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96982/diff/3-4/

Comment 32

[:Gijs (he/him)]

Comment on attachment 8816215 [details]
Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96984/diff/3-4/

Comment 33

[:Gijs (he/him)]

Comment on attachment 8816216 [details]
Bug 1285577 - part 2: keep track of added logins in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96986/diff/3-4/

Comment 34

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96988/diff/3-4/

Comment 35

[:Gijs (he/him)]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

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

Comment 36

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96988/diff/4-5/

Comment 37

[:Gijs (he/him)]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97514/diff/3-4/

Comment 38

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #29)
> In many cases we'll be reading the URLs from disk, and JSON.stringify(new
> URL(...)) returns "{}", so storing as strings seems more sensible anyway.

I meant "ensure urls are valid (through URL or nsIURI) BEFORE writing them, still write them as strings".
Btw, fine to keep the check, was just trying to simplify the code.

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

https://reviewboard.mozilla.org/r/97514/#review100252

::: browser/components/migration/AutoMigrate.jsm:186
(Diff revision 4)
>      try {
> -      start = parseInt(Preferences.get(kAutoMigrateStartedPref, "0"), 10);
> +      fileExists = yield OS.File.exists(kUndoStateFullPath);
> -      finish = parseInt(Preferences.get(kAutoMigrateFinishedPref, "0"), 10);
>      } catch (ex) {
>        Cu.reportError(ex);
> +      return fileExists;

what's the point of this early return?

Comment 40

[:Gijs (he/him)]

Comment on attachment 8816213 [details]
Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96980/diff/3-4/

Comment 41

[:Gijs (he/him)]

Comment on attachment 8816214 [details]
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96982/diff/4-5/

Comment 42

[:Gijs (he/him)]

Comment on attachment 8816215 [details]
Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96984/diff/4-5/

Comment 43

[:Gijs (he/him)]

Comment on attachment 8816216 [details]
Bug 1285577 - part 2: keep track of added logins in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96986/diff/4-5/

Comment 44

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96988/diff/5-6/

Comment 45

[:Gijs (he/him)]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97514/diff/4-5/

Comment 46

[:Gijs (he/him)]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

https://reviewboard.mozilla.org/r/97514/#review99420

> do we want try/catch around single removals? if bookmarks fail, nothing else will succeed.

Not doing try...catch's now that I got rid of the explicit throw in the bookmarks code. Things should succeed now, and if they don't at least this provides you the opportunity to keep the file and debug what went wrong (whereas try...catch would ignore failure and then remove the file).

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61029bd1995a
part 0a: make login manager return the login we're creating with addLogin, r=MattN
https://hg.mozilla.org/integration/autoland/rev/884a499dcc85
part 0b: expand History.removeVisitsByFilter functionality, r=mak
https://hg.mozilla.org/integration/autoland/rev/12aa73f4a982
part 1: keep track of added bookmarks in an import and allow removing them, r=mak
https://hg.mozilla.org/integration/autoland/rev/78a3cd5bf313
part 2: keep track of added logins in an import and allow removing them, r=MattN
https://hg.mozilla.org/integration/autoland/rev/10388e609fe3
part 3: keep track of added visits in an import and allow removing them, r=mak
https://hg.mozilla.org/integration/autoland/rev/1ffe78f2ab5a
part 4: save, use and delete implementations for import undo state, r=mak

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e66794fae8f7
followup: Fix eslint complaining about unused vars and spaces, rs=bustage a=bustage

Comment 49

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing test_autofill_https_upgrade.html at least on Linux:

https://hg.mozilla.org/integration/autoland/rev/0ccdda383ce7b6420e66eb01005207c7d6eca8f4
https://hg.mozilla.org/integration/autoland/rev/0f51e50b2e5fd72ec413a4dcfd483b4786a219b2
https://hg.mozilla.org/integration/autoland/rev/07e8e4c8e6aadfc064c892d78d107d5ac0255239
https://hg.mozilla.org/integration/autoland/rev/0591c29975f7bd20be09bed3e62cf51cfd3fe0f1
https://hg.mozilla.org/integration/autoland/rev/5aa3d2006e2b40fdccf6b326a3e48eb9d90e900f
https://hg.mozilla.org/integration/autoland/rev/a23efb31e4e3586be4551f9fba19fae6770eda5e
https://hg.mozilla.org/integration/autoland/rev/2910e14b19d0187d5397fc558ba70402135edf84

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1ffe78f2ab5a70c620f7fc5f6bf647570efb4e24
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8197039&repo=autoland

[task 2016-12-20T21:15:10.821434Z] 21:15:10     INFO - TEST-START | toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html
[task 2016-12-20T21:15:11.181805Z] 21:15:11     INFO - TEST-PASS | http://mochi.test:8888/tests/toolkit/components/passwordmgr/test/mochitest/parent_utils.js | Got autocomplete popup - [object XULElement] == true
[task 2016-12-20T21:15:11.451917Z] 21:15:11     INFO - TEST-INFO | started process screentopng
[task 2016-12-20T21:15:13.200919Z] 21:15:13     INFO - TEST-INFO | screentopng: exit 0
[task 2016-12-20T21:15:13.201197Z] 21:15:13     INFO - Buffered messages logged at 21:15:11
[task 2016-12-20T21:15:13.201457Z] 21:15:13     INFO - SpawnTask.js | Entering test setup
[task 2016-12-20T21:15:13.201647Z] 21:15:13     INFO - SpawnTask.js | Leaving test setup
[task 2016-12-20T21:15:13.201858Z] 21:15:13     INFO - SpawnTask.js | Entering test test_simpleNoDupesNoAction
[task 2016-12-20T21:15:13.202022Z] 21:15:13     INFO - Buffered messages finished
[task 2016-12-20T21:15:13.204082Z] 21:15:13     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html | TypeError: chromeScript.sendSyncMessage(...)[0] is undefined 
[task 2016-12-20T21:15:13.204323Z] 21:15:13     INFO -     add_task/</<@SimpleTest/SpawnTask.js:282:15
[task 2016-12-20T21:15:13.204592Z] 21:15:13     INFO -     onRejected@SimpleTest/SpawnTask.js:85:15
[task 2016-12-20T21:15:13.204850Z] 21:15:13     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
[task 2016-12-20T21:15:13.205944Z] 21:15:13     INFO -     promise callback*next@SimpleTest/SpawnTask.js:104:45
[task 2016-12-20T21:15:13.206205Z] 21:15:13     INFO -     onFulfilled@SimpleTest/SpawnTask.js:73:7
[task 2016-12-20T21:15:13.208962Z] 21:15:13     INFO -     co/<@SimpleTest/SpawnTask.js:58:5
[task 2016-12-20T21:15:13.212917Z] 21:15:13     INFO -     co@SimpleTest/SpawnTask.js:54:10
[task 2016-12-20T21:15:13.214425Z] 21:15:13     INFO -     add_task/<@SimpleTest/SpawnTask.js:270:9
[task 2016-12-20T21:15:13.215956Z] 21:15:13     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:625:12
[task 2016-12-20T21:15:13.217570Z] 21:15:13     INFO -     add_task@SimpleTest/SpawnTask.js:269:7
[task 2016-12-20T21:15:13.219236Z] 21:15:13     INFO -     @toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html:59:1

Comment 50

[:Gijs (he/him)]

Comment on attachment 8816213 [details]
Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96980/diff/4-5/

Comment 51

[:Gijs (he/him)]

Comment on attachment 8816214 [details]
Bug 1285577 - part 0b: expand History.removeVisitsByFilter functionality,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96982/diff/5-6/

Comment 52

[:Gijs (he/him)]

Comment on attachment 8816215 [details]
Bug 1285577 - part 1: keep track of added bookmarks in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96984/diff/5-6/

Comment 53

[:Gijs (he/him)]

Comment on attachment 8816216 [details]
Bug 1285577 - part 2: keep track of added logins in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96986/diff/5-6/

Comment 54

[:Gijs (he/him)]

Comment on attachment 8816217 [details]
Bug 1285577 - part 3: keep track of added visits in an import and allow removing them,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96988/diff/6-7/

Comment 55

[:Gijs (he/him)]

Comment on attachment 8817088 [details]
Bug 1285577 - part 4: save, use and delete implementations for import undo state,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97514/diff/5-6/

Comment 56

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4179f14e9cf1

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5651a6942059
part 0a: make login manager return the login we're creating with addLogin, r=MattN
https://hg.mozilla.org/integration/autoland/rev/b47437c531cc
part 0b: expand History.removeVisitsByFilter functionality, r=mak
https://hg.mozilla.org/integration/autoland/rev/f128cff62d21
part 1: keep track of added bookmarks in an import and allow removing them, r=mak
https://hg.mozilla.org/integration/autoland/rev/16ca9b1a9503
part 2: keep track of added logins in an import and allow removing them, r=MattN
https://hg.mozilla.org/integration/autoland/rev/8412c23be23e
part 3: keep track of added visits in an import and allow removing them, r=mak
https://hg.mozilla.org/integration/autoland/rev/16849313bde4
part 4: save, use and delete implementations for import undo state, r=mak

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5651a6942059
https://hg.mozilla.org/mozilla-central/rev/b47437c531cc
https://hg.mozilla.org/mozilla-central/rev/f128cff62d21
https://hg.mozilla.org/mozilla-central/rev/16ca9b1a9503
https://hg.mozilla.org/mozilla-central/rev/8412c23be23e
https://hg.mozilla.org/mozilla-central/rev/16849313bde4

Comment 59

[:Gijs (he/him)]

Comment on attachment 8816213 [details]
Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

Approval Request Comment
[Feature/Bug causing the regression]: funnelcake for 51, see bug 1322718 comment #1

[User impact if declined]:
Uplifting this isn't *strictly* necessary for the funnelcake to work.

However, the funnelcake flow will suggest users sign up for sync. Without these patches, as soon as that happens (or when users otherwise change bookmarks/passwords), we will stop offering them the possibility of undoing the automatic migration that was executed on startup.

We know from telemetry on previous funnelcakes and the beta experiment we ran on 51 that most users hit this path - they sign into sync or modify bookmarks/logins, causing the undo option to be disabled. This has two very significant negative effects: we don't get conclusive information about whether those users are happy with the import (ie we lack data to make decisions about automigration), and if users are unhappy, they lack an easy "get rid of this" option.

These patches solve these problems.

[Is this code covered by automated tests?]:
Yes. All the changes come with a considerable amount of automated test coverage, to the point where test changes make up the majority of the combined patch size. 

[Has the fix been verified in Nightly?]:
No, because the feature they affect is behind a pref that is off by default on all branches.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes - it will need manual testing on a build with automigration enabled. Testing would require checking that the undo option remains enabled after changing bookmarks or passwords or signing into sync, and that any imported data is deleted successfully if the user does decide to "undo" the automated import/migration of data from another browser.

[List of other uplifts needed for the feature/fix]:
I don't believe this patch needs any other changes in order to be uplifted. The funnelcake will need the deps of bug 1322718.

[Is the change risky?]:
The change is not risky for general release of 51, but it carries some risk for the funnelcake.

[Why is the change risky/not risky?]:
The majority of the changes are behind a pref that will only be turned on for the funnelcake. A small set of changes (to internal password manager and history APIs) are not behind this pref. Considering the automated test coverage of the password manager and History.jsm APIs, as well as the tests in these patches, I believe that the patches are low risk for the 51 general release. The risk for the funnelcake itself is obviously higher as there are modifications to APIs it uses, but I believe that the trade-off for the funnelcake is such that it's worth taking these patches.

[String changes made/needed]:
No.

(NB: because 51 doesn't have the changes in bug 1311043, I expect it might need a branch patch, but if so that won't be difficult to generate given the nature of the changes in bug 1311043 (linting fixes).)

Comment 60

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

Comment on attachment 8816213 [details]
Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin,

Looks useful for the funnelcake experiment!
Thanks for adding extra tests. 
It would be good to do some manual testing around this (either on nightly now, or once it lands on beta, or both)

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/053902fa07fb
https://hg.mozilla.org/releases/mozilla-aurora/rev/263cdae5e897
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b044befe15a
https://hg.mozilla.org/releases/mozilla-aurora/rev/cba88336e84f
https://hg.mozilla.org/releases/mozilla-aurora/rev/81bd0d4207b2
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e8d7063d14e

Comment 62

[Carsten Book [:Tomcat]]

needs rebasing for beta

merging browser/components/migration/tests/unit/test_automigration.js
warning: conflicts while merging browser/components/migration/tests/unit/test_automigration.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 63

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/6ab8d4e5fda02cd6d0d038a4717d9cf43ef7322a
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9c30726b5e60705a9e837bfb335033d5ed1789f1
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/1777c645e568f64617dbaf21e896d074322c81ea
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/ab66d33ab50d61018172a6e240ec05dafc1e0759
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/8782cf9b673b954a4e0fd73f4bfbf2596300a90f
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/faa5342a058268bdba1b87b65e56a55d51522bbf

Comment 64

[Andrei Vaida [:avaida]]

Setting the flag for verification, instructions available in Comment 59.

Comment 66

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The bug is verified fixed on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 using 51.0.1 build3 (20170125094131), 52.0b2 build1 (20170130065342) and 53.0a2 (2017-01-31). Marking the flags accordingly.