<Swing Dev> 7189299 DefaultButtonModel instance keeps stale listeners of JButton in case of multiple SwingUtilities.updateComponentTreeUI() calls
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Mon Dec 3 10:53:11 UTC 2012
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