-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Initialize nss explicitly #6596
base: main
Are you sure you want to change the base?
Conversation
e23748c
to
6a74eca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea in general, the only question for me is how applications should initialize NSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of what I don't understand here is what initialization looks like when the "keydb" feature is enabled? Something like logins
is eventually going to be used in both contexts, so how would that look? (Sorry if I missed this!)
578d4e7
to
8b539bb
Compare
// TODO: would be nice if we could write | ||
// #[uniffi::export] | ||
// pub use nss::ensure_initialized as ensure_nss_initialized; | ||
// instead. | ||
#[uniffi::export] | ||
pub fn ensure_nss_initialized() { | ||
ensure_initialized() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bendk is there a way to do something like
#[uniffi::export]
pub use nss::ensure_initialized as ensure_nss_initialized;
or a way to avoid wrapping the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is https://mozilla.github.io/uniffi-rs/latest/proc_macro/functions.html#renaming-functions-methods-and-constructors, but I don't think that will actually work here :( So I don't think there is a good way, but it seems like a reasonable feature request!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't look like there's already an issue filed for this, will do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for filing that. IMO, we could extend the concept of "external types" to also support "external functions".
8a4bea3
to
d6ef0ab
Compare
// TODO: would be nice if we could write | ||
// #[uniffi::export] | ||
// pub use nss::ensure_initialized as ensure_nss_initialized; | ||
// instead. | ||
#[uniffi::export] | ||
pub fn ensure_nss_initialized() { | ||
ensure_initialized() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for filing that. IMO, we could extend the concept of "external types" to also support "external functions".
BREAKING CHANGE: you now need to call `nss::ensure_initialized()` before any calls to NSS functions, including dependants like rc_crypto or jwcrypto.
d6ef0ab
to
b1cd271
Compare
Initialize NSS explicitly, rather than implicitly in NSS-using functions.
To have control over NSS initialization, it must be done explicitly. This pull request removes implicit initialization from the relevant methods and instead checks whether NSS has been initialized and throws an appropriate error if it has not.
Initialization can be done either directly via NSS.
BREAKING CHANGE:
you now need to call
nss::ensure_initialized()
before any calls to functions which depend on NSS. Watch out forCryptoError(Error(NSS error: NSS has not been initialized))
.Pull Request checklist
[ci full]
to the PR title.Branch builds: add
[firefox-android: branch-name]
to the PR title.