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? |
zcbenz
left a comment
zcbenz
left a comment
There was a problem hiding this comment.
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. |
zcbenz
left a comment
zcbenz
left a comment
There was a problem hiding this comment.
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 |
) Co-authored-by: Jeremy Apthorp <[email protected]>
4 participants
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 testpassesRelease Notes
Notes: none