Opened 11 years ago

Closed 7 years ago

#7207 closed defect (wontfix)

Rhino: setTimeout / clearTimeout implementation is dangerous

Reported by: eponymousalias Owned by: dylan
Priority: high Milestone: future
Component: General Version: 1.1.1
Keywords: Cc: mark-wubben
Blocked By: Blocking:

Description

In this file:

dojo-release-1.1.1/dojo/_base/_loader/hostenv_rhino.js

the implementation of the setTimeout() and clearTimeout() routines is simply dangerous and has to be fixed.

The problem is the use of the java.lang.Thread.stop() function to cancel a pending timeout. This is dangerous in general; read the documentation for this method, and you'll see that it is declared to be inherently unsafe. In the context of the timeout you're trying to cancel, the stop() could be called at any time during the execution of the JavaScript func() call within setTimeout(). That means that the JavaScript interpreter could be called upon to abort its own execution at any time, with possibly disastrous results, possibly destabilizing the browser.

(On a side point, note also that the existing code contains a memory leak: the dojo._timeouts array is forever appended to [with the dojo._timeouts.push() operation at the end of the setTimeout() routine], and never truncated or otherwise cleaned up.)

The fix runs something like the following. Only the java.lang.Thread.currentThread().sleep() call preceding the execution of the func() can be allowed to be interrupted by a clearTimeout() call. After all, you're trying to clear the timeout itself, not to abort the function. Interrupting the sleep alone should be a safe thing to do if it is done cleanly, because there is an established, supported mechanism for such an interruption. If the function itself has started to execute, then it's too late to clear the timeout, and the function execution must be allowed to complete. It is up to the function itself to guarantee that, once started, it will finish in a reasonable time, and that it will coordinate its actions with those of other program threads to avoid undesired effects in case an attempt to clear a possibly pending timeout is too late to stop the function from running.

I am myself not a Java programmer, so I cannot give you precise replacement code, but a good replacement algorithm needs to implement the following:

(*) better synchronization between setting and clearing the timeout,

to only interrupt an ongoing sleep and never the called function

(*) having either the interruption of the sleep() or the completion

of the func() execution invalidate the handle which is passed back by the setTimeout() function, both to avoid memory leaks and to prevent any attempted use of such a handle once it is no longer valid; that probably means using a hash instead of an array, so while the hash key assigned each time can still be implemented with a simple increasing counter, invalid hash keys can simply be deleted from the hash to invalidate any access to the corresponding (dead) threads

So the algorithm might look like:

    // FIX THIS:  We need to make this a hash rather than a linear array,
    // to allow deleting old thread handles without causing memory leaks.
    dojo._timeouts = [];

    function clearTimeout(idx){
        // FIX THIS:  We need a stronger test of whether it is
        // appropriate to use this thread handle.  And that test must
        // be synchronized to avoid problems with the handle possibly
        // disappearing just as we try to test and otherwise use it.
        if(!dojo._timeouts[idx]){ return; }

        // FIX THIS:  We need some method to reach inside the thread to
        // access its .is_interruptible flag, or to use some equivalent
        // thread-external flag, perhaps a corresponding hash entry in
        // a second global hash, and to synchronize access to that flag
        // between the current thread and the possibly sleeping thread.
        // More completely, whether the flag is stored internal to or
        // external to the thread, we have to guarantee that neither it
        // nor the sleeping-thread handle can disappear from view between
        // the test above and our use of them for calling .interrupt() here.

        if (dojo._timeouts[idx].is_interruptible) {
            // FIX THIS:  Think about the potential for a
            // SecurityException to be thrown here.
            dojo._timeouts[idx].interrupt();
        }
    }

    function setTimeout(func, delay){
        // summary: provides timed callbacks using Java threads

        var def={
            // FIX THIS:  Rather than using a thread-local flag here for
            // isInterruptible, we must use some storage that can be easily
            // accessed from outside of this thread.  And our access to
            // that flag must have synchronization protections applied.
            // How to do this (touch a flag here in Java code, and have it
            // accessible within clearTimeout() in JavaScript code) is a
            // challenge for the reader.
            isInterruptible:false,
            sleepTime:delay,
            hasSlept:false,

            run:function(){
                // FIX THIS:  What is the purpose of the hasSlept flag?  And
                // given the logic flow, does it actually serve that purpose?
                if(!this.hasSlept){
                    this.hasSlept=true;
                    try {
                        // FIX THIS:  Apply synchronization to these
                        // isInterruptible flag accesses.
                        this.isInterruptible=true;
                        java.lang.Thread.currentThread().sleep(this.sleepTime);
                        this.isInterruptible=false;
                    }catch(e){
                        // FIX THIS:  Perhaps there's no real reason to
                        // qualify the exception here, and we should return
                        // no matter what kind of exception kicked us awake.
                        if (e == InterruptedException) {
                            // FIX THIS:  we need to invalidate the thread
                            // handle here, including releasing all resources
                            // tied to the handle, such as deleting the
                            // associated hash key/value and whatever external
                            // flags we implement for thread state synchronization
                            return;
                        }
                    }
                }
                try{
                    func();
                }catch(e){
                    console.debug("Error running setTimeout thread:" + e);
                }
                // FIX THIS:  we need to invalidate the thread handle here,
                // including releasing all resources tied to the handle,
                // such as deleting the associated hash key/value and whatever
                // external flags we implement for thread state synchronization
            }
        };

        var runnable = new java.lang.Runnable(def);
        var thread = new java.lang.Thread(runnable);
        thread.start();

        // FIX THIS:  This code needs to add a hash key using a continuously
        // incrementing counter, with the thread being the hash value for that
        // key, instead of pushing onto the end of an ever-growing linear array.
        // Of course, access to that counter will also need to be synchronized.
        return dojo._timeouts.push(thread)-1;
    }

However this code is rewritten, it needs to be reviewed by a second pair of fresh eyes to check for possible remaining synchronization problems.

Change History (16)

comment:1 Changed 11 years ago by Adam Peller

Summary: setTimeout / clearTimeout implementation is dangerousRhino: setTimeout / clearTimeout implementation is dangerous

Thanks for the pointers. The Rhino code (not used in a browser) clearly needs some help. Have you signed a CLA?

comment:2 in reply to:  1 Changed 11 years ago by eponymousalias

Replying to peller:

Thanks for the pointers. The Rhino code (not used in a browser) clearly needs some help. Have you signed a CLA?

No, I haven't, but the code shown is nothing more than your own existing code plus the idea that it needs to be modified to use the proper thread-interruption facilities. It will need to be rewritten to be fleshed out. I do hereby release any and all rights to any interest in it.

comment:3 Changed 11 years ago by Adam Peller

Sorry, we don't accept code without a CLA.

comment:4 Changed 11 years ago by dylan

Milestone: tbdfuture

Mostly that we're not allowed do... any interest in signing a CLA and pitching in on this?

Marking as future as this does not block the 1.2 release. Note that future does not mean it will be ignored, just that it won't get assigned to a release until someone commits to fixing it for a specific release.

comment:5 Changed 11 years ago by Mark Wubben

I'm doing some work on Dojo inside Rhino, wouldn't mind pitching in here.

comment:6 Changed 11 years ago by Adam Peller

Owner: changed from anonymous to Mark Wubben

Mark, it's yours. We could definitely use the help on this one. Thanks!

comment:7 Changed 11 years ago by Adam Peller

Cc: mark-wubben added

see #8159

comment:8 Changed 11 years ago by Ben Lowery

Maybe we could piggy-back on Java's Timer implementation instead of dealing with the threading details ourselves? Details on the Timer at http://java.sun.com/j2se/1.4.2/docs/api/java/util/Timer.html

comment:9 Changed 11 years ago by Adam Peller

see also #8668

comment:10 Changed 10 years ago by matthw

I'd like to see this fixed - I'm using this setTimeout implementation to mock asynchronous ajax calls in my tests, and I'm getting unpredictable test failures, presumably due to race conditions.

I'd be fine with just a really simple single-threaded event-loop-based implementation, it needn't even respect real clock time provided it runs events in the order scheduled.

Or if it can be made to work using threads (but still behaving to the user like a single-threaded browser-based implementation) then great!

BTW: some relevant discussion going on here: http://groups.google.com/group/envjs/browse_thread/thread/b65cbc84a66f1259

comment:11 Changed 10 years ago by Mark Wubben

My Dojo-inside-Rhino work stopped before I had time to fix this… any patches you could offer?

comment:12 Changed 10 years ago by matthw

Here's a patch we threw together for hostenv_rhino.js, with a simple single-threaded event-loop based setTimeout/clearTimeout.

Downside: to use it you need to wrap your rhino runner code in dojo.enterEventLoop(function() {....})

diff --git a/dojo/_base/_loader/hostenv_rhino.js b/dojo/_base/_loader/hostenv_rhino.js
index fc1e881..eb7f447 100644
--- a/dojo/_base/_loader/hostenv_rhino.js
+++ b/dojo/_base/_loader/hostenv_rhino.js
@@ -207,38 +207,61 @@ dojo.body = function(){
 // Note: this assumes that we define both if one is not provided... there might
 // be a better way to do this if there is a use case where one is defined but
 // not the other
+// Changed by Playlouder to stop the main thread exiting on us, by using an
+// event loop rather than threads.
 if(typeof setTimeout == "undefined" || typeof clearTimeout == "undefined"){
-	dojo._timeouts = [];
-	function clearTimeout(idx){
-		if(!dojo._timeouts[idx]){ return; }
-		dojo._timeouts[idx].stop();
-	}
+  dojo._timeouts = [];
 
-	function setTimeout(func, delay){
-		// summary: provides timed callbacks using Java threads
-
-		var def={
-			sleepTime:delay,
-			hasSlept:false,
-		
-			run:function(){
-				if(!this.hasSlept){
-					this.hasSlept=true;
-					java.lang.Thread.currentThread().sleep(this.sleepTime);
-				}
-				try{
-					func();
-				}catch(e){
-					console.debug("Error running setTimeout thread:" + e);
-				}
-			}
-		};
-	
-		var runnable = new java.lang.Runnable(def);
-		var thread = new java.lang.Thread(runnable);
-		thread.start();
-		return dojo._timeouts.push(thread)-1;
-	}
+  // keep a track of unique timeout ids
+  dojo._timeoutId = 1;
+
+  /**
+   * Kick of the event loop, running the given function, then running through
+   * our list of queued timeout functions for ever.
+   */
+  dojo.enterEventLoop = function(startWith) {
+    startWith && startWith();
+
+    while (dojo._timeouts.length > 0) {
+      var timeout = dojo._timeouts.shift();
+
+      // sleep until the next timeout is ready to go
+      var now = new Date().getTime();
+      if (timeout.scheduledFor > now) {
+        java.lang.Thread.currentThread().sleep(timeout.scheduledFor - now);
+      }
+
+      try {
+        timeout.func();
+      } catch (e) {
+        print("ERROR: error running timeout");
+        print(e.printStackTrace());
+      }
+    };
+  };
+
+  function clearTimeout(id){
+    var timeouts = dojo._timeouts;
+    for (var i=0; i < timeouts.length; i++)
+      if (timeouts[i].id == id) {timeouts.splice(i,1); return}
+  }
+
+  function setTimeout(func, delay){
+    // summary: bungs a function on the timeout queue
+
+    var timeout = {
+        scheduledFor:  new Date().getTime() + delay,
+        id:            dojo._timeoutId++,
+        func:          func
+    };
+
+    dojo._timeouts.push(timeout);
+    dojo._timeouts.sort(function(lhs, rhs) {
+      return lhs.scheduledFor < rhs.scheduledFor;
+    });
+
+    return timeout.id;
+  }
 }
 
 //Register any module paths set up in djConfig. Need to do this

comment:13 Changed 10 years ago by Adam Peller

matthw, have you signed a CLA?

comment:14 Changed 10 years ago by matthw

we have yeah, CCLA for MSP ltd

Although, just spotted a bug in that patch. Should be

    // insert timeout into the queue, maintaining sort order:
    var timeouts = dojo._timeouts;
    for (var i=0; i < timeouts.length && timeouts[i].scheduledFor <= timeout.scheduledFor; i++);
    timeouts.splice(i, 0, timeout);

instead of

    dojo._timeouts.push(timeout);
    dojo._timeouts.sort(function(lhs, rhs) {
      return lhs.scheduledFor < rhs.scheduledFor;
    });

should fix it, guess that's what you get when you don't test your test code :)

comment:15 Changed 8 years ago by Chris Mitchell

Owner: changed from Mark Wubben to dylan

comment:16 Changed 7 years ago by bill

Resolution: wontfix
Status: newclosed

Presumably we wont fix these because in the future dojo will only run (server side) under NodeJS.

Note: See TracTickets for help on using tickets.