Opened 11 years ago

Closed 11 years ago

#9093 closed defect (fixed)

[patch][ccla][IE]Two connects on the same event can cause problem if disconnecting the second one in the first function called

Reported by: jfcunat Owned by: sjmiles
Priority: high Milestone: 1.3.1
Component: Events Version: 1.3.0
Keywords: Cc: mbrechet.ext@…
Blocked By: Blocking:

Description

On all IE, there is a problem when you do two dojo.connect on the same event. If you disconnect the second handle in the first function, it causes an error on IE

This did not occur in Dojo 1.2

A test file is provided. Just hover the indicated area and you'll se an error in IE but not in FF.

It seems related to Changeset [14604] that makes an iteration over a copy of the listener array instead of the original one.

As it solves another bug, what I propose is just to test the existance of the target function

I submitted a patch for that

Attachments (2)

test_disconnect.html (985 bytes) - added by jfcunat 11 years ago.
test file
ieDispatcher.patch (600 bytes) - added by jfcunat 11 years ago.

Download all attachments as: .zip

Change History (8)

Changed 11 years ago by jfcunat

Attachment: test_disconnect.html added

test file

Changed 11 years ago by jfcunat

Attachment: ieDispatcher.patch added

comment:1 Changed 11 years ago by Adam Peller

Component: CoreEvents
Owner: changed from anonymous to sjmiles

comment:2 Changed 11 years ago by sjmiles

Milestone: 1.3.1tbd

The patch solves the problem for connects to nodes on IE, but a variation of the problem exists for non-node connects.

x = {
  y:function(){
    console.log("y");
  }
};
c1 = dojo.connect(x, "y", function() {
    dojo.disconnect(c2);
    console.log("c1");
});
c2 = dojo.connect(x, "y", function() {
    console.log("c2");
});
x.y();
x.y();

The above code produces output like so:

y
c1
c2
y
c1

Iow, the list of listeners is made immutable during event processing, so the removal of c2 doesn't take effect until the second call to x.y.

This effect has been known to cause confusion. And on IE (for connects to nodes) it will actually cause an exception as noted in this ticket.

So, even though the patch solves the exception problem, I'm not convinced it's a good idea to change the behavior in for this one case.

Suggest we consult the standards documents for what the behavior is suppose to be for add/removeEventLister and ensure that all connects/disconnects behave in the expected fashion. Dylan mentioned that somebody has been doing a rewrite of connect, but I haven't seen the code yet.

Fwiw, it should be possible to avoid the exception by doing the disconnect in a timeout block, although I understand the semantics are different. But right now, we just don't support a connection while executing the connected function.

I'm interested to hear more opinions on this topic.

comment:3 Changed 11 years ago by sjmiles

After thinking about it, I think it's right to commit this patch after all.

There is still an inconsistancy in the way disconnects are handled between nodes and non-nodes, but at least the patch above brings IE in line with the other browsers.

After that, there will remain a bug in regular non-node connect, which is that removing an event listener during connect processing does not prevent it from being called. I call it a bug because of the standards info: https://developer.mozilla.org/En/DOM:element.removeEventListener

comment:4 Changed 11 years ago by Adam Peller

Milestone: tbd1.3.1

comment:5 Changed 11 years ago by Adam Peller

fixed in trunk: [17272]. Thanks, jfcunat

comment:6 Changed 11 years ago by Adam Peller

Resolution: fixed
Status: newclosed

fixed in 1.3 branch: [17273]

Note: See TracTickets for help on using tickets.