Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
On Thu, 5 Mar 2020 18:00:25 GMT, Kevin Rushforth wrote: >> why a second pr for the same issue (see >> https://github.com/openjdk/jfx/pull/73)? particularly one that doesn't do >> much more/else/better (than clamping, which isn't good enough)? > > I have exactly the same question. > > In general, it's better to work with the author of an existing PR instead of > creating a new one. If the original PR #73 is completely stalled, then it > might make sense, but not until then. I wasn't aware of PR #73, I only saw the (very old) issue in the JDK Bug System and started to look into the issue. The fact that two different people start to look into the same issue shows it is important however :-). Should I close this PR and participate in PR #73? - PR: https://git.openjdk.java.net/jfx/pull/138
Re: ComboBox keypress discrepancy
Hi Dirk,
Thanks for reaching out. As stated earlier, I want to know what exactly is
causing this change in behaviour. I also want to know what is the expected
behaviour in this case: should TAB key press trigger when the popupwindow is
showing?
-- Abhinay
From: Dirk Lemmermann
Sent: Thursday, March 5, 2020 6:39 PM
To: Abhinay Agarwal
Cc: openjfx-dev@openjdk.java.net
Subject: Re: ComboBox keypress discrepancy
So what info do you need? What test do you want us to run?
I ran it on MacOS X with Java 14ea and I DO NOT see the „TAB“ output.
Dirk
Am 05.03.2020 um 11:43 schrieb Abhinay Agarwal
mailto:abhinay_agar...@live.com>>:
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.ComboBox;
import javafx.scene.input.KeyEvent;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;
public class Main extends Application {
@Override
public void start(Stage primaryStage) {
final ComboBox stringComboBox = new ComboBox<>();
stringComboBox.getItems().addAll("John", "Jacob", "Schmidt");
stringComboBox.addEventHandler(KeyEvent.KEY_PRESSED, kp ->
System.out.println(kp.getCode()));
final Scene scene = new Scene(new BorderPane(stringComboBox), 300, 275);
primaryStage.setScene(scene);
primaryStage.show();
}
public static void main(String[] args) {
launch(args);
}
}
Re: [Rev 03] RFR: 8236259: MemoryLeak in ProgressIndicator
On Mon, 2 Mar 2020 10:09:57 GMT, Florian Kirmaier wrote:
>> Hi everyone,
>>
>> ticket: https://bugs.openjdk.java.net/browse/JDK-8236259
>>
>> The fix itself is quite straight forward.
>> It basically just removed the listener which causes the leak.
>>
>> The unit-test for the fix is a bit more complicated.
>>
>> I added a library JMemoryBuddy https://github.com/Sandec/JMemoryBuddy
>> (written by myself), which simplifies testing for memory leaks.
>> I think there are many places in the JavaFX-codebase that could highly
>> benefit from this library.
>> It could also simplify many of the already existing unit tests.
>> It makes testing for memory-leaks readably and reliable.
>> It would also be possible to just copy the code of the library into the
>> JavaFX-codebase.
>> It only contains a single class.
>>
>> I also had to make a method public, to write the test. I'm open to ideas,
>> how I could solve it differently.
>
> The pull request has been updated with 1 additional commit.
The fix looks good to me. I've verified that it fixes the leak.
The newly added test looks good as well, with some comments left inline
(formatting, a missing copyright header, and a couple other suggestions).
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java
line 331:
> 330: // clean up the old determinateIndicator
> 331: if(determinateIndicator != null) {
> 332: determinateIndicator.unregisterListener();
Minor: add a space after the `if`.
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 1:
> 1: package test.javafx.scene.control;
> 2:
You need a copyright header for this file.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java
line 60:
> 59: import javafx.scene.shape.Circle;
> 60: import javafx.scene.text.Font;
> 61: import javafx.scene.text.Text;
You don't use any of the 3 newly added imports in this patch, so they can be
removed (reverted).
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 42:
> 41: indicator.setProgress(1.0);
> 42: Assert.assertTrue("size was: " +
> indicator.getChildrenUnmodifiable().size(),
> indicator.getChildrenUnmodifiable().size() == 1);
> 43: detIndicator = new
> WeakReference(indicator.getChildrenUnmodifiable().get(0));
This assertion can be done as:
assertEquals("size is wrong", 1, indicator.getChildrenUnmodifiable().size());
It will be easier to read and more straight-forward.
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 57:
> 56: @BeforeClass
> 57: public static void initFX() {
> 58: startupLatch = new CountDownLatch(1);
The `initFX` method can be simplified a bit using a pattern we've adopted in
our newer tests. See
[QuadraticCssTimeTest.java](https://github.com/openjdk/jfx/blob/jfx14/tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java#L84).
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 72:
> 71: public void memoryTest() throws
> NoSuchFieldException,IllegalAccessException {
> 72: System.out.println("detIndicator: " + detIndicator.get());
> 73: assertCollectable(detIndicator);
I recommend to comment out this debugging statement.
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 80:
> 79: createGarbage();
> 80: System.gc();
> 81:
We recommend also calling `System.runFinalization();` for GC related tests.
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 85:
> 84: Thread.sleep(100);
> 85: } catch (InterruptedException e) {}
> 86: counter = counter + 1;
You can skip the try / catch if you declare the test method as `throws
Exception`
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 82:
> 81:
> 82: while(counter < 10 && weakReference.get() != null) {
> 83: try {
Please add a space after the `while`.
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 91:
> 90:
> 91: if(weakReference.get() != null) {
> 92: throw new AssertionError("Content of WeakReference was not
> collected. content: " + weakReference.get());
Add a space after the `if`
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 95:
> 94: }
> 95: public static void createGarbage() {
> 96: LinkedList list = new LinkedList();
I think this is fine, but I'm curious as to whether you've actually found this
(creating garbage) to be necessary.
tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java
line 98:
> 97: int counter = 0;
> 98: while(counter < 99) {
Re: [Rev 03] RFR: 8236259: MemoryLeak in ProgressIndicator
On Thu, 19 Dec 2019 14:43:29 GMT, Jeanette Winzenburg wrote: >> I don't see an API in the discussion, on how to remove a specific listener. >> Did i miss something? It seems to me, that the API is written to support >> only a single listener per property. > > the api is in a related issue (should be noted in the issue above). The > use-cases mentioned in the isssue: it's a do-once scenario with the skin (and > its subclasses) as the only user: multiple listeners can be registered, > typically by a skin and/or its subclasses, each can be pre/postpended (or > replaced) in the chain for that property and all are unregistered at dispose > time. So I think it's really bad to use it in another collaborator like the > indicator, particularly if that is re-created often and each of the instances > need to remove the listener. I think this will be fine. The `text` node is private to the `DeterminateIndicator` inner class and not exposed, so there would never be another listener. - PR: https://git.openjdk.java.net/jfx/pull/71
Re: [Integrated] RFR: 8240451: JavaFX javadoc build fails with JDK 14
Changeset: b2ac76a9 Author:Kevin Rushforth Date: 2020-03-05 20:58:03 + URL: https://git.openjdk.java.net/jfx/commit/b2ac76a9 8240451: JavaFX javadoc build fails with JDK 14 Reviewed-by: nlisker, jvos ! modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanObjectProperty.java ! modules/javafx.base/src/main/java/javafx/beans/property/adapter/JavaBeanObjectPropertyBuilder.java ! modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanObjectProperty.java ! modules/javafx.base/src/main/java/javafx/beans/property/adapter/ReadOnlyJavaBeanObjectPropertyBuilder.java ! modules/javafx.base/src/main/java/javafx/beans/value/WritableObjectValue.java
Re: RFR: 8240451: JavaFX javadoc build fails with JDK 14
On Tue, 3 Mar 2020 16:12:23 GMT, Kevin Rushforth wrote: > The JDK 14 javadoc will emit a warning for an `@pram` tag of a generic > parameter if not surrounded by `<` and `>`. This will in turn fail the build, > since we treat warnings as errors. There are 5 classes in JavaFX with this > error. The fix is to replace `@param T` with `@param ` in those 5 places. > > > I have tested this fix using javadoc from both JDK 13 and JDK 14. Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/133
Re: RFR: 8240451: JavaFX javadoc build fails with JDK 14
On Thu, 5 Mar 2020 17:17:57 GMT, Nir Lisker wrote: >> The JDK 14 javadoc will emit a warning for an `@pram` tag of a generic >> parameter if not surrounded by `<` and `>`. This will in turn fail the >> build, since we treat warnings as errors. There are 5 classes in JavaFX with >> this error. The fix is to replace `@param T` with `@param ` in those 5 >> places. >> >> >> I have tested this fix using javadoc from both JDK 13 and JDK 14. > > Using a rough regex search of `@param [A-Z]\s` I also found: > > `com.sun.glass.ui.Window#requestInput`: `@param M` > `com.sun.javafx.tk.TKStage#requestInput`: `@param M` > > Though those are internal. Thanks for the review. Yeah, I only looked at the public API classes. Can I get a "R"eviewer to look at this? Maybe @arapte or @aghaisas ? - PR: https://git.openjdk.java.net/jfx/pull/133
Re: [Integrated] RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
Changeset: cf0bba62 Author:Arun Joseph Committer: Kevin Rushforth Date: 2020-03-05 19:28:25 + URL: https://git.openjdk.java.net/jfx/commit/cf0bba62 8240211: Stack overflow on Windows 32-bit can lead to crash Reviewed-by: ghb, kcr, jvos ! modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp
Re: RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
On Thu, 5 Mar 2020 14:51:03 GMT, Arun Joseph wrote: > Issue: The stack pointer is checked close to the stack limit during the last > iteration of calling frameLoaded() and then, grows beyond the thread's stack > range causing a stack overflow and crashes. This occurs as the stack grows by > an amount larger than the reserved zone at the end of the stack. > > Fix: Reduce the stack range visible to the thread in > [StackBounds.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp) > similar to Mac and Linux. This causes the stack pointer check to throw a > StackOverflowError during the last iteration. Marked as reviewed by jvos (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/137
Re: RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
On Thu, 5 Mar 2020 14:51:03 GMT, Arun Joseph wrote: > Issue: The stack pointer is checked close to the stack limit during the last > iteration of calling frameLoaded() and then, grows beyond the thread's stack > range causing a stack overflow and crashes. This occurs as the stack grows by > an amount larger than the reserved zone at the end of the stack. > > Fix: Reduce the stack range visible to the thread in > [StackBounds.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp) > similar to Mac and Linux. This causes the stack pointer check to throw a > StackOverflowError during the last iteration. I did a full build / test and verified that on Windows 32-bit the apply-style-iframe-crash.html test crashes without the fix and passes with the fix. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/137
Re: RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
On Thu, 5 Mar 2020 17:46:17 GMT, Johan Vos wrote: >> Looks good to me. >> I believe you have executed DRT on both 64 & 32 bit build. > > This looks correct and very valuable to me. Would be great if there was a > test that results in a crash before? Here is a pointer to the [apply-style-iframe-crash.html](https://github.com/WebKit/webkit/blob/8e46d906531cf38fd860e30fafdf5a3021568d6c/LayoutTests/editing/style/apply-style-iframe-crash.html) test case that crashes on Windows 32-bit without this fix and passes with this fix. - PR: https://git.openjdk.java.net/jfx/pull/137
Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
On Thu, 5 Mar 2020 17:01:25 GMT, Jeanette Winzenburg wrote: >> This PR fixes JDK-8176270 by clamping the end index of the selected text to >> the length of the text. > > why a second pr for the same issue (see > https://github.com/openjdk/jfx/pull/73)? particularly one that doesn't do > much more/else/better (than clamping, which isn't good enough)? I have exactly the same question. In general, it's better to work with the author of an existing PR instead of creating a new one. If the original PR #73 is completely stalled, then it might make sense, but not until then. - PR: https://git.openjdk.java.net/jfx/pull/138
Re: RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
On Thu, 5 Mar 2020 16:50:55 GMT, Guru Hb wrote: >> Issue: The stack pointer is checked close to the stack limit during the last >> iteration of calling frameLoaded() and then, grows beyond the thread's stack >> range causing a stack overflow and crashes. This occurs as the stack grows >> by an amount larger than the reserved zone at the end of the stack. >> >> Fix: Reduce the stack range visible to the thread in >> [StackBounds.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp) >> similar to Mac and Linux. This causes the stack pointer check to throw a >> StackOverflowError during the last iteration. > > Looks good to me. > I believe you have executed DRT on both 64 & 32 bit build. This looks correct and very valuable to me. Would be great if there was a test that results in a crash before? - PR: https://git.openjdk.java.net/jfx/pull/137
Re: RFR: 8240451: JavaFX javadoc build fails with JDK 14
On Tue, 3 Mar 2020 16:12:23 GMT, Kevin Rushforth wrote: > The JDK 14 javadoc will emit a warning for an `@pram` tag of a generic > parameter if not surrounded by `<` and `>`. This will in turn fail the build, > since we treat warnings as errors. There are 5 classes in JavaFX with this > error. The fix is to replace `@param T` with `@param ` in those 5 places. > > > I have tested this fix using javadoc from both JDK 13 and JDK 14. Using a rough regex search of `@param [A-Z]\s` I also found: `com.sun.glass.ui.Window#requestInput`: `@param M` `com.sun.javafx.tk.TKStage#requestInput`: `@param M` Though those are internal. - Marked as reviewed by nlisker (Committer). PR: https://git.openjdk.java.net/jfx/pull/133
Re: RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
On Thu, 5 Mar 2020 16:01:10 GMT, Robert Lichtenberger wrote: > This PR fixes JDK-8176270 by clamping the end index of the selected text to > the length of the text. why a second pr for the same issue (see https://github.com/openjdk/jfx/pull/73)? particularly one that doesn't do much more/else/better (than clamping, which isn't good enough)? - PR: https://git.openjdk.java.net/jfx/pull/138
Re: RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
On Thu, 5 Mar 2020 14:51:03 GMT, Arun Joseph wrote: > Issue: The stack pointer is checked close to the stack limit during the last > iteration of calling frameLoaded() and then, grows beyond the thread's stack > range causing a stack overflow and crashes. This occurs as the stack grows by > an amount larger than the reserved zone at the end of the stack. > > Fix: Reduce the stack range visible to the thread in > [StackBounds.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp) > similar to Mac and Linux. This causes the stack pointer check to throw a > StackOverflowError during the last iteration. Looks good to me. I believe you have executed DRT on both 64 & 32 bit build. - Marked as reviewed by ghb (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/137
Re: RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
On Thu, 5 Mar 2020 14:51:03 GMT, Arun Joseph wrote: > Issue: The stack pointer is checked close to the stack limit during the last > iteration of calling frameLoaded() and then, grows beyond the thread's stack > range causing a stack overflow and crashes. This occurs as the stack grows by > an amount larger than the reserved zone at the end of the stack. > > Fix: Reduce the stack range visible to the thread in > [StackBounds.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp) > similar to Mac and Linux. This causes the stack pointer check to throw a > StackOverflowError during the last iteration. I will review this. @guruhb can you also review it? - PR: https://git.openjdk.java.net/jfx/pull/137
RFR: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
This PR fixes JDK-8176270 by clamping the end index of the selected text to the length of the text. - Commits: - e78e8793: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException - d849c67c: Merge remote-tracking branch 'upstream/master' - 846d0483: Merge remote-tracking branch 'upstream/master' - 9ceb21bc: Merge remote-tracking branch 'upstream/master' - 2109d5a0: Merge remote-tracking branch 'upstream/master' - acfa63be: Merge remote-tracking branch 'upstream/master' - 7c5cf198: 8232524: Test cleanup: terminate background thread upon failure. - 7e80839f: 8232524: SynchronizedObservableMap cannot be be protected for copying/iterating - 8ecf3545: JDK-8232524 fixed. Changes: https://git.openjdk.java.net/jfx/pull/138/files Webrev: https://webrevs.openjdk.java.net/jfx/138/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8176270 Stats: 20 lines in 3 files changed: 19 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/138.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/138/head:pull/138 PR: https://git.openjdk.java.net/jfx/pull/138
RFR: 8240211: Stack overflow on Windows 32-bit can lead to crash
Issue: The stack pointer is checked close to the stack limit during the last iteration of calling frameLoaded() and then, grows beyond the thread's stack range causing a stack overflow and crashes. This occurs as the stack grows by an amount larger than the reserved zone at the end of the stack. Fix: Reduce the stack range visible to the thread in [StackBounds.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WTF/wtf/StackBounds.cpp) similar to Mac and Linux. This causes the stack pointer check to throw a StackOverflowError during the last iteration. - Commits: - f780c079: 8240211: Stack overflow on Windows 32-bit can lead to crash Changes: https://git.openjdk.java.net/jfx/pull/137/files Webrev: https://webrevs.openjdk.java.net/jfx/137/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8240211 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/137.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/137/head:pull/137 PR: https://git.openjdk.java.net/jfx/pull/137
Re: ComboBox keypress discrepancy
So what info do you need? What test do you want us to run?
I ran it on MacOS X with Java 14ea and I DO NOT see the „TAB“ output.
Dirk
> Am 05.03.2020 um 11:43 schrieb Abhinay Agarwal :
>
> import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.ComboBox;
> import javafx.scene.input.KeyEvent;
> import javafx.scene.layout.BorderPane;
> import javafx.stage.Stage;
>
> public class Main extends Application {
>
>@Override
>public void start(Stage primaryStage) {
>final ComboBox stringComboBox = new ComboBox<>();
>stringComboBox.getItems().addAll("John", "Jacob", "Schmidt");
>stringComboBox.addEventHandler(KeyEvent.KEY_PRESSED, kp ->
> System.out.println(kp.getCode()));
>
>final Scene scene = new Scene(new BorderPane(stringComboBox), 300,
> 275);
>primaryStage.setScene(scene);
>primaryStage.show();
>}
>
>public static void main(String[] args) {
>launch(args);
>}
> }
ComboBox keypress discrepancy
Hi,
We have come across a behavioural change in ComboBox b/w JavaFX 8 and 9+.
We are not sure if its a regression or a bug fix which is causing it.
Therefore, I am reaching out to the community for insight.
If you run the following example, open the popup window in ComboBox and press
TAB: JavaFX 8 registers the key press event, where as,
JavaFX 9 and later do not register it. The TAB keypress is registered
successfully in bot cases when the popup window is not showing.
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.ComboBox;
import javafx.scene.input.KeyEvent;
import javafx.scene.layout.BorderPane;
import javafx.stage.Stage;
public class Main extends Application {
@Override
public void start(Stage primaryStage) {
final ComboBox stringComboBox = new ComboBox<>();
stringComboBox.getItems().addAll("John", "Jacob", "Schmidt");
stringComboBox.addEventHandler(KeyEvent.KEY_PRESSED, kp ->
System.out.println(kp.getCode()));
final Scene scene = new Scene(new BorderPane(stringComboBox), 300, 275);
primaryStage.setScene(scene);
primaryStage.show();
}
public static void main(String[] args) {
launch(args);
}
}
Re: [Rev 02] RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Mon, 10 Feb 2020 13:27:27 GMT, Ambarish Rapte wrote: >> Memory allocated in initDecompressor() and decompressIndirect() is not freed >> in error case. >> In error case, >> 1. Allocated memory should be freed. >> 2. Appropriate de-initialization jpeg library calls should be added. >> >> Verified that, >> 1. All unit and systems tests pass on three platforms, and >> 2. Memory consumption with and without fix is similar by comparing memory >> before and after showing 10 jpeg images for 100 times. > > The pull request has been updated with a new target base due to a merge or a > rebase. Looks good to me. I paid special attention to null pointer dereferencing when cleaning up resources, but I couldn't spot a trace that could cause this. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/54
