buffer: move process.binding('buffer') to internalBinding

[starkwang]

Refs: #22160

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

@starkwang build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/579/pipeline

[starkwang]

node/test/parallel/test-buffer-bindingobj-no-zerofill.js

Lines 29 to 34 in 3814534

	// Load from file system because internal buffer is already loaded and we're
	// testing code that runs on first load only.
	// Do not move this require() to top of file. It is important that
	// `require('internal/buffer').setupBufferJS` be monkey-patched before this
	// runs.
	const monkeyPatchedBuffer = require('../../lib/buffer');

The tests is failed now because the monkeyPatchedBuffer is loaded from file system and internal/bootstrap/loaders cannot be required in that way.

out/Release/node --expose-internals /Users/apple/Desktop/node/test/parallel/test-buffer-bindingobj-no-zerofill.js
internal/modules/cjs/loader.js:583
    throw err;
    ^

Error: Cannot find module 'internal/bootstrap/loaders'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.Module._load (internal/modules/cjs/loader.js:507:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/apple/Desktop/node/lib/buffer.js:24:29)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)

How can we solve it?

[jasnell]

This is going to be a tricky one because the way that test is written, there is no other way to test that particular bit. One question is: do we actually need to test that particular case?

[BridgeAR]

I am +1 on removing that part from the test.

[jdalton]

@starkwang I believe this should be added to the allow list at here.

[Trott]

This is going to be a tricky one because the way that test is written, there is no other way to test that particular bit. One question is: do we actually need to test that particular case?

Ideally, that entire test file should be replaced by an addon test. See conversation at 27e84ddd4e1#r19182129 and #11706.

The test was written to cover the || [0] part of this code from lib/buffer.js:

// |zeroFill| can be undefined when running inside an isolate where we
// do not own the ArrayBuffer allocator.  Zero fill is always on in that case.
const zeroFill = bindingObj.zeroFill || [0];

If you can come up with an addon test that makes sure that code is run in the appropriate condition, then this rather inelegant way to test that bit of code can be removed.

[Trott]

Moreover, that assertion was added by @bengl in 8172f45. It seems to me like it is inessential and can be removed, but he may believe otherwise?

[BridgeAR]

This needs a rebase.

[starkwang]

I've removed the tricky test-buffer-bindingobj-no-zerofill.js. We can follow up it in #22547.

[starkwang]

CI: https://ci.nodejs.org/job/node-test-pull-request/17098/

[starkwang]

CI: https://ci.nodejs.org/job/node-test-pull-request/17734/

[starkwang]

@nodejs/buffer PTAL

[starkwang]

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/17861/

[starkwang]

This change is semver-major so it needs another member in TSC to approve.

@nodejs/tsc PTAL.

[mcollina]

LGTM

[starkwang]

Landed in ac23e65