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
refactor: try just using regular [Sync] for MessageSync #20797
Conversation
This seems to be passing CI... could it be that we've removed our need for synchronous IPC to be well-ordered w.r.t asynchronous IPC? |
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 think tests pass this time because lots of tests have been moved to main process.
This might still cause problems with remote module:
win = remote.getCurrentWindow()
win = null && gc() // async DereferenceRemoteJSObject sent to main process
win = remote.getCurrentWindow() // sync GetGlobal sent to main process
// DereferenceRemoteJSObject handled, remote reference cleared
// win now references to non-exist remote object.
Hm, I don't think the scenario you laid out would be a problem in the usual case. The only time that sync calls will be reordered with respect to async calls is if the main process is waiting on a sync call, which almost never happens. We previously observed some issues relating to window close, because there's a compositor shutdown call that the browser invokes synchronously during window close. If I recall correctly, it was because we were triggering synchronous calls during window close to release objects, which I don't think is the case any more? But you're right that we're exercising the remote module a lot less. |
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 the clarification, I'm good with the change then.
Let's merge this and see how it goes. We can always revert if we discover issues. |
No Release Notes |
@MarshallOfSound has manually backported this PR to "7-1-x", please check out #21776 |
@nornagon what about |
it should. @MarshallOfSound want to do the honors? |
@erickzhao has manually backported this PR to "8-x-y", please check out #21948 |
Description of Change
This aligns us closer with how Mojo expects to work, and as a bonus fixes some tricky race conditions that can happen if the browser process closes its end of the mojo pipe while we're waiting for a synchronous response.
Checklist
npm test
passesRelease Notes
Notes: none