<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
Wed Dec 5 06:12:46 UTC 2012
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