Freezing EventTarget.prototype breaks MessageChannel

[corrideat]

Version

v19.9.0

Platform

Linux WORKSTATION 5.15.90.1-microsoft-standard-WSL2 #1 SMP Fri Jan 27 02:56:13 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

node:internal/worker/io:305:30

What steps will reproduce the bug?

Run the following snippet:

Object.freeze(EventTarget.prototype);
new MessageChannel();

How often does it reproduce? Is there a required condition?

It always reproduces

What is the expected behavior? Why is that the expected behavior?

Freezing prototypes should not affect the behaviour of native methods. In particular, user code should not interfere with internals.

What do you see instead?

node:internal/worker/io:305
  eventEmitter[kNewListener] = function(size, type, ...args) {
                             ^

TypeError: Cannot assign to read only property 'Symbol(kNewListener)' of object '#<MessagePort>'
    at setupPortReferencing (node:internal/worker/io:305:30)
    at MessagePort.oninit (node:internal/worker/io:232:3)
    at Object.<anonymous> (/tmp/tmp.ddwmESs9uN.js:2:1)
    at Module._compile (node:internal/modules/cjs/loader:1275:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1329:10)
    at Module.load (node:internal/modules/cjs/loader:1133:32)
    at Module._load (node:internal/modules/cjs/loader:972:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47

Additional information

No response

[bnoordhuis]

user code should not interfere with internals

That's something node tries to do when it's reasonable but not at all costs. Trying to work around a tampered EventEmitter prototype likely impacts performance and is not a good trade-off.

Clobbering EventEmitter is setting yourself up for failure anyway. MessageChannel almost certainly isn't the only thing that's affected.

[benjamingr]

@addaleax thoughts?

[corrideat]

Clobbering EventEmitter is setting yourself up for failure anyway. MessageChannel almost certainly isn't the only thing that's affected.

That's probably right, although in this case at least it does seem like an issue of leaking implementation internals into userspace.

Looking at the code where it fails, it seems fixable by using Object.defineProperty instead of assignment (see the 'override mistake'). Note: I haven't tested whether using Object.defineProperty would fix this.

If this theory is correct, the fix would be replacing

eventEmitter[kNewListener] = function(size, type, ...args) { (...) by ObjectDefineProperty(eventEmitter, kNewListener, function(size, type, ...args) { (...), although the same needs to be done for kRemoveListener as well as kCurrentlyReceivingPorts. A more general fix but significantly larger change would be not storing these in the same object that is user-facing.

Freezing the ports themselves also breaks things (in a different place) due to the same issue of modifying the underlying object from the implementation. Although I'd prefer that not to be the case either, I realise that that'd a much larger fix involving how functionality is implemented in Node, some of which may even be relied on by existing applications.

However, a stronger argument is perhaps that freezing prototypes, in general, and in this particular case, doesn't suddenly break the runtime unexpectedly in other platforms, such as browsers or Deno (since EventTarget is part of the DOM).

[benjamingr]

@corrideat see https://www.youtube.com/watch?v=SOPC3aLoD4U for context