worker: fix crash when SharedArrayBuffer outlives creating thread

[addaleax]

Use the parent thread’s ArrayBuffer::Allocator when creating a
Worker instance, as that allocator is guaranteed to outlive the
Worker itself.

Fixes: #28777
Fixes: #28773

Checklist

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

[nodejs-github-bot]

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

[Trott]

The two failures on other worker tests in CI might be related?

[addaleax]

@Trott I don’t know about test-worker-memory – I doubt it – but the other one is almost certainly unrelated, I’m looking into it right now 👍

[addaleax]

Oh, yes, the test-worker-console-listeners failure is related – it happens because after this patch, all threads share a single zero_fill_field for the array buffer allocator, and so it can happen that an ArrayBuffer is not zero filled when it should be, because a different thread does want uninitialized memory.

I’ll figure out something for that.

[addaleax]

I’ve fixed that issue by now by using a thread_local but I’m not really happy with that as a solution … I’ll think about it a bit more. My ideal solution would be to turn the zero-fill field into an atomic counter and increasing/decreasing that from the threads, but I’m having a hard time thinking of a way to make it atomically accessible both from JS and C++.

[Trott]

I’ve fixed that issue by now by using a thread_local but I’m not really happy with that as a solution … I’ll think about it a bit more. My ideal solution would be to turn the zero-fill field into an atomic counter and increasing/decreasing that from the threads, but I’m having a hard time thinking of a way to make it atomically accessible both from JS and C++.

Good to land this in the meantime or you'd like to think on this bit some more?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[addaleax]

@Trott I strongly dislike this solution, and I’m still thinking about it … but haven’t been able to come up with somebody better so far. And hopefully, in the worst case we can get rid of it anyway when V8 removes ArrayBuffer::Allocator altogether in the future…

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

AIX failure in CI is genuine?

[addaleax]

Hm … I’m assuming that it’s because of the thread_local definition here, but I wouldn’t know how to fix that. I’d ping the platform team but maybe that’s just another good reason to throw this solution away…

[addaleax]

Sorry it took me so long (the HTTP/2 security stuff got preference treatment here), but I came up with a solution that isn’t ideal either but definitely better than this and which should pass CI (→ #29190). I’ll close this PR as the code changes are completely different from the ones in this one.