Closed Bug 1626506 Opened 4 years ago Closed 4 years ago

Vendor webext-storage from applications-services into mozilla-central

Categories

(WebExtensions :: Storage, enhancement, P2)

enhancement

Tracking

(firefox77 fixed)

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: markh, Assigned: lina)

References

Details

(Whiteboard: SACI)

Attachments

(2 files)

The implementation of chrome.storage.sync which syncs with the usual Sync servers is being written in Rust and lives in the application-services repo. This will need to be vendored into mozilla-central before it can be used.

Whiteboard: SACI

Hi Mark,
we are setting the priority to P2 mainly to filter it out by our weekly bug triaging, feel free to bump its priority to P1 once it is ready to be assigned.

Priority: -- → P2

Hooray, our first Application Services Rust component! This is a
mechanical run of mach vendor rust, split out into its own commit
to make reviewing the Firefox bindings easier.

Assignee: nobody → lina
Status: NEW → ASSIGNED

The webext_storage_bridge crate introduced in this commit bridges the
mozIExtensionStorageArea XPCOM interface to the webext_storage Rust
component from Application Services.

This commit factors out the following parts from bug 1623245, so that
we can land them piecemeal:

  • The mozIExtensionStorageArea interfaces, which implement all the
    methods needed to support the storage.sync API.
  • A Rust implementation of the above, in StorageSyncArea.
  • A StorageTask type, for dispatching storage operations to a
    background task queue.
  • A LazyStore, which wraps the Rust component's Store and lazily
    initializes it on the background queue the first time it's used.
  • A StorageSyncService, which is our singleton. It holds on to a
    configured StorageSyncArea, and hands out references to it via
    getInterface. Eventually, we'll extend this to support syncing,
    too.

Depends on D71895

On Phabricator, Luca asked:

I'd like to know more about this comment and the rationale behind it, especially because I may be missing or misreading something related to it ;-)

If this part is used for the storage.sync implementation part that takes care of storing the data locally (in the new storage-sync2.sqlite sqlite > file) as I assume from the quick look a gave to this part, I would expect that this would be in the GeckoView builds too, and then the part related > to actually sync the data from/to the sync backend to be the part integrated at the "Android Components" level.

is that right or am I missing something?

Since Phabricator likes to munch comments, and Mark and I were also chatting about Android earlier, I wanted to capture this here! 😁

[So, that's almost right! On Desktop, as you saw, we link our Rust component into libxul, and call into it directly. But, on Android, our Rust components are "peer dependencies" of GeckoView: our Android Components libraries depend on both, and glue them together. For our other components, like bookmarks, we have our Rust component expose a C-compatible FFI (on Desktop, XPCOM fulfills this role), and then a Kotlin API on top of that for Android Components to call. For bookmarks, it's easier, because GV doesn't need to access bookmarks directly: it's only told "go load this URL", so it doesn't need to talk to the Rust component at all. But, storage.sync (and history, and password autofill), is special, because we need to expose data that's managed by our Rust component to WebExtensions running in GV.

Splitting it up the way you suggested—including webext_storage in GV, and the Sync parts in Android Components—is a little tricky, because both the storage and syncing APIs need to write to the same database. That would mean having two writer connections—one inside GV, the other in our Rust component embedded into Android Components—accessing the same database file concurrently, which we've learned is super unreliable. It would also mean linking the Rust component twice: once into the libxul that ships with GV, and the second time into our Rust "megazord" (a library containing all our Rust components, packaged this way to reduce the size footprint of our Rust code) that we provide for Android Components.

The way we've solved this problem for other data types, like history, is to have GV provide a delegate for the API. In our case, the GV delegate will look a lot like the mozIExtensionStorageArea interface, maybe something like this:

class WebExtensionStorageAreaChange {
    @Nullable public String key;
    @Nullable public String oldValue;
    @Nullable public String newValue;
};

interface WebExtensionStorageAreaDelegate {
    GeckoResult<String> get(String extensionId, String key);

    GeckoResult<WebExtensionStorageAreaChange> set(String extensionId,
                                                   String key,
                                                   String newValue);

    GeckoResult<WebExtensionStorageAreaChange> remove(String extensionId,
                                                      String key);

    GeckoResult<List<WebExtensionStorageAreaChange>> clear(String extensionId);
};

Then, in GV, we'll include a JS implementation of mozIExtensionStorageArea that calls into the Java delegate (using GV's EventDispatcher), and passes the results back. And Android Components will implement this delegate in Java/Kotlin, and wire it up to our Rust component. That way, only the Rust component has a connection to the storage DB, it integrates well with the other Sync code that's already there, and we only link one copy of webext_storage into our megazord.

Web Push is actually an even better example of this, because it follows the same pattern I described above! There's an nsIPushService interface, which GeckoView implements to pass data back and forth between web content and a WebPushDelegate. Android Components provides an implementation of this delegate that calls into the Kotlin API for our Push Rust component.

That's quite the tiramisu! 😂 But that's the suuuper long answer of why we don't build webext_storage on Android—we expect it to provide a different implementation that calls into a GV delegate, instead of managing storage directly!

Depends on: 1633289
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00b40d247500
Vendor the `webext_storage` component. r=markh
https://hg.mozilla.org/integration/autoland/rev/f3deedfe235d
Add a binding for the new extension storage Rust component. r=markh

Sorry about that! It would help if I landed what I actually pushed to Try, wouldn't it? 😂

Flags: needinfo?(lina)
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42cadb78e630
Vendor the `webext_storage` component. r=markh
https://hg.mozilla.org/integration/autoland/rev/63dacb1f4104
Add a binding for the new extension storage Rust component. r=markh

Of course! :D

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Regressions: 1633639

(In reply to :Lina Cambridge from comment #4)

On Phabricator, Luca asked:

I'd like to know more about this comment and the rationale behind it, especially because I may be missing or misreading something related to it ;-)

If this part is used for the storage.sync implementation part that takes care of storing the data locally (in the new storage-sync2.sqlite sqlite > file) as I assume from the quick look a gave to this part, I would expect that this would be in the GeckoView builds too, and then the part related > to actually sync the data from/to the sync backend to be the part integrated at the "Android Components" level.

is that right or am I missing something?

Since Phabricator likes to munch comments, and Mark and I were also chatting about Android earlier, I wanted to capture this here! 😁
...
That's quite the tiramisu! 😂 But that's the suuuper long answer of why we don't build webext_storage on Android—we expect it to provide a different implementation that calls into a GV delegate, instead of managing storage directly!

Thanks a lot Lina! Personally I would define the answer as "as detailed as actually needed" :-)

I did really appreciate the level of detail you provided, that was exactly what I needed to know
(and I'm really glad I asked, keeping these detail in mind is going to be invaluable for the reviews on Bug 1623245 that will come next).

Regressions: 1633750
Blocks: 1634191
Blocks: 1633943
No longer depends on: 1633289
See Also: → 1633289
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: