The fix looks good for me.
Thanks,
Alexandr.
On 1/29/2013 9:31 AM, 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
