<Swing Dev> 7189299 DefaultButtonModel instance keeps stale listeners of JButton in case of multiple SwingUtilities.updateComponentTreeUI() calls
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Tue Nov 13 11:49:06 UTC 2012
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
>>>>>
>>>>>
>>>>> On 11/1/2012 8:08 PM, Alexander Scherbatiy wrote:
>>>>>> On 11/1/2012 10:35 AM, Frank Ding wrote:
>>>>>>> Hi Alexandr,
>>>>>>> Thanks for your review. The problem that the JButton does not
>>>>>>> release itself as you mentioned is, after taking closer look at
>>>>>>> relevant code again, that I removed instance
>>>>>>> AbstractButton.Handler as change listener where repainting the
>>>>>>> button press animation is defined. So this time I get around
>>>>>>> this issue and here comes v6 patch @
>>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.06/
>>>>>>>
>>>>>>> Below are answers to what you have raised in previous email.
>>>>>>> 1. if ( if (!listener.equals(this)) { that 'this' means the
>>>>>>> JButtonWrapper instead of an action listener.
>>>>>>> 'this' refers to JButtonWrapper.this because the closest class
>>>>>>> definition is JButtonWrapper. I also verified it during my
>>>>>>> debugging.
>>>>>>
>>>>>> Could you give an example there this check is useful? It
>>>>>> seems that the JButtonWrapper does not implement ActionListener
>>>>>> interface and so the listener and the this always should not be
>>>>>> equal.
>>>>>>
>>>>>>
>>>>>> I dumped action listeners from the button and model before the
>>>>>> fix and after it.
>>>>>> Before the fix the button has the latest FormView listener and
>>>>>> the model has AbstractButton$Handler added each time after a
>>>>>> component input creation.
>>>>>> After the fix both the button and the model have the FormView
>>>>>> listener and the button does not have the AbstractButton$Handler.
>>>>>> Could you preserve the listeners location and just remove old
>>>>>> listeners from the model?
>>>>>>
>>>>>> The fix sets the model to the JButtonWrapper first and than
>>>>>> removes unneccessary listeners from the model. Is it possible to
>>>>>> rearrange order of these operations?
>>>>>>
>>>>>> I see that you check number of the listeners in the test.
>>>>>> 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.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>>
>>>>>>> 2. Why are two FormViews created in this sample?
>>>>>>> SwingUtilities.updateComponentTreeUI() triggers html View
>>>>>>> hierarchy to update by instantiating new UI instances involved
>>>>>>> in the html form. It could have been avoided to have 2 View
>>>>>>> instances but we are restricted to current design that LF change
>>>>>>> causes creation of View instance which further leads to creation
>>>>>>> of new UI instances. Creation of new UI instances upon LF
>>>>>>> change can be referenced in BasicTextUI.modelChanged().
>>>>>>>
>>>>>>> 3. Why are there 2 listeners added to the button model in this
>>>>>>> sample?
>>>>>>> The issue is caused by combination of the 2 factors. First, a
>>>>>>> shared ButtonModel is created per html Element. Second,
>>>>>>> JButton.setModel() automatically registers its private Handler
>>>>>>> in the new model but no way to deregister it. The first is a
>>>>>>> design choice but the second is a design flaw. Consequence is
>>>>>>> that the shared ButtonModel instance bears more and more
>>>>>>> JButton.Handler.
>>>>>>>
>>>>>>> I think v6 patch is promising as a final fix. If you have any
>>>>>>> further concern or comment, please don't hesitate to let me
>>>>>>> know. Thanks again for your careful review.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Frank
>>>>>>>
>>>>>>> On 10/30/2012 9:38 PM, Alexander Scherbatiy wrote:
>>>>>>>> Hi Frank,
>>>>>>>>
>>>>>>>> I tried your V5 patch and run the test described in the issue
>>>>>>>> (see below).
>>>>>>>> I pressed the button and it does not release. This is because
>>>>>>>> the AbstractButton.Handler listener is removed because of the fix.
>>>>>>>> You could check the line
>>>>>>>> 941 if (!listener.equals(this)) {
>>>>>>>> that 'this' means the JButtonWrapper instead of an action
>>>>>>>> listener.
>>>>>>>>
>>>>>>>> Could you also look deeper in the code and say why two
>>>>>>>> FormViews are created (and each adds it's own listener to the
>>>>>>>> button model) in this sample?
>>>>>>>>
>>>>>>>> -----------------------------------------------
>>>>>>>> import java.awt.*;
>>>>>>>> import javax.swing.*;
>>>>>>>>
>>>>>>>> public class FormSubmit {
>>>>>>>>
>>>>>>>> public static void main(String[] args) throws Exception {
>>>>>>>> SwingUtilities.invokeAndWait(new Runnable() {
>>>>>>>>
>>>>>>>> @Override
>>>>>>>> public void run() {
>>>>>>>>
>>>>>>>> JEditorPane html = new JEditorPane("text/html",
>>>>>>>> "<html><body><FORM ACTION=\"examplescript.cgi\"><INPUT
>>>>>>>> type=\"submit\" value=\"Submit\"></FORM></BODY></HTML>");
>>>>>>>> JFrame frame = new JFrame()
>>>>>>>> frame.setLayout(new BorderLayout());
>>>>>>>> frame.add(html, BorderLayout.CENTER);
>>>>>>>> frame.setSize(200, 100);
>>>>>>>> frame.setDefaultCloseOperation(frame.EXIT_ON_CLOSE);
>>>>>>>> frame.setVisible(true); // Uncomment this line
>>>>>>>> to see the Exception on JDK 7
>>>>>>>> SwingUtilities.updateComponentTreeUI(html);
>>>>>>>> }
>>>>>>>> });
>>>>>>>> }
>>>>>>>> }
>>>>>>>> -----------------------------------------------
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alexandr.
>>>>>
>>>>
>>>
>>
>
More information about the swing-dev
mailing list