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

Frank Ding dingxmin at linux.vnet.ibm.com
Sat Feb 16 02:18:36 UTC 2013 

Hi Alex and SAM
   Thank you both.

Best regards,
Frank

On 2/15/2013 7:12 PM, Alexander Scherbatiy wrote:
>
>   The fix is pushed to JDK 8: 
> http://hg.openjdk.java.net/jdk8/awt/jdk/rev/4bf242def958
>
>   The java incident 7189299 has not been migrated yet to the new bug 
> system so the bug is pushed with the new id 8008289.
>
>   Thanks,
>   Alexandr.
>
> On 2/13/2013 6:26 PM, Sergey Malenkov wrote:
>> The fix looks like a hack, but it is acceptable in this case.
>>
>> Thanks,
>> SAM
>>
>> On 29.01.2013 9:31, Frank Ding wrote:
>>> Hi Alexandr,
>>>    Please review my 9th attempt @
>>>    http://cr.openjdk.java.net/~dingxmin/7189299/webrev.09/
>>>    The revision also passed test LoadingWebPageToJEditorPane.
>>>
>>> Thanks && Best regards,
>>> Frank
>>>
>>> On 1/28/2013 5:04 PM, Frank Ding wrote:
>>>> Hi Alexandr,
>>>>   Thanks for your suggestion.  I will prepare a patch according to it
>>>> soon.
>>>>
>>>> Best regards,
>>>> Frank
>>>>
>>>> On 1/21/2013 9:41 PM, Alexander Scherbatiy wrote:
>>>>> On 1/18/2013 10:50 AM, Frank Ding wrote:
>>>>>> Hi Alexandr,
>>>>>>   Do you have any idea?
>>>>>
>>>>>    What about one more way removing listeners:
>>>>>     - disadvantage: listeners are hardcoded
>>>>>     - advantage: there is no need to override JButton, JList,... 
>>>>> classes
>>>>>
>>>>>      ------------------------------------------
>>>>>     private void clearModel(Object model) {
>>>>>         if (model instanceof DefaultButtonModel) {
>>>>>             DefaultButtonModel buttonModel = (DefaultButtonModel) 
>>>>> model;
>>>>>             String listenerClass = 
>>>>> "javax.swing.AbstractButton$Handler";
>>>>>
>>>>>             for (ActionListener listener :
>>>>> buttonModel.getActionListeners()) {
>>>>>                 if
>>>>> (listenerClass.equals(listener.getClass().getName())) {
>>>>> buttonModel.removeActionListener(listener);
>>>>>                 }
>>>>>             }
>>>>>             // remove change listeners
>>>>>             // remove item listeners
>>>>>
>>>>>         } else if(model instanceof ListModel){
>>>>>             // remove list listeners
>>>>>         }
>>>>>     }
>>>>>      ------------------------------------------
>>>>>
>>>>>    Thanks,
>>>>>     Alexandr.
>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Frank
>>>>>>
>>>>>> On 12/28/2012 11:16 AM, Frank Ding wrote:
>>>>>>> Hi Alexandr,
>>>>>>>   I did some more investigations.
>>>>>>>   1. The model can be accessed as below which is also illustrated
>>>>>>> in the jtreg test
>>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.07/test/javax/swing/text/html/7189299/bug7189299.java.html 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>   HTMLEditorKit htmlEditorKit = (HTMLEditorKit) 
>>>>>>> html.getEditorKit();
>>>>>>>   StyleContext.NamedStyle style = ((StyleContext.NamedStyle)
>>>>>>> htmlEditorKit.getInputAttributes());
>>>>>>>   DefaultButtonModel model = ((DefaultButtonModel)
>>>>>>> style.getAttribute(StyleConstants.ModelAttribute));
>>>>>>>
>>>>>>>   Though it needs type conversion, model can be exposed to client
>>>>>>> code.  This implies client code has the chance to add listeners.
>>>>>>>   Another way of updating listener in model is by first getting
>>>>>>> JButton or other html Swing components and then call listener
>>>>>>> related api that affects model.  I dumped vars above but did not
>>>>>>> find out possibility for application to get Swing component 
>>>>>>> reference.
>>>>>>>
>>>>>>>   2. As of swing.text.html library, I searched 'ModelAttribute'
>>>>>>> under javax/swing directory with command "grep -R 'ModelAttribute'
>>>>>>> ." which shows aside from FormView and StyleConstants, the classes
>>>>>>> referencing it are HTMLDocument, HTMLWriter.  This means only
>>>>>>> FormView adds listeners to model.
>>>>>>>
>>>>>>>   My first attempt at fixing the problem (only for JButton listener
>>>>>>> leak) is trying to remove all listeners completely. See
>>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.01/. Pavel
>>>>>>> argued that it may cause existing application to malfunction. Then
>>>>>>> I came up with
>>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.02/ where only
>>>>>>> stale listeners are identified and removed using reflection.  But
>>>>>>> Pavel insisted reflection should be prohibited.  So I tried to work
>>>>>>> around it by extending JButton in FormView to access the private
>>>>>>> listener instance such that only stale listener is removed.  The
>>>>>>> perfect revision is
>>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.07/.
>>>>>>>
>>>>>>>   I propose we fix JButton issue as it leads to NPE visible to
>>>>>>> application.  Bug in other components can be put off if Swing team
>>>>>>> intend to fix it elegantly or duplicate code like what is in v.07.
>>>>>>>   Any idea or comment, please let me know.
>>>>>>>
>>>>>>>   Thanks and best regards,
>>>>>>>   Frank
>>>>>>>
>>>>>>> On 12/10/2012 6:40 PM, Alexander Scherbatiy wrote:
>>>>>>>> On 12/6/2012 12:39 PM, Frank Ding wrote:
>>>>>>>>> Hi Alexandr,
>>>>>>>>>    I did several attempts but still have hang issues somewhere.
>>>>>>>>> It seems the new approach of caching gui components created each
>>>>>>>>> time is not practical because of the threading restriction
>>>>>>>>> already imposed on HTMLDocument.
>>>>>>>>>    Can we make compromise by turning to previous solution (v7 in
>>>>>>>>> particular) and generalize it to other gui components (which
>>>>>>>>> means there would be JCheckBoxWrapper, JRadioButtonWrapper,
>>>>>>>>
>>>>>>>>     The issue is that models contain listeners from all previous
>>>>>>>> components. Models are used to store data and state of components
>>>>>>>> when form view is recreated.
>>>>>>>>     FormView adds it's own listeners to components (and as result
>>>>>>>> to the models). Could you make a little investigation to check
>>>>>>>> which other listeners can the models hold?
>>>>>>>>     Can a user adds a listener?  Can listeners be added by other
>>>>>>>> parts of the swing.text.html library?
>>>>>>>>
>>>>>>>>     If old listeners does not play role, it is possible just to
>>>>>>>> remove them all.
>>>>>>>>
>>>>>>>>     Thanks,
>>>>>>>>     Alexandr.
>>>>>>>>
>>>>>>>>> JPasswordFieldWrapper, JTextFieldWrapper, JListWrapper,
>>>>>>>>> JComboBoxWrapper, and JButtonWrapper).  Or shall we fix JButton
>>>>>>>>> only because it causes obvious exception visible to client?  I am
>>>>>>>>> short of ideas...
>>>>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.07/
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Frank
>>>>>>>>>
>>>>>>>>> On 12/5/2012 2:12 PM, Frank Ding wrote:
>>>>>>>>>> Hi Alexandr,
>>>>>>>>>>   I observed the same issue in my environment as well. My
>>>>>>>>>> proposed patch would cause severe regression issues. I am
>>>>>>>>>> looking into the locking issue.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Frank
>>>>>>>>>>
>>>>>>>>>> On 12/3/2012 6:53 PM, Alexander Scherbatiy wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>  Could you check the following sample with your latest fix? It
>>>>>>>>>>> just hangs on my side.
>>>>>>>>>>>
>>>>>>>>>>>    - put the sample.html  and the response.html files on a 
>>>>>>>>>>> server
>>>>>>>>>>>    - update path to the html files in the
>>>>>>>>>>> LoadingWebPageToJEditorPane class
>>>>>>>>>>>
>>>>>>>>>>> -------------     sample.html  ------------
>>>>>>>>>>> <form name="input" action="response.html" method="get">
>>>>>>>>>>> Username: <input type="text" name="user">
>>>>>>>>>>> <input type="submit" value="Submit">
>>>>>>>>>>> </form>
>>>>>>>>>>> -------------     response.html  ------------
>>>>>>>>>>> <html>
>>>>>>>>>>> <body>
>>>>>>>>>>> Hello World!
>>>>>>>>>>> </body>
>>>>>>>>>>> </html>
>>>>>>>>>>> ------------- LoadingWebPageToJEditorPane.html ------------
>>>>>>>>>>> import java.net.URL;
>>>>>>>>>>> import javax.swing.JEditorPane;
>>>>>>>>>>> import javax.swing.JFrame;
>>>>>>>>>>> import javax.swing.JScrollPane;
>>>>>>>>>>> import javax.swing.SwingUtilities;
>>>>>>>>>>>
>>>>>>>>>>> public class LoadingWebPageToJEditorPane {
>>>>>>>>>>>
>>>>>>>>>>>     private static final String HTML_PATH =
>>>>>>>>>>> "http://serever/path/sample.html";
>>>>>>>>>>>
>>>>>>>>>>>     public static void main(String[] a) throws Exception {
>>>>>>>>>>>
>>>>>>>>>>>         SwingUtilities.invokeAndWait(new Runnable() {
>>>>>>>>>>>
>>>>>>>>>>>             @Override
>>>>>>>>>>>             public void run() {
>>>>>>>>>>>                 try {
>>>>>>>>>>>
>>>>>>>>>>>                     JFrame frame = new JFrame();
>>>>>>>>>>> frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
>>>>>>>>>>>
>>>>>>>>>>>                     JEditorPane editorPane = new JEditorPane();
>>>>>>>>>>>                     editorPane.setPage(new URL(HTML_PATH));
>>>>>>>>>>>
>>>>>>>>>>>                     frame.add(new JScrollPane(editorPane));
>>>>>>>>>>>                     frame.setSize(300, 200);
>>>>>>>>>>>                     frame.setVisible(true);
>>>>>>>>>>> SwingUtilities.updateComponentTreeUI(editorPane);
>>>>>>>>>>>                 } catch (Exception ex) {
>>>>>>>>>>>                     throw new RuntimeException(ex);
>>>>>>>>>>>                 }
>>>>>>>>>>>             }
>>>>>>>>>>>         });
>>>>>>>>>>>     }
>>>>>>>>>>> }
>>>>>>>>>>> ---------------------------------------
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Alexandr.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 11/28/2012 10:29 AM, Frank Ding wrote:
>>>>>>>>>>>> Hi Alexandr,
>>>>>>>>>>>>   I created a new patch v8 @
>>>>>>>>>>>> http://cr.openjdk.java.net/~dingxmin/7189299/webrev.08/. It
>>>>>>>>>>>> uses the newly proposed approach mentioned in my last email.
>>>>>>>>>>>> Could you please help to review it again?
>>>>>>>>>>>>
>>>>>>>>>>>>   The patch, of course, passed jtreg test bug7189299.java in
>>>>>>>>>>>> the patch.  What's more, I did additional tests for JComboBox,
>>>>>>>>>>>> JTextField and JList in my IDE by comparing listener numbers
>>>>>>>>>>>> observed during debugging with/without my patch. The listener
>>>>>>>>>>>> numbers were doubled without the fix.  This proves that v8
>>>>>>>>>>>> patch works for all components. I think I can write those
>>>>>>>>>>>> additional tests as jtreg tests after the patch passes review.
>>>>>>>>>>>>
>>>>>>>>>>>>    One notable change is that I had to restrict the scope of
>>>>>>>>>>>> holding a write lock in DefaultStyledDocument because
>>>>>>>>>>>> otherwise, we cannot store the new component created in
>>>>>>>>>>>> FormView.createComponent().  The stack trace is pasted below
>>>>>>>>>>>> for reference.
>>>>>>>>>>>>
>>>>>>>>>>>> Exception in thread "main"
>>>>>>>>>>>> java.lang.reflect.InvocationTargetException
>>>>>>>>>>>>     at java.awt.EventQueue.invokeAndWait(EventQueue.java:1270)
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.SwingUtilities.invokeAndWait(SwingUtilities.java:1350) 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     at bug7189299.main(bug7189299.java:116)
>>>>>>>>>>>> Caused by: java.lang.IllegalStateException: Attempt to mutate
>>>>>>>>>>>> in notification
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.text.AbstractDocument.writeLock(AbstractDocument.java:1357) 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.text.html.HTMLDocument.obtainLock(HTMLDocument.java:1708) 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.text.html.FormView.createComponent(FormView.java:211) 
>>>>>>>>>>>>
>>>>>>>>>>>> ......(omitted)......
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.plaf.basic.BasicTextUI$RootView.insertUpdate(BasicTextUI.java:1602) 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.plaf.basic.BasicTextUI$UpdateHandler.insertUpdate(BasicTextUI.java:1861) 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.text.AbstractDocument.fireInsertUpdate(AbstractDocument.java:201) 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.text.DefaultStyledDocument.create(DefaultStyledDocument.java:155) 
>>>>>>>>>>>>
>>>>>>>>>>>> <------   FormView.createComponent is triggered by this call
>>>>>>>>>>>> which has already held the lock
>>>>>>>>>>>>     at
>>>>>>>>>>>> javax.swing.text.html.HTMLDocument.create(HTMLDocument.java:469) 
>>>>>>>>>>>>
>>>>>>>>>>>> ......(omitted)......
>>>>>>>>>>>>
>>>>>>>>>>>> This change did violate what is documented in
>>>>>>>>>>>> AbstractDocument.writeLock that "This situation violates the
>>>>>>>>>>>> bean event model where order of delivery is not guaranteed and
>>>>>>>>>>>> all listeners should be notified before further mutations are
>>>>>>>>>>>> allowed. " However, without the change of shrinking lock
>>>>>>>>>>>> scope, the component cannot be saved once it is created in
>>>>>>>>>>>> FormView.createComponent(). I found it is even harder to save
>>>>>>>>>>>> it later in code but perhaps there does exist a more
>>>>>>>>>>>> appropriate place to do this. If you have any better
>>>>>>>>>>>> suggestion, I am glad to know.
>>>>>>>>>>>>
>>>>>>>>>>>>   Also I searched references to
>>>>>>>>>>>> 'StyleConstants.ComponentAttribute' as you asked. The result
>>>>>>>>>>>> is listed below.  Note that the command 'grep' is invoked
>>>>>>>>>>>> under openjdk 8 top directory.
>>>>>>>>>>>> $ grep -R 'ComponentAttribute' .
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/text/StyleConstants.java:
>>>>>>>>>>>> public static final Object ComponentAttribute = new
>>>>>>>>>>>> CharacterConstants("component");
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/text/StyleConstants.java:
>>>>>>>>>>>> return (Component) a.getAttribute(ComponentAttribute);
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/text/StyleConstants.java:
>>>>>>>>>>>> a.addAttribute(ComponentAttribute, c);
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/text/StyleConstants.java:
>>>>>>>>>>>> Background, ComponentAttribute, IconAttribute,
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/text/StyledEditorKit.java:
>>>>>>>>>>>> set.removeAttribute(StyleConstants.ComponentAttribute);
>>>>>>>>>>>> Binary file ./jdk/test/sun/tools/jhat/jmap.bin matches
>>>>>>>>>>>>
>>>>>>>>>>>> So ComponentAttribute is not referenced by other classes
>>>>>>>>>>>> except StyledEditorKit.  StyleConstants exposes
>>>>>>>>>>>> ComponentAttribute in getComponent() and setComponent()
>>>>>>>>>>>> methods.  References to StyleConstants.getComponent and
>>>>>>>>>>>> StyleConstants.setComponent are further searched in repo.
>>>>>>>>>>>>
>>>>>>>>>>>> $ grep -R 'StyleConstants.getComponent' .
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/text/ComponentView.java: *
>>>>>>>>>>>> the element (by calling StyleConstants.getComponent). A
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/text/ComponentView.java:
>>>>>>>>>>>> Component comp = StyleConstants.getComponent(attr);
>>>>>>>>>>>>
>>>>>>>>>>>> $ grep -R 'StyleConstants.setComponent' .
>>>>>>>>>>>> ./jdk/src/share/classes/javax/swing/JTextPane.java:
>>>>>>>>>>>> StyleConstants.setComponent(inputAttributes, c);
>>>>>>>>>>>>
>>>>>>>>>>>> From the facts above, I think we are sufficiently confident to
>>>>>>>>>>>> use ComponentAttribute.
>>>>>>>>>>>>
>>>>>>>>>>>> Please let me know if you have any comments and I can do more
>>>>>>>>>>>> regression tests and provide more jtreg test cases as next 
>>>>>>>>>>>> step.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks and regards,
>>>>>>>>>>>> Frank
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/15/2012 5:18 PM, Alexander Scherbatiy wrote:
>>>>>>>>>>>>> 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