Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#6165 closed defect (fixed)

Calling dojo.connect within event fires event in Internet Explorer

Reported by: guest Owned by: sjmiles
Priority: high Milestone: 1.2
Component: Events Version: 1.1b1
Keywords: Cc: simon@…
Blocked By: Blocking:

Description (last modified by sjmiles)

email:simon@cambridgesemantics.com

This bug manifests itself in all versions of dojo including the current version in trunk.

The example below illustrates the problem. Firefox will work perfectly. Internet Explorer will go into an infinite loop.

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
	"http://www.w3.org/TR/html4/strict.dtd">
<html>
	<head>
		<title>Testing dojo.connect</title>
		<style type="text/css">
			@import "../resources/dojo.css";
			@import "../../dijit/themes/tundra/tundra.css";
		</style>
		<script type="text/javascript" 
			src="../dojo.js" djConfig="isDebug: true"></script>
		<script type="text/javascript">
var obj2 = { 
	handler : function() {
		dojo.disconnect(this.handle);
		obj1.handle = dojo.connect(objEventRaiser, 'func', obj1, 'handler')
	}
};

var obj1 = { 
	handler : function() {
		dojo.disconnect(this.handle);
		obj2.handle = dojo.connect(objEventRaiser, 'func', obj1, 'handler')
	}
};

    
var objEventRaiser = {
	func : function() {}
}
obj1.handle = dojo.connect(objEventRaiser, 'func', obj1, 'handler')

function go() {
	objEventRaiser.func();
}

  		</script>
	</head>
	<body class="tundra">
	<button onclick="go()">Go</button>
	</body>
</html>

The code above works perfectly in firefox but causes an infinite loop in Internet Explorer. This is caused by different behavior of a for( in ) loop in Internet Explorer. The code for getDispatcher in connect.js has a for (in) loop. If an item is added to the c._listeners array (using dojo.connect) whilst the for in loop is executing then that member is added to the array currently being looped over and therefore causing the event which has just been connected to to fire.

getDispatcher: function(){
  // following comments pulled out-of-line to prevent cloning them 
  // in the returned function.
  // - indices (i) that are really in the array of listeners (ls) will 
  //   not be in Array.prototype. This is the 'sparse array' trick
  //   that keeps us safe from libs that take liberties with built-in 
  //   objects
  // - listener is invoked with current scope (this)
  return function(){
   var ap=Array.prototype, c=arguments.callee, ls=c._listeners, t=c.target;
   // return value comes from original target function
   var r=t && t.apply(this, arguments);
   // invoke listeners after target function
   for(var i in ls){
    if(!(i in ap)){
     ls[i].apply(this, arguments);
    }
   }
   // return value comes from original target function
   return r;
  }
 },

One solution would be to copy the contents of the array and loop using those - something like this

	getDispatcher: function(){
		// following comments pulled out-of-line to prevent cloning them 
		// in the returned function.
		// - indices (i) that are really in the array of listeners (ls) will 
		//   not be in Array.prototype. This is the 'sparse array' trick
		//   that keeps us safe from libs that take liberties with built-in 
		//   objects
		// - listener is invoked with current scope (this)
		return function(){
			var ap=Array.prototype, c=arguments.callee, ls=c._listeners, t=c.target;
			// return value comes from original target function
			var r=t && t.apply(this, arguments);
			// invoke listeners after target function
			// copy ls to a new array so that if a member is 
			// removed whilst in the loop it does not cause an issue in IE 
			var _ls = [].concat(ls);
			for(var i in ls){
				if(!(i in ap)){
					ls[i].apply(this, arguments);
				}
			}
			// return value comes from original target function
			return r;
		}
	},

Change History (7)

comment:1 Changed 12 years ago by bill

Milestone: 1.4

Hmm, it's a pretty obscure case to call dojo.connect() within an event handler for the thing you are connecting to but anyway Scott should either set a milestone for this or mark as "wontfix". I'll temporarily set the milestone to 1.4.

comment:2 Changed 12 years ago by guest

It is a bug that we are experiencing and means that we have to patch our dojo source tree for each dojo release. The only way we could think to work around this bug (without it being fixed in the dojo source tree) is to use a setTimeout to hook the event once it has returned from from the event - this seems like a less than ideal solution.

comment:3 Changed 11 years ago by sjmiles

Description: modified (diff)
Milestone: 1.4tbd

This is a thorny problem. Here are some thoughts while I work on it.

I agree that insulating the dispatcher from modifications to the listener list is a good idea, but I'm not sure it's worth the cost of copying the list on every invocation. The case of adding a listener seems like a corner case, and IMO is easily done with setTimeout as mentioned. However, removing a listener seems more likely.

For maximum efficiency, I suppose one could get tricky by having additions/removals done in a secondary array, and copying into a primary array in the dispatcher. It's hard to know if it's worth the extra work. If I truly managed to isolate this work in dojo._listener as intended, it should be easy, but I'll have to make sure that object is not cracked/replaced in some other module.

comment:4 Changed 11 years ago by sjmiles

So, I think it's best to copy the array as suggested. There is some performance cost, but it seems like better engineering.

But of course, the code is not isolated to _listener, in fact there are three code paths:

  1. dojo._listener as shown above
  2. built-in add/removeEventListener (non-IE browsers)
  3. dojo._ie_listener

(1) is invoked for connects to regular functions or custom events on nodes. (2) and (3) are used for connects to proper events on nodes [(3) is only for IE].

The Mozilla docs for (2) indicate that you can add a listener during processing and it will not be invoked. Similar for remove.

Copying the array as suggested will emulate Mozilla for add but *not* for remove, which is possibly troublesome. This difference is subtle, I'm hoping it won't matter.

My plan is to add code to duplicate the listener array before iterating in dojo._listener and dojo._ie_listener.

comment:5 Changed 11 years ago by dante

Milestone: tbdfuture

tbd is for incoming/unprocessed bugs. this should be 'future' or a milestone, depending on how you feel about it. setting future for now.

comment:6 Changed 11 years ago by sjmiles

Resolution: fixed
Status: newclosed

(In [14604]) Iterate over cloned listener arrays in case the original arrays are modified during event processing. Fixes #6165. !strict

comment:7 Changed 11 years ago by dante

Milestone: future1.2

milestones help track things in a release.

Note: See TracTickets for help on using tickets.