Bugzilla – Full Text Bug Listing
| Summary: | [DataBinding] Listeners in swt Observables are not removed on dispose | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Ashley Cambrell <acambrell+eclipsebugs> | ||||||||
| Component: | UI | Assignee: | Boris Bokowski <bokowski> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | bradleyjames | ||||||||
| Version: | 3.4 | Keywords: | greatbug | ||||||||
| Target Milestone: | 3.3.1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
Created attachment 75410 [details]
Patch to overide dispose and clean up listeners
This patch modifies
* ComboSingleSelectionObservableValue
* TableSingleSelectionObservableValue
* CComboSingleSelectionObservableValue
* ListSingleSelectionObservableValue
* ComboObservableValue
* SpinnerObservableValue
* ListObservableValue
* ButtonObservableValue
* CComboObservableValue
the extracts listeners into a field so that they can be removed in an override dispose method.
Classes that extend SingleSelectionObservableValue could easily be refactored to avoid duplication.
3.3.1 candidate? (In reply to comment #2) > 3.3.1 candidate? > +1 Created attachment 76136 [details] combined patch Updated the class headers listing Ashley's contribution. I noticed that the test and fix for bug 198903 is part of this patch as well, is this correct, Ashley? I'm fine with releasing this patch and marking the other bug as fixed at the same time, but would like you to double-check that this patch will also fix bug 198903. An exemplary bug report with test cases and a fix ready to go. Thanks a lot for your effort, Ashley! I must have left the fix for bug 198903 in the patch. As it was a one liner, I didn't submit a patch for the other. Not as I intended but the fix for bug 198903 is correct. Thanks Boris Released to the 3.3 maintenance stream >20070816. Verified in M20070905-1045 and released to HEAD as well. Couple of quick comments... 1. Thank you for writing tests. 2. In my merge of bug 182059 to HEAD I removed the addition of AbstractSWTTestCase.notifySelection(Widget). You can accomplish the same with widget.notifyListeners(SWT.Selection, null). Also since our testing strategy is changing this won't be very useful as our code moves into the delegates. 3. I removed TestCounterValueChangeListener. There was an existing class, ValueChangeEventTracker, that alreadys performed this function. For future referenct there are trackers for the other event types as well. 4. I removed some of the tests since our conformance tests cover the use cases. I think we can add some dispose tests to the conformance test suite (bug 202735) to test all observables as well. I just didn't want you to be surprised when you saw that some of this was removed. Thanks again! |
Created attachment 75409 [details] Additional test cases and test method to test that dipose correctly cleans up listeners. Build ID: HEAD Steps To Reproduce: 1. Apply patch that contains test cases for dispose methods. 2. Tests fail with a NullPointerException as the listeners try and fire events to bindings that have been disposed (realm is null, so it throws the NPE).