Bugzilla – Full Text Bug Listing
| Summary: | [DataBinding] NPE in SimplePropertyObservableList after disposal | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Simon Chemouil <eclipse> | ||||||||||||
| Component: | UI | Assignee: | Matthew Hall <qualidafial> | ||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Matthew Hall <qualidafial> | ||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | mallo.ovidio, ob1.eclipse | ||||||||||||
| Version: | 4.1 | ||||||||||||||
| Target Milestone: | 3.8 M3 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 197787 [details]
Fix for Databinding Observable (solution #3)
Patch.
Planning for M3 Hi Simon, I think I actually favor solution 1 since this lowers the surface area we have to cover to fix the bug. From my perspective, if somebody overrides one of our hasListeners() methods in an observable and does so in a way which causes an error, then there is not much we can do to avoid it. Created attachment 203469 [details]
Alternative patch
Multi-threaded testing isn't so bad any more thanks to java.util.concurrency in Java 5. Inter-thread race conditions can be coordinated and simulated using CountDownLatch. I'll take a look at testing this weekend. (In reply to comment #5) > Multi-threaded testing isn't so bad any more thanks to java.util.concurrency in > Java 5. Inter-thread race conditions can be coordinated and simulated using > CountDownLatch. > > I'll take a look at testing this weekend. Thanks for taking care of this issue. Have fun testing it :). Created attachment 203536 [details]
Patch with tests
Turns out we didn't need any threading stuff. Just make hasListeners() visible in the tests, and call it after dispose().
Ovidio, +1? Matthew, I think you would have to declare the following methods "synchronized" since otherwise you might still have race conditions: - AbstractObservableList#hasListeners() - AbstractObservableMap#addDisposeListener() - AbstractObservableMap#removeDisposeListener() If you do so, you could afford the extra "volatile" modifier on AbstractObservableList#disposed and AbstractObservableMap#disposed since the variable is then always accessed within a synchronized block. You might also want to add a reference to this bug to the necessary file headers. Beside that, +1. (In reply to comment #9) > Matthew, I think you would have to declare the following methods "synchronized" > since otherwise you might still have race conditions: > - AbstractObservableList#hasListeners() > - AbstractObservableMap#addDisposeListener() > - AbstractObservableMap#removeDisposeListener() I'm not convinced it's necessary or even desirable on the hasListeners() method. This is protected, and all the expected places for it to be used are when subclasses need to decide when to fire an event. By definition, this means that hasListeners() will be invoked from within the realm. I'm not sure the locking cost is worth the trade-off--I have to think about this one some more. I have no problem adding it to the dispose listener methods though. > You might also want to add a reference to this bug to the necessary file > headers. Will do. Hmm, AbstractObservableMap#hasListeners is already synchronized. I guess this wouldn't be such a big deal. Created attachment 203633 [details]
Revised patch
Includes changes per Ovidio's comments
Created attachment 203634 [details]
mylyn/context/zip
Released to HEAD > 20110919 This should have been committed to git. Reopening. Released changeset 9d898dbe37e374efa4797af076a21f6be51150ba to R3_development > 2011-09-23 |
The bug is quite similar to #247367 (already fixed) but happens on AbstractObservableList#hasListeners() (changeSupport is null after the disposal). Since this method is protected, clients are supposed to check !isDisposed() before calling it, but SimplePropertyObservableList doesn't. In method SimplePropertyObservableList#notifyIfChanged(ListDiff), it calls hasListeners() without that disposal check. The notifyIfChanged(ListDiff) method is itself called by various public methods (add, etc) and by the Runnable in firstListenerAdded(). Before that runnable is scheduled using Realm#exec(Runnable), a disposal check is made, however the Realm#exec method may be asynchronous and we enter a race condition in which the disposal takes place between the scheduling of the runnable and its actual execution. (and because I fear it can take some time to reproduce that condition I'm not providing a test case, please excuse me :-)). There are 3 potential fixes: (1) Just prevent the NPE in AbstractObservableList: --------------------------------8<-------------------------------- protected boolean hasListeners() { if (changeSupport == null) return false; return changeSupport.hasListeners(); } --------------------------------8<-------------------------------- (2) add a disposal check within the Runnable. The NPE can still happen from other methods (add, remove, etc), but these methods are public, and we could consider the clients need to do the disposal check before calling them. SimplePropertyObservableList: --------------------------------8<-------------------------------- protected void firstListenerAdded() { if (!isDisposed()) { if (listener == null) { listener = property .adaptListener(new ISimplePropertyListener() { public void handleEvent( final SimplePropertyEvent event) { if (!isDisposed() && !updating) { getRealm().exec(new Runnable() { public void run() { if (!isDisposed() && !updating) { if (event.type == SimplePropertyEvent.CHANGE) { modCount++; notifyIfChanged((ListDiff) event.diff); } else if (event.type == SimplePropertyEvent.STALE && !stale) { stale = true; fireStale(); } } } }); } } }); } getRealm().exec(new Runnable() { public void run() { cachedList = new ArrayList(getList()); stale = false; if (listener != null) listener.addTo(source); } }); } } --------------------------------8<-------------------------------- (the check is duplicated because it's useless to schedule the Runnable if the list is already disposed, but still we need to make sure it will work even in asynchronous cases). 3) Guard SimplePropertyObservableList#notifyIfChanged(ListDiff), the only method from which we call AbstractObservableList#hasListeners() from SimplePropertyObservableList: --------------------------------8<-------------------------------- private void notifyIfChanged(ListDiff diff) { if (!isDisposed() && hasListeners()) { List oldList = cachedList; List newList = cachedList = new ArrayList(getList()); if (diff == null) diff = Diffs.computeListDiff(oldList, newList); if (!diff.isEmpty() || stale) { stale = false; fireListChange(diff); } } } --------------------------------8<-------------------------------- I tested all three solutions and they all solve the NPE. I'm favoring #3 because AbstractObservableList#hasListeners(ListDiff) is protected and I believe it's a matter of clients respecting a contract and not calling it when it's disposed. Since I did not encounter the problem with other databinding list implementations, I believe they do respect the contract and SimplePropertyObservableList is the class at fault. I'm preferring #3 to #2 because it's a more elegant and more general solution, but #2 might be more appropriate if we wanted to make sure modCount or staling can't be changed once disposed: I think it doesn't matter since the list is disposed anyway! (the bug is still there with HEAD). Stacktrace: !ENTRY org.eclipse.core.databinding 4 0 2011-06-10 16:26:30.142 !MESSAGE Unhandled exception: null !STACK 0 java.lang.NullPointerException at org.eclipse.core.databinding.observable.list.AbstractObservableList.hasListeners(AbstractObservableList.java:93) at org.eclipse.core.internal.databinding.property.list.SimplePropertyObservableList.notifyIfChanged(SimplePropertyObservableList.java:560) at org.eclipse.core.internal.databinding.property.list.SimplePropertyObservableList.access$3(SimplePropertyObservableList.java:559) at org.eclipse.core.internal.databinding.property.list.SimplePropertyObservableList$2.run(SimplePropertyObservableList.java:78) at org.eclipse.core.databinding.observable.Realm$1.run(Realm.java:148) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.core.databinding.observable.Realm.safeRun(Realm.java:152) at org.eclipse.jface.databinding.swt.SWTObservables$DisplayRealm.access$0(SWTObservables.java:1) at org.eclipse.jface.databinding.swt.SWTObservables$1.run(SWTObservables.java:502) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3527) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3174) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:712) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:632) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:106) at org.eclipse.e4.ui.internal.workbench.swt.E4Application.start(E4Application.java:124) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:369) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:619) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:574) at org.eclipse.equinox.launcher.Main.run(Main.java:1407) at org.eclipse.equinox.launcher.Main.main(Main.java:1383)