[FIXED] [1.2.2] CheckForComodification issue in BaseObservable.fireEvent()

[Cputerace]

Below is the fireEvent code for BaseObservable:


public boolean fireEvent(int eventType, BaseEvent be) {
if (firesEvents && listeners != null) {
activeEvent = true;
be.type = eventType;
be.source = be.source;

List<Listener> list = listeners.get(eventType);
if (list != null) {
for (Listener l : list) {
l.handleEvent(be);
}
}
activeEvent = false;
return be.doit;
}
return true;
}

In this section:

List<Listener> list = listeners.get(eventType);
if (list != null) {
for (Listener l : list) {
l.handleEvent(be);
}
}
the checkforcomodification exception at the bottom of the post is occurring within our code. This will happen whenever you add a listener to a baseObservable for an event during handleEvent() for that event. What you should do with this above code is something like this:



List<Listener> list = listeners.get(eventType);
if (list != null && list.size > 0) {
ArrayList<Listener> clonelist = new ArrayList<Listener>();
for (Listener l : listeners.get(eventType))
clonelist.add(l);
for (Listener l : clonelist) {
l.handleEvent(be);
}
}
this way you are making a shallow clone of the list immediately(no possibility of checkForComodification), then iterating through that shallow list (which will never change, even if listeners are added, therefore no checkForComodification) to dispatch events.

java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
java.util.AbstractList$Itr.next(AbstractList.java:343)
com.extjs.gxt.ui.client.event.BaseObservable.fireEvent(BaseObservable.java:73)
com.extjs.gxt.ui.client.widget.Component.fireEvent(Component.java:440)
com.extjs.gxt.ui.client.widget.Component.onBrowserEvent(Component.java:686)
com.google.gwt.user.client.DOM.dispatchEventImpl(DOM.java:1308)
com.google.gwt.user.client.DOM.dispatchEventAndCatch(DOM.java:1287)
com.google.gwt.user.client.DOM.dispatchEvent(DOM.java:1255)
sun.reflect.GeneratedMethodAccessor114.invoke(Unknown Source)
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
java.lang.reflect.Method.invoke(Method.java:597)
com.google.gwt.dev.shell.MethodAdaptor.invoke(MethodAdaptor.java:103)
com.google.gwt.dev.shell.ie.IDispatchImpl.callMethod(IDispatchImpl.java:126)
com.google.gwt.dev.shell.ie.IDispatchProxy.invoke(IDispatchProxy.java:155)
com.google.gwt.dev.shell.ie.IDispatchImpl.Invoke(IDispatchImpl.java:294)
com.google.gwt.dev.shell.ie.IDispatchImpl.method6(IDispatchImpl.java:194)
org.eclipse.swt.internal.ole.win32.COMObject.callback6(COMObject.java:117)
org.eclipse.swt.internal.win32.OS.DispatchMessageW(Native Method)
org.eclipse.swt.internal.win32.OS.DispatchMessage(OS.java:1925)
org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:2966)
com.google.gwt.dev.GWTShell.pumpEventLoop(GWTShell.java:720)
com.google.gwt.dev.GWTShell.run(GWTShell.java:593)
com.google.gwt.dev.GWTShell.main(GWTShell.java:357)

[sven]

Why are you adding a listener to the same eventtype in a listener? I dont see a big usecase to make this change. As this change would only increase memory usage and also the time spend in that method.

[Cputerace]

One of the problems with this bug (and therefore IMHO even more reason to handle it properly) is the fact that it is so hard to track down what is actually causing it. So far all I know is that it is happening when I mouse over a combobox. We have no code that explicitly adds a listener for the combobox, I think it must be somehow connected to something like adding a tooltip, but I am not able to tell even when stepping through in hosted-debug mode.
As far as memory increase, it should only ever involve an arraylist (plus a few more if nested events occur), all which point to already existing objects. Yes, the code would be marginally slower, but this is a glaring hole in the heart of your event handling that will cause headache for anyone who comes across it. We originally came across it months ago, and after a week of trying to figure it out we simply swallowed the error. I would think we are not the only ones to have come across it. The only reason we re-visited it was that we actually came across the exact same issue in our own UI EventBus which we had implemented. We took the same approach that I listed above to fix the issue.

[sven]

It is not really a bug as it works fine in normal situation. You are just do something that is not supported. You can simply defer the removing of that listener and it will work quite well.

The use case for that is so rare.

[Cputerace]

I cannot simply defer the removing of that listener, as 1) we are not adding listeners explicitly and 2) there is no way to tell what listener is causing the error. You cant even tell when the error happens what the event was as it is a JavascriptObject and not an Event.

[sven]

I run some tests, we are doing the change as it is not that big.

[Cputerace]

Thanks. I appreciate it.

[takayser]

Hi Sven. We have the same usecase. How about this, that way you don't have extra memory consumption and it is only a boolean check -> so no overhead.


@SuppressWarnings("unchecked")
public void addListener(EventType eventType, Listener<? extends BaseEvent> listener) {
if (listener == null) return;

if (activeEvent) {
DeferredCommand.addCommand(new Command() {
public void execute() {
addListener(eventType, listener);
}
});
return;
}

if (listeners == null) {
listeners = new HashMap<EventType, List<Listener<BaseEvent>>>();
}
List<Listener<BaseEvent>> list = listeners.get(eventType);
if (list == null) {
list = new ArrayList<Listener<BaseEvent>>();
listeners.put(eventType, list);
}

if (!list.contains(listener)) {
list.add((Listener) listener);
}
hasListeners = true;
}

it's the same procedure like removing a listener.

[sven]

We are doing the copy list thing as it is almost nothing in overhead. I run a couple of tests.

[sven]

Fixed in 1.2.4

[takayser]

Fixed in 1.2.4

no changes in 2.0 code?

[sven]

It will be also fixed there.