<Swing Dev> 7189299 DefaultButtonModel instance keeps stale listeners of JButton in case of multiple SwingUtilities.updateComponentTreeUI() calls

[Frank Ding]

Hi Alexandr,
   After a couple of hours investigating the possibility of fixing 
JComboBox.setModel(null) and JTextComponent.setComponent(null), I found 
that
1. In JComboBox.setModel, the new model, null in this case, is 
eventually passed to BasicComboPopup.propertyChange where JList.setModel 
is called.  JList.setModel rejects the null model because of its api 
restriction.  Below I listed the offending call stacks in my IDE.  This 
makes sense as the associated drop down JList needs new model.  However, 
it makes fixing JComboBox.setModel(null) hard or impossible.
Exception in thread "main" java.lang.IllegalArgumentException: model 
must be non null
     at javax.swing.JList.setModel(JList.java:1674)
     at 
javax.swing.plaf.basic.BasicComboPopup$Handler.propertyChange(BasicComboPopup.java:939)
     at 
java.beans.PropertyChangeSupport.fire(PropertyChangeSupport.java:335)
     at 
java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:327)
     at 
java.beans.PropertyChangeSupport.firePropertyChange(PropertyChangeSupport.java:263)
     at java.awt.Component.firePropertyChange(Component.java:8413)
     at javax.swing.JComboBox.setModel(JComboBox.java:322)

2. JTextComponent.setComponent(null) can be fixed but code change in 
BasicTextUI is also required.

Since setting null model to JComboBox, JList and JTextComponent is 
impossible or dangerous, just as you mentioned, we could set a non null 
new model to these UI components just for the purpose of having the side 
effect of removing listeners from old model.  Are you ok with this approach?
By the way, I will investigate your another question "Could you also 
check that the StyleConstants.ComponentAttribute property value can't be 
rewritten by the JDK code or by public methods." and reply soon.

Best regards,
Frank

On 11/13/2012 7:49 PM, Alexander Scherbatiy wrote:
> On 11/13/2012 11:53 AM, Frank Ding wrote:
>> Hi Alexandr,
>>   As for your comment "Could you create an issue that a 
>> JComboBox.setModel(null) call throws NPE? You could fix it before the 
>> 7189299 issue. ", I created a bug with internal review ID of 2381499 
>> on JComboBox.setModel(null).  But test shows that 
>> JPasswordField.setDocument(null), JTextField.setDocument(null), 
>> JList.setModel(null) and JTextArea.setDocument(null) all throw NPE.  
>> Particularly, JList.setModel(null) has null check and throws 
>> IllegalArgumentException("model" must be non null").  Shall I include 
>> those components as well?
>
>     There is the javadoc for the JList setModel() method: Throws 
> IllegalArgumentException - if model is null.  So it is undesirable to 
> change the public API.
>     However, it is possible to set a new empty model for the JList. 
> The list listeners should be removed from the old model in this case.
>
>     You could have only 2 issues: one for components that allow to set 
> a null model but throws NPE (like JComboBox) and another for 
> components that does not allow to set null model but they do not 
> remove listeners from the old model in case if a new model is set.
>
>   Thanks,
>   Alexandr.
>
>>   Thanks for your guidance in advance.
>>
>> Best regards,
>> Frank
>>
>> On 11/8/2012 10:55 PM, Alexander Scherbatiy wrote:
>>> On 11/7/2012 10:56 AM, Frank Ding wrote:
>>>> Hi Alexandr,
>>>>   Unfortunately, all the JComponents involved in 
>>>> FormView.createComponent() have bugs!
>>>>   I have done tests for all other swing components, i.e. JCheckBox, 
>>>> JRadioButton, JPasswordField, JTextField, JList, JComboBox and 
>>>> JTextField. Sadder news is that if we fix all components in the 
>>>> same way as I did for JButton, we need to subclass them all, 
>>>> resulting in JCheckBoxWrapper, JRadioButtonWrapper, 
>>>> JPasswordFieldWrapper, JTextFieldWrapper, JListWrapper, 
>>>> JComboBoxWrapper, JTextFieldWrapper plus JButtonWrapper!  This 
>>>> approach becomes unacceptable when all swing components are affected.
>>>>   Shall we fix it in other way illustrated below?
>>>> 1. Whenever a swing component is created, it is kept in 
>>>> AttributeSet, as what is now for model.
>>>> 2. In FormView.createComponent(), the old swing component can be 
>>>> retrieved via 
>>>> attr.getAttribute(StyleConstants.ComponentAttribute).  Note that 
>>>> ComponentAttribute is newly added.
>>>       This way should be carefully investigated that it does not 
>>> introduce new memory leaks.
>>>       Could you also check that the 
>>> StyleConstants.ComponentAttribute property value can't be rewritten 
>>> by the JDK code or by public methods.
>>>
>>>> 3. Before setting shared model to a newly initialized swing 
>>>> component, call oldComp.setModel(null), delegating deregistration 
>>>> to setModel method.
>>>> 4. Seems some setModel such as AbstractButton.setModel() can 
>>>> function well when the param, new model, is null while 
>>>> JComboBox.setModel() will throw NPE in case of null param.
>>>      Could you create an issue that a JComboBox.setModel(null) call 
>>> throws NPE? You could fix it before the 7189299 issue.
>>>
>>>      Thanks,
>>>      Alexandr.
>>>
>>>> 5. Add null check code in those problematic setModel or setDocument 
>>>> methods.
>>>>
>>>> Any idea is appreciated.  Thanks.
>>>>
>>>> Best regards,
>>>> Frank
>>>>
>>>> On 11/6/2012 9:26 PM, Alexander Scherbatiy wrote:
>>>>> On 11/5/2012 11:20 AM, Frank Ding wrote:
>>>>>> Hi Alexander,
>>>>>>   Based on your comments last time, I refined my patch of v6 and 
>>>>>> offered v7 @
>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.07/
>>>>>
>>>>>      This version of the fix looks good for me.
>>>>>      It seems that it is the only good way to check that a button 
>>>>> model contains AbstarctButton.Handler listener.
>>>>>
>>>>>     Could you also check that others models  used in the 
>>>>> createComponent() method do not have such memory leaks (even the 
>>>>> NPE is not thrown)?
>>>>>
>>>>>>   4.  Could you add the check that the action listener is invoked 
>>>>>> only once after a component tree updating and the action does the 
>>>>>> same that it does before a component tree updating?
>>>>>>   Answer:  I am afraid I could not make it in the auto test 
>>>>>> (bug7189299.java) but I can achieve it to some degree in manual 
>>>>>> test FormSubmit, the one you illustrated below.
>>>>>>   My idea of checking it in FormSubmit.java is subclassing 
>>>>>> JEditorPane and overriding 'public EditorKit getEditorKit()' 
>>>>>> where stack traces can be examined in the overridden method to 
>>>>>> make sure FormView.submitData occurs only once.  This approach 
>>>>>> works because FormView.submitData() calls 
>>>>>> JEditorPane.getEditorKit but is tricky.  However, it's the only 
>>>>>> way I can think of without any additional framework support.  If 
>>>>>> you are in favor of adding the check in FormSubmit.java, I am 
>>>>>> willing to do that.
>>>>>
>>>>>       At least a separated manual test can be added that asks a 
>>>>> user to put a response.html file to a server and according to the 
>>>>> server url checks that JTeditor pane show the response text after 
>>>>> a button pressing.
>>>>>
>>>>>         html = new JEditorPane("text/html",
>>>>>                 "<html><body><form action=\"" + userURL + "\">"
>>>>>                         + "<input type=submit name=submit 
>>>>> value=\"submit\"/>"
>>>>>                         + "</form></body></html>");
>>>>>
>>>>>  response.html:
>>>>> <html> <body> Hello World! </body> </html>
>>>>>
>>>>>   Thanks,
>>>>>   Alexandr.
>>>>>
>>>>>>   Thanks again for all your comments and your support.  Once 
>>>>>> again, if you have any further concern or comment, please don't 
>>>>>> hesitate to let me know.
>>>>>>
>>>>>> Best regards,
>>>>>> Frank
>>>>>>