Add a way to convert a non-thread safe `nsIPropertyBag` into thread-safe `HashPropertyBag` in Rust
Categories
(Core :: XPCOM, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox76 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(1 file)
In bug 1596322, we're adding a scriptable interface that takes an in nsIPropertyBag parameter, with primitive (number, boolean, and string) arguments. Unfortunately, (I don't think?) property bags created from JS are thread-safe—the implementation of @mozilla.org/hash-property-bag;1 is cycle-collected, and the XPCVariants that back the properties definitely aren't thread-safe.
But the HashPropertyBag wrapper that we use in Rust does use a thread-safe property bag, and its variants are also thread-safe. So let's provide a TryFrom<&nsIPropertyBag> implementation for HashPropertyBag, so we can clone a thread-safe bag from a potentially unsafe one.
|
Assignee | |
Comment 1•5 years ago
|
||
Implementations of nsIPropertyBag, particularly ones used from JS,
aren't thread-safe: they're cycle-collected on the main thread, and
their property types are XPCVariants. However, the
HashPropertyBag implementation in Rust is thread-safe, since it's
backed by a thread-safe instance of nsHashPropertyBag (non-cycle
collected), and its values are thread-safe primitive mozStorage
variants.
This commit adds a TryFrom<&nsIPropertyBag> implementation, to make
it possible to clone any kind of nsIPropertyBag into a safe
HashPropertyBag.
It also marks HashPropertyBag as Send + Sync, given that the only
way to construct one is through HashPropertyBag::new, and the only
variant type we support in Rust is the mozStorage one.
Finally, it changes the type tags for storage variants to UTF8STRING
and ASTRING. WSTRING was not the correct type to use here; that's
for in wstring parameters. CSTRING is, in theory, the same type as
UTF8STRING (nsCString), but mozStorage only implements
GetAsAUTF8String, not GetAsACString.
nsHashPropertyBag does use threadsafe refcounting and it isn't cycle collected. I'm sure you are aware of that because you implemented bug 1530506 which exposed it to Rust, but I'm just curious why that isn't sufficient for your uses.
|
|
Assignee | |
Comment 3•5 years ago
|
||
nsHashPropertyBag does use threadsafe refcounting and it isn't cycle collected.
Hmm, that's what we're using on the Rust side, yes. But the interface I'm adding has a method like void initialize(in nsIPropertyBag options), that takes a property bag from JS. It then needs to pass that bag to a runnable running on a background thread.
So, if I do, Cc["@mozilla.org/hash-property-bag;1"].createInstance(Ci.nsIWritablePropertyBag), I think that's going to give me a cycle-collected one? And then, if I call setProperty on that bag in JS, it'll set the properties using XPCVariants, right? So, if I try to pass that to initialize, then use it from the background thread, it's going to trip some XPConnect assertions.
This bug is about adding a way to convert that nsIPropertyBag we get from JS, to a HashPropertyBag that we know we can use from multiple threads (because, as you pointed out, nsHashPropertyBag and the variants we use in Rust are also thread-safe).
Does that make sense, or am I overthinking it?
|
|
Assignee | |
Updated•5 years ago
|
(In reply to :Lina Cambridge from comment #3)
Does that make sense, or am I overthinking it?
Yes, thanks for the explanation!
|
|
Assignee | |
Comment 5•5 years ago
|
||
No problem, thank you for double-checking! Your question inspired me to add better comments to the patch! 😁
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 6•5 years ago
|
||
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
Description
•