fix: send ELECTRON_BROWSER_CONTEXT_RELEASE asynchronously

[miniak]

Description of Change

Fixes parts of #20086 by sending ELECTRON_BROWSER_CONTEXT_RELEASE asynchronously.
The synchronous send was added in #9389, which was necessary back then as we didn't have a unique identifier for each JS context.

Fixing ipcRenderer.sendSync() to not hang when the mojo connection is lost is also important, but more complicated.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed hang when closing a scriptable popup window using the remote module.

[nornagon]

works for me!

[nornagon]

cc @loc

[loc]

lgtm!

[MarshallOfSound]

Blocking while I go back to my remote repro cases and check this, I believe the synchronous nature is required for race condition handling

[MarshallOfSound]

Also this doesn't fix the root cause of #20086 it just hides the underlying issue

[zcbenz]

To add some background here:

The ELECTRON_BROWSER_CONTEXT_RELEASE message used to be required to be sent synchronously, so the remote references for current WebContents can be cleared before loading the new page, otherwise race conditions would happen.

However later there was some changes in Chromium that, the new page is pre-loaded before the old page gets destroyed, so race condition would happen even though the message was sent synchronously.

The race condition was then fixed by passing the ID of current context with the ELECTRON_BROWSER_CONTEXT_RELEASE message (#13603), so the remote references are cleared per-context instead of per-WebContents, and the sequence of messages does not matter anymore since then.

So technically the ELECTRON_BROWSER_CONTEXT_RELEASE message no longer has to be sent synchronously. However we have to ensure that the message has actually been sent when navigation happens, and I think we should add a test for it.

Also some note on how remote references are cleared:

Currently we have 2 ways of clearing remote references: one is listening to render-view-deleted event, and the other is sending the ELECTRON_BROWSER_CONTEXT_RELEASE message.

Both ways are required because render-view-deleted does not work for certain page navigations (depending on which Chromium version), and the ELECTRON_BROWSER_CONTEXT_RELEASE message can not be sent successfully when page is destroyed directly.

[zcbenz]

For the original issue #20086, before IPC module got migrated to mojo, the sendSync would simply fail when the page is being destroyed; but with mojo the sendSync is waiting for signal manually and Chromium would not unlock it for us automatically, resulting in hang when page is being destroyed.

[zcbenz]

I have added tests to ensure the remote references can be cleared for all cases (on navigation and on destroy()).

There is still problem that sendSync would hang when used in exit event while page is being destroyed directly, but I think it should be a separate problem.

[lkonstantinov]

I have to agree with @MarshallOfSound - this is just fixing piecemeal and sweeping under the carpet the underlying issue. After we've patched this particular sendSync (about a month or so ago - see #20350) we've been hitting slew of IPC deadlock issues in other situations.

[miniak]

@lkonstantinov This PR is not pretending to be fixing the root cause, it's fixing #20086. The change to use an async IPC could have been made sooner as a refactoring as it does not have to be sync anymore as @zcbenz explained.

[zcbenz]

@MarshallOfSound Can you take another look of this?

[MarshallOfSound]

All the old repro cases are fine with this change, thinking it through it should be OK. I don't want this PR to close the referenced issue though. All this does is minimize, it doesn't fix it.

[zcbenz]

I'm merging this and keeping the original issue open.

[release-clerk]

Release Notes Persisted

Fixed hang when closing a scriptable popup window using the remote module.

[trop]

I was unable to backport this PR to "6-1-x" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

[trop]

@miniak has manually backported this PR to "7-0-x", please check out #20715

[trop]

@miniak has manually backported this PR to "6-1-x", please check out #20716