juce-framework / JUCE

JUCE is an open-source cross-platform C++ application framework for desktop and mobile applications, including VST, VST3, AU, AUv3, LV2 and AAX audio plug-ins.
https://juce.com
Other
6.59k stars 1.74k forks source link

[Bug]: [Juce8 / QuickJS] Random but inevitable script stopping with error "interrupted" #1445

Closed benkuper closed 1 hour ago

benkuper commented 1 week ago

Detailed steps on how to reproduce the bug

I have this very simple script, the function "update" is called in a dedicated thread at 100fps.

function update(deltaTime) {
    for (var i = 0; i < 100; i++) {

    }
}

Eventually, this code will stop, the returned error code being "interrupted".

This is the stack trace when breaking at the throwing of this interruption :

>   Chataigne.exe!`anonymous namespace'::choc::javascript::quickjs::__js_poll_interrupts(`anonymous-namespace'::choc::javascript::quickjs::JSContext * ctx) Line 16707  C++
    Chataigne.exe!`anonymous namespace'::choc::javascript::quickjs::js_poll_interrupts(`anonymous-namespace'::choc::javascript::quickjs::JSContext * ctx) Line 16718    C++
    Chataigne.exe!`anonymous namespace'::choc::javascript::quickjs::JS_CallInternal(`anonymous-namespace'::choc::javascript::quickjs::JSContext * caller_ctx, `anonymous-namespace'::choc::javascript::quickjs::JSValue func_obj, `anonymous-namespace'::choc::javascript::quickjs::JSValue this_obj, `anonymous-namespace'::choc::javascript::quickjs::JSValue new_target, int argc, `anonymous-namespace'::choc::javascript::quickjs::JSValue * argv, int flags) Line 27286   C++
    Chataigne.exe!`anonymous namespace'::choc::javascript::quickjs::JS_CallFree(`anonymous-namespace'::choc::javascript::quickjs::JSContext * ctx, `anonymous-namespace'::choc::javascript::quickjs::JSValue func_obj, `anonymous-namespace'::choc::javascript::quickjs::JSValue this_obj, int argc, `anonymous-namespace'::choc::javascript::quickjs::JSValue * argv) Line 28655   C++
    Chataigne.exe!`anonymous namespace'::choc::javascript::quickjs::JS_Invoke(`anonymous-namespace'::choc::javascript::quickjs::JSContext * ctx, `anonymous-namespace'::choc::javascript::quickjs::JSValue this_val, unsigned int atom, int argc, `anonymous-namespace'::choc::javascript::quickjs::JSValue * argv) Line 28809  C++
    Chataigne.exe!juce::JavascriptEngine::Impl::callFunction(const juce::Identifier & function, const juce::var::NativeFunctionArgs & args, juce::Result * errorMessage) Line 736   C++
    Chataigne.exe!juce::JavascriptEngine::callFunction(const juce::Identifier & function, const juce::var::NativeFunctionArgs & args, juce::Result * errorMessage) Line 802 C++
    Chataigne.exe!Script::callFunction(const juce::Identifier & function, const juce::Array<juce::var,juce::DummyCriticalSection,0> args, juce::Result * result) Line 288   C++
    Chataigne.exe!Script::run() Line 399    C++
    Chataigne.exe!juce::Thread::threadEntryPoint() Line 110 C++
    Chataigne.exe!juce::juce_threadEntryPoint(void * userData) Line 132 C++
    Chataigne.exe!juce::threadEntryProc(void * userData) Line 75    C++

In the js_poll_interrupts function, the ctx->interrupt_counter has a value of 10000, same value in multiple runs

changing the loop count to 1 makes it run more before crashing removing the for loop and keeping just the update function seems to be ok, but it could also mean that it would run way more before crashing.

EDIT : I let the script live a bit longer with only the update() function and it ended up crashing as well, same error

What is the expected behaviour?

No interrupt, or at least a more informative error message. But with the minimalism of this code, I'd rather hope for no crash :)

Operating systems

Windows

What versions of the operating systems?

Win 11

Architectures

64-bit

Stacktrace

No response

Plug-in formats (if applicable)

No response

Plug-in host applications (DAWs) (if applicable)

No response

Testing on the develop branch

The bug is present on the develop branch

Code of Conduct

reuk commented 1 week ago

the function "update" is called in a dedicated thread at 100fps

Is the JavascriptEngine shared-with or accessed-from the main thread? If so, do you synchronise accesses to the engine in any way?

reuk commented 1 week ago

Assuming the threading is in order, perhaps the problem is to do with JavascriptEngine::maximumExecutionTime. It looks like this used to be initialised to 15 seconds, but is now initialised to zero, which I think was an unintentional change.

reuk commented 6 days ago

I've pushed a potential fix here: https://github.com/juce-framework/JUCE/commit/5453192a0caf0dfbe0222b52eb9f617e04e46c90

Please try it out and let us know if the problem is resolved.

benkuper commented 4 days ago

Thank you for the fast update, unfortunately this doesn't make it. I actually have exposed a method to change the timeout and executionTime from the script itself and had suspected already that it may be that. I tested with the latest dev branch (8.0.3 merged with main), same problem..

reuk commented 4 days ago

Is the JavascriptEngine shared-with or accessed-from the main thread? If so, do you synchronise accesses to the engine in any way?

Please could you confirm that you're not accessing the javascript engine from multiple threads? Have you tried testing with Thread Sanitizer just in case?

If there are no threading issues, then a minimal example that demonstrates the problem would be very helpful in tracking down the issue.

benkuper commented 4 days ago

I don't see any reason another thread would access it but I can check and confirm.

I feel like the timing of the crash is not so random, actually quite steady over many tests. I'll have to dig further to provide valuable informations but it may be a clue.

I don't know about thread sanitizer but I'll check how to use it.

Thank you !

benkuper commented 4 days ago

So I went minimal and created a fresh empty project with this code in Main.cpp :

   void initialise (const juce::String& commandLine) override
   {
       // This method is where you should put your application's initialisation code..

       mainWindow.reset (new MainWindow (getApplicationName()));

    code = "function test() { for(var i=0;i<1000;i++) { var test = 2; } }";
       engine.execute(code);

      // engine.maximumExecutionTime = juce::RelativeTime(3); //doesn't change anything

       juce::var thisObject = juce::var(new juce::DynamicObject());
       juce::var args;
       int index = 0;
       double time = juce::Time::getMillisecondCounterHiRes() / 1000.0;
       while (true)
       {
           juce::Result result = juce::Result::ok();
           engine.callFunction("test", juce::var::NativeFunctionArgs(thisObject, &args, 0), &result);
        double t = juce::Time::getMillisecondCounterHiRes() / 1000.0;
           DBG(index++ << " / " << (t-time) << " / " << result.getErrorMessage());
        juce::Thread::getCurrentThread()->sleep(200);
       }

   }

Here are all my findings :

So I guess there is something set in the constructor that isn't updated later, and this timeout is checked internally against the last "execute" function time, not the last callFunction time

I hope this helps, I started checking AddressSanitizer but didn't go further as those findings seem already valuable for going further, and discard the thread theory imho

benkuper commented 3 days ago

I posted a PR to fix the problem in a simple way, that should at least be good for most use cases. (The first PR was wrongly made against my own dev branch which has many other differences)

reuk commented 1 hour ago

Thanks for the additional details. I've pushed a change that should resolve this issue here: https://github.com/juce-framework/JUCE/commit/3cca812d3804b3dbc729685d068428b25b384d3d

benkuper commented 1 hour ago

Thank you, tested and seems all good !