1927543 - Don't save history metadata for URLs that aren't in Places

Status: ASSIGNED

(Application Services :: Places, defect, P1)

Description

[Lina Butler [:lina]]

When a caller wants to add some history metadata for a URL that isn't in moz_places, apply_metadata_observation() will automatically insert a row for that URL.

Rebeca explained the problem with doing that in this Phabricator diff: Fenix's address bar autocomplete queries history metadata, too; so if a page is deleted from history, but still has metadata—or writes it after the fact—that page will still be suggested in the address bar. It might also be a privacy concern for the user: they thought they'd deleted the page from history, but it's still hanging around in the database.

It looks like the metadata observation API has done this since it was implemented. Mark / Christian, do you remember why? Was it to avoid a race where Fenix might try to record metadata for a page before recording the initial visit for it? 🤔

I wonder if we can just change this logic to bail if the URL doesn't exist in moz_places, or if that'll have other knock-on effects, like occasionally dropping metadata observations for pages that the user is visiting for the first time.

Comment 1

[Christian Sadilek [:csadilek]]

It looks like the metadata observation API has done this since it was implemented. Mark / Christian, do you remember why? Was it to avoid a race where Fenix might try to record metadata for a page before recording the initial visit for it?

Yes, it was to make sure that a places entry exists when the first metadata record is created. These two writes are otherwise not coordinated.

I wonder if we can just change this logic to bail if the URL doesn't exist in moz_places, or if that'll have other knock-on effects, like occasionally dropping metadata observations for pages that the user is visiting for the first time.

I think this can work if we also check for the observation type. We can still proactively create the places record when writing a DocumentTypeObservation, which should only happen when the metadata record is newly created, e.g., through a fresh visit, but bail for all other type of observations.

Comment 2

[BMO Github Automation]

Attachment: [mozilla/application-services] Bug 1927543 - Add an `if_page_missing` option for history metadata. (#6443)

Comment 3

[Lina Butler [:lina]]

Thanks Christian! That sounds like a good approach to me—I made it a bit more generic, with an explicit option to control what to do if the page doesn't exist in moz_places, and defaulting to ignoring the observation. We can change Fenix to pass the option to insert the page before recording the observation here.