-
Notifications
You must be signed in to change notification settings - Fork 939
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
Set brave wallet string state synchronously. #12748
Conversation
@@ -193,6 +193,8 @@ interface BatLedgerClient { | |||
SetDoubleState(string name, double value); | |||
[Sync] | |||
GetStringState(string name) => (string value); | |||
[Sync] | |||
SetStringStateSync(string name, string value) => (); |
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.
It seems to me that all of the Set*State
methods are going to be prone to the same race condition.
As far as I know, all the "Set" methods are used exactly as if they were sync. Instead of introducing a "Sync" version of SetStringState
, can we just add the [Sync]
attribute to all of these "SetState" methods?
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.
Done, thanks for the idea!
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.
LGTM
Verification PASSED on
|
When wallet is first created, it is written to profile prefs by asynchronous
SetStringState
mojo call (inWallet::SetWallet
function). After that wallet is requested shortly by running synchronousGetStringState
mojo call (inPostWalletBrave::Request
function). This leads to race condition so requested wallet could be null (it happens due to mojo async tasks queue execution is interrupted on mojo sync tasks execution).This PR changes wallet set state mojo calls to synchronous calls (like
SetStringState
)Resolves brave/brave-browser#21809
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: