worker: keep allocators for transferred SAB instances alive longer #29637

addaleax · 2019-09-20T22:02:44Z

Keep the ArrayBuffer::Allocator behind a SharedArrayBuffer instance
alive for at least as long as the receiving Isolate lives, if the
SharedArrayBuffer instance isn’t already destroyed through GC.

This is to work around the fact that V8 7.9 started refactoring
how backing stores for SharedArrayBuffer instances work, changing
the timing of the call that releases the backing store to be
during Isolate disposal.

The flag added to the test is optional but helps verify that the
backing store is actually free’d at the end of the test and does not
leak memory.

Fixes: nodejs/node-v8#115

Checklist

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

Keep the `ArrayBuffer::Allocator` behind a `SharedArrayBuffer` instance alive for at least as long as the receiving Isolate lives, if the `SharedArrayBuffer` instance isn’t already destroyed through GC. This is to work around the fact that V8 7.9 started refactoring how backing stores for `SharedArrayBuffer` instances work, changing the timing of the call that releases the backing store to be during Isolate disposal. The flag added to the test is optional but helps verify that the backing store is actually free’d at the end of the test and does not leak memory. Fixes: nodejs/node-v8#115

nodejs-github-bot · 2019-09-20T22:27:43Z

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

Trott · 2019-09-24T05:56:11Z

src/sharedarraybuffer_metadata.cc

+    //
+    // This is a workaround for https://github.com/nodejs/node-v8/issues/115
+    // (introduced in V8 7.9) and we should be able to remove it once V8
+    // ArrayBuffer::Allocator refactoring/removal is complete.


Shout out to long and helpful comments!

bnoordhuis · 2019-09-24T16:23:21Z

src/env.cc

  return new_data;
 }

+void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(


Suggested change

void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(

void Environment::ThisMethodNameIsSoLongItMakesMyEyeballsBleedIDontEven(

I mean, I’m happy to take a more concise suggestion, but given the very specific use case I’m fine with a long name.

src/sharedarraybuffer_metadata.h

…nger

nodejs-github-bot · 2019-09-24T20:40:44Z

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

Trott · 2019-09-25T04:36:05Z

Landed in 7899a96

Keep the `ArrayBuffer::Allocator` behind a `SharedArrayBuffer` instance alive for at least as long as the receiving Isolate lives, if the `SharedArrayBuffer` instance isn’t already destroyed through GC. This is to work around the fact that V8 7.9 started refactoring how backing stores for `SharedArrayBuffer` instances work, changing the timing of the call that releases the backing store to be during Isolate disposal. The flag added to the test is optional but helps verify that the backing store is actually free’d at the end of the test and does not leak memory. Fixes: nodejs/node-v8#115 PR-URL: #29637 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Add an `array_buffer_allocator_shared` field to the `Isolate::CreateParams` struct that allows embedders to share ownership of the ArrayBuffer::Allocator with V8, and which in particular means that when this method is used that the BackingStore deleter will not perform an use-after-free access to the Allocator under certain circumstances. For Background: tl;dr: This is necessary for Node.js to perform the transition to V8 7.9, because of the way that ArrayBuffer::Allocators and their lifetimes currently work there. In Node.js, each Worker thread has its own ArrayBuffer::Allocator. Changing that would currently be impractical, as each allocator depends on per-Isolate state. However, now that backing stores are managed globally and keep a pointer to the original ArrayBuffer::Allocator, this means that when transferring an ArrayBuffer (e.g. from one Worker to another through postMessage()), the original Allocator has to be kept alive until the ArrayBuffer no longer exists in the receiving Isolate (or until that Isolate is disposed). See [1] for an example Node.js test that fails with V8 7.9. This problem also existed for SharedArrayBuffers, where Node.js was broken by V8 earlier for the same reasons (see [2] for the bug report on that and [3] for the resolution in Node.js). For SharedArrayBuffers, we already had extensive tracking logic, so adding a shared_ptr to keep alive the ArrayBuffer::Allocator was not a significant amount of work. However, the mechanism for transferring non-shared ArrayBuffers is quite different, and it seems both easier for us and better for V8 from an API standpoint to keep the Allocator alive from where it is being referenced. By sharing memory with the custom deleter function/data pair, this comes at no memory overhead. [1]: nodejs/node#30044 [2]: nodejs/node-v8#115 [3]: nodejs/node#29637 Bug: v8:9380 Change-Id: Ibc2c4fb6341b53653cbd637bd8cb3d4ac43809c7 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1874347 Commit-Queue: Ulan Degenbaev <[email protected]> Reviewed-by: Ulan Degenbaev <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{#64542}

Original commit message: [api] Add possibility for BackingStore to keep Allocator alive Add an `array_buffer_allocator_shared` field to the `Isolate::CreateParams` struct that allows embedders to share ownership of the ArrayBuffer::Allocator with V8, and which in particular means that when this method is used that the BackingStore deleter will not perform an use-after-free access to the Allocator under certain circumstances. For Background: tl;dr: This is necessary for Node.js to perform the transition to V8 7.9, because of the way that ArrayBuffer::Allocators and their lifetimes currently work there. In Node.js, each Worker thread has its own ArrayBuffer::Allocator. Changing that would currently be impractical, as each allocator depends on per-Isolate state. However, now that backing stores are managed globally and keep a pointer to the original ArrayBuffer::Allocator, this means that when transferring an ArrayBuffer (e.g. from one Worker to another through postMessage()), the original Allocator has to be kept alive until the ArrayBuffer no longer exists in the receiving Isolate (or until that Isolate is disposed). See [1] for an example Node.js test that fails with V8 7.9. This problem also existed for SharedArrayBuffers, where Node.js was broken by V8 earlier for the same reasons (see [2] for the bug report on that and [3] for the resolution in Node.js). For SharedArrayBuffers, we already had extensive tracking logic, so adding a shared_ptr to keep alive the ArrayBuffer::Allocator was not a significant amount of work. However, the mechanism for transferring non-shared ArrayBuffers is quite different, and it seems both easier for us and better for V8 from an API standpoint to keep the Allocator alive from where it is being referenced. By sharing memory with the custom deleter function/data pair, this comes at no memory overhead. [1]: nodejs#30044 [2]: nodejs/node-v8#115 [3]: nodejs#29637 Bug: v8:9380 Change-Id: Ibc2c4fb6341b53653cbd637bd8cb3d4ac43809c7 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1874347 Commit-Queue: Ulan Degenbaev <[email protected]> Reviewed-by: Ulan Degenbaev <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{#64542} Refs: v8/v8@6b0a953

Original commit message: [api] Add possibility for BackingStore to keep Allocator alive Add an `array_buffer_allocator_shared` field to the `Isolate::CreateParams` struct that allows embedders to share ownership of the ArrayBuffer::Allocator with V8, and which in particular means that when this method is used that the BackingStore deleter will not perform an use-after-free access to the Allocator under certain circumstances. For Background: tl;dr: This is necessary for Node.js to perform the transition to V8 7.9, because of the way that ArrayBuffer::Allocators and their lifetimes currently work there. In Node.js, each Worker thread has its own ArrayBuffer::Allocator. Changing that would currently be impractical, as each allocator depends on per-Isolate state. However, now that backing stores are managed globally and keep a pointer to the original ArrayBuffer::Allocator, this means that when transferring an ArrayBuffer (e.g. from one Worker to another through postMessage()), the original Allocator has to be kept alive until the ArrayBuffer no longer exists in the receiving Isolate (or until that Isolate is disposed). See [1] for an example Node.js test that fails with V8 7.9. This problem also existed for SharedArrayBuffers, where Node.js was broken by V8 earlier for the same reasons (see [2] for the bug report on that and [3] for the resolution in Node.js). For SharedArrayBuffers, we already had extensive tracking logic, so adding a shared_ptr to keep alive the ArrayBuffer::Allocator was not a significant amount of work. However, the mechanism for transferring non-shared ArrayBuffers is quite different, and it seems both easier for us and better for V8 from an API standpoint to keep the Allocator alive from where it is being referenced. By sharing memory with the custom deleter function/data pair, this comes at no memory overhead. [1]: nodejs#30044 [2]: nodejs/node-v8#115 [3]: nodejs#29637 Bug: v8:9380 Change-Id: Ibc2c4fb6341b53653cbd637bd8cb3d4ac43809c7 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1874347 Commit-Queue: Ulan Degenbaev <[email protected]> Reviewed-by: Ulan Degenbaev <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{#64542} Refs: v8/v8@6b0a953 PR-URL: nodejs#30020 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

Original commit message: [api] Add possibility for BackingStore to keep Allocator alive Add an `array_buffer_allocator_shared` field to the `Isolate::CreateParams` struct that allows embedders to share ownership of the ArrayBuffer::Allocator with V8, and which in particular means that when this method is used that the BackingStore deleter will not perform an use-after-free access to the Allocator under certain circumstances. For Background: tl;dr: This is necessary for Node.js to perform the transition to V8 7.9, because of the way that ArrayBuffer::Allocators and their lifetimes currently work there. In Node.js, each Worker thread has its own ArrayBuffer::Allocator. Changing that would currently be impractical, as each allocator depends on per-Isolate state. However, now that backing stores are managed globally and keep a pointer to the original ArrayBuffer::Allocator, this means that when transferring an ArrayBuffer (e.g. from one Worker to another through postMessage()), the original Allocator has to be kept alive until the ArrayBuffer no longer exists in the receiving Isolate (or until that Isolate is disposed). See [1] for an example Node.js test that fails with V8 7.9. This problem also existed for SharedArrayBuffers, where Node.js was broken by V8 earlier for the same reasons (see [2] for the bug report on that and [3] for the resolution in Node.js). For SharedArrayBuffers, we already had extensive tracking logic, so adding a shared_ptr to keep alive the ArrayBuffer::Allocator was not a significant amount of work. However, the mechanism for transferring non-shared ArrayBuffers is quite different, and it seems both easier for us and better for V8 from an API standpoint to keep the Allocator alive from where it is being referenced. By sharing memory with the custom deleter function/data pair, this comes at no memory overhead. [1]: #30044 [2]: nodejs/node-v8#115 [3]: #29637 Bug: v8:9380 Change-Id: Ibc2c4fb6341b53653cbd637bd8cb3d4ac43809c7 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1874347 Commit-Queue: Ulan Degenbaev <[email protected]> Reviewed-by: Ulan Degenbaev <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Cr-Commit-Position: refs/heads/master@{#64542} Refs: v8/v8@6b0a953 Backport-PR-URL: #30513 PR-URL: #30020 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 20, 2019

addaleax mentioned this pull request Sep 20, 2019

Segmentation fault in worker test nodejs/node-v8#115

Closed

addaleax added dont-land-on-v10.x v8 engine Issues and PRs related to the V8 dependency. worker Issues and PRs related to Worker support. labels Sep 20, 2019

Trott approved these changes Sep 24, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 24, 2019

Trott reviewed Sep 24, 2019

View reviewed changes

targos approved these changes Sep 24, 2019

View reviewed changes

bnoordhuis approved these changes Sep 24, 2019

View reviewed changes

fixup! worker: keep allocators for transferred SAB instances alive lo…

e762ec7

…nger

Trott closed this Sep 25, 2019

addaleax deleted the worker-sab-isolate-dispose branch September 25, 2019 10:10

targos mentioned this pull request Oct 1, 2019

v12.11.1 release proposal #29796

Merged

beevelop mentioned this pull request Oct 1, 2019

2019-10-01 Version 12.11.1 (Current) @targos beevelop/docker-nodejs#304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Uh oh!

worker: keep allocators for transferred SAB instances alive longer #29637

worker: keep allocators for transferred SAB instances alive longer #29637

Uh oh!

addaleax commented Sep 20, 2019

Uh oh!

nodejs-github-bot commented Sep 20, 2019

Uh oh!

Trott Sep 24, 2019

Uh oh!

bnoordhuis Sep 24, 2019

Uh oh!

addaleax Sep 24, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2019

Uh oh!

Trott commented Sep 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
	void Environment::ThisMethodNameIsSoLongItMakesMyEyeballsBleedIDontEven(

Uh oh!

Uh oh!

worker: keep allocators for transferred SAB instances alive longer #29637

worker: keep allocators for transferred SAB instances alive longer #29637

Uh oh!

Conversation

addaleax commented Sep 20, 2019

Checklist

Uh oh!

nodejs-github-bot commented Sep 20, 2019

Uh oh!

Trott Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 24, 2019

Uh oh!

Trott commented Sep 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants