Closing child window hangs main window

[zarubond]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

I have enabled nativeWindowOpen and nodeIntegrationInSubFrames for main window, and I have a preload script which required a remote from electron. Which this configuration I open a child window and then I close it. The main window render process hangs for ever so I have to restart whole app. Here is and example app https://github.com/zarubond/electron-quick-start/tree/child_hang

• Electron Version:
  6.0.7, 7.0.0-beta3
• Operating System:
  Windows, Linux, MacOS
• Last Known Working Electron version:
  5.0.10

Expected Behavior

Closing child window should not hang main window for ever

Actual Behavior

Main window hangs after closing child window.

To Reproduce

Open child window and then close it

Screenshots

Additional Information

[VishwasShashidhar]

@sofianguy This is actually a blocker for stable (for some of us at least) as pop-outs (child windows) are common use cases.

[NikolayMakhonin]

Details of this error:

(I tested it on v6.0.11)

For reproduce use this code:

win = new BrowserWindow({
	width: 1200,
	height: 700,
	webPreferences: {
		nativeWindowOpen: true,
		nodeIntegration: false,
		sandbox: true,
		preload: require.resolve('./preload'), // required for reproduce the error
	},
	frame: false,
})

win.webContents.openDevTools({
	mode    : 'undocked',
	activate: true,
})

preload.js

// may only contain this line for reproduce the error
const {remote} = require('electron')

Run the app and run in the console:

var win = window.open('about:blank', '', 'width=600,height=400')
win.document.write('Some text')

win.close()

// after this main window will hung, and dev tools will partly not work (you cannot run another console command)

Temporary solution (hack):

var win = window.open('about:blank', '', 'width=600,height=400')

// fix electron bug with close about:blank windows
const winClose = win.close.bind(window)
win.close = () => {
	if (window.minimize) {
		window.minimize()
	}
	window.location.href = 'http://0.0.0.0/' // if you change window url and wait, you can close window without error
	setTimeout(() => winClose(), 2000)
}

[NikolayMakhonin]

Improved solution of bypass the error:

win = new BrowserWindow({
	...
	webPreferences: {
		...
		preload: require.resolve('./preload'),
	},
})

preload.js

function delay(timeMilliseconds) {
	return new Promise(resolve => setTimeout(resolve, timeMilliseconds))
}

const open = window.open.bind(window)
window.open = function (...args) {
	const childWindow = open(...args)

	const allWindows = remote.BrowserWindow.getAllWindows()
	const remoteChildWindow = allWindows[allWindows.length - 1]

	childWindow.close = async () => {
		const href = childWindow.location.href
		childWindow.location.href = 'http://0.0.0.0/'
		remoteChildWindow.hide()
		try {
			while (href === childWindow.location.href) {
				await delay(50)
			}
		} catch {
		}
		remoteChildWindow.close()
	}

	return childWindow
}

[miniak]

I've investigated the issue and found the root cause.

The remote module listens for process.on('exit') to notify remote/server:

electron/lib/renderer/api/remote.js

Lines 20 to 23 in db91673

	process.on('exit', () => {
	const command = 'ELECTRON_BROWSER_CONTEXT_RELEASE'
	ipcRendererInternal.sendSync(command, contextId)
	})

It's emitted when the JS context is being destroyed:

electron/shell/renderer/atom_renderer_client.cc

Lines 154 to 164 in db91673

	void AtomRendererClient::WillReleaseScriptContext(
	v8::Handle<v8::Context> context,
	content::RenderFrame* render_frame) {
	if (injected_frames_.erase(render_frame) == 0)
	return;
	node::Environment* env = node::Environment::GetCurrent(context);
	if (environments_.erase(env) == 0)
	return;
	gin_helper::EmitEvent(env->isolate(), env->process_object(), "exit");

The problem is that ipcRenderer.sendSync() never returns as it's blocked waiting for the reply:

electron/shell/renderer/api/atom_api_renderer_ipc.cc

Lines 191 to 200 in db91673

	base::WaitableEvent response_received_event;
	task_runner_->PostTask(
	FROM_HERE, base::BindOnce(&IPCRenderer::SendMessageSyncOnWorkerThread,
	base::Unretained(this),
	base::Unretained(&response_received_event),
	base::Unretained(&result), internal, channel,
	std::move(message)));
	response_received_event.Wait();
	return result;
	}

@nornagon This regressed when ipcRenderer was refactored to use mojo directly. Could it be because the ElectronBrowser interface is already released and the response callback is never called?
@zcbenz Does ELECTRON_BROWSER_CONTEXT_RELEASE need to be sent synchronously?

[miniak]

This bug is platform independent.

[miniak]

[loc]

@miniak what version are you testing with? I have a fix for 6-0-x in #20547. When I implement a similar fix in 7-0-x/master, I hit a V8 error that I haven't been able to track down yet. Ultimately, the problem is that we allow Chrome to create a WebContents and immediately kill it — the long term fix is an API change. But I'm still hoping to get a short term fix up this week, if possible.

[miniak]

This is a repro on master. But we need to fix it in 6 and 7 as well

[miniak]

@loc when I tried using an async IPC instead, I am not receiving it in the main process, which is expected as the mojo interface is disconnected. Even if we fix the sync version, the problem would be that the rpc-server in main won't get a notification that the context is released and memory would leak

[miniak]

@loc which should be ok since:

electron/lib/renderer/api/remote.js

Lines 16 to 23 in 825e671

	// Notify the main process when current context is going to be released.
	// Note that when the renderer process is destroyed, the message may not be
	// sent, we also listen to the "render-view-deleted" event in the main process
	// to guard that situation.
	process.on('exit', () => {
	const command = 'ELECTRON_BROWSER_CONTEXT_RELEASE'
	ipcRendererInternal.sendSync(command, contextId)
	})

[cynx]

Curious how this is not a blocker for stable build? Unable to upgrade from v5 due to prevalence of this issue v6 onwards

[ckerr]

https://github.com/zarubond/electron-quick-start/tree/child_hang does not reproduce the error in the currently-supported versions of Electron. Tested with

• 7.3.2
• 8.4.0
• 9.1.0
• 10.0.0-beta.11

Unless there's a critical security issue that would merit patching an unsupported version, this won't be fixed in 6. In any case, the better solution would be for all applications still running on Electron 6 to upgrade.

[UstymUkhman]

It does reproduce with version 13.6.9. Maybe even with latest ones too, unfortunately, that's the highest one I can use in my project so I haven't tested above that. But I tested with versions:

• 12.2.3
• 11.5.0
• 10.4.7
• 9.4.4

and the only one where closing a child window works smoothly is the 9.4.4.