chore: bump node to v12.18.0 (master)

[electron-bot]

Updating Node.js to v12.18.0.

See all changes in v12.16.3..v12.18.0

Notes: Updated Node.js to v12.18.0.

[deepak1556]

Thanks @codebytere for making the zlib changes, but now that I think more, a couple questions come to my mind.

• How does node track //thrid_party/zlib ? Do they keep updated with ToT ?
• Our zlib roll tracks ToT as it comes with our nightly chromium updates, if this causes any ABI issues do we take responsibility to fix them ?
• What is the size implications when we bundle two copies of zlib, can you measure this ?

I would like to bring this up in the upgrades-wg meeting tomorrow, can you add it to the agenda. Thanks!

[deepak1556]

@jkleinsc why was the sandbox change required for this PR specifically ? Can it be isolated to a separate PR ?

[jkleinsc]

@deepak1556 running with no-sandbox on this PR causes the arm/arm64 tests to fail:
eg: https://github.com/electron/electron/runs/727941638
and https://github.com/electron/electron/runs/727929651.
Those tests do not fail on master.

[deepak1556]

Weird it starts failing on a node update. The change made doesn't enable sandbox, just fixes the permission of chrome-sandbox used by renderers with webPreference sandbox: true. I am not sure how this relates to the failing tests. Also I thought we don't want to add --cap-add SYS_ADMIN to container

/cc @nornagon

[jkleinsc]

Hmm... looking at this comment: https://github.com/electron/electron/blob/master/spec-main/api-app-spec.ts#L1192, I wonder if I can get a docker seccomp profile working with our CI. I think that might be safer than --cap-add SYS_ADMIN

[jkleinsc]

digging into this further it appears that we won't be able to use a docker seccomp profile.

[deepak1556]

Thanks for checking @jkleinsc , but I still don't have a clarity on why these tests started failing suddenly in this PR and why is fixing suid sandbox permissions turned out to be the fix.

[jkleinsc]

@deepak1556 here's what I'm wondering - the main tests that are failing with --no-sandbox are debugger tests and I'm wondering if the --no-sandbox option isn't being properly passed which would then make sense as to why turning it off would work.

[codebytere]

@deepak1556

How does node track //thrid_party/zlib ? Do they keep updated with ToT ?

They don't at present

Our zlib roll tracks ToT as it comes with our nightly chromium updates, if this causes any ABI issues do we take responsibility to fix them ?

I would say no - i doubt it would have nontrivial impact potential but even so i think it's an acceptable downside to minimizing our zlib bundle and staying more up-to-date

What is the size implications when we bundle two copies of zlib, can you measure this ?

I believe this change would reduce binary since, since we're de-duplicating a dependency 🤔

[codebytere]

Seeing a slightly different ARM failure now i'm not quite sure how to approach: https://github.visualstudio.com/electron/_build/results?buildId=72603&view=logs&j=877ace19-74f4-59bc-441c-1ef8c83d8f95&t=f3841320-31a2-57c0-0cc6-7ff521662a00&l=991

[codebytere]

@deepak1556 after some investigation, i think we have no choice but to add -cap-add SYS_ADMIN to the sandbox on ARM. What's happening is that Node.js unflagged their implementation of the esmodule loader, and since we're not in sandbox mode, Node.js is enabled and propagates to every renderer process including devtools windows.

Then, because we have a Node.js environment, what's happening is that the devtools windows uses Node.js' implementation of the esmodule loader, which is at present incapable of loading modules from URLs (for good reason). There's no real way to specifically enable sandbox for devtools only, since at the time we're calling CreateContentRendererClient() we don't know what url we're loading (and therefore can't verify it's devtools or not).

[deepak1556]

@codebytere since #16864 we sandbox devtools renderer by default. I am not sure how node esmloader is leaking into it. Were you able to confirm that the devtool renderer had node environment ?

Also why does this change only affect ARM builds ?

[MarshallOfSound]

@deepak1556 Since that PR sandbox is enabled by default for devtools webContents. However this case we are overriding the default. The arm tests on VSTS run with the ELECTRON_DISABLE_SANDBOX environment variable which forcefully adds the --disable-sandbox flag to the command line of all processes. This will force the rendererclient to use the ElectronRendererClient (the one with node, this is where the esmloader comes from) and it only affects arm builds because they are the only ones that run with sandbox disabled

[deepak1556]

The arm tests on VSTS run with the ELECTRON_DISABLE_SANDBOX environment variable

because of container 🤦 now I understand the problem.

In that case I am good with the proposed solution. Thanks for the context.

[deepak1556]

@codebytere thanks for the context on zlib adoption. I am good with maintaining a single dependency for now, we can revisit the issue incase incompatibility arises.

[deepak1556]

For future reference, bundling two different zlib only increases the size by 121kb unstripped, zlib is a really minimal library with no dependencies.

[deepak1556]

LGTM, Thanks!

[release-clerk]

Release Notes Persisted

Updated Node.js to v12.18.0.