Skip to content

Commit

Permalink
buffer: replace deprecated SetWeak usage
Browse files Browse the repository at this point in the history 
Old style SetWeak is now deprecated, and weakness now works like
phantom references. This means we no longer have a reference to the
object in the weak callback. We use a kInternalFields style weak
callback which provides us with the contents of 2 internal fields
where we can squirrel away the native buffer pointer.

We can no longer neuter the buffer in the weak callback, but that
should be unnecessary as the object is going to be GC'd during the
current gc cycle.

PR-URL: #5204
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: indutny - Fedor Indutny <[email protected]>
  • Loading branch information
@ofrobots
ofrobots authored and Ali Sheikh committed Mar 4, 2016
1 parent 34aac23 commit ebbbc5a
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 11 deletions.
24 changes: 13 additions & 11 deletions src/node_buffer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ using v8::Uint32;
using v8::Uint32Array;
using v8::Uint8Array;
using v8::Value;
using v8::WeakCallbackData;
using v8::WeakCallbackInfo;


class CallbackInfo {
Expand All @@ -83,8 +83,8 @@ class CallbackInfo {
FreeCallback callback,
void* hint = 0);
private:
static void WeakCallback(const WeakCallbackData<ArrayBuffer, CallbackInfo>&);
inline void WeakCallback(Isolate* isolate, Local<ArrayBuffer> object);
static void WeakCallback(const WeakCallbackInfo<CallbackInfo>&);
inline void WeakCallback(Isolate* isolate, char* const data);
inline CallbackInfo(Isolate* isolate,
Local<ArrayBuffer> object,
FreeCallback callback,
Expand Down Expand Up @@ -122,7 +122,10 @@ CallbackInfo::CallbackInfo(Isolate* isolate,
if (object->ByteLength() != 0)
CHECK_NE(data, nullptr);

persistent_.SetWeak(this, WeakCallback);
object->SetAlignedPointerInInternalField(kBufferInternalFieldIndex, data);

This comment has been minimized.

Sign in to view

Copy link
@addaleax

addaleax Mar 16, 2016

Member

@ofrobots Is there any restriction on the alignment of pointers passed to node::Buffer::New? Expecting data to be aligned to a 2-byte boundary breaks node-ffi – and probably other native modules – when using the current Node.js master.

This comment has been minimized.

Sign in to view

Copy link
@ofrobots

ofrobots Mar 16, 2016

Author Contributor

@addaleax Thanks for bringing this to my attention. There is indeed a 2-byte alignment restriction on data. I can try to think about possible solutions. Do you have a concrete test-case you can point me to that demonstrates the problem?

/cc @bnoordhuis.

This comment has been minimized.

Sign in to view

Copy link
@addaleax

addaleax Mar 16, 2016

Member

Well, there is this function in node-ffi that gets called (from here) with (char*)RTLD_NEXT == -1 as data. (Simply cloning + npm it should be enough to reproduce.)

In the addon’s tests there are similar calls, where data can even be a pointer to a C function, i.e. where the linker decides the alignment at will.

This comment has been minimized.

Sign in to view

Copy link
@addaleax

addaleax Mar 16, 2016

Member

@ofrobots Is this alignment restriction documented somewhere? I have never come across any explicit statement of that kind in writing C++ addons, and I think this should be mentioned (at least) in the description of Nan::NewBuffer.

This comment has been minimized.

Sign in to view

Copy link
@ofrobots

ofrobots Mar 17, 2016

Author Contributor

I am hoping I can eliminate the alignment restriction on data. Stay tuned.

This comment has been minimized.

Sign in to view

Copy link
@ofrobots

ofrobots Mar 17, 2016

Author Contributor

@addaleax I hacked together a fix here: https://github.com/ofrobots/node/tree/fix-buffer-alignment-restriction. Could you try this and let me know if this works?

This comment has been minimized.

Sign in to view

Copy link
@addaleax

addaleax Mar 17, 2016

Member

@ofrobots Yep, all tests passing – Thanks!

This comment has been minimized.

Sign in to view

Copy link
@addaleax

addaleax Mar 17, 2016

Member

lgtm in general, let me know when you create the PR :)

This comment has been minimized.

Sign in to view

Copy link
@ofrobots

ofrobots Mar 17, 2016

Author Contributor

#5752

persistent_.SetWeak(this, WeakCallback,
v8::WeakCallbackType::kInternalFields);
persistent_.SetWrapperClassId(BUFFER_ID);
persistent_.MarkIndependent();
isolate->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
Expand All @@ -135,16 +138,15 @@ CallbackInfo::~CallbackInfo() {


void CallbackInfo::WeakCallback(
const WeakCallbackData<ArrayBuffer, CallbackInfo>& data) {
data.GetParameter()->WeakCallback(data.GetIsolate(), data.GetValue());
const WeakCallbackInfo<CallbackInfo>& data) {
data.GetParameter()->WeakCallback(
data.GetIsolate(),
static_cast<char*>(data.GetInternalField(kBufferInternalFieldIndex)));
}


void CallbackInfo::WeakCallback(Isolate* isolate, Local<ArrayBuffer> buf) {
ArrayBuffer::Contents obj_c = buf->GetContents();
char* const obj_data = static_cast<char*>(obj_c.Data());
buf->Neuter();
callback_(obj_data, hint_);
void CallbackInfo::WeakCallback(Isolate* isolate, char* const data) {
callback_(data, hint_);
int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));
isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);

Expand Down
4 changes: 4 additions & 0 deletions src/node_buffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ namespace Buffer {
static const unsigned int kMaxLength =
sizeof(int32_t) == sizeof(intptr_t) ? 0x3fffffff : 0x7fffffff;

// Buffers have two internal fields, the first of which is reserved for use by
// Node.
static const unsigned int kBufferInternalFieldIndex = 0;

NODE_EXTERN typedef void (*FreeCallback)(char* data, void* hint);

NODE_EXTERN bool HasInstance(v8::Local<v8::Value> val);
Expand Down

0 comments on commit ebbbc5a

Please sign in to comment.