-
Notifications
You must be signed in to change notification settings - Fork 925
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
Wrap window.ethereum._metamask in gin::Wrappable #18411
Conversation
so that _metamask and ethereum have equivalent lifetime.
v8::Global<v8::Promise::Resolver>(isolate, resolver.ToLocalChecked())); | ||
auto context(v8::Global<v8::Context>(isolate, isolate->GetCurrentContext())); | ||
ethereum_provider_->IsLocked(base::BindOnce( | ||
&JSEthereumProvider::MetaMask::OnIsUnlocked, base::Unretained(this), |
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.
with owning mojo::Remote, we don't need weak_ptr because the previous bound callback won't be called when mojo disconnected
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.
Is it possible to avoid using base::Unretained(this) here? It seems like this opens up the possibility of another issue occurring if this object is destroyed before the async operation is completed based on what's being described here.
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.
This is intentional, when dealing with mojo interface, using weak_ptr is unnecessary. Mojo pipe will invalidate all of the ongoing message when disconnected so OnIsUnlocked
won't be called when the class who owns mojo::Remote<mojom::EthereumProvider>
is gone.
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
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.
Got the needed ++, going to merge so we can uplift into 1.51.x
and get RC builds running for tomorrow.
so that _metamask and ethereum have equivalent lifetime.
Resolves brave/brave-browser#30204
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
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Described in the issue