<Swing Dev> 7189299 DefaultButtonModel instance keeps stale listeners of JButton in case of multiple SwingUtilities.updateComponentTreeUI() calls
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Nov 15 09:18:33 UTC 2012
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
>>>>>>>
>
More information about the swing-dev
mailing list