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

[Alexander Scherbatiy]

On 11/14/2012 10:22 AM, Frank Ding wrote:
> 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?
      Yes. Please, try this and run the html swing  automated tests from 
the test/javax/swing/text/html diroctory to check possible regressions.

      Thanks,
      Alexandr.

> 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
>>>>>>>
>