Io.js 3.0 support

[unbornchikken]

So, I've made tweaks based on @kkoopa's suggestions in #33. Also replaced explicit NewBuffer calls with the WrapBuffer method by @kkoopa at ffi's https://github.com/node-ffi/node-ffi/pull/221.

Unfortunatelly it still segafults. I know where, but I dunno why. It segfaults during a GC call when there is a living Buffer instances in the memory created by WrapBuffer, for eample there: https://github.com/unbornchikken/ref/blob/io.js-3.0-support/src/binding.cc#L256 By my understanding it looks like Buffer tries to free its pointer despite there is a free callback that is an empty function. @kkoopa, is some additional magic required to tell to a Buffer that there is no need to free its pointer? Or there is some other issue that we're facing here?

[unbornchikken]

About https://github.com/unbornchikken/ref/commit/b8a93f9db835dcafe42910e4237f10f239770c2a: restored original object handling code. Sorry @kkoopa, it works that way, and I really don't have time to decypher why. :) @TooTallNate knows.

[kkoopa]

The buffer should not try to free the underlying memory when an empty FreeCallback is given. https://github.com/nodejs/node/blob/master/src/node_buffer.cc#L326-L337

[unbornchikken]

It is not possible that it tries to free the memory when Buffer's length is non zero despite there is a callback? Somehow Buffer internal callback throws a null ref exception when those "wrapped" pointers GCed and the memory gets corrupted.

[kkoopa]

cc @trevnorris

On August 18, 2015 11:32:35 PM EEST, "Gábor Mező" notifications@github.com wrote:

It is not possible that it tries to free the memory when Buffer's length is non zero despite there is a callback? Somehow Buffer internal callback throws a null ref exception when those "wrapped" pointers GCed and the memory gets corrupted.

---

Reply to this email directly or view it on GitHub: https://github.com/TooTallNate/ref/pull/35#issuecomment-132342051

[unbornchikken]

@kkoopa, @trevnorris sorry for guessing there, I didn't have too mutch time for investigating this. Later today I'm gonna build an io.js 3.0 from source and do a bit of debugging, and I can provide you the stack trace and maybe some more clues on this.

[trevnorris]

If a callback is passed then node does nothing for memory management. It should be explicitly free'd by the user. And that is some strange black magic going on. Using a Buffer to store Persistent objects.

[unbornchikken]

Hi, @trevnorris , thanks for feedback. Please do not concentrate on that Buffer to Persistent "black magic" code for now, because the actual issue is not originating from there, because if I skip its tests, then mocha still crashes with segfault.

So I did a bit debugging tonight, and the situation is the following:

We create a buffer like:

var buff = new Buffer("putypurutty");

Then we create an other Buffer holding a pointer to the JS side crated Buffer's char* value, see there:

https://github.com/unbornchikken/ref/blob/io.js-3.0-support/src/binding.cc#L249

By calling our WrapPointer method:

https://github.com/unbornchikken/ref/blob/io.js-3.0-support/src/binding.cc#L145

So we're end up having two buffers holding the same char, and the owner of this pointer that should free it is the JS side created, the other one just hold it. The problem is, if a GC invoked when we're having this two instances in the memory, iojs 3.0 are gonna crash for good. It looks like the new Buffer implementation somehow touches its stuff behind that char during GC, and it crashes because the original Buffer's destructor already freed that. Older versions are not affected.

There is the stack trace:

>   iojs.exe!v8::internal::Map::instance_type() Line 4557   C++
    iojs.exe!v8::internal::ShortCircuitConsString(v8::internal::Object * * p) Line 1288 C++
    iojs.exe!v8::internal::MarkCompactMarkingVisitor::MarkObjectByPointer(v8::internal::MarkCompactCollector * collector, v8::internal::Object * * anchor_slot, v8::internal::Object * * p) Line 1364   C++
    iojs.exe!v8::internal::MarkCompactMarkingVisitor::VisitPointers(v8::internal::Heap * heap, v8::internal::Object * * start, v8::internal::Object * * end) Line 1340  C++
    iojs.exe!v8::internal::StaticMarkingVisitor<v8::internal::MarkCompactMarkingVisitor>::VisitJSArrayBuffer(v8::internal::Map * map, v8::internal::HeapObject * object) Line 538   C++
    iojs.exe!v8::internal::StaticMarkingVisitor<v8::internal::MarkCompactMarkingVisitor>::IterateBody(v8::internal::Map * map, v8::internal::HeapObject * obj) Line 405 C++
    iojs.exe!v8::internal::MarkCompactCollector::EmptyMarkingDeque() Line 2113  C++
    iojs.exe!v8::internal::RootMarkingVisitor::MarkObjectByPointer(v8::internal::Object * * p) Line 1763    C++
    iojs.exe!v8::internal::RootMarkingVisitor::VisitPointer(v8::internal::Object * * p) Line 1732   C++
    iojs.exe!v8::internal::GlobalHandles::IterateWeakRoots(v8::internal::ObjectVisitor * v) Line 608    C++
    iojs.exe!v8::internal::MarkCompactCollector::MarkLiveObjects() Line 2381    C++
    iojs.exe!v8::internal::MarkCompactCollector::CollectGarbage() Line 339  C++
    iojs.exe!v8::internal::Heap::MarkCompact() Line 1300    C++
    iojs.exe!v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) Line 1185  C++
    iojs.exe!v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector collector, const char * gc_reason, const char * collector_reason, const v8::GCCallbackFlags gc_callback_flags) Line 911  C++
    iojs.exe!v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace space, const char * gc_reason, const v8::GCCallbackFlags callbackFlags) Line 536  C++
    iojs.exe!v8::internal::Heap::CollectAllGarbage(int flags, const char * gc_reason, const v8::GCCallbackFlags gc_callback_flags) Line 796 C++
    iojs.exe!v8::Isolate::RequestGarbageCollectionForTesting(v8::Isolate::GarbageCollectionType type) Line 6747 C++
    iojs.exe!v8::internal::GCExtension::GC(const v8::FunctionCallbackInfo<v8::Value> & args) Line 24    C++
    iojs.exe!v8::internal::FunctionCallbackArguments::Call(void (const v8::FunctionCallbackInfo<v8::Value> &) * f) Line 34  C++
    iojs.exe!v8::internal::HandleApiCallHelper<0>(v8::internal::Isolate * isolate, v8::internal::`anonymous-namespace'::BuiltinArguments<1> & args) Line 1110   C++
    iojs.exe!v8::internal::Builtin_Impl_HandleApiCall(v8::internal::`anonymous-namespace'::BuiltinArguments<1> args, v8::internal::Isolate * isolate) Line 1133 C++
    iojs.exe!v8::internal::Builtin_HandleApiCall(int args_length, v8::internal::Object * * args_object, v8::internal::Isolate * isolate) Line 1128  C++

[unbornchikken]

I gues that VisitPointers method is about freeing custom memory blocks contained by Buffers, right?

[trevnorris]

How was the Buffer passed to ReadPointer created?

[unbornchikken]

Yeah, I'm thinking about that too. I'm just creating a PR there, so dunno, but I'm gonna take a look at that tomorrow.

[unbornchikken]

Read/WritePointer's target is created by calling:

new Buffer(sizeOfPointer)

So it has enough bytes those capable to hold a pointer to data that the original instance is having. I cannot see there anything extraordinary, that module should work as is and as with older runtimes.

[unbornchikken]

@trevnorris to sum it up, the issue by my understanding is that we have two separate Buffers pointing to exactly the same memory location, and one of the Buffers is the owner who should free the memory, and the other is just referencing that memory (created with WrapPointer). GC will crash in this case. In the stack trace I can see some Map related code. Might there be some deallocating logic that maps to-be-freed buffers by using those internal memory addresses for the mapping key?

[kkoopa]

This should be the relevant part of the Buffer code. IIRC Buffer was reimplemented using ArrayBuffer in io.js 3.0. Something in this change has led to this new behavior, since everything worked in previous versions of Node / io.js.

[unbornchikken]

Well, by the first look that's make sense, I can see no issue there. I'm gonna launch a debugger with the actual situation, and see the stuff around there.

[unbornchikken]

So, I've isolated the repro case for this, it's in test/iojs3issue.js. Pretty weird stuff is happening:

describe('iojs3issue', function () {
    it('should not crash', function() {
        for (var i = 0; i < 10; i++) {
            gc()
            var buf = new Buffer(8)
            buf.fill(0)
            var buf2 = ref.ref(buf)
            var buf3 = ref.deref(buf2)
        }
    })
    it('should crash', function() {
        for (var i = 0; i < 10; i++) {
            gc()
            var buf = new Buffer(7)
            buf.fill(0)
            var buf2 = ref.ref(buf)
            var buf3 = ref.deref(buf2)
        }
    })
})

The second one crashes on x64. The only difference is the size of data in the original buffer. It seems, the new Buffer or its backend specialized v8 array is doing some optimizations for the case when the data fits in the size of a pointer. @kkoopa @trevnorris do you know something about this?

[unbornchikken]

And I've also noticed some weird race condition to apply, because when I modified the loop end condition to 2, then there wasn't crash. The crash trigger threshold is looks like 3.

[unbornchikken]

I think it grew beyond the scope of this PR, so I've opened an issue there: https://github.com/nodejs/node/issues/2484

[trevnorris]

Can I see the internal values at v8::internal::Map::instance_type() where it crashes? Specifically I'm interested in knowing what the internal pointer is.

This very well might be a limitation of using the new implementation. If some internal Map depends on the pointer, even if the memory is externalized, then there's no way I can think of to emulate the old behavior. Only thing I think you could do is simply make a new typed array around the ArrayBuffer backing the other typed array.

[unbornchikken]

We gotta wait for this commit to land to the main release to finish this PR: https://github.com/nodejs/node/pull/2487 This resolves our issue there.

[unbornchikken]

So, it's completed.

[unbornchikken]

Dunno why Appveyor'd got that braincock, all tests got passed on Windows at my side.

[TooTallNate]

+1

On Wed, Aug 26, 2015 at 7:25 AM, Gábor Mező notifications@github.com wrote:

Dunno why Appveyor'd got that braincock, all tests got passed on Windows at my side.

— Reply to this email directly or view it on GitHub https://github.com/TooTallNate/ref/pull/35#issuecomment-135039103.

[unbornchikken]

Could you release this please? I've got some other dependencies than node-ff too.

[TooTallNate]

@unbornchikken v1.1.0 published to npm!