1607341 - browser.bookmarks.onChanged event has datedAdded 1000x too high

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[cal]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0

Steps to reproduce:

Bookmark a web page on Firefox mobile. Then wait (or force) synchronisation to desktop browser via Firefox Sync.

Actual results:

The "dateAdded" field in the event (supposed to be milliseconds since epoch) is 1000 times too high.

Expected results:

The "dateAdded" field should be in the correct number of milliseconds since epoch.

Comment 1

[cal]

I think this problem only applies to bookmarks synced from mobile Firefox.

If you then look up the bookmark id using await browser.bookmarks.get(browserId) the dateAdded field is correct.

Comment 2

[cal]

Attachment: Log message capturing incorrect dateAdded on event

Comment 3

[cal]

Attachment: Log message capturing correct dateAdded returned from browser.bookmarks.get

Comment 4

[cal]

No, doesn't seem to be related to mobile firefox - applies when syncing a bookmark from desktop -> desktop too

Comment 5

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

I wonder if this is a 2020 date bug related issue?

Comment 6

[Mark Banner (:standard8)]

Moving to Firefox Sync, as I suspect this is more a client side rather than server side. iirc, places stores bookmarks in microseconds, whereas they are generally handled in milliseconds, so I suspect there's a conversion wrong somewhere.

Comment 7

[Lina Butler (ex-Mozilla)]

I think this is us passing the wrong argument to PlacesBookmarkAddition here—b.dateAdded is in microseconds here, but PlacesBookmarkAddition expects dateAdded to be in milliseconds.

(As part of this bug, I think we should also rename those temp table columns to dateAddedMicroseconds, like we do for itemsToApply, because this isn't the first time we've run into unit confusion 😅)

Comment 8

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1607341 - Ensure the bookmarks mirror passes millisecond timestamps for `bookmark-added`. r?LougeniaBailey!

The legacy nsINavBookmarkObserver notifications use microsecond
timestamps that are rounded to the nearest millisecond, while the
new bookmark-added Places event uses milliseconds directly. This
commit fixes that, and also changes the store to use the given
localTimeSeconds as the current time, instead of querying the
current system time. This, in turn, lets tests set deterministic
last modified times, which is useful for comparing lastModified
timestamps.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb57df7ef0c2
Ensure the bookmarks mirror passes millisecond timestamps for `bookmark-added`. r=LougeniaBailey

Comment 10

[Cristina Coroiu [:ccoroiu]]

Backed out changeset cb57df7ef0c2 (bug 1607341) for build bustage

Backout: https://hg.mozilla.org/integration/autoland/rev/bb276699d640f79501e5a8185c52213e72ab1a90

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cb57df7ef0c2b28b311cbddef0900c7a1029d9e8

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=288787949&repo=autoland&lineNumber=30202

Comment 11

[Lina Butler (ex-Mozilla)]

Oops, sorry about that! 😬 The last-minute "oh, surely this one-line change will be fine" was not, in fact, fine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cbc7f7d88bbbbd3971be5d50e621220eda2960a

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81f6e3c67aa8
Ensure the bookmarks mirror passes millisecond timestamps for `bookmark-added`. r=LougeniaBailey

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/81f6e3c67aa8