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
malloc: *** error for object 0x100000000: pointer being freed was not allocated #171
Comments
@modeswitch Hi, |
@guymguym I'm having a look now. |
👍 |
@guymguym I'm seeing a crash. I'm also seeing that only some of the buffers are arriving in the browser. |
by "some buffers are arriving" do you mean that when you run browser vs. browser you see "BAD SEQUENCE" prints? because the code tries to check that all packets arrive in order... |
No, I mean only some of the buffers are printed to the console. I'm using the modified bridge from #170. |
I didn't follow which prints you mean, so not sure if I can assist. BTW you maintain a copy of webrtc repo in libwebrtc. Doesn't it make it hard to sync with changes and avoid collisions? Wasn't it possible to use the original repo and just make a custom build and bridge for node? |
The buffers are printed to the console as they are received. I was originally using upstream libjingle, but it has many dependencies and is therefore difficult to build for many people. The fork I have is missing the implementation for media streams, which dramatically reduces the number of external dependencies. |
Hey @modeswitch just wondering if you got any lead on that? I would be happy to help investigating if you can share your thoughts and point out suspected areas, because I am not yet familiar with webrtc's inner implementation and specific versions status (pending critical issues and such). Thanks! |
@modeswitch - I wonder, am I the only one trying to use data-channels to send lots of data? |
@guymguym I'm hoping to take a peak at this issue later this week |
@rosskukulinski 👍 Great. I'm digging my way as well. |
submitted pull request #182 with a simple local reproduction of the error |
so I'm in debug land looking into a few issues. By all means, node-webrtc shouldn't crash. but, it seems you're sending too large of a packet:
|
@rosskukulinski The code in #182 currently sends 16K packets as I saw recommended, and that works perfectly in chrome and firefox. I tried changing the sends to various smaller sizes and it always fails, even for packets of 8 bytes! you can also change the PACKET_SIZE in the code, but it's not affecting the original error (pointer being freed was not allocated). I found this error here: |
@guymguym - Yes, I found it to crash no matter what as well. More just something I noticed. The debug messages are by building the node-webrtc with debug symbols, then run node with --debug. |
@rosskukulinski
|
regarding the MTU I found this - |
oh -- regarding that resolveFilename error -- tweak package.json path to use 'Debug' rather than ''. |
good find on the error -- looks like we should update libwrtc. For now, could possibly patch: https://github.com/js-platform/libwebrtc/blob/master/talk/media/sctp/sctpdataengine.cc |
actually it seems like it's already patched - but it doesn't help |
@guymguym notice the last comment:
The kSctpMtu = 1200; in libwebrtc All that said, I'm not sure this is related to the crash |
@rosskukulinski That's right, I changed to 1280 and the errors were gone. probably unrelated. |
I'm trying to understand the memory handling of the buffers in this webrtc code and not yet sure what's going on. I used valgrind and saw multiple issues like I copied below. from what I understand in the code in
|
after fixing #184 locally and also fixing the previous issue by releasing the buffer from the scoped_ptr if breaking, i can now run the test with valgrind for longer time and now I get these issues which seem to be related to node-webrtc allocations:
|
the first free is coming from datachannel.cc:131
|
I found that webrtc already fixed the code for SendQueuedDataMessages ~2 months ago to avoid deleting buffer and leaving in queue. @modeswitch can libwebrtc be updated? do you go to latest or pick specific patches?
|
OK, found the issue and updated the pull request with the fix. apparently NanMakeWeakPersistent was called with the array as the handle, but should be the callback function according to the (docs)[https://github.com/rvagg/nan#api_nan_make_weak_persistent]. so @modeswitch to close this issue we need to pull fix(es) from webrtc to libwebrtc, and merge #182 . |
@modeswitch Thanks! But this issue is still open without fixing the bug in webrtc - What is the plan about updating libwebrtc from upstream? |
It looks like that patch will apply cleanly to libwebrtc. Easiest thing would be to cherrypick it from upstream. |
@modeswitch cherrypicking might be the fastest path to fix this issue. for the long term I would prefer to see this project allowing to pick a specific revision from upstream webrtc as I suggested in #174 (which I see you closed). |
I added pull-request #186 that currently fails on this issue, and should hopefully pass once updating libwebrtc (though my tests are only on mac, so I hope linux builds will behave the same). |
@guymguym The whole reason for using a fork of libjingle was to reduce the complexity for building the node bindings, which was a common complaint. Pre-built binaries didn't solve the problem for all platforms. If you make a pull request with the upstream patch (make sure the commit hash is the same), I'll merge it. Also, if you have a better way of maintaining the libjingle fork (via build scripts as you suggested) please make a pull request and we can discuss it. |
@modeswitch I'm not sure how to do that technically. I forked libwebrtc, added upstream webrtc as remote (from https://chromium.googlesource.com/external/webrtc/), checkedout upstream/master to find the commit to cherry-pick which is this one:
so then I checkedout back to master, cheery-pick cceb166a3fd2724c679da7d093149b0511e8d99b, and then I get a different commit hash:
Although the diff is perfectly fine... |
According to this explanation it's not really possible - |
@guymguym Ah, yeah. The repos have diverged so that won't be possible. I guess the next best thing is to mark the upstream commit in the commit log. Then at least we can figure out where the patch came from. |
done - js-platform/libwebrtc#5 |
hi
I see this issue like discussed in #170
though I use a different test -
https://gist.github.com/guymguym/728bf41ab38466ccca4f (see usage in gist comment).
when the two peers are chrome/firefox it works well (though with 100% cpu for just ~5 MB/s on Macbook Air 🐌 ).
but any wrtc client fails pretty quickly.
this occurs on OSX 10.10.2 with latest wrtc version which is 0.0.49 in npm (which is not the same as the github release numbers ?!).
The text was updated successfully, but these errors were encountered: