Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: expand Worker test for non-shared ArrayBuffer #30044

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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
SharedArrayBuffers). 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


Fyi @targos @titzer

There would be two ways to approach fixing this – either we make V8 more aware of the lifetime management of ArrayBuffer::Allocators by holding a shared_ptr to them itself from the backing stores and get rid of the mechanism we use for tracking SharedArrayBuffer lifetime manually in Node.js, or we expand that mechanism to also account for non-shared ArrayBuffers. Both solutions would require a non-trivial amount of work, although they should be somewhat straightforward.

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

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
@addaleax addaleax added v8 engine Issues and PRs related to the V8 dependency. test Issues and PRs related to the tests. worker Issues and PRs related to Worker support. labels Oct 20, 2019
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 20, 2019
Trott pushed a commit that referenced this pull request Oct 22, 2019
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

PR-URL: #30044
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 22, 2019

Landed in 4201cdd

@Trott Trott closed this Oct 22, 2019
@addaleax addaleax deleted the test-worker-ab-transfer branch October 22, 2019 19:17
@addaleax
Copy link
Member Author

addaleax commented Oct 22, 2019

As for resolving this issue in V8 7.9, I’ve opened https://chromium-review.googlesource.com/c/v8/v8/+/1874347 and we’ll see where that goes. If it lands as-is, addaleax/node@a3bc60f would be one way to fix this in Node.js – by removing a lot of code. :)

EDIT: addaleax/node@7cd9ddc

MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
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

PR-URL: #30044
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
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

PR-URL: #30044
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
addaleax added a commit to addaleax/node that referenced this pull request Oct 23, 2019
Remove 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, 2019
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}
targos added a commit to targos/node that referenced this pull request Oct 24, 2019
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
targos pushed a commit to targos/node that referenced this pull request Oct 24, 2019
Remove 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, 2019
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
targos pushed a commit to targos/node that referenced this pull request Oct 24, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
Remove 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, 2019
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

PR-URL: #30044
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos added a commit to targos/node that referenced this pull request Nov 8, 2019
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]>
targos pushed a commit to targos/node that referenced this pull request Nov 8, 2019
Remove 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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
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

PR-URL: #30044
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2019
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

PR-URL: #30044
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos added a commit to targos/node that referenced this pull request Nov 17, 2019
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]>
targos pushed a commit to targos/node that referenced this pull request Nov 19, 2019
Remove 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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 21, 2019
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]>
MylesBorins pushed a commit that referenced this pull request Nov 21, 2019
Remove 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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants