1645598 - storage.sync.get does not return default values any more when the new storage.sync implementation is enabled

Status: RESOLVED FIXED

(WebExtensions :: Storage, defect)

Description

[Rob Wu [:robwu]]

The storage.{sync,local,managed}.get APIs are supposed to retrieve a value from storage, and default to the input parameters if the value is not found. This has always worked, but when the new sync storage backend was enabled (bug 1634615), this started to break. Now undefined is returned instead of the actual storage.

STR:

1. Run the following from the console of any extension that uses the storage API:

(await browser.storage.sync.get({ abcdef: 123456 }) ).abcdef

Expected:

• 123456, because the storage does not contain the key and should default to the input value for the given key.

Actual:

• undefined, meaning that the storage.sync.get API ignored the input.

I discovered this bug because a user reported broken functionality in one of my add-ons at https://github.com/Rob--W/crxviewer/issues/93

Comment 1

[Rob Wu [:robwu]]

The behavior of storage.get with an object parameter was documented in the Parameters section of the extension API documentation: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get#Syntax

A key (string) or keys (an array of strings, or an object specifying default values) to identify the item(s) to be retrieved from storage.

But it is easy to overlook those three words, so I expanded the article on MDN: https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get$compare?locale=en-US&to=1628409&from=1603555

Comment 2

[Rob Wu [:robwu]]

A chance to play with Rust, why not?
https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/third_party/rust/webext-storage/src/api.rs#237

Comment 3

[Rob Wu [:robwu]]

The crate seems to be born at https://github.com/mozilla/application-services/tree/54875c00ce464748c7747683d7dc5ba1d1c8bb92/components/webext-storage,
and vendored in the tree at https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/third_party/rust/webext-storage/Cargo.toml

It is not obvious which repository is canonical, but since I'm not seeing the crate on crates.io and this bug is an actual regression that impacts users, I'm going to submit the patch against m-c.

Comment 4

[Lina Butler [:lina]]

Ouch, thank you for catching that!

The canonical repo is https://github.com/mozilla/application-services/tree/master/components/webext-storage. Would you mind making your PR on GitHub, against the vendor-mc/78 branch? (We're using that branch to minimize churn, and to make uplifts easier).

We can merge your PR ASAP, and then use this bug to bump the rev and re-vendor.

Thank you!

Comment 5

[Rob Wu [:robwu]]

Here is the PR: https://github.com/mozilla/application-services/pull/3235

I will attach a regression test and the vendor version bump to this bug.

To test the changes, I modified m-c's top-level Cargo.toml and referred to the location of my cloned/forked application-services repository, as follows:

[patch.'https://github.com/mozilla/application-services']
fxa-client = { path = "../../repositories/application-services/components/fxa-client" }
viaduct = { path = "../../repositories/application-services/components/viaduct" }
sync15-traits = { path = "../../repositories/application-services/components/support/sync15-traits" }
interrupt-support = { path = "../../repositories/application-services/components/support/interrupt" }
sql-support = { path = "../../repositories/application-services/components/support/sql" }
webext-storage = { path = "../../repositories/application-services/components/webext-storage" }

The lookup and listing of unrelated crates is not ideal, but without it the code wouldn't compile or symbols could not be linked.

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1645598 - Add unit test for storageArea.get with defaults

Comment 7

[Rob Wu [:robwu]]

Attachment: Bug 1645598 - Vendor application-services for fix in webext-storage

To get this fix: https://github.com/mozilla/application-services/pull/3235

Updated as follows:

sed -i 's/e8d7530319fa6c20d9de78d031c9398630eca3cd/61dcc364ac0d6d0816ab88a494bbf20d824b009b/g' services/fxaccounts/rust-bridge/firefox-accounts-bridge/Cargo.toml services/sync/golden_gate/Cargo.toml toolkit/components/extensions/storage/webext_storage_bridge/Cargo.toml toolkit/components/glean/Cargo.toml toolkit/library/rust/shared/Cargo.toml
./mach vendor rust

Verified by running the new regression test that I added in the bug:

 ./mach test toolkit/components/extensions/test/mochitest/test_ext_storage_smoke_test.html --tag=remote-webextensions

Comment 8

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

Wow, thanks for being all over this!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/424c361f3d54
Add unit test for storageArea.get with defaults r=rpl
https://hg.mozilla.org/integration/autoland/rev/88e7bf87b79d
Vendor application-services for fix in webext-storage r=lina

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0613bb2a6a94
Fix lint error on head_storage.js a=lint-fix

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 1645598 - follow-up fix linting error in test

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/424c361f3d54
https://hg.mozilla.org/mozilla-central/rev/88e7bf87b79d
https://hg.mozilla.org/mozilla-central/rev/0613bb2a6a94

Comment 13

[Lina Butler [:lina]]

Comment on attachment 9156607 [details]
Bug 1645598 - Vendor application-services for fix in webext-storage

Beta/Release Uplift Approval Request

• User impact if declined: See comment 0. Extensions that rely on storage.sync.get to return default values if no data is stored locally—if the extension has just been installed, for example, or cleared its data—will break if the new Rust backend is enabled. Rob's extension is an example.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Includes test coverage upstream and an xpcshell test for Firefox. The new Rust backend is also disabled by default, but we'd like to keep our options open to enable it via a pref later.
• String changes made/needed: None

Comment 14

[Rob Wu [:robwu]]

Thanks Lina for writing the uplift request.

This follow-up linting fix should also be uplifted: https://hg.mozilla.org/mozilla-central/rev/0613bb2a6a94 (from comment 10).

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9156607 [details]
Bug 1645598 - Vendor application-services for fix in webext-storage

approved for 78.0b8

Comment 16

[Lina Butler [:lina]]

Attachment: vendor-beta.patch

This is Rob's vendoring patch, grafted on top of https://hg.mozilla.org/mozilla-central/rev/424c361f3d54 and bug 1644297, which added that test.

So the order of the uplifts is:

1. https://bugzilla.mozilla.org/show_bug.cgi?id=1644297
2. This patch.
3. https://hg.mozilla.org/mozilla-central/rev/88e7bf87b79d
4. https://hg.mozilla.org/mozilla-central/rev/0613bb2a6a94

The difference between this and what landed on m-c in https://hg.mozilla.org/mozilla-central/rev/424c361f3d54 is that this includes vendored changes from bug 1629127, but not the Firefox-specific parts of that patch. It also changes store.rs to work with the new migrate API.

Try in progress here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f09a54cbe318fbc7d1ec1d13507a342f919f9df8

Comment 17

[Lina Butler [:lina]]

Comment on attachment 9157058 [details] [diff] [review]
vendor-beta.patch

Talked to :jcristau and we'll just uplift bug 1629127 directly.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/00f55851cbf1
https://hg.mozilla.org/releases/mozilla-beta/rev/2d763c682268
https://hg.mozilla.org/releases/mozilla-beta/rev/06cee26ac42e