<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 Jan 18 06:50:50 UTC 2013
Hi Alexandr,
Do you have any idea?
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