1392271 - dateAdded/lastModified requirements when using PlacesUtils.bookmarks.insert are confusing

Status: NEW

(Toolkit :: Places, enhancement, P3)

Description

[Mark Banner (:standard8)]

Bug 676563 modified how `PlacesUtils.bookmarks.insert()` works and allowed it to take lastModified data.

However, we're slightly confused by how it is working:

- If dateAdded is in the future, lastModified is set to now.
- It accepts lastModified times that are now, or in the future.
  - But, if it is in the past, then lastModified must be later that dateAdded.

This means that we can get cases where lastModified is earlier than dateAdded. Which could be confusing and unexpected.

Thom/Kit, please can you clarify why Sync needs this case? We feel we should be always be checking that lastModified is later than dateAdded.

Comment 1

[Marco Bonardo [:mak]]

and also, why can't we fix future dates to "now"? would that break Sync?

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

(In reply to Marco Bonardo [::mak] from comment #1)
> and also, why can't we fix future dates to "now"? would that break Sync?

If it's ever in the future, we know the current local clock is wrong. Since we prefer earlier dates, it would cause the other devices to be inaccurate. So no, it wouldn't break sync, it would just cause the dateAdded to be wrong on other devices, even though a correct device added it.

(In reply to Mark Banner (:standard8) from comment #0)
> Bug 676563 modified how `PlacesUtils.bookmarks.insert()` works and allowed
> it to take lastModified data.
> 
> However, we're slightly confused by how it is working:
> 
> - If dateAdded is in the future, lastModified is set to now.

We don't want lastModified to be in the future since that could cause it to be preferred for conflict resolution even if there's a newer record from the server.

> - It accepts lastModified times that are now, or in the future.

This sounds like a bug/accident (I think I had thought you could provide it already, and didn't want to change this -- the intent was only to add support for providing dateAdded, which, as mentioned, can be in the future), I don't think sync ever provides lastModified, so this probably can change (leaving kit's needinfo so he can correct me on this if necessary).

>   - But, if it is in the past, then lastModified must be later that
> dateAdded.
> 
> This means that we can get cases where lastModified is earlier than
> dateAdded. Which could be confusing and unexpected.

It's confusing and unexpected, but yes, it's important for sync. It shouldn't be possible for clients with correct clocks, for what it's worth.

Comment 3

[Lina Butler [:lina]]

I think Thom covered everything. The crux of the issue is that the date added might come from another device, but the last modified time is always local. https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/toolkit/components/places/Bookmarks.jsm#556-558 couples the two, but that doesn't always hold when the bookmark is synced. As Thom said, if our modified time is in the future, we'll assume our local changes are newer, even if they're chronologically older.

That said, changing the local system clock raises the same issue. In the interest of consistency, we could fix the last modified time so that it's always >= date added, even if it's in the future...and accept that we might pick the wrong side if the same bookmark is changed on multiple devices.

> This sounds like a bug/accident (I think I had thought you could provide it
> already, and didn't want to change this -- the intent was only to add
> support for providing dateAdded, which, as mentioned, can be in the future),
> I don't think sync ever provides lastModified, so this probably can change

Correct, Sync never updates `lastModified`. If we keep `dateAdded` and `lastModified` separate, `lastModified` won't need to be in the future. If we decide that `lastModified` should never be older than `dateAdded`, we'll want to allow `lastModified` to be in the future in case `dateAdded` is, too.

Comment 4

[Lina Butler [:lina]]

I've been thinking about this some more, and it might make sense to change how Places handles dates, especially when it comes to Sync:

* Sync treats "date added" and "last modified" independently. "Date added" is a piece of metadata that's synced to all your devices, and always ratchets backward: we'll always use the oldest date added that we see for a bookmark. "Last modified" is used to resolve merge conflicts in case a bookmark is changed locally and remotely.

* Places is a bit too clever in validating `lastModified` and `dateAdded`. On the one hand, it doesn't make sense to have added or last modified dates that are newer than the local clock. A time like "in 3 hours" may be technically correct if the local clock is wrong, but isn't meaningful. The concept of time becomes fuzzy when multiple clients are syncing, sometimes with clocks that are wildly off. (We've seen Sync telemetry pings with times from 1601 and 2029!) I wonder if Places could automatically clamp these dates, instead of throwing and moving the normalization to Sync.

* Unrelated to bookmarks: `History.insertMany` throws if it sees a future visit date. Sync kludges around this by clamping dates between the oldest sensible date and current time, mainly so that we don't end up with dates from 1970 if a client's clock is buggy. It looks like we still have some "Visit date can't be in the future" errors on 58, even after bug 1405566 landed, so I wonder if something else is going on. 

Here's what I think we could do:

* If the date added for an incoming bookmark is ahead of the local clock, clamp it to now. (Don't throw; just ignore the given date and use the current local date instead). Flag the bookmark for weak reupload, since the current local time is older than the incoming bookmark's date added.

* If date added is <= the local clock, but newer than last modified, then set `lastModified = dateAdded`. Again, don't throw. This does mean we might pick the wrong side to resolve merge conflicts, but I expect value conflicts to be rare: how often do you change the title or URL of the same bookmark on two different devices? Structure conflicts are likely more common (for example, adding different bookmarks to the same folder is a "conflict," though one that's easy to resolve)...but I think our merger will handle this correctly, and we do something that's close to the right thing now.

Comment 5

[Marco Bonardo [:mak]]

In bug 1404631 I'm adding a "fixup" property for validators that can be used also for dates, it will initially be used only for insertTree, but we can then reuse it here.

I agree we can be less strict on dates. I think it made sense to start with a really strict API to identify the pain points, now it's time to move on with alleviating those.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #4)
> On the one hand, it doesn't make sense to have added or last modified dates
> that are newer than the local clock.

There is a possible problem with time skews and timers resolutions, so it's possible to have a time in the "immediate" future.

> * Unrelated to bookmarks: `History.insertMany` throws if it sees a future
> visit date.

Should we clamp in history as well?

> * If the date added for an incoming bookmark is ahead of the local clock,
> clamp it to now.

For the above problem, we'll have to consider an interval after which we think it's "future". Not sure if it should be 1 minute or 1 hour.

> Flag the bookmark for weak reupload, since the
> current local time is older than the incoming bookmark's date added.

What's a weak reupload?

> * If date added is <= the local clock, but newer than last modified, then
> set `lastModified = dateAdded`. Again, don't throw

SGTM

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

> * If the date added for an incoming bookmark is ahead of the local clock,
> clamp it to now.

I don't know if I think this is a good idea. It's worth noting that if we do this, and you have one device set to the past, we'll converge on that wrong answer for all your bookmarks (due to weak reupload).

If you have a device set into the future OTOH, it will only effect bookmarks it uploads, not all bookmarks it uploads or receives.

Comment 8

[Thom Chiovoloni [:tcsc] (ex-moco)]

> What's a weak reupload?

Missed this -- Weak reupload is a pseudo-stage of sync where it reuploads bookmarks it receives that are wrong in some way -- The only current example being due to the date added being newer than our local date added for the same bookmark. It's considered "weak" in that

1. It's stored in memory, and so if you shut down during this, we'll miss those uploads.
2. If we get a newer record from the server while we have a bookmark that is pending (only) weak reupload, we consider it unchanged for reconciliation -- e.g. we take the server record. This prevents a case where we change the date to newer, the user changes a title or something, and we choose our local date change over the server record.

We're considering eventually removing weak reupload, since now that all clients do upload dateAdded, it might not be as necessary, but it requires some thought as to whether or not the correctness of our ad-hoc consensus algorithm still depends on it.

Comment 9

[Stephanie Cunnane [:scunnane]]

This is still an issue, but needs someone to read through the comments to summarize how we'd like to proceed.
When a patch is attached to this bug, we'll need to ask for review from someone on the Services team.