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

buffer: FreeCallback should be tied to ArrayBuffer #3198

Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Oct 6, 2015

FreeCallback should be invoked on the storage disposal (ArrayBuffer),
not when the view (Uint8Array or Buffer) is disposed. This causes
bug and crashes in addons which create buffers and store only slices of
them.

cc @trevnorris @bnoordhuis @nodejs/collaborators

@indutny
Copy link
Member Author

indutny commented Oct 6, 2015

@bnoordhuis I wonder if it may be a good opportunity to write our first C++ test.

@Fishrock123 Fishrock123 added the buffer Issues and PRs related to the buffer subsystem. label Oct 6, 2015
@indutny indutny force-pushed the fix/buffer-wrong-free-callback-handle branch from 21c3543 to 1ce1e17 Compare October 6, 2015 04:30
@indutny
Copy link
Member Author

indutny commented Oct 6, 2015

Nah, gtest didn't work out. It was simpler to just write an test addon.

v8::Isolate* isolate = args.GetIsolate();
v8::HandleScope scope(isolate);
isolate->RequestGarbageCollectionForTesting(
v8::Isolate::kFullGarbageCollection);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, is it viable to use a technique similar to https://groups.google.com/d/msg/v8-users/JmhzmswiqvM/IC6UtV5FEC4J so that --expose-gc doesn't have to be passed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, seems to be a legit use of RequestGC here

@trevnorris
Copy link
Contributor

LGTM. Does CI run these tests?

#include <util.h>
#include <v8.h>

static bool alive = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I suggest you make this a counter and check that it's > 0 in FreeCallback?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@bnoordhuis
Copy link
Member

LGTM

FreeCallback should be invoked on the storage disposal (`ArrayBuffer`),
not when the view (`Uint8Array` or `Buffer`) is disposed. This causes
bug and crashes in addons which create buffers and store only slices of
them.

PR-URL: nodejs#3198
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@indutny indutny force-pushed the fix/buffer-wrong-free-callback-handle branch from 1ce1e17 to 9d4063e Compare October 6, 2015 17:37
@indutny
Copy link
Member Author

indutny commented Oct 6, 2015

All fixed and rebased. Running CI.

@indutny
Copy link
Member Author

indutny commented Oct 6, 2015

@indutny
Copy link
Member Author

indutny commented Oct 6, 2015

Landed in d1f2404, please backport to v4.x

indutny added a commit that referenced this pull request Oct 6, 2015
FreeCallback should be invoked on the storage disposal (`ArrayBuffer`),
not when the view (`Uint8Array` or `Buffer`) is disposed. This causes
bug and crashes in addons which create buffers and store only slices of
them.

PR-URL: #3198
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@indutny indutny closed this Oct 6, 2015
@indutny indutny deleted the fix/buffer-wrong-free-callback-handle branch October 6, 2015 22:58
@jasnell jasnell mentioned this pull request Oct 8, 2015
29 tasks
indutny added a commit that referenced this pull request Oct 8, 2015
FreeCallback should be invoked on the storage disposal (`ArrayBuffer`),
not when the view (`Uint8Array` or `Buffer`) is disposed. This causes
bug and crashes in addons which create buffers and store only slices of
them.

PR-URL: #3198
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins
Copy link
Contributor

landed in v4.x-staging in 660f759

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants