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
