node
node copied to clipboard
nodejs
lib: the REPL should survive deletion of Array.prototype methods
Specifically, delete Array.prototype.lastIndexOf immediately crashes the REPL, as does deletion of a few other Array prototype methods.
Checklist
- [x]
make -j4 test(UNIX), orvcbuild test(Windows) passes - [ ] tests and/or benchmarks are included
- [ ] documentation is changed or added
- [x] commit message follows commit guidelines
Rebased, and added a bunch of other primordials I found when poking around the repl.
Not sure why the python tests are failing :-/
For the same reason Travis is failing; there are several failing repl tests.
alright, got tests passing :-D this is ready for any additional/final/repeat reviews, and is ready to land whenever it's allowed to!
Can/should we add a test that crashes the current REPL but not the REPL with these changes?
I took the liberty of adding a test. Hope that's OK. (It's in a separate commit so it is easy to rebase out.)
Are these 3 test failures something due to my PR, or are they expected?
It looks like the test fails if run in a worker (tools/test.py --worker path/to/test-file.js). That needs to be addressed here, but the Windows failures are CI and I'm taking some nodes offline temporarily to work around that.
Test failure when run with tools/test.py --worker:
07:51:07 not ok 1665 parallel/test-repl-delete-array-prototype
07:51:07 ---
07:51:07 duration_ms: 0.272
07:51:07 severity: fail
07:51:07 exitcode: 1
07:51:07 stack: |-
07:51:07 > undefined
07:51:07 events.js:298
07:51:07 throw er; // Unhandled 'error' event
07:51:07 ^
07:51:07
07:51:07 Error [ERR_WORKER_UNSERIALIZABLE_ERROR]: Serializing an uncaught exception failed
07:51:07 at Worker.[kOnCouldNotSerializeErr] (internal/worker.js:198:24)
07:51:07 at Worker.[kOnMessage] (internal/worker.js:212:45)
07:51:07 at MessagePort.<anonymous> (internal/worker.js:141:57)
07:51:07 at MessagePort.emit (events.js:321:20)
07:51:07 at MessagePort.onmessage (internal/worker/io.js:78:8)
07:51:07 Emitted 'error' event on Worker instance at:
07:51:07 at Worker.[kOnCouldNotSerializeErr] (internal/worker.js:198:10)
07:51:07 at Worker.[kOnMessage] (internal/worker.js:212:45)
07:51:07 [... lines matching original stack trace ...]
07:51:07 at MessagePort.onmessage (internal/worker/io.js:78:8) {
07:51:07 code: 'ERR_WORKER_UNSERIALIZABLE_ERROR'
07:51:07 }
07:51:07 ...
@addaleax i'm having trouble figuring out how to trace where this error comes from. What code determines how errors are serialized, and where does it live?
@ljharb You may be interested in workerOnGlobalUncaughtException in lib/internal/main/worker_thread.js, and serializeError in lib/internal/error-serdes.js?
Let me know if you’d like me to look into it. The code is supposed to be resilient against prototype deletion, so I’d be very curious to know what happens there…
@addaleax thank you, that is most helpful! Unfortunately, spread syntax (..., as in ...args) is not robust against modification, because it invokes Array.prototype[Symbol.iterator]. I'll update the PR to avoid using that syntax, but that still didn't seem to be fixing the problem - so I kept digging, and it turns out that SafeSet itself is not safe, because it's invoking the iterator protocol on its constructor argument. Rather than try to fix SafeSet, i refactored to avoid passing the constructor argument.
That still didn't fix the problem, though, because I'd previously modified the test to only delete string properties off of Array :-/
That led me to the internal inspect, which is relying on prototype methods, spread syntax, and for..of; I'm working on fixing that now.
I'm at a loss; is there any way I can locally initiate a debugger inside the worker, so i can introspect any errors prior to them being serialized?
ok, i did a bunch of manual retries and figured out a few more methods that weren't robustly being accessed.
There's still at least one call to .push on an array somewhere, so the test currently does not delete that method in order to pass in the worker.
What would be nice is to land this now-quite-large PR, and then I can work on push and the Symbol properties, and perhaps String.prototype properties, in a followup PR?
(a few tests are failing in Travis; I’ll fix those in the next hour or three)
Once tests are passing, that’d be great - there’s only one change i made that comments imply was in a hot path, but that’s one more than i want to make slower.
The performance overhead should be much less after we land V8 8.0: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins
I don’t think it’s true that any other program would necessarily fail in the case of these methods being removed; altho many people don’t code robustly, there are many of us that do. Either way, it’s one thing for a user modifying the env to break their own app; it’s another for it to break the platform itself.
As for the performance overhead, that’s definitely an issue :-/ but my personal philosophy is that correctness always trumps performance.
Where does this go from here? What are possible next steps?
Most of these changes have now landed, thanks (i believe) to @aduh95.
I've rebased this, and I think it should still land. cc @BridgeAR?
