Closed Bug 1199077 Opened 9 years ago Closed 6 years ago

Store the sync ID and last sync time for bookmarks and history in Places

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: markh, Assigned: lina)

References

Details

Attachments

(8 files, 1 obsolete file)

59 bytes, text/x-review-board-request
markh
: review+
Details
59 bytes, text/x-review-board-request
eoger
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
ursula
: review+
Details
59 bytes, text/x-review-board-request
tcsc
: review+
Details
59 bytes, text/x-review-board-request
eoger
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
tcsc
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
markh
: review+
Details
Firefox has code to recover from a missing or corrupt places.sqlite - it recreates it (also recreating all default bookmarks etc). But Sync doesn't re-populate the bookmarks etc - they remain missing from that profile.

To reproduce:
* Make a profile, make some bookmarks, sync.
* Make a second profile, sync, observer all bookmarks from first profile exist.
* Exit, then delete places.sqlite from second profile.
* Restart second profile - observe default bookmarks exist but none of those new bookmarks from the first profile exist, even after syncing.

This is Sync optimizing for the "correct" case but it should have some way of detecting and handling this (ie, doing a reset automatically).

Things tend to get back to normal after signing out of Sync on that second profile then signing back in.

The simplest fix might be for the code that handles the creation of places to write a pref or similar with the time that happened, and sync could compare that with the timestamps it already uses.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P2
FTR, from bug 1274896 comment 5:

So the 2 tasks here are:

* Work out some sane way for Places to let us know that an "auto restore" of bookmarks happened, and ensure that our existing "bookmarks-restore-success" observer does *not* kick in here (that's bug 1230011; user-initiated-restore and auto-restore get different treatment. Note that auto-restore almost certainly happens before Sync is initialized.

* Make the .resetClient() call when Sync initializes and it finds an auto restore was done [for only the bookmarks and history engines]
Priority: P2 → P3
Assignee: nobody → kcambridge
Assignee: kcambridge → nobody
Priority: P3 → P2
I started looking into this.
Assignee: nobody → kit
Status: NEW → ASSIGNED
See Also: → 1426554
Train musings.

Automatic restores after corruption should do a full reconcile:

* We should not reset the sync ID, and should run a full merge.
* In the mirror, I think we can just flag everything as `needsMerge = 1` if we detect any items in Places with `syncStatus = UNKNOWN`, and run a normal merge after that. In the best case (assuming synchronized clocks, etc.), we'll use the value and structure from the mirror to bring Places back up to date, reset `syncChangeCounter = 0`, and upload nothing.
* In the non-mirror world, we'll need to reset the last sync time and download everything again.

An explicit, user-initiated restore:

* Should reset the sync ID, and should wipe the server and other clients.
* Drop the mirror. After we sync and upload the restored tree, we'll write the records we just uploaded into a new mirror.

I think the two behaviors are different enough where splitting the catch-all `UNKNOWN` sync status into `RESTORED` and `RECOVERED` is worthwhile. We could use the as-yet-nonexistent Places key-value table to store a flag instead, but then we'd need to make sure we clear it after syncing, so that we don't wipe the server *again* if the first sync is interrupted. (Or worse, if we're interrupted after we finished uploading, but before we ran our cleanup logic).

Side note: there's a case where you might change some bookmarks on device A, then restore on device B. When B syncs, it'll tell A to wipe itself, losing the changes you made. I'm not sure it's worth investing much into this, because explicit restores seem fairly rare (citation needed), and you can restore the changes from a backup on A.
Starting with history because it's easier, since it doesn't have a mirror. For bookmarks, we'll want to store the sync ID in the mirror and in Places. If the mirror's sync ID doesn't match what's in Places, we drop the mirror, clear out the sync ID, and reset sync flags. I'll also split these up into separate bugs later.
Depends on: 1439038
Depends on: 1439061
I think I figured out how to make manual restores durable:

1. In `eraseEverything`, we forget the sync ID and last sync time.
2. We listen for the `bookmarks-restore-success` observer notification, and trigger an immediate sync.
3. `ensureCurrentSyncId` kicks in, with the (now stale) sync ID from `meta/global`, sees we don't have a sync ID, and bails.
4. We catch this, send a `wipeEngine` command to all other clients, wipe the server, and assign ourselves a new sync ID.
5. `_syncStartup` sees that the ID we assigned ourselves in (4) doesn't match the one from `meta/global`, and updates `meta/global` to reflect it.

The idea behind doing this in `_syncStartup`, instead of the `bookmarks-restore-success` observer, is that we can repeat this process multiple times if we're interrupted. If we fail to wipe the server, we'll go through the same flow again on the next sync, and won't assign ourselves a new sync ID until we've wiped the server and queued the wipe command for other clients.

The side note in comment 5 still applies: if you interrupt a wipe on A, then make changes on B, then sync A, the changes on B will be lost.

How does that sound?
Priority: P2 → P1
Thom encouraged me to keep thinking about this by pointing out that `RESTORED` and `RECOVERED` are similar, and that we can use global flags instead of per-item sync statuses to capture some of these behaviors. I'm also thinking we can make the "prefer deletions" behavior apply to `UNKNOWN`, meaning we don't have to add new sync statuses at all.

Mulling this over some more...

* We add a Boolean key, `sync/bookmarks/wipeServer`.
* For manual restores, delete `sync/bookmarks/syncId` and `sync/bookmarks/lastSync`, and set `sync/bookmarks/wipeServer = 1`. All bookmarks have `syncStatus = NEW`. (Since we'll be replacing them on the server, it doesn't seem like we need the special `UNKNOWN` behavior).
* For automatic restores, set `sync/bookmarks/wipeServer = 1`. All restored bookmarks have `syncStatus = UNKNOWN`. This gives us the "prefer delete" behavior for automatic restores, too.
* For an `eraseEverything` call from `SOURCE_SYNC`, delete `sync/bookmarks/syncId`, `sync/bookmarks/lastSync`, and `sync/bookmarks/wipeServer`. This is what clients will call when they receive a `wipeEngine` command.
* In `BookmarksEngine#ensureCurrentSyncID`, check if `sync/bookmarks/wipeServer = 1`.
* If so, send the `wipeEngine` command to other clients, wipe the server, assign a new sync ID, reset `sync/bookmarks/wipeServer = 0`, and upload a new `meta/global` with the assigned sync ID. Order matters here. If we're interrupted before we can assign a new sync ID, that's fine. On the next sync, we'll see that `sync/bookmarks/wipeServer = 1`, and start over again. Once we've assigned a new sync ID, we know we've told other clients to erase their bookmarks, so, if we're interrupted while uploading *our* restored bookmarks, the only bookmarks that should be on the server before the next sync (if any) are ones the user added on other clients *after* the wipe. Let's say we're client A with sync ID "abc123", and we're interrupted as we're uploading restored bookmarks, and before we can upload `meta/global`. If client B then syncs, wipes itself, and assigns sync ID "xyz567", A will take "xyz567" on the next sync, merge in B's new bookmarks, and finish uploading its restored ones.
* Otherwise, check if we have a sync ID.
* If not, take the server's sync ID without resetting sync statuses. There are two cases here: either we've automatically restored, in which case we have `UNKNOWN` bookmarks that we need to merge, or we're syncing for the first time with all `NEW` bookmarks.
* If we do have a sync ID, check if it matches. If so, great!
* If we have a sync ID, and it doesn't match, then we were probably node reassigned. Reset everything to `syncStatus = UNKNOWN` and reconcile, preferring deletions.

It's a schema error to have `NORMAL` bookmarks without a sync ID, which we can enforce through a trigger on `moz_bookmarks`: something like `SELECT RAISE(ABORT, "Can't mark item as syncing without a sync ID") WHERE NOT EXISTS(SELECT 1 FROM moz_meta WHERE key = "sync/bookmarks/syncId")`.

How does that sound to you, Thom?
Flags: needinfo?(tchiovoloni)
Having given it some thought, this sounds a lot better. Not only does this avoid a proliferation of sync_status variations, but this means the tombstone patch (bug 1343013) can only concern itself with whether or not a record has sync_status = UNKNOWN, and doesn't need to care about bookmark restore/reset/etc directly, reducing the complexity.

Overall, this will result in very predictable behavior, makes fixing issues straightforward, and reduces the amount of special cases in that code. It also means that patch doesn't need to directly depend on this, although we probably still want to land this first to prevent bad behavior.
Flags: needinfo?(tchiovoloni)
Comment on attachment 8951037 [details]
Bug 1199077 - Add a key-value metadata table to Places.

https://reviewboard.mozilla.org/r/220304/#review230524

I'm going to do a cleanup pass tomorrow before requesting review, but I'm pretty happy with how this is shaping up. Thanks again for pushing me to rethink the proliferation of sync statuses, Thom. :-)

::: toolkit/components/places/PlacesSyncUtils.jsm:2586
(Diff revision 5)
> +    WHERE anno_attribute_id = (SELECT id FROM moz_anno_attributes
> +                               WHERE name = :orphanAnno)`,
> +    { orphanAnno: BookmarkSyncUtils.SYNC_PARENT_ANNO });
> +
> +  // Drop stale tombstones.
> +  await db.execute("DELETE FROM moz_bookmarks_deleted");

I think this is the only part we'll need to change for your tombstone patch.
(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #23)
> It's a schema error to have `NORMAL` bookmarks without a sync ID, which we
> can enforce through a trigger on `moz_bookmarks`: something like `SELECT
> RAISE(ABORT, "Can't mark item as syncing without a sync ID") WHERE NOT
> EXISTS(SELECT 1 FROM moz_meta WHERE key = "sync/bookmarks/syncId")`.

This is going to make testing painful, and we'll also want a matching trigger that resets the status if we remove "sync/bookmarks/syncId"...except whether we should set the status to NEW or UNKNOWN depends on the source, which the trigger doesn't know. So let's not do this. :-P
OK, I think this is ready. I split out the Autofill and Activity Stream changes into separate commits to make them easier to backport. IIRC, Autofill doesn't jump trains yet, but Activity Stream will need a PR once this lands.

I'm happy to move the asyncification of `syncID` and `lastSync`, change source spliting, and key-value table into their own bugs, but I thought it would be helpful to keep them together so it's easier to see how everything fits in.
Summary: Sync doesn't recover after corruption and replacement of places.sqlite → Store the sync ID and last sync time for bookmarks and history in Places
Comment on attachment 8952973 [details]
Bug 1199077 - Sync changes to store the bookmarks sync ID and last sync time in Places.

https://reviewboard.mozilla.org/r/222230/#review230850

::: services/sync/modules/engines/bookmarks.js:324
(Diff revision 4)
>    _defaultSort: "index",
>  
>    syncPriority: 4,
>    allowSkippedRecord: false,
>  
> +  async getSyncID() {

We're going to need to migrate existing values from prefs, too, and write to prefs for a couple of releases...otherwise, everyone will do a full sync on upgrade, and again if they downgrade.
Comment on attachment 8952973 [details]
Bug 1199077 - Sync changes to store the bookmarks sync ID and last sync time in Places.

https://reviewboard.mozilla.org/r/222230/#review230852

::: services/sync/modules/engines/bookmarks.js:362
(Diff revision 4)
> +  setLastSync(lastSync) {
> +    return PlacesSyncUtils.bookmarks.setLastSync(lastSync);
> +  },
> +
> +  resetLastSync() {
> +    this.lastSyncLocal = 0;

This should probably call `await this.setLastSync(0)` for completeness, even though we always call `resetLastSync` from `_resetClient`, which calls `PSU.b.reset()`, which takes care of clearing out last sync for us. Ditto for history.
Blocks: 1443021
Comment on attachment 8955818 [details]
Bug 1199077 - Add Places APIs to store the bookmarks sync ID and last sync time.

https://reviewboard.mozilla.org/r/224854/#review230866

Just checking out this patch series, so here's a couple of drive-by comments :)

::: toolkit/components/places/PlacesSyncUtils.jsm:360
(Diff revision 2)
> +  async ensureCurrentSyncId(newSyncId) {
> +    if (!newSyncId || typeof newSyncId != "string") {
> +      throw new TypeError("Invalid new bookmarks sync ID");
> +    }
> +    await PlacesUtils.withConnectionWrapper(
> +      "BookmarkSyncUtils: ensureCurrentSyncId",

ISTM that some logging in this function might be useful?

::: toolkit/components/places/PlacesSyncUtils.jsm:2450
(Diff revision 2)
>        observer[notification](...args);
>      } catch (ex) {}
>    }
>  }
> +
> +async function getCollectionSyncId(db, key) {

The signature of this seems a little odd - it appears to allow you to specify *any* key. Contrast with setBookmarksSyncId which doesn't take the key as a param.

If you agree with this (and I've not much context here, so I'll accept if you don't ;), 2 possible ideas:

1) this function could be, say `getPlacesMetadataAsString` and `getCollectionLastSync` could become, say, `getPlacesMetadataAsTimestamp` and both continue to accept arbitrary keys.

2) It becomes the same as `setBookmarksSyncId` (ie, doesn't have the key passed in.)
Comment on attachment 8955819 [details]
Bug 1199077 - Add Places APIs to store the history sync ID and last sync time.

https://reviewboard.mozilla.org/r/224856/#review230870

LGTM - I'm not too bothered by how the API is exposed, so you can feel free to ignore my comment if you think it's better how it is.

::: toolkit/components/places/PlacesSyncUtils.jsm:2594
(Diff revision 2)
>      VALUES (:key, :lastSync)`,
>      { key, lastSync });
>  }
>  
> +// Sets the history sync ID and clears the last sync time.
> +async function setHistorySyncId(db, newSyncId) {

This reinforces the comment I made on the other patch - it appears we have a duplicate function here that differs only by the key name for the "set", but the "get" remains generic.
Attachment #8955819 - Flags: review?(markh) → review+
Comment on attachment 8951038 [details]
Bug 1199077 - Sync changes to store the history sync ID and last sync time in Places.

https://reviewboard.mozilla.org/r/220306/#review230872
Attachment #8951038 - Flags: review?(markh) → review+
Nice work Kit!
Comment on attachment 8951832 [details]
Bug 1199077 - Convert `lastSync` and `syncID` into async accessors.

https://reviewboard.mozilla.org/r/221126/#review230996

The patch is looking good and mostly straightforward, I left a few comments. Thanks for taking this Kit!

::: services/sync/modules/engines.js:1032
(Diff revision 6)
>        let error = new String("New data: " + [engineData.version, this.version]);
>        error.failureCode = VERSION_OUT_OF_DATE;
>        throw error;
> -    } else if (engineData.syncID != this.syncID) {
> +    } else {
>        // Changes to syncID mean we'll need to upload everything
> -      this._log.debug("Engine syncIDs: " + [engineData.syncID, this.syncID]);
> +      let assignedSyncID = await this.ensureCurrentSyncID(engineData.syncID);

IIUC, ensureCurrentSyncID doesn't mutate the 1st arg and returns it, are you sure this is what you want?

::: services/sync/modules/engines.js:1864
(Diff revision 6)
>      if (response.status != 200 && response.status != 404) {
>        throw response;
>      }
> +  },
> +
> +  async wipeServer() {

I'm a bit confused now on the difference between wipeServer, wipeClient, \_resetClient and \_resetServer, do you think we could clear that up, maybe with a different name or by reorganizing these methods?
Feel free to ignore that comment since the problem has been there before your patch.
Attachment #8951832 - Flags: review?(eoger)
Oh and side note from IRC: I think we can remove lastSyncLocal from engines.js
Comment on attachment 8951832 [details]
Bug 1199077 - Convert `lastSync` and `syncID` into async accessors.

https://reviewboard.mozilla.org/r/221126/#review230996

> IIUC, ensureCurrentSyncID doesn't mutate the 1st arg and returns it, are you sure this is what you want?

It's a bit convoluted, but yes. In most cases, `ensureCurrentSyncID` should just return `newSyncID`. The exception is bookmark restore, where we ignore the ID we're given, wipe the server and other clients, assign a new ID, and return that instead. Check out part 7.

I considered factoring out `shouldWipeRemote` into a separate method that the bookmarks engine could override and send the `wipeEngine` command, but that felt clunkier than having `ensureCurrentSyncID` take care of everything. Updated the comment to make this a bit clearer; does that look good to you?

> I'm a bit confused now on the difference between wipeServer, wipeClient, \_resetClient and \_resetServer, do you think we could clear that up, maybe with a different name or by reorganizing these methods?
> Feel free to ignore that comment since the problem has been there before your patch.

Me too. :-/ I renamed `_resetServer` to `_deleteServerCollection`, and added some comments.

The gist of it is: `resetClient` removes Sync metadata, but keeps actual user data, and doesn't touch the server, `wipeClient` removes Sync metadata and user data, and also doesn't touch the server, `_deleteServerCollection` removes user data from the server, but doesn't touch local Sync metadata or local user data, and `wipeServer` removes user data from the server and local metadata, but doesn't touch local user data.
Comment on attachment 8955818 [details]
Bug 1199077 - Add Places APIs to store the bookmarks sync ID and last sync time.

https://reviewboard.mozilla.org/r/224854/#review230866

> ISTM that some logging in this function might be useful?

Good call! Added.

> The signature of this seems a little odd - it appears to allow you to specify *any* key. Contrast with setBookmarksSyncId which doesn't take the key as a param.
> 
> If you agree with this (and I've not much context here, so I'll accept if you don't ;), 2 possible ideas:
> 
> 1) this function could be, say `getPlacesMetadataAsString` and `getCollectionLastSync` could become, say, `getPlacesMetadataAsTimestamp` and both continue to accept arbitrary keys.
> 
> 2) It becomes the same as `setBookmarksSyncId` (ie, doesn't have the key passed in.)

Also a good call. :-) I went with your first suggestion, and added `{get, set}PlacesMetadataAsString` and `{get, set}PlacesMetadataAsTimestamp`, so we can use (and cache) the same statements for both, and also tighten up the other metadata functions a bit.
Comment on attachment 8955818 [details]
Bug 1199077 - Add Places APIs to store the bookmarks sync ID and last sync time.

https://reviewboard.mozilla.org/r/224854/#review231012

Looks good to me!

::: toolkit/components/places/PlacesSyncUtils.jsm:370
(Diff revision 2)
> +          { syncIdKey: BookmarkSyncUtils.SYNC_ID_META_KEY });
> +
> +        // If we don't have a sync ID, take the server's without resetting
> +        // sync statuses.
> +        if (!existingSyncIdRows.length) {
> +          await db.executeTransaction(function() {

nit: Any reason not to use an arrow function for a one liner like this?

::: toolkit/components/places/tests/unit/test_sync_utils.js:2211
(Diff revision 2)
>  
>      let existingFields = await PlacesTestUtils.fetchBookmarkSyncFields(
>        PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid,
>        PlacesUtils.bookmarks.unfiledGuid, PlacesUtils.bookmarks.mobileGuid,
>        "NnvGl3CRA4hC", "APzP8MupzA8l");
> -    deepEqual(existingFields.map(field => field.syncStatus), [
> +    ok(existingFields.map(field =>

I think this is always true. I think you want `every` and not `map`.
Attachment #8955818 - Flags: review?(tchiovoloni) → review+
Comment on attachment 8952972 [details]
Bug 1199077 - Activity Stream changes for new bookmark change sources.

https://reviewboard.mozilla.org/r/222228/#review231292

Activity Stream changes look good, thanks!
Attachment #8952972 - Flags: review?(usarracini) → review+
Comment on attachment 8951832 [details]
Bug 1199077 - Convert `lastSync` and `syncID` into async accessors.

https://reviewboard.mozilla.org/r/221126/#review231298

Looks good thanks!
Attachment #8951832 - Flags: review?(eoger) → review+
Comment on attachment 8955817 [details]
Bug 1199077 - Autofill changes for async `lastSync` and `syncID` accessors.

https://reviewboard.mozilla.org/r/224852/#review231300

Mechanical patch, r+
Attachment #8955817 - Flags: review?(eoger) → review+
Blocks: 1426556
Comment on attachment 8952973 [details]
Bug 1199077 - Sync changes to store the bookmarks sync ID and last sync time in Places.

https://reviewboard.mozilla.org/r/222230/#review231020

This looks fine, modulo nits.

::: services/sync/modules/engines/bookmarks.js:326
(Diff revision 7)
>    syncPriority: 4,
>    allowSkippedRecord: false,
>  
> +  _migratedSyncMetadata: false,
> +  async _migrateSyncMetadata({ migrateLastSync = true } = {}) {
> +    if (this._migratedSyncMetadata) {

Nit: Logging might be useful in this function

::: services/sync/modules/engines/bookmarks.js:347
(Diff revision 7)
> +      }
> +    }
> +    this._migratedSyncMetadata = true;
> +  },
> +
> +  async ensureCurrentSyncID(newSyncID) {

Ditto (loggin)

::: services/sync/modules/engines/bookmarks.js:1294
(Diff revision 7)
>  
>    set changedIDs(obj) {
>      throw new Error("Don't set initial changed bookmark IDs");
>    },
>  
> -  async observe(subject, topic, data) {
> +  observe(subject, topic, data) {

I have a little hesitation with removing the async observer (we just have to remember to re-add it if we need it, and not create another one for this class), but it doesnt matter either way.
Attachment #8952973 - Flags: review?(tchiovoloni) → review+
Thanks for the reviews, everyone! :-)

More musings: I think this, along with bug 1426556 and bug 1443245, should handle copying `places.sqlite` between profiles. Ed's proposal for profile copy fingerprinting should tidy up the corner cases; as Richard suggests, the fingerprint can also live in Places.

* Copying between two profiles that aren't signed in to Sync: easy. No sync IDs in `moz_meta` or tombstones in `moz_bookmarks_deleted`, and all bookmarks remain in NEW.

* Copying between profiles signed in to the same account. This is OK, too. The mirror might be out of sync with what's in Places, but the tree walk should produce a sensible tree. Alternatively, we could store the last sync time in *both* the mirror and Places, and either reset the mirror, or flag everything as `needsMerge` if we see that the timestamps don't match.

* Copying from a profile signed in to one account to one that's not signed in: we'll write unnecessary tombstones for NORMAL bookmarks, but that's about it. If you create a new Sync account on the second profile, we'll reset the sync ID, and change all statuses to NEW. If you sign in to an existing Sync account that's different from the first profile, the sync IDs won't match, so we'll take the new sync ID, change all statuses to UNKNOWN, and do a full reconcile. (In practice, this is going to be the same as the first case...the account likely has a completely different tree, so the "prefer deletions" behavior won't apply, and we'll dedupe correctly with bug 1443245). If you sign in to the same Sync account as on the first profile, and the sync IDs match, then there's no problem. `places.sqlite` might be a bit out of date, but the statuses are still correct.

* Copying between profiles signed in to different accounts. The sync IDs won't match, so we'll reset the mirror, change all statuses to UNKNOWN, and start over.

* Copying from a profile not signed in to one that's already signed in to a different account. All bookmarks in Places are NEW, so we'll take the sync ID, reset the mirror, do a full sync to download the bookmarks from that account, merge them in to Places, then upload remaining NEW ones.

Does that sound right to y'all?
Comment on attachment 8951037 [details]
Bug 1199077 - Add a key-value metadata table to Places.

https://reviewboard.mozilla.org/r/220304/#review231550

::: toolkit/components/places/Database.cpp:1227
(Diff revision 9)
> -      // Firefox 60 uses schema version 43.
> +      if (currentSchemaVersion < 44) {
> +        rv = MigrateV44Up();
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }
> +
> +      // Firefox 61 uses schema version 44.

you should also keep the previous comment about Firefox 60 and version 43.
Attachment #8951037 - Flags: review?(mak77) → review+
is there a reason to not abstract away an API to manage moz_meta in PlacesUtils, rather than spreading queries touching it all around?
In the future we may want to add more stuff there, and if every consumer implements its own it will be a mess.
It could also come with a memory cache, since the values are just a few.
Off-hand, looks like the whole moz_meta thing should be a separate bug that blocks this one.
Comment on attachment 8952971 [details]
Bug 1199077 - Split change sources for automatic and manual bookmark restores.

https://reviewboard.mozilla.org/r/222226/#review231552

::: browser/components/migration/MigrationUtils.jsm:380
(Diff revision 6)
>          // Import the default bookmarks. We ignore whether or not we succeed.
>          await BookmarkHTMLUtils.importFromURL(
> -          "chrome://browser/locale/bookmarks.html", true).catch(r => r);
> +          "chrome://browser/locale/bookmarks.html", {
> +            replace: true,
> +            source: PlacesUtils.bookmarks.SOURCES.RESTORE_ON_STARTUP,
> +          }).catch(r => r);

Not your fault, but I wonder why this is not a Cu.reportError

::: toolkit/components/places/BookmarkHTMLUtils.jsm:162
(Diff revision 6)
>     *
>     * @param aFilePath
>     *        OS.File path string of the "bookmarks.html" file to be loaded.
>     * @param aInitialImport
>     *        Whether this is the initial import executed on a new profile.
>     *

Could you please fix the javadoc to point out the second param is an options object with such supported properties?
I don't care about retaining the current aInitialImport/aSource naming (nor the xpcom a prefix convention in js)

::: toolkit/components/places/BookmarkJSONUtils.jsm:46
(Diff revision 6)
>     *
>     * @param aSpec
>     *        url of the bookmark data.
>     * @param aReplace
>     *        Boolean if true, replace existing bookmarks, else merge.
>     *

ditto

::: toolkit/components/places/tests/bookmarks/test_1129529.js:62
(Diff revision 6)
>      }]
>    };
>  
>    let contentType = "application/json";
>    let uri = "data:" + contentType + "," + JSON.stringify(aData);
> -  await BookmarkJSONUtils.importFromURL(uri, false);
> +  await BookmarkJSONUtils.importFromURL(uri, { replace: false });

can omit the options

::: toolkit/components/places/tests/sync/test_sync_utils.js:2028
(Diff revision 6)
>      ), "Unsynced bookmark statuses should match");
>    }
>  
>    info("Import new bookmarks from HTML");
>    let { path } = do_get_file("./sync_utils_bookmarks.html");
> -  await BookmarkHTMLUtils.importFromFile(path, false);
> +  await BookmarkHTMLUtils.importFromFile(path, { replace: false });

can omit

::: toolkit/components/places/tests/sync/test_sync_utils.js:2090
(Diff revision 6)
>      syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
>    });
>  
>    info("Import new bookmarks from JSON");
>    let { path } = do_get_file("./sync_utils_bookmarks.json");
> -  await BookmarkJSONUtils.importFromFile(path, false);
> +  await BookmarkJSONUtils.importFromFile(path, { replace: false });

can omit

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:214
(Diff revision 6)
>    let file = await promiseFile("bookmarks-test_restoreNotification.html");
>    await addBookmarks();
>    await BookmarkHTMLUtils.exportToFile(file);
>    await PlacesUtils.bookmarks.eraseEverything();
>    try {
> -    BookmarkHTMLUtils.importFromFile(file, false)
> +    BookmarkHTMLUtils.importFromFile(file, { replace: false })

ditto

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:234
(Diff revision 6)
>    let expectPromises = registerObservers(true);
>  
>    info("HTML restore: empty file should succeed");
>    let file = await promiseFile("bookmarks-test_restoreNotification.init.html");
>    try {
> -    BookmarkHTMLUtils.importFromFile(file, false)
> +    BookmarkHTMLUtils.importFromFile(file, { replace: false })

ditto

::: toolkit/components/places/tests/unit/test_bookmarks_restore_notification.js:254
(Diff revision 6)
>    let expectPromises = registerObservers(false);
>  
>    info("HTML restore: nonexistent file should fail");
>    let file = Services.dirsvc.get("ProfD", Ci.nsIFile);
>    file.append("this file doesn't exist because nobody created it 2");
> -  Assert.rejects(BookmarkHTMLUtils.importFromFile(file.path, false),
> +  Assert.rejects(BookmarkHTMLUtils.importFromFile(file.path, { replace: false }),

ditto
Attachment #8952971 - Flags: review?(mak77) → review+
See comment 86
Flags: needinfo?(kit)
(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #84)
> * Copying between profiles signed in to the same account. This is OK, too.
> The mirror might be out of sync with what's in Places, but the tree walk
> should produce a sensible tree. Alternatively, we could store the last sync
> time in *both* the mirror and Places, and either reset the mirror, or flag
> everything as `needsMerge` if we see that the timestamps don't match.

That makes sense to me (although we should do whatever the least work and complexity is for us - which I'm guessing means "throw away the mirror".

All the other points sound like a fine state to end up in too.

> Does that sound right to y'all?

It does - thanks for thinking this through!
Depends on: 1426554
Attachment #8951037 - Attachment is obsolete: true
(In reply to Marco Bonardo [::mak] from comment #86)
> is there a reason to not abstract away an API to manage moz_meta in
> PlacesUtils, rather than spreading queries touching it all around?
> In the future we may want to add more stuff there, and if every consumer
> implements its own it will be a mess.
> It could also come with a memory cache, since the values are just a few.
> Off-hand, looks like the whole moz_meta thing should be a separate bug that
> blocks this one.

Done and done. :-)
Flags: needinfo?(kit)
Comment on attachment 8955818 [details]
Bug 1199077 - Add Places APIs to store the bookmarks sync ID and last sync time.

https://reviewboard.mozilla.org/r/224854/#review232340

::: toolkit/components/places/PlacesSyncUtils.jsm:409
(Diff revision 5)
> +   *
> +   * @param lastSyncSeconds
> +   *        The collection last sync time, in seconds, as a number or string.
> +   */
> +  async setLastSync(lastSyncSeconds) {
> +    let lastSync = Math.floor(lastSyncSeconds * 1000);

Is there a reason to convert to milliseconds, and then convert back to seconds when getting the data back? Why not just storing and retrieving seconds?
Attachment #8955818 - Flags: review?(mak77) → review+
Comment on attachment 8955819 [details]
Bug 1199077 - Add Places APIs to store the history sync ID and last sync time.

https://reviewboard.mozilla.org/r/224856/#review232348
Attachment #8955819 - Flags: review?(mak77) → review+
Comment on attachment 8955818 [details]
Bug 1199077 - Add Places APIs to store the bookmarks sync ID and last sync time.

https://reviewboard.mozilla.org/r/224854/#review232340

> Is there a reason to convert to milliseconds, and then convert back to seconds when getting the data back? Why not just storing and retrieving seconds?

Mostly consistency with the mirror, which stores all modified times in milliseconds to match JS dates. (Although it stores dates added in microseconds to match `moz_bookmarks`, so ¯\_(ツ)_/¯).
Attachment #8951832 - Attachment is obsolete: true
Attachment #8955817 - Attachment is obsolete: true
Attachment #8952971 - Attachment is obsolete: true
Attachment #8952972 - Attachment is obsolete: true
Attachment #8955818 - Attachment is obsolete: true
Attachment #8952973 - Attachment is obsolete: true
Attachment #8955819 - Attachment is obsolete: true
Attachment #8955819 - Attachment is obsolete: false
Attachment #8952973 - Attachment is obsolete: false
Attachment #8955818 - Attachment is obsolete: false
Attachment #8952972 - Attachment is obsolete: false
Attachment #8952971 - Attachment is obsolete: false
Attachment #8955817 - Attachment is obsolete: false
Attachment #8951832 - Attachment is obsolete: false
Depends on: 1441156
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad272cb9392
Convert `lastSync` and `syncID` into async accessors. r=eoger
https://hg.mozilla.org/integration/mozilla-inbound/rev/e986d8ebc640
Autofill changes for async `lastSync` and `syncID` accessors. r=eoger
https://hg.mozilla.org/integration/mozilla-inbound/rev/5018319d0db6
Split change sources for automatic and manual bookmark restores. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ef5dc8063f
Activity Stream changes for new bookmark change sources. r=ursula
https://hg.mozilla.org/integration/mozilla-inbound/rev/e292398098e1
Add Places APIs to store the bookmarks sync ID and last sync time. r=mak,tcsc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f96f2c929f09
Sync changes to store the bookmarks sync ID and last sync time in Places. r=tcsc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a584dd75961
Add Places APIs to store the history sync ID and last sync time. r=mak,markh
https://hg.mozilla.org/integration/mozilla-inbound/rev/125203e8a2d5
Sync changes to store the history sync ID and last sync time in Places. r=markh
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/f1e62440378a112b8fbd872c9a2547475147a23d
Backport Bug 1199077 - Activity Stream changes for new bookmark change sources
Depends on: 1448929
See Also: → 1597358
You need to log in before you can comment on or make changes to this bug.