Closed Bug 1426554 Opened 2 years ago Closed 2 years ago

Add a key-value metadata table to Places

Categories

(Toolkit :: Places, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

Attachments

(2 files)

Mak suggests this is feasible in bug 1305563, comment 170, and we talked a bit about it at the all-hands.

Sync could use this table to store the sync IDs for the bookmarks and history collections, which are currently stored in the `services.sync.bookmarks.syncID` and `services.sync.history.syncID` prefs. We could also store `services.sync.history.lastSync` here.

The goal is to avoid confusion caused by copying `places.sqlite` between profiles connected to different accounts (bug 1305563, comment 169). Since the sync IDs wouldn't match in that case, we would drop the mirror, reset all sync status flags, and start over as a first sync.

This would also be a (very small) reduction in the number of prefs we write.
See Also: → 1426556
Priority: -- → P3
See Also: → 1199077
Assignee: nobody → kit
Blocks: 1199077
Status: NEW → ASSIGNED
Comment on attachment 8957356 [details]
Bug 1426554 - Add a key-value metadata table to Places.

https://reviewboard.mozilla.org/r/226278/#review232326

::: toolkit/components/places/Database.h:22
(Diff revision 2)
>  #include "Shutdown.h"
>  #include "nsCategoryCache.h"
>  
>  // This is the schema version. Update it at any schema change and add a
>  // corresponding migrateVxx method below.
> -#define DATABASE_SCHEMA_VERSION 43
> +#define DATABASE_SCHEMA_VERSION 44

there will be a bit of bitrotting around migrations early in 61, since there are at least 3 patches around trying to use version 44. Just pay attention when landing.

::: toolkit/components/places/nsPlacesTables.h:231
(Diff revision 2)
> +// last sync time for history.
> +#define CREATE_MOZ_META NS_LITERAL_CSTRING( \
> +  "CREATE TABLE moz_meta (" \
> +    "  key TEXT PRIMARY KEY" \
> +    ", value NOT NULL" \
> +  ") WITHOUT ROWID" \

please respect the coding style of the other above definitions, with trailing commas

::: toolkit/components/places/tests/migration/test_current_from_v43.js:5
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +add_task(async function setup() {
> +  await setupPlacesDatabase("places_v43.sqlite");

it's not mandatory anymore to add a db for each version, so if you wish you can reuse the last existing one
Attachment #8957356 - Flags: review?(mak77) → review+
Comment on attachment 8957357 [details]
Bug 1426554 - Add a `PlacesUtils.metadata` API.

https://reviewboard.mozilla.org/r/226280/#review232328

The only big problem is the possible SQL injection, apart that it looks ok.

::: toolkit/components/places/PlacesUtils.jsm:1996
(Diff revision 3)
> +   * @return {*}
> +   *         The value associated with the key, or `null` if not set.
> +   */
> +  async get(key) {
> +    let db = await PlacesUtils.promiseDBConnection();
> +    return this.getWithConnection(db, key);

I'm a bit worried about mixing up the 2 connections here due to concurrency. Though, since we have a cache, it should be fine, since setting and fetching immediately will fetch from the cache. Maybe it's worth a comment above promiseDBConnection

::: toolkit/components/places/PlacesUtils.jsm:2020
(Diff revision 3)
> +   * @param {String...}
> +   *        One or more metadata keys to remove.
> +   */
> +  delete(...keys) {
> +    return PlacesUtils.withConnectionWrapper("PlacesUtils.metadata.delete",
> +      db => db.executeTransaction(() => this.deleteWithConnection(db, ...keys)));

The transaction doesn't seem necessary here? You are executing a single write, that automatically runs in an implicit transaction

::: toolkit/components/places/PlacesUtils.jsm:2026
(Diff revision 3)
> +  },
> +
> +  /**
> +   * Removes all stored metadata.
> +   */
> +  clear() {

This sounds a bit scary to expose, since different consumers shouldn't clear each other data... is it strictly necessary? tests can have their own helper

::: toolkit/components/places/PlacesUtils.jsm:2055
(Diff revision 3)
> +    this.cache.set(key, value);
> +    return value;
> +  },
> +
> +  async setWithConnection(db, key, value) {
> +    if (value === null) {

in all the 3 methods, please let's limit the key to "[a-z0-9_-]" and make the API case insensitive by lowercasing the key. Especially for delete we don't want to introduce a sql injection problem.
An we don't want confusing keys like "Ion" VS "lon".

These will need new tests
Attachment #8957357 - Flags: review?(mak77) → review+
I suppose / could also be an acceptable char, you seem to use that in the blocked bug
Comment on attachment 8957357 [details]
Bug 1426554 - Add a `PlacesUtils.metadata` API.

https://reviewboard.mozilla.org/r/226280/#review232328

> I'm a bit worried about mixing up the 2 connections here due to concurrency. Though, since we have a cache, it should be fine, since setting and fetching immediately will fetch from the cache. Maybe it's worth a comment above promiseDBConnection

I changed this to use the read-write connection just in case. It's less surprising for the default behavior, and code that already has a connection can use `getWithConnection`.

> The transaction doesn't seem necessary here? You are executing a single write, that automatically runs in an implicit transaction

I removed chunking, since we don't expect this table to store 1k values, let alone delete them one-by-one, so the transaction is gone.

> This sounds a bit scary to expose, since different consumers shouldn't clear each other data... is it strictly necessary? tests can have their own helper

Good call, moved to `PlacesTestUtils.clearMetadata`.

> in all the 3 methods, please let's limit the key to "[a-z0-9_-]" and make the API case insensitive by lowercasing the key. Especially for delete we don't want to introduce a sql injection problem.
> An we don't want confusing keys like "Ion" VS "lon".
> 
> These will need new tests

Done. `deleteWithConnection` uses bound params, that should mitigate the SQLi risk.
Comment on attachment 8957356 [details]
Bug 1426554 - Add a key-value metadata table to Places.

https://reviewboard.mozilla.org/r/226278/#review232326

> please respect the coding style of the other above definitions, with trailing commas

Whoops, bad copy-pasta. Fixed.

> it's not mandatory anymore to add a db for each version, so if you wish you can reuse the last existing one

I think I'll keep the test, since Adw, Standard8, and I are all adding migrations in 61, and it might make `test_current_from_v42.js` a bit busy if we keep adding to it. Depends on who lands first after the merge, though. :-)
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee2dc3244e5f
Add a key-value metadata table to Places. r=mak
https://hg.mozilla.org/integration/autoland/rev/8ffbd911bd28
Add a `PlacesUtils.metadata` API. r=mak
https://hg.mozilla.org/mozilla-central/rev/ee2dc3244e5f
https://hg.mozilla.org/mozilla-central/rev/8ffbd911bd28
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.