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

Possible null-dereference in onTick? #277

Closed
blalor opened this Issue Apr 19, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@blalor

blalor commented Apr 19, 2013

I'm integrating Q v0.9.3 with some code I've written that runs in the Rhino JS runtime. Rhino doesn't have setTimeout or any of the other basic timing mechanisms you'd expect, so the implementation I'm using could have problems, but it looks to me like this could be a problem with Q.

I'm getting occasional errors within onTick when fired from setTimeout:

Exception in thread "Timer-0" org.mozilla.javascript.EcmaError: TypeError: Cannot read property "task" from null (src/main/javascript/lib/q.js#124)
    at org.mozilla.javascript.ScriptRuntime.constructError(ScriptRuntime.java:3687)
    at org.mozilla.javascript.ScriptRuntime.constructError(ScriptRuntime.java:3665)
    at org.mozilla.javascript.ScriptRuntime.typeError(ScriptRuntime.java:3693)
    at org.mozilla.javascript.ScriptRuntime.typeError2(ScriptRuntime.java:3712)
    at org.mozilla.javascript.ScriptRuntime.undefReadError(ScriptRuntime.java:3725)
    at org.mozilla.javascript.ScriptRuntime.getObjectProp(ScriptRuntime.java:1483)
    at org.mozilla.javascript.Interpreter.interpretLoop(Interpreter.java:1239)
    at script.onTick(src/main/javascript/lib/q.js:124)
    at org.mozilla.javascript.Interpreter.interpret(Interpreter.java:815)
    at org.mozilla.javascript.InterpretedFunction.call(InterpretedFunction.java:109)
    at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:394)
    at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3091)
    at org.mozilla.javascript.InterpretedFunction.call(InterpretedFunction.java:107)
    at org.mozilla.javascript.Context$1.run(Context.java:478)
    at org.mozilla.javascript.Context.call(Context.java:489)
    at org.mozilla.javascript.Context.call(Context.java:476)
    at org.mozilla.javascript.JavaAdapter.callMethod(JavaAdapter.java:573)
    at adapter1.runnable(<adapter>)
    at com.autosportlabs.rc_live.db_busmod.RunnableTimerTask.run(RunnableTimerTask.java:20)
    at java.util.TimerThread.mainLoop(Timer.java:555)
    at java.util.TimerThread.run(Timer.java:505)

Line 124 is the var task = head.task; in this block:

        while (queuedTasks) {
            --queuedTasks; // decrement here to ensure it's never negative
            head = head.next;
            var task = head.task;
            head.task = void 0;
            task();
        }

I'm not familiar with the flow but it does look to me like head could end up being null when head.task is referenced.

@domenic

This comment has been minimized.

Collaborator

domenic commented Apr 21, 2013

@rkatic, any comments?

@blalor, a small reproducible test case would be extremely valuable.

@rkatic

This comment has been minimized.

Collaborator

rkatic commented Apr 21, 2013

I will take a look on this from Monday. For now I have two main suspects: 1) a minifier that makes some wrong assumptions when queuedTasks is incremented (unlikely); 2) a problematic custom implementation of setTimeout (some multithreading issues). However, I am not familiar with Rhino and showing the respective code would certainly help.

@blalor

This comment has been minimized.

blalor commented Apr 21, 2013

On Apr 20, 2013, at 10:24 PM, Robert Katić notifications@github.com wrote:

I will take a look on this from Monday. For now I have two main suspects: 1) a minifier that makes some wrong assumptions when queuedTasks is incremented (unlikely); 2) a problematic custom implementation of setTimeout (some multithreading issues). However, I am not familiar with Rhino and showing the respective code would certainly help.

I'm not using the minified version. I have tried a couple of different setTimeout versions (both thread-based, unfortunately; one with java.util.Timer, one with with a new Thread and Thread.sleep) and both have consistently had problems around this task queue. I do strongly suspect a threading issue, however the exceptions I'm seeing happen so frequently in this spot that I thought it worth mentioning. Don't spend much time on this; I just thought maybe more experienced eyes might see something obvious. I'll see if I can come up with a test case. I've abandoned the Jasmine JUnit runner I was using because of this problem, but I don't blame Q.

@domenic

This comment has been minimized.

Collaborator

domenic commented Apr 21, 2013

Oh wow, multithreading!? Yeah I imagine that breaks a bunch of assumptions that Q (and every other JS library on the planet) makes :-/.

If it's an easy fix we'll be happy to take it though. E.g. if you get a small reproducible test case, and then make a small change to the queueing code, and that fixes your test case, we'll very likely accept it.

@domenic

This comment has been minimized.

Collaborator

domenic commented May 5, 2013

Let's close this for now, but my above words stand: if you have an easy fix and a reproducible test case, we'll take it, even if just for Rhino's sake :).

@domenic domenic closed this May 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment