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

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Fri Feb 15 11:12:34 UTC 2013


   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