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

Native buf checks #3080

Closed
wants to merge 2 commits into from

Conversation

@trevnorris
Copy link
Contributor

commented Sep 26, 2015

Native Buffer method calls do not require anything from the prototype.
So it is unnecessary to check if the Object's prototype is equal to
Buffer.prototype.

This fixes an issue that prevents Buffer from being inherited the ES5
way. Now the following will work:

function A(n) {
  const b = new Buffer(n);
  Object.setPrototypeOf(b, A.prototype);
  return b;
}

Object.setPrototypeOf(A.prototype, Buffer.prototype);
Object.setPrototypeOf(A, Buffer);

console.log(new A(4));

R=@bnoordhuis
Also, will those native buffer functions need to be re-added for this to land on v4?

@trevnorris trevnorris force-pushed the trevnorris:native-buf-checks branch Sep 26, 2015

@trevnorris

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2015

@rvagg

This comment has been minimized.

Copy link
Member

commented Sep 29, 2015

I'd say semver-minor for this, anyone else?

@trevnorris

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2015

Agreed.

@trevnorris

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2015

@indutny mind giving this a review?

Should I leave in the old function signatures for ABI compatibility? /cc @bnoordhuis

@bnoordhuis

View changes

src/node_buffer.cc Outdated
char* Data(Local<Object> obj) {
CHECK(obj->IsUint8Array());
Local<Uint8Array> ui = obj.As<Uint8Array>();
CHECK(HasInstance(val));

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 6, 2015

Member

I'd use CHECK(val->IsUint8Array()) here like you do below.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

I'd keep the function signatures for now, else it's a semver-major change.

trevnorris added 2 commits Sep 26, 2015
buffer: cleanup usage of __proto__
Prefer using Object.setPrototypeOf() instead.
buffer: Only check if instance is Uint8Array
Native Buffer method calls do not require anything from the prototype.
So it is unnecessary to check if the Object's prototype is equal to
Buffer.prototype.

This fixes an issue that prevents Buffer from being inherited the ES5
way. Now the following will work:

    function A(n) {
      const b = new Buffer(n);
      Object.setPrototypeOf(b, A.prototype);
      return b;
    }

    Object.setPrototypeOf(A.prototype, Buffer.prototype);
    Object.setPrototypeOf(A, Buffer);

    console.log(new A(4));

Fix: #2882

@trevnorris trevnorris force-pushed the trevnorris:native-buf-checks branch to b7eb4f1 Oct 6, 2015

@trevnorris

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2015

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

LGTM but can you do s/Only/only/ in the second commit log for consistency?

EDIT: And while I'm being pedantic, s/cleanup/clean up/.

trevnorris added a commit that referenced this pull request Oct 6, 2015
buffer: clean up usage of __proto__
Prefer using Object.setPrototypeOf() instead.

PR-URL: #3080
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
trevnorris added a commit that referenced this pull request Oct 6, 2015
buffer: only check if instance is Uint8Array
Native Buffer method calls do not require anything from the prototype.
So it is unnecessary to check if the Object's prototype is equal to
Buffer.prototype.

This fixes an issue that prevents Buffer from being inherited the ES5
way. Now the following will work:

    function A(n) {
      const b = new Buffer(n);
      Object.setPrototypeOf(b, A.prototype);
      return b;
    }

    Object.setPrototypeOf(A.prototype, Buffer.prototype);
    Object.setPrototypeOf(A, Buffer);

    console.log(new A(4));

Fix: #2882
PR-URL: #3080
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@trevnorris

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2015

Thanks. Both suggestions taken care of. Landed in 8a685e7 and 05d424c.

@indutny

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

Belated LGTM

@trevnorris trevnorris deleted the trevnorris:native-buf-checks branch Oct 7, 2015

@jasnell jasnell referenced this pull request Oct 8, 2015
29 of 29 tasks complete
trevnorris added a commit that referenced this pull request Oct 8, 2015
buffer: clean up usage of __proto__
Prefer using Object.setPrototypeOf() instead.

PR-URL: #3080
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
trevnorris added a commit that referenced this pull request Oct 8, 2015
buffer: only check if instance is Uint8Array
Native Buffer method calls do not require anything from the prototype.
So it is unnecessary to check if the Object's prototype is equal to
Buffer.prototype.

This fixes an issue that prevents Buffer from being inherited the ES5
way. Now the following will work:

    function A(n) {
      const b = new Buffer(n);
      Object.setPrototypeOf(b, A.prototype);
      return b;
    }

    Object.setPrototypeOf(A.prototype, Buffer.prototype);
    Object.setPrototypeOf(A, Buffer);

    console.log(new A(4));

Fix: #2882
PR-URL: #3080
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@mikemorris

This comment has been minimized.

Copy link

commented Oct 12, 2015

I'm seeing an assertion failure (at https://github.com/nodejs/node/blob/master/src/node_buffer.cc#L151) that I think is related to this change while testing https://github.com/mapbox/mapbox-gl-native against Node.js v4.2.0 (was previously passing on v4.1.2). Compiled using nan 2.1.0, stack trace follows, can try to distill a simpler test case if there isn't a clear fix here.

Assertion failed: ((object)->IsUint8Array()), function WeakCallback, file ../src/node_buffer.cc, line 151.
Process 54799 stopped
* thread #1: tid = 0xdff217, 0x00007fff90e97286 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x00007fff90e97286 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff90e97286 <+10>: jae    0x7fff90e97290            ; <+20>
    0x7fff90e97288 <+12>: movq   %rax, %rdi
    0x7fff90e9728b <+15>: jmp    0x7fff90e92c53            ; cerror_nocancel
    0x7fff90e97290 <+20>: retq
(lldb) bt
* thread #1: tid = 0xdff217, 0x00007fff90e97286 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff90e97286 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff9836b9f9 libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00007fff981059b3 libsystem_c.dylib`abort + 129
    frame #3: 0x00007fff980cda99 libsystem_c.dylib`__assert_rtn + 321
    frame #4: 0x000000010068ac7e node`node::Buffer::CallbackInfo::WeakCallback(v8::Isolate*, v8::Local<v8::Object>) + 238
    frame #5: 0x0000000100315b41 node`v8::internal::GlobalHandles::Node::PostGarbageCollectionProcessing(v8::internal::Isolate*) + 225
    frame #6: 0x0000000100314864 node`v8::internal::GlobalHandles::PostGarbageCollectionProcessing(v8::internal::GarbageCollector) + 148
    frame #7: 0x0000000100338a11 node`v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) + 1505
    frame #8: 0x000000010033813f node`v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, char const*, char const*, v8::GCCallbackFlags) + 703
    frame #9: 0x00000001003377a1 node`v8::internal::Heap::HandleGCRequest() + 177
    frame #10: 0x00000001002ed298 node`v8::internal::StackGuard::HandleInterrupts() + 104
    frame #11: 0x0000000100549b65 node`v8::internal::Runtime_StackGuard(int, v8::internal::Object**, v8::internal::Isolate*) + 53
    frame #12: 0x00000f1461506295
    frame #13: 0x00000f1461826f4b
    frame #14: 0x00000f14616e75c0
    frame #15: 0x00000f146151981d
    frame #16: 0x00000f14616e72cb
    frame #17: 0x00000f1461519494
    frame #18: 0x00000f14616e538a
    frame #19: 0x00000f14616e4ce3
    frame #20: 0x00000f1461519494
    frame #21: 0x00000f14616e4b7e
    frame #22: 0x00000f14616e41e7
    frame #23: 0x00000f14616e378a
    frame #24: 0x00000f1461519f7d
    frame #25: 0x00000f14615189e2
    frame #26: 0x00000001002ebaac node`v8::internal::Invoke(bool, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 732
    frame #27: 0x00000001001665e4 node`v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 276
    frame #28: 0x000000010067c018 node`node::MakeCallback(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*) + 637
    frame #29: 0x00000001006837c8 node`node::CheckImmediate(uv_check_s*) + 98
    frame #30: 0x00000001007aa6b3 node`uv__run_check + 33
    frame #31: 0x00000001007a6369 node`uv_run + 343
    frame #32: 0x00000001006830d0 node`node::Start(int, char**) + 573
    frame #33: 0x0000000100001234 node`start + 52
@indutny

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

Argh... I guess I know what's happening

indutny added a commit to indutny/io.js that referenced this pull request Oct 12, 2015
buffer: fix assertion error in WeakCallback
`CallbackInfo` is now bound to `ArrayBuffer` instance, not `Uint8Array`,
therefore `SPREAD_ARG` will abort with:

    Assertion failed: ((object)->IsUint8Array())

Make changes necessary to migrate it to `ArrayBuffer`.

See: nodejs#3080 (comment)
@indutny

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

@mikemorris I'm terribly sorry for this, it should be fixed by #3329

indutny added a commit that referenced this pull request Oct 13, 2015
buffer: fix assertion error in WeakCallback
`CallbackInfo` is now bound to `ArrayBuffer` instance, not `Uint8Array`,
therefore `SPREAD_ARG` will abort with:

    Assertion failed: ((object)->IsUint8Array())

Make changes necessary to migrate it to `ArrayBuffer`.

See: #3080 (comment)

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #3329
indutny added a commit that referenced this pull request Oct 13, 2015
buffer: fix assertion error in WeakCallback
`CallbackInfo` is now bound to `ArrayBuffer` instance, not `Uint8Array`,
therefore `SPREAD_ARG` will abort with:

    Assertion failed: ((object)->IsUint8Array())

Make changes necessary to migrate it to `ArrayBuffer`.

See: #3080 (comment)

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #3329
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

landed in v4.x-staging as e0fffca...651a5b5

@mikemorris mikemorris referenced this pull request Jan 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.