test: expand Worker test for non-shared ArrayBuffer by addaleax · Pull Request #30044 · nodejs/node
This test would be broken by V8 7.9 due to the changed `ArrayBuffer` backing store management (the same way that V8 7.8 broke this for `SharedArrayBuffer`s). While working on a solution, it would be good to already have this test in Node.js to avoid unnecessary accidental breakage. Refs: nodejs/node-v8#115
added
v8 engine
labels
Oct 20, 2019
addaleax
added
the
author ready
label
Oct 20, 2019Trott pushed a commit that referenced this pull request
Oct 22, 2019This test would be broken by V8 7.9 due to the changed `ArrayBuffer` backing store management (the same way that V8 7.8 broke this for `SharedArrayBuffer`s). While working on a solution, it would be good to already have this test in Node.js to avoid unnecessary accidental breakage. Refs: nodejs/node-v8#115 PR-URL: #30044 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax
deleted the
test-worker-ab-transfer
branch
MylesBorins pushed a commit that referenced this pull request
Oct 23, 2019This test would be broken by V8 7.9 due to the changed `ArrayBuffer` backing store management (the same way that V8 7.8 broke this for `SharedArrayBuffer`s). While working on a solution, it would be good to already have this test in Node.js to avoid unnecessary accidental breakage. Refs: nodejs/node-v8#115 PR-URL: #30044 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request
Oct 23, 2019This test would be broken by V8 7.9 due to the changed `ArrayBuffer` backing store management (the same way that V8 7.8 broke this for `SharedArrayBuffer`s). While working on a solution, it would be good to already have this test in Node.js to avoid unnecessary accidental breakage. Refs: nodejs/node-v8#115 PR-URL: #30044 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request
Oct 23, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs#30044
pull bot pushed a commit to ksti/v8 that referenced this pull request
Oct 24, 2019Add 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 <ulan@chromium.org> Reviewed-by: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Igor Sheludko <ishell@chromium.org> Cr-Commit-Position: refs/heads/master@{#64542}
targos added a commit to targos/node that referenced this pull request
Oct 24, 2019Original 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 <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64542}
Refs: v8/v8@6b0a953
targos pushed a commit to targos/node that referenced this pull request
Oct 24, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs#30044
targos added a commit to targos/node that referenced this pull request
Oct 24, 2019Original 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 <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64542}
Refs: v8/v8@6b0a953
targos pushed a commit to targos/node that referenced this pull request
Oct 24, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs#30044
targos pushed a commit that referenced this pull request
Oct 25, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: #30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Oct 25, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Nov 1, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
targos pushed a commit that referenced this pull request
Nov 1, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: #30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Nov 2, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Nov 3, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Nov 3, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Nov 4, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
targos pushed a commit that referenced this pull request
Nov 4, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: #30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Nov 5, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
targos pushed a commit that referenced this pull request
Nov 6, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: #30044
nodejs-ci pushed a commit to nodejs/node-v8 that referenced this pull request
Nov 6, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs/node#30044
targos pushed a commit that referenced this pull request
Nov 8, 2019This test would be broken by V8 7.9 due to the changed `ArrayBuffer` backing store management (the same way that V8 7.8 broke this for `SharedArrayBuffer`s). While working on a solution, it would be good to already have this test in Node.js to avoid unnecessary accidental breakage. Refs: nodejs/node-v8#115 PR-URL: #30044 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit to targos/node that referenced this pull request
Nov 8, 2019Original 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 <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64542}
Refs: v8/v8@6b0a953
PR-URL: nodejs#30020
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request
Nov 8, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs#30044 PR-URL: nodejs#30020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request
Nov 10, 2019This test would be broken by V8 7.9 due to the changed `ArrayBuffer` backing store management (the same way that V8 7.8 broke this for `SharedArrayBuffer`s). While working on a solution, it would be good to already have this test in Node.js to avoid unnecessary accidental breakage. Refs: nodejs/node-v8#115 PR-URL: #30044 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request
Nov 11, 2019This test would be broken by V8 7.9 due to the changed `ArrayBuffer` backing store management (the same way that V8 7.8 broke this for `SharedArrayBuffer`s). While working on a solution, it would be good to already have this test in Node.js to avoid unnecessary accidental breakage. Refs: nodejs/node-v8#115 PR-URL: #30044 Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos added a commit to targos/node that referenced this pull request
Nov 17, 2019Original 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 <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64542}
Refs: v8/v8@6b0a953
PR-URL: nodejs#30020
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request
Nov 19, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: nodejs#30044 PR-URL: nodejs#30020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request
Nov 21, 2019Original 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 <ulan@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64542}
Refs: v8/v8@6b0a953
Backport-PR-URL: #30513
PR-URL: #30020
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request
Nov 21, 2019Remove custom tracking for `SharedArrayBuffer`s and their allocators and instead let V8 do the tracking of both. This is required starting in V8 7.9, because lifetime management for `ArrayBuffer::Allocator`s differs from what was performed previously (i.e. it is no longer easily possible for one Isolate to release an `ArrayBuffer` and another to accept it into its own allocator), and the alternative would have been adapting the `SharedArrayBuffer` tracking logic to also apply to regular `ArrayBuffer` instances. Refs: #30044 Backport-PR-URL: #30513 PR-URL: #30020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters