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 Crash #20

Closed
hrueger opened this issue Dec 1, 2022 · 17 comments
Closed

Native Crash #20

hrueger opened this issue Dec 1, 2022 · 17 comments
Assignees

Comments

@hrueger
Copy link
Contributor

hrueger commented Dec 1, 2022

Hi, when trying to use this lib on a Windows 10 machine, my application crashed with the following error:

FATAL ERROR: HandleScope::HandleScope Entering the V8 API without proper locking in place
 1: 00007FF6A265B34F v8::internal::CodeObjectRegistry::~CodeObjectRegistry+123599
 2: 00007FF6A25E8CB6 v8::internal::MicrotaskQueue::GetMicrotasksScopeDepth+65206
 3: 00007FF6A25E9D8D node::OnFatalError+301
 4: 00007FF6A2F10A0B v8::HandleScope::Initialize+107
 5: 00007FF6A2EFAFF1 v8::EscapableHandleScope::EscapableHandleScope+81
 6: 00007FF6A262CC78 napi_open_escapable_handle_scope+120
 7: 00007FF9CA33280D Napi::FunctionReference::New+61 [A:\_Source\native-sound-mixer\node_modules\node-addon-api\napi-inl.h]:L3370
 8: 00007FF9CA333663 SoundMixer::DeviceObject::New+83 [A:\_Source\native-sound-mixer\cppsrc\win\sound-mixer.cpp]:L83
 9: 00007FF9CA333465 SoundMixer::MixerObject::GetDevices+341 [A:\_Source\native-sound-mixer\cppsrc\win\sound-mixer.cpp]:L4710: 00007FF9CA336A3E Napi::details::WrapCallback<<lambda_2d6a469e53703925d71c48cc7e420a87> >+46 [A:\_Source\native-sound-mixer\node_modules\node-addon-api\napi-inl.h]:L75
11: 00007FF9CA335F58 Napi::details::TemplatedCallback<&SoundMixer::MixerObject::GetDevices>+40 [A:\_Source\native-sound-mixer\node_modules\node-addon-api\napi-inl.h]:L153
12: 00007FF6A262564B node::Stop+32747
13: 00007FF6A2F3E74F v8::internal::SetupIsolateDelegate::SetupHeap+53823
14: 00007FF6A2F6FAE9 v8::internal::SetupIsolateDelegate::SetupHeap+255449
15: 000002D94CCBDBBE

Any idea?

@m1dugh
Copy link
Owner

m1dugh commented Dec 1, 2022

could you show me your node code causing this crash ?

@hrueger
Copy link
Contributor Author

hrueger commented Dec 1, 2022

Thanks for the fast response. I'll try to find the exact call. It's a pretty complex project, it might take a while.

@hrueger
Copy link
Contributor Author

hrueger commented Dec 1, 2022

So, I've finally found it. This is the code:

const SoundMixer = require("native-sound-mixer").default;
const pollInterval = setInterval(() => {
    for (const device of SoundMixer.devices) {
        console.log(device, device.volume);
    }
}, 500);

It works find for the first 6 iterations, then in the 7th one it crashes. However, I was not able to reproduce that in a standalone example, I don't know why. It is definitely that line, though, as it works fine if I comment it out.
It does not matter if I read the mute or the volume property by the way.

@m1dugh
Copy link
Owner

m1dugh commented Dec 1, 2022

Are you using the call in a multi threaded context ? It looks like the problem is the same as this one nodejs/node-addon-api#779.
I can hardly fix the code right now, i'll try to make it as fast as I can.

@hrueger
Copy link
Contributor Author

hrueger commented Dec 1, 2022

I am using Worker Threads on my application, but this library is used in the main thread.

@m1dugh
Copy link
Owner

m1dugh commented Dec 1, 2022

I might have located the problem, i'll try to fix it as soon as possible.

@hrueger
Copy link
Contributor Author

hrueger commented Dec 1, 2022

Thanks in advance!

@m1dugh
Copy link
Owner

m1dugh commented Dec 2, 2022

Since the bug is not reproductible in a standalone, it will be hard to test the code on my side. I can still push the fix on develop and you can test it before I push a new version if that's ok for you 😄. Otherwise, I will push the fix to a new version on npm registry but I can't be sure it will fix the problem ...

@hrueger
Copy link
Contributor Author

hrueger commented Dec 2, 2022

I can still push the fix on develop and you can test it before I push a new version if that's ok for you

Sure 👍

@m1dugh
Copy link
Owner

m1dugh commented Dec 7, 2022

It took longer than I expected, I should be done by next week I guess.

@hrueger
Copy link
Contributor Author

hrueger commented Dec 7, 2022

No problem, thanks for your efforts 👍

@m1dugh
Copy link
Owner

m1dugh commented Dec 12, 2022

I pushed something to 20-native-crash branch, I have no clue whether it might be better but you can try anyway.

@m1dugh m1dugh self-assigned this Dec 12, 2022
@hrueger
Copy link
Contributor Author

hrueger commented Dec 13, 2022

Hm, I can't even reproduce the original error anymore... Very strange.

However, while testing, I found that there may be a memory leak? Not sure, though.
If you run the following code, you can see the memory usage of the Node Process increasing by about 1MB / s.
Maybe this gives a hint? Could also be completely unreleated, though ;-)

const nsm = require("native-sound-mixer").default;

let mute;
let volume;
(async () => {
    while (true) {
        console.log("next round")
        for (let i = 0; i < 1000; i++) {
            for (const device of nsm.devices) {
                mute = device.mute;
                volume = device.volume;
            }
        }
        await new Promise((resolve) => { setTimeout(() => { resolve(); }, 500); });
    }
})();

@m1dugh
Copy link
Owner

m1dugh commented Dec 13, 2022

If you can't reproduce the original error, I hope it was some strange node issue. Concerning the memory leak problem, I'm currently working on it. However, due to the way node handles memory, the memory should be flushed regularly, fixing the memory leak as it goes.

@hrueger
Copy link
Contributor Author

hrueger commented Dec 13, 2022

I hope it was some strange node issue

I agree 👍

Concerning the memory leak problem, I'm currently working on it.

Thanks a lot

@m1dugh
Copy link
Owner

m1dugh commented Dec 16, 2022

The memory leaks concerning the devices should be fixed on develop (for windows only)

@m1dugh m1dugh closed this as completed Dec 16, 2022
@hrueger
Copy link
Contributor Author

hrueger commented Dec 16, 2022

Yes, can confirm that the memory leak is gone 👍 Thanks for your effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants