fix: support async child process methods without callback in asar

[alexgreenland]

Description of Change

In Node land, async child process methods do not require a callback to be provided. In Electron land in the packaged asar environment, there's a bug which means if you don't provide a callback the method does not run — it runs synchronously and blocks execution when a long running child process is launched. I saw this with execFile, I had to add a callback to make the child process run.

In development, original Node is used, but in production, Electron's Node (with changes) is used. This means there is a discrepancy between development and production environments and will catch users out, as it did to me. As it is a silent failure, it's quite difficult to spot.

This change no longer branches to call the synchronous method but continues to use the async method as requested and makes this flow work. I added a new asar test and all asar tests pass.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests added
• PR title follows semantic commit guidelines

Release Notes

Notes: support async child process methods without callback in asar

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[nornagon]

This seems reasonable to me! It looks like most of this code was written by @kevinsawicki who's no longer maintaining Electron. Cc @alexeykuzmin for a second review.

[zcbenz]

overrideAPI is also called for fs.open and fs.copyFile, which require the callback to be passed.

As for child_process.execFile, errors should be silently discarded when no callback is passed, currently overrideAPI would try to invoke undefined when path does not exist.

[MarshallOfSound]

In agreement with @zcbenz this will break the invalid usage of async APIs. Perhaps a canBeSync flag passed to overrideAPI might be the solution here

[alexgreenland]

Looking closer, I don't think this has ever worked, the flow from async to sync (overrideAPI -> overrideAPISync).

If you look at the original code:

electron/lib/common/asar.js

Lines 165 to 176 in 90d1c0b

	const overrideAPI = function (module, name, pathArgumentIndex) {
	if (pathArgumentIndex == null) pathArgumentIndex = 0
	const old = module[name]
	module[name] = function () {
	const pathArgument = arguments[pathArgumentIndex]
	const { isAsar, asarPath, filePath } = splitPath(pathArgument)
	if (!isAsar) return old.apply(this, arguments)
	const callback = arguments[arguments.length - 1]
	if (typeof callback !== 'function') {
	return overrideAPISync(module, name, pathArgumentIndex)
	}

When there's no callback overrideAPI invokes overrideAPISync but overrideAPISync merely overrides the API with module[name] = function (), it crucially doesn't run, which it needs to because we're already in our own invocation module[name] = function () from overrideAPI. It also has the side effect of overriding the async method with the sync method for future invocations.

electron/lib/common/asar.js

Lines 146 to 149 in 90d1c0b

	const overrideAPISync = function (module, name, pathArgumentIndex) {
	if (pathArgumentIndex == null) pathArgumentIndex = 0
	const old = module[name]
	module[name] = function () {

I've introduced a more comprehensive change that passes a flag fromAsync from overrideAPI into overrideAPISync when there's no callback. When we detect that we're coming from an async flow, we return the overridden function and overrideAPI invokes it with the original arguments.

My change also means that we don't force the sync method on all further async calls, so if you didn't include a callback when calling once, it will allow you to use a callback in future invocations — there's an at-call check so it doesn't set module[name] in overrideAPISync in this flow.

The new code:

electron/lib/common/asar.js

Lines 146 to 180 in c58517f

	const overrideAPISync = function (module, name, pathArgumentIndex, fromAsync) {
	if (pathArgumentIndex == null) pathArgumentIndex = 0
	const old = module[name]
	const func = function () {
	const pathArgument = arguments[pathArgumentIndex]
	const { isAsar, asarPath, filePath } = splitPath(pathArgument)
	if (!isAsar) return old.apply(this, arguments)
	const archive = getOrCreateArchive(asarPath)
	if (!archive) throw createError(AsarError.INVALID_ARCHIVE, { asarPath })
	const newPath = archive.copyFileOut(filePath)
	if (!newPath) throw createError(AsarError.NOT_FOUND, { asarPath, filePath })
	arguments[pathArgumentIndex] = newPath
	return old.apply(this, arguments)
	}
	if (fromAsync) {
	return func
	}
	module[name] = func
	}
	const overrideAPI = function (module, name, pathArgumentIndex) {
	if (pathArgumentIndex == null) pathArgumentIndex = 0
	const old = module[name]
	module[name] = function () {
	const pathArgument = arguments[pathArgumentIndex]
	const { isAsar, asarPath, filePath } = splitPath(pathArgument)
	if (!isAsar) return old.apply(this, arguments)
	const callback = arguments[arguments.length - 1]
	if (typeof callback !== 'function') {
	return overrideAPISync(module, name, pathArgumentIndex, true).apply(this, arguments)
	}

All original and introduced asar tests pass.

[codebytere]

LGTM with latest changes!

[alexgreenland]

@zcbenz @MarshallOfSound ^^

[zcbenz]

Nice change!

[release-clerk]

Release Notes Persisted

support async child process methods without callback in asar

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉