1376232 - When updating date added the async Bookmarks.update API should also change the last modified date

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mark Banner (:standard8)]

In transitioning test_isvisited.js to the new async API, I came across an issue which highlighted a difference between the old and new APIs.

It turns out the old API would set the lastModified date to be the same as dateAdded when setItemDateAdded was called.

I think update() should do the same, but only when dateAdded is specified and not lastModified.

Comment 1

[Mark Banner (:standard8)]

And I meant test_lastModified.js rather than test_isvisited.js.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1376232 - When updating date added the async Bookmarks.update API should also change the last modified date.

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

Comment 3

[Mark Hammond [:markh] [:mhammond]]

I'm not quite sure this work as we need for sync (where for historical reasons, we "ratchet" date added backwards based on the dates from other devices. Thom, do you think this might cause problems?

Comment 4

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

Yes, that's correct. This would cause the lastModified date to be wrong (possibly very wrong) when sync changes an item's dateAdded property. Sync might do this a couple times before it converges on an "earliest sane" dateAdded value (this is the ratcheting markh is describing).

Comment 5

[Marco Bonardo [:mak]]

So, what's the solution? I see this as a possible issue, but at the same time I don't see a way out proposed.
The reason for the API to fix lastModified is that per API contract we should not have a lastModified older than dateAdded, it wouldn't make sense.

Can we proceed? Should we re-evaluate this? Is this an annoyance but not blocking us?

Comment 6

[Marco Bonardo [:mak]]

Also, this was already broken since the old bookmarks API always did it.

Comment 7

[Mark Banner (:standard8)]

Note: I'm pretty sure we will notify an onItemChanged event for the lastModified, not sure if that will help. We could/should add that into the test as well.

Comment 8

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

The only time this seems problematic is only when sync updates the dateAdded to be a value in the past, since this would cause lastModified to be often very wrong -- e.g. you may update a record locally, sync "finds out" that some other device has a much earlier dateAdded for it, and updates the local dateAdded to match[0]. If you use dateAdded for the lastModified value, this would result in a lastModified in the past, possibly very far in the past.

Anyway, I think setting lastModified to `Math.max(oldLastModified, newDateAdded)` (in the case that lastModified isn't provided) avoids this issue while maintaining the constraint that lastModified will never be older than dateAdded. IIRC we read the record out of the DB before the update so this should be possible?

> Also, this was already broken since the old bookmarks API always did it.

Sure, but AFAICT this is only an issue with sync's usage of the API (e.g. typically moving dateAdded into the past -- possibly far into the past), and it doesn't.

[0]: Note that this could be in the future (from the perspective of this device), if the device's local time is wrong. We limit it to a timestamp from the sync server, so it should never be "objectively" in the future. This seems irrelevant to the bug, but is possibly worth noting.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8881186 [details]
Bug 1376232 - When updating date added the async Bookmarks.update API should also change the last modified date.

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

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8881186 [details]
Bug 1376232 - When updating date added the async Bookmarks.update API should also change the last modified date.

I've updated the patch to set last modified to the max of the old last modified & date added.

Thom: can you check this looks like what you intended? Thanks.

Comment 11

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

Comment on attachment 8881186 [details]
Bug 1376232 - When updating date added the async Bookmarks.update API should also change the last modified date.

https://reviewboard.mozilla.org/r/152456/#review157776

::: toolkit/components/places/Bookmarks.jsm:544
(Diff revision 2)
>        }
>        const now = new Date();
> +      let lastModifiedDefault = now;
> +      // In the case where `dateAdded` is specified, but `lastModified` is not,
> +      // we only update `lastModified` if it is older than the new `dateAdded` -
> +      // this will keep Sync happy.

Well, to be fair, sync will continue to work fine in either case. This will keep it from setting the lastModified to something crazy like 3 years in the past just because you synced an old device that happens to be the "source" for this record, and similar cases.

Comment 12

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

Comment on attachment 8881186 [details]
Bug 1376232 - When updating date added the async Bookmarks.update API should also change the last modified date.

Yep, that's what I meant

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8881186 [details]
Bug 1376232 - When updating date added the async Bookmarks.update API should also change the last modified date.

https://reviewboard.mozilla.org/r/152456/#review157790

::: toolkit/components/places/tests/unit/test_lastModified.js:34
(Diff revision 2)
> -  var dateAdded = bs.getItemDateAdded(itemId);
> -  do_check_eq(dateAdded, bs.getItemLastModified(itemId));
> +
> +  // Check the bookmark from the database.
> +  bookmark = await PlacesUtils.bookmarks.fetch(guid);
> +
> +  let dateAdded = PlacesUtils.toPRTime(bookmark.dateAdded);
> +  do_check_date_eq(dateAdded, bookmark.lastModified);

nit: we should be using Assert.* methods in new code instead of legacy do_check_*

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8881186 [details]
Bug 1376232 - When updating date added the async Bookmarks.update API should also change the last modified date.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11de870775c1
When updating date added the async Bookmarks.update API should also change the last modified date. r=mak

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11de870775c1