<Swing Dev> 7189299 DefaultButtonModel instance keeps stale listeners of JButton in case of multiple SwingUtilities.updateComponentTreeUI() calls
Frank Ding
dingxmin at linux.vnet.ibm.com
Fri Dec 28 03:16:01 UTC 2012
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