1910669 - Implement StorageArea.getKeys() in extension storage API

Status: RESOLVED FIXED

(WebExtensions :: Storage, enhancement, P3)

Description

[Rob Wu [:robwu]]

This bug tracks the implementation of getKeys() for each StorageArea in the storage API. It enables extensions to efficiently query storage keys without also reading the associated data.

References:

• https://github.com/w3c/webextensions/issues/601
• https://github.com/w3c/webextensions/pull/649
• https://github.com/w3c/webextensions/blob/main/proposals/storage-get-keys.md

Comment 1

[Rob Wu [:robwu]]

Relevant implementation files are here:

• https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-storage.js
• https://searchfox.org/mozilla-central/source/toolkit/components/extensions/child/ext-storage.js
• https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/storage.json

For unit tests, there are multiple test files, with common test logic in this file: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/head_storage.js

To find the related files, click on some of the helper functions to see their usage, e.g. https://searchfox.org/mozilla-central/search?q=test_background_page_storage&redirect=false

To run the relevant tests, use the following command:

./mach test toolkit/components/extensions/test/xpcshell/test_ext_storage_local.js toolkit/components/extensions/test/xpcshell/test_ext_storage_session.js toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js toolkit/components/extensions/test/xpcshell/test_ext_storage_sync_kinto.js --log-mach-verbose --verbose --sequential

Note that I added --log-mach-verbose and --verbose; these increase the verbosity of the test output that can help with observing test results.
I also added --sequential, so that only one test runs at any given time (instead of parallel, by default).

These tests are also run with multiple test configurations. Add --tag=remote-webextensions to run fewer tests but still get reasonable test coverage.

Comment 2

[Nate Gross]

I'll work on this.

Comment 3

[Nate Gross]

Attachment: Bug 1910669 - Implemented StorageArea.getKeys() in extension storage API. r?robwu!

Comment 4

[Nate Gross]

tl;dr

• local StorageArea is done, others are in progress.
• I haven't started writing tests yet.
• WIP is in Phabricator in case that's of interest.
• Two questions:
  1. Does it make sense to implement getKeys() for managed storage since developers can't write new data to it?
  2. For sync storage, do I need to figure out how to implement it in the underlying Rust store or should I just use something like Object.keys(this.get(extension)) to simply use the already-implemented get() method?

details

As far as I can tell there are four types of StorageArea in the API:

1. local
   • I believe I have this working. I made a little temporary add-on and it successfully sets data and then gets the keys.
2. sync
   • Not sure if I have this working yet. The sync storage API doesn't work with temporary add-ons (which is my quick-and-dirty testing method). I tried commenting out the relevant enforceNoTemporaryAddon() calls in t/c/e/parent/ext-storage.js and that let me clear() and set() some data in "sync" storage, but then I got some Promise-related errors and I don't know if that's because there's something wrong with my implementation or it's just related to some other issue with it being a temporary add-on. I'm not stumped yet - this is just an update.
   • One question about this - it looks like sync storage is actually plumbed down to a Rust implementation that's reached through a UniFFI-generated interface. The way I'm trying to implement it is to just use the existing get() method and then return the keys from that (with Object.keys()). That way I don't have to figure out what's going on in the underlying implementation, but it somewhat defeats the purpose of having a getKeys() method. Question: Should I try to figure out how to implement this in Rust or is my current approach okay?
3. session
   • This is not working, but I'm not stumped yet.
4. managed
   • Per the description in the storage.json schema, the managed is read-only for extensions. Question: I'm not sure it makes sense to implement getKeys() for this because I can't think why extension developers would want to use it. However should I do it for consistency across all StorageAreas?

I haven't written tests yet, but I'll tackle that tomorrow.

Comment 5

[Rob Wu [:robwu]]

(In reply to Nate Gross from comment #4)

1. Does it make sense to implement getKeys() for managed storage since developers can't write new data to it?

For consistency across the interfaces, it makes sense to support it. Although the implementation diverges significantly, the public interface is shared, and also documented as if StorageArea is a type. This type is only visible in the API definition and documentation (MDN: storage.StorageArea), not what extensions see when they use the API.

2. For sync storage, do I need to figure out how to implement it in the underlying Rust store or should I just use something like Object.keys(this.get(extension)) to simply use the already-implemented get() method?

I suggest to start with a simple prototype that works (getKeys defined as you wrote) and write unit tests. After that you can consider refactoring further. That task could also be split to a separate patch (or bug) as needed.

2. sync
   • Not sure if I have this working yet. The sync storage API doesn't work with temporary add-ons (which is my quick-and-dirty testing method).

When not logged in into Firefox, sync keeps all data local, and therefore you don't have to worry about polluting some backend server. To test manually, you can install any extension with the storage permission and use the developer tools to run code in the context of an extension, via about:debugging. A trick that I sometimes use is to click on the manifest.json link displayed at about:debugging for the given extension, which opens a new tab. Then at that tab, open the Developer Tools (e.g. right-click, inspect element) and then you can open the console where you can enter code that now runs in the context of the extension.

The storage permission is commonly requested. Here is a simple extension that you can try (authored by me): https://addons.mozilla.org/addon/dont-track-me-google1/

• Per the description in the storage.json schema

Already answered elsewhere, but tip: click on the permalink button in the right sidebar of Searchfox to get a permalink to the current revision of the source code. That ensures that the link to a specific line will still point to that line even if the file changes.

Comment 6

[Nate Gross]

I've gotten getKeys() working and updated the WIP revision. I'll start writing tests tomorrow.

Comment 7

[Bekir Öztürk]

What's the point without Rust? As extension developers we can already use Object.keys() and browser.storage.get().

Comment 8

[Nate Gross]

(In reply to Bekir Öztürk from comment #7)

What's the point without Rust? As extension developers we can already use Object.keys() and browser.storage.get().

I think the point is to make some incremental progress and keep the functionality at parity, even if the performance isn't great yet. Developers can use getKeys() now and then they'll benefit from the back end improvements whenever someone is able to make them. Once this revision lands the method will exist in the API and there's a test for it so the Rust implementation should be a discrete change.

That said, I think I agree with your underlying point and if Rob Wu is able to support me then I'll tackle the Rust implementation next (unless someone else wants to work on it). It will be a challenge for me since I haven't worked in Rust before, but I found the relevant code and will start reading the documentation on how to generate the ffi bindings for the function.

Comment 9

[Rob Wu [:robwu]]

(In reply to Nate Gross from comment #8)

(In reply to Bekir Öztürk from comment #7)

What's the point without Rust? As extension developers we can already use Object.keys() and browser.storage.get().

I think the point is to make some incremental progress and keep the functionality at parity, even if the performance isn't great yet. Developers can use getKeys() now and then they'll benefit from the back end improvements whenever someone is able to make them. Once this revision lands the method will exist in the API and there's a test for it so the Rust implementation should be a discrete change.

+1 to this. Even a naive implementation offers an improved performance over the naive implementation, by reducing the amount of data exchanged over IPC. From the engineering point of view, having a reference implementation with the expected behavior makes it easier to incrementally improve the implementation, and rely on the test coverage that is already passing with the reference implementation. This test coverage makes it easier to verify that the refactored version is still behaving as desired.

That said, I think I agree with your underlying point and if Rob Wu is able to support me then I'll tackle the Rust implementation next (unless someone else wants to work on it). It will be a challenge for me since I haven't worked in Rust before, but I found the relevant code and will start reading the documentation on how to generate the ffi bindings for the function.

Note that Rust is compiled code, so you cannot use artifact builds any more with that. With an artifact build you can get start with running your build and tests within a few minutes; with non-artifact builds you have to wait for several more minutes before the build completes. Could be as fast as 10 minutes on powerful laptops/macbooks, or 30 minutes on slightly older computers.

It has been a while since I authored Rust code, but I can still help with debugging and finding answers as needed.

Comment 10

[Bekir Öztürk]

Thanks for the informative answers.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7bc3164ef68d
https://hg.mozilla.org/integration/autoland/rev/890ad42f131e
Implemented StorageArea.getKeys() in extension storage API. r=robwu

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/890ad42f131e

Comment 13

[rbloor]

Documentation updates available in:

• (content) Bug-1910669 add StorageArea.getKeys() doc and release note #40560
• (BCD) Bug-1910669 add StorageArea.getKeys() #27442