1978718 - Implement get_keys API method in Rust webext sync storage

Status: RESOLVED FIXED

(WebExtensions :: Storage, enhancement, P3)

Description

[Nate Gross]

This is a follow-up to Bug 1910669 - Implement StorageArea.getKeys() in extension storage API. The goal is to implement the get_keys method in the Rust back end and then update the sync storage method getKeys() to use it instead of using Object.keys() and get().

I believe the Rust implementation needs to be added in the following files (there may be more but this is at least a starting point):

1. https://searchfox.org/mozilla-central/source/third_party/rust/webext-storage/src/api.rs
2. https://searchfox.org/mozilla-central/source/third_party/rust/webext-storage/src/store.rs
3. https://searchfox.org/mozilla-central/source/third_party/rust/webext-storage/src/webext-storage.udl

Once that's working then we just need to add (or update) the method in https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionStorageSync.sys.mjs

Comment 1

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

Note that the correct place to make these changes would be under https://github.com/mozilla/application-services/tree/main/components/webext-storage

Comment 2

[Nate Gross]

Per markh the changes need to be made in a different repo, and when I look at the documentation there the info for building for Firefox Desktop seems to be missing (https://github.com/mozilla/application-services/blob/main/docs/building.md#building-for-firefox-desktop). Given that I'm a relative newbie and the documentation seems to have some gaps, it might take quite a bit of hand-holding to get me to be able to actually build and test the changes. Unless you think I'm overestimating the complexity, I think you should probably unassign this bug from me.

Comment 3

[Rob Wu [:robwu]]

Although I did not have prior experience working on this specific component, I got tests running within 10 minutes from scratch in a blank Linux environment. Based on my experience, I think that it is feasible for you as a new contributor to succeed in implementing the desired changes, but it is also okay if you prefer to do something else (in which case I'll leave this bug open for someone else).

This is what I did:

Install Rust

This is Rust code, you need Rust. When you run ./mach bootstrap and choose 2. Firefox for Desktop, it will also install rust. After installation, the relevant files are in ~/.cargo/.

Because I tested on a blank Linux environment, I did not have a Gecko clone there, so I followed the instructions to install Rust from: https://www.rust-lang.org/tools/install

Clone repo and run tests

First I cloned the repo according to step 1 of https://github.com/mozilla/application-services/blob/main/docs/building.md#building-the-rust-components :

$ git clone https://github.com/mozilla/application-services # (or use the ssh link)
$ cd application-services
$ git submodule update --init --recursive

After that I entered the directory containing the code of interest and ran the test. It will download dependencies (crates), (re)build relevant code if needed before running tests:

$ cd components/webext-storage
$ cargo test

This runs all relevant tests. The last lines are:

test sync:tests::test_get_synced_changes ... ok
test api::tests::test_quota_maxitems ... ok

test result: ok. 54 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.32s

To make sure that my code changes affect the test outcome, I edited an existing unit test and changed the expectation to something different, e.g. changed 7 to 6 at https://github.com/mozilla/application-services/blob/365edb4392e621ede6c3daf9c8d80505d3a9c4aa/components/webext-storage/src/api.rs#L707 . After that I ran cargo test again, and it showed a test failure, as expected.

Implement changes

Not tested of course, but based on reading some code, I think that the steps to implement get_keys are as follows:

1. Possibly rename the existing get_keys local helper so you can introduce a new get_keys method.
   • https://github.com/mozilla/application-services/blob/365edb4392e621ede6c3daf9c8d80505d3a9c4aa/components/webext-storage/src/api.rs#L707
2. Add a new get_keys implementation.
   • For inspiration, look at get (link to source), and cut out a lot of code, since get_keys is very simple and doesn't need special parameter conversions.
   • For more inspiration you can also look for other callers of the current get_keys helper in that file.
3. Unit tests. Add logic to the existing unit test at https://github.com/mozilla/application-services/blob/365edb4392e621ede6c3daf9c8d80505d3a9c4aa/components/webext-storage/src/api.rs#L438-L527
4. Add glue to expose the functionality:
   • WebExtStorageStore implementation at https://github.com/mozilla/application-services/blob/365edb4392e621ede6c3daf9c8d80505d3a9c4aa/components/webext-storage/src/store.rs
   • the interface at https://github.com/mozilla/application-services/blob/365edb4392e621ede6c3daf9c8d80505d3a9c4aa/components/webext-storage/src/webext-storage.udl
   • possibly more, I'm doing some handwaving here.

This should be enough to make some changes and get a PR up on Github for review.

Once merged, a next step is to vendor the updated webext-storage component. I can give that a try, or if you'd like, you can do it yourself too.
The dependency is listed at https://searchfox.org/mozilla-central/rev/7e80e8d113e0f2743499d7be39e88ec1a3001a8e/Cargo.lock#7504
Documentation on vendoring Rust is at https://firefox-source-docs.mozilla.org/build/buildsystem/rust.html

Comment 4

[Rob Wu [:robwu]]

Mark, could you look at my comment 3 above and check for accuracy and/or better ways to do stuff?
And do you have recommendations on a workflow to test the code in Firefox and the webext-storage component at once? I tried something without success, see below.

What I did was as follows:

1. Replaced Ok(size) with Ok(size + 100) in the get_bytes_in_use implementation at https://searchfox.org/mozilla-central/rev/7e80e8d113e0f2743499d7be39e88ec1a3001a8e/third_party/rust/webext-storage/src/api.rs#354
2. Build with my local mozconfig with a non-artifact build: MOZCONFIG=.mozconfig-debug ./mach build
3. Ran the test that should notice the behavioral change: MOZCONFIG=.mozconfig-debug ./mach test toolkit/components/extensions/test/xpcshell/test_StorageSyncService.js
   • https://searchfox.org/mozilla-central/rev/7e80e8d113e0f2743499d7be39e88ec1a3001a8e/toolkit/components/extensions/test/xpcshell/test_StorageSyncService.js#246-250

The test passed, which implies that my change to third_party/rust/webext-storage/src/api.rs did not have any effect. I can think of more ways to hack something together (e.g. vendoring a custom fork of webext-storage), but I hope that there is something less cumbersome than that.

Comment 5

[Rob Wu [:robwu]]

(unassigning as requested in comment 2; feel free to pick it up if you'd like following the instructions above)

Comment 6

[Nate Gross]

I should have gotten back to you more quickly! The approach you're suggesting is exactly what I was thinking, down to the need to rename the existing private get_keys helper, so that gives me a lot of confidence that I'm on the right track. I've got the patch and test written and it works. Hopefully I'll have a PR for review later this morning.

Comment 7

[BMO Github Automation]

Attachment: [mozilla/application-services] Rust changes for Bug 1978718 - Implement get_keys API method in Rust webext sync storage (#6845)

Comment 8

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

Sorry I missed some of this - everything above looks good, and that PR also looks good. Modifying code in third_party/rust is fragile if it works at all, but for testing in desktop firefox, you can follow the instructions at https://github.com/mozilla/application-services/blob/main/docs/howtos/vendoring-into-mozilla-central.md - it has a section for testing, which is a little tedious, but should work fine.

Comment 9

[Nate Gross]

Attachment: WIP: Bug 1978718 - Implement get_keys API method in Rust webext sync storage (vendoring & API change)

Comment 10

[Nate Gross]

I've submitted the PR for review in Github. I managed to successfully "vendor" the file following instructions at the link Mark shared and wrote and tested the change to the Javascript. Everything seems to be working end-to-end. My draft patch in Phabricator is failing a test, but I'm guessing that's because the PR in application-services hasn't been merged.

Do you know if there's anything else for me to do at the moment or do I just need to wait for a review on the PR?

Comment 11

[Rob Wu [:robwu]]

(In reply to Nate Gross from comment #10)

My draft patch in Phabricator is failing a test, but I'm guessing that's because the PR in application-services hasn't been merged.

What test is failing? I checked your patch out locally, ran tests and they pass:
MOZCONFIG=.mozconfig-debug ./mach test toolkit/components/extensions/test/xpcshell/test_StorageSyncService.js toolkit/components/extensions/test/xpcshell/test_ext_storage_sync*.js --log-mach-verbose --verbose

Do you know if there's anything else for me to do at the moment or do I just need to wait for a review on the PR?

That's all for now. Once the patch in application-services lands, the patch on Phabricator should be updated to point to the new revision, but other than that that probably is all.

If you're interested, you could also help out with expanding the developer documentation to write down pointers on maintaining the implementation, at https://searchfox.org/mozilla-central/rev/26c15614d920cf347a77e352b73c7d29cb345150/toolkit/components/extensions/docs/webext-storage.rst . A new bug would have to be filed for that, of the task, in this component. At the very least there is one broken link to fix, at https://searchfox.org/mozilla-central/rev/26c15614d920cf347a77e352b73c7d29cb345150/toolkit/components/extensions/docs/webext-storage.rst#12 the correct destination is https://firefox-source-docs.mozilla.org/rust-components/developing-rust-components/uniffi.html .

Comment 12

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

That PR should land in the next 30 minutes or so, after which you can update your second patch with the "real" revision as Rob mentioned and we are good to go!

Comment 13

[BMO Github Automation]

Authored by https://github.com/GrossNate
https://github.com/mozilla/application-services/commit/268992e724783e25f5b9614071d8a66a4ea6c902
[main] Bug 1978718 - Implement get_keys API method in Rust webext sync storage (Rust changes)

Authored by https://github.com/GrossNate
https://github.com/mozilla/application-services/commit/6624da2a87b475f8ed47aa57e9ab21b9ca430b70
[main] Bug 1978718 - Implement get_keys API method in Rust webext sync storage (Rust changes)

Comment 14

[Rob Wu [:robwu]]

This bug got closed because a patch landed. To prevent that from happening in the future, put the leave-open value in the keyword field.

For an example, see:

• added leave-open keyword at https://bugzilla.mozilla.org/show_bug.cgi?id=1959339#c27
• merged into main at https://bugzilla.mozilla.org/show_bug.cgi?id=1959339#c35
• removed leave-open keyword at https://bugzilla.mozilla.org/show_bug.cgi?id=1959339#a8315165_447061
• merged (and closed bug) at https://bugzilla.mozilla.org/show_bug.cgi?id=1959339#c38

Comment 15

[BMO Github Automation]

Attachment: [mozilla/application-services] Added empty storage condition to `test_check_get_keys_impl` and some cleanup for Bug 1978718 (#6853)

Comment 16

[Nate Gross]

I made the changes Rob suggested this morning and that PR should be ready to merge. Once that's done I'll update the patch, check that it's still working end-to-end, and remove the --wip tag.

Comment 17

[BMO Github Automation]

Authored by https://github.com/GrossNate
https://github.com/mozilla/application-services/commit/7d22d35b59dc623116ef6216fd3a4c4a572ef9b3
[main] Added additional test and did some cleanup for Bug 1978718

Comment 18

[Nate Gross]

Attachment: Bug 1978718 - Implemented get_keys API method in Rust webext sync storage (added method to ExtensionStorageSync.sys.mjs)

Comment 19

[Nate Gross]

Please let me know if there's anything else I need to do on this patch in order to get it reviewed and (hopefully) landed. Following the instructions on https://github.com/mozilla/application-services/blob/main/docs/howtos/vendoring-into-mozilla-central.md I've added Mark Hammond as a reviewer (assuming he's from application-services) as well. Do I also need a "build peer"? Or is that you, Rob?

I know I'm following up on this after only a few weekdays, but my understanding is that the vendoring process is a little fragile and I'm afraid if too much time elapses then I'll need to re-do pulling in the "third party" code. Thanks!

(In case it helps, I'd point out that even though the diff is 100s of lines, the only file I manually changed is toolkit/components/extensions/ExtensionStorageSync.sys.mjs - everything else is just generated code from the vendoring process, based on Rust code that was already merged successfully in application_services on GitHub.)

Comment 20

[Rob Wu [:robwu]]

I've been busy investigating incidents (3) last week, sorry about the lack of feedback.

A quick look at the patch shows that there are many unrelated changes (third_party/rust/remote_settings for example).

Could you rebase your code to the latest main branch, and try vendoring again? I'll schedule a review of your code later today.

FWIW, I took a look at the code when it was in WIP state, and it looked good back then.

Comment 21

[Nate Gross]

Great! I rebased on the latest main branch and now it seems like I don't even need to vendor the changes in. I'm guessing they got handled by someone else's patch. Since that's the case and now I'm only modifying a single file I've removed Mark as a reviewer.

Comment 22

[Rob Wu [:robwu]]

The desired functionality was vendored by the second patch in bug 1881344, https://searchfox.org/mozilla-central/commit/0290248a92d8d9861b5d93163132f2dc32d71f45

I found that bug/patch by looking in Searchfox for the get_keys_helper function (which you newly introduced) and using the left side to see the last change to the line: https://searchfox.org/mozilla-central/rev/2a8a30f4c9b918b726891ab9d2d62b76152606f1/third_party/rust/webext-storage/src/api.rs#235

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b25a731dc807
https://hg.mozilla.org/integration/autoland/rev/200ed77e9c51
Implemented get_keys API method in Rust webext sync storage (added method to ExtensionStorageSync.sys.mjs) r=robwu

Comment 24

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/200ed77e9c51