Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sun, 19 Apr 2020 09:51:07 GMT, Jesper Skov wrote: >> Make the two ways of associating a ToggleButton with a ToggleGroup interact >> correctly. >> >> This fixes https://bugs.openjdk.java.net/browse/JDK-8198402 > > Jesper Skov has updated the pull request incrementally with three additional > commits since the last revision: > > - Fail instead of print message > - addedToggles list is final > - Leave unrelated file alone Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sun, 19 Apr 2020 09:51:07 GMT, Jesper Skov wrote: >> Make the two ways of associating a ToggleButton with a ToggleGroup interact >> correctly. >> >> This fixes https://bugs.openjdk.java.net/browse/JDK-8198402 > > Jesper Skov has updated the pull request incrementally with three additional > commits since the last revision: > > - Fail instead of print message > - addedToggles list is final > - Leave unrelated file alone Fix and tests look good to me. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Thu, 23 Apr 2020 07:55:06 GMT, Jesper Skov wrote: >> Ah, well. Ended with the same VM crash when building webkit myself. >> >> So I am kinda stuck. Suggestions? > > I have found out that my :web:test failed because I did not have the > media libs properly installed. > > So the changes in this PR pass all tests. Good to know. I'll finish my review today. - PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sun, 19 Apr 2020 13:50:18 GMT, Jesper Skov wrote: >> Uh, the exception is (as the comment note suggests) from using a prebuilt >> webkit. >> I will try a locally built one now. > > Ah, well. Ended with the same VM crash when building webkit myself. > > So I am kinda stuck. Suggestions? I have found out that my :web:test failed because I did not have the media libs properly installed. So the changes in this PR pass all tests. - PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sun, 19 Apr 2020 09:34:00 GMT, Jesper Skov
wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/ToggleButton.java
>> line 196:
>>
>>> 195: private ObjectProperty toggleGroup;
>>> 196: @Override
>>> 197: public final void setToggleGroup(ToggleGroup value) {
>>
>> This is unrelated to the fix. The changes in this file should be reverted.
>
> OK. They are gone.
>
> Would this (keeping the changes very specific to the bug?) be worth
> mentioning in CONTRIBUTING.md?
> That is, like the note about imports: do not fix warnings that are not
> directly related to the issue?
Good idea. I'll add that to my growing list of things to improve in
CONTRIBUTING.md.
-
PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sun, 19 Apr 2020 12:44:23 GMT, Jesper Skov wrote: >> I have failed getting web:tests to work. >> Both with java 11.0.7 and 14.0.0 (adoptajdk 14.0.1 not ready yet), I get the >> error below. >> >> And that is with both a locally built webkit, and the one from >> javafx-web-15-ea+3-linux.jar >> >> So it seems I am unable to run web:tests task on my box (Fedora 31, FWIW). >> >> Any suggestions for how to resolve this? >> >> ` >>> Task :web:test >> * >> WARNING: running web tests without building webkit. >> The webkit native library will be copied from the JDK, >> which might lead to failures in some web tests. >> To avoid these failures, you should either build >> webkit locally, copy the native webkit library from a >> recent build, or skip execution of web test cases with >> '-x :web:test' >> * >> Exception in thread "JavaFX Application Thread" >> # >> # A fatal error has been detected by the Java Runtime Environment: >> # >> # SIGSEGV (0xb) at pc=0x7f4e01ae8a61, pid=78774, tid=78815 >> # >> # JRE version: OpenJDK Runtime Environment (11.0.7+10) (build 11.0.7+10) >> # Java VM: OpenJDK 64-Bit Server VM (11.0.7+10, mixed mode, tiered, >> compressed oops, g1 gc, linux-amd64) >> # Problematic frame: >> # V [libjvm.so+0x579a61] >> AccessInternal::PostRuntimeDispatch> G1BarrierSet>, >> (AccessInternal::BarrierType)2, 1097844ul>::oop_access_barrier(void*)+0x1 # >> # Core dump will be written. Default location: Core dumps may be processed >> with "/usr/lib/systemd/systemd-coredump %P >> %u %g %s %t %c %h" (or dumping to >> /opt/sources/github/jfx/modules/javafx.web/core.78774) # >> # An error report file with more information is saved as: >> # /opt/sources/github/jfx/modules/javafx.web/hs_err_pid78774.log >> # >> # If you would like to submit a bug report, please visit: >> # https://github.com/AdoptOpenJDK/openjdk-support/issues >> # >> ` > > Uh, the exception is (as the comment note suggests) from using a prebuilt > webkit. > I will try a locally built one now. Ah, well. Ended with the same VM crash when building webkit myself. So I am kinda stuck. Suggestions? - PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sun, 19 Apr 2020 12:42:09 GMT, Jesper Skov wrote: >> @kevinrushforth I tested by: >> ` >> bash ./gradlew clean all test -x :web:test >> ` >> I assumed that would do it. >> But I see use of ToggleButton in javafx.web, so that was clearly a faulty >> assumption. >> >> I will try to get webkit working. > > I have failed getting web:tests to work. > Both with java 11.0.7 and 14.0.0 (adoptajdk 14.0.1 not ready yet), I get the > error below. > > And that is with both a locally built webkit, and the one from > javafx-web-15-ea+3-linux.jar > > So it seems I am unable to run web:tests task on my box (Fedora 31, FWIW). > > Any suggestions for how to resolve this? > > ` >> Task :web:test > * > WARNING: running web tests without building webkit. > The webkit native library will be copied from the JDK, > which might lead to failures in some web tests. > To avoid these failures, you should either build > webkit locally, copy the native webkit library from a > recent build, or skip execution of web test cases with > '-x :web:test' > * > Exception in thread "JavaFX Application Thread" > # > # A fatal error has been detected by the Java Runtime Environment: > # > # SIGSEGV (0xb) at pc=0x7f4e01ae8a61, pid=78774, tid=78815 > # > # JRE version: OpenJDK Runtime Environment (11.0.7+10) (build 11.0.7+10) > # Java VM: OpenJDK 64-Bit Server VM (11.0.7+10, mixed mode, tiered, > compressed oops, g1 gc, linux-amd64) > # Problematic frame: > # V [libjvm.so+0x579a61] > AccessInternal::PostRuntimeDispatch G1BarrierSet>, > (AccessInternal::BarrierType)2, 1097844ul>::oop_access_barrier(void*)+0x1 # > # Core dump will be written. Default location: Core dumps may be processed > with "/usr/lib/systemd/systemd-coredump %P > %u %g %s %t %c %h" (or dumping to > /opt/sources/github/jfx/modules/javafx.web/core.78774) # > # An error report file with more information is saved as: > # /opt/sources/github/jfx/modules/javafx.web/hs_err_pid78774.log > # > # If you would like to submit a bug report, please visit: > # https://github.com/AdoptOpenJDK/openjdk-support/issues > # > ` Uh, the exception is (as the comment note suggests) from using a prebuilt webkit. I will try a locally built one now. - PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sun, 19 Apr 2020 09:56:27 GMT, Jesper Skov wrote: >> The fix looks correct to me. Have you run all tests to ensure no regressions? >> >> I left a couple inline comments. > > @kevinrushforth I tested by: > ` > bash ./gradlew clean all test -x :web:test > ` > I assumed that would do it. > But I see use of ToggleButton in javafx.web, so that was clearly a faulty > assumption. > > I will try to get webkit working. I have failed getting web:tests to work. Both with java 11.0.7 and 14.0.0 (adoptajdk 14.0.1 not ready yet), I get the error below. And that is with both a locally built webkit, and the one from javafx-web-15-ea+3-linux.jar So it seems I am unable to run web:tests task on my box (Fedora 31, FWIW). Any suggestions for how to resolve this? ` > Task :web:test * WARNING: running web tests without building webkit. The webkit native library will be copied from the JDK, which might lead to failures in some web tests. To avoid these failures, you should either build webkit locally, copy the native webkit library from a recent build, or skip execution of web test cases with '-x :web:test' * Exception in thread "JavaFX Application Thread" # # A fatal error has been detected by the Java Runtime Environment: # # SIGSEGV (0xb) at pc=0x7f4e01ae8a61, pid=78774, tid=78815 # # JRE version: OpenJDK Runtime Environment (11.0.7+10) (build 11.0.7+10) # Java VM: OpenJDK 64-Bit Server VM (11.0.7+10, mixed mode, tiered, compressed oops, g1 gc, linux-amd64) # Problematic frame: # V [libjvm.so+0x579a61] AccessInternal::PostRuntimeDispatch, (AccessInternal::BarrierType)2, 1097844ul>::oop_access_barrier(void*)+0x1 # # Core dump will be written. Default location: Core dumps may be processed with "/usr/lib/systemd/systemd-coredump %P %u %g %s %t %c %h" (or dumping to /opt/sources/github/jfx/modules/javafx.web/core.78774) # # An error report file with more information is saved as: # /opt/sources/github/jfx/modules/javafx.web/hs_err_pid78774.log # # If you would like to submit a bug report, please visit: # https://github.com/AdoptOpenJDK/openjdk-support/issues # ` - PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Sat, 18 Apr 2020 16:04:34 GMT, Kevin Rushforth wrote: >> Jesper Skov has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Fail instead of print message >> - addedToggles list is final >> - Leave unrelated file alone > > The fix looks correct to me. Have you run all tests to ensure no regressions? > > I left a couple inline comments. @kevinrushforth I tested by: ` bash ./gradlew clean all test -x :web:test ` I assumed that would do it. But I see use of ToggleButton in javafx.web, so that was clearly a faulty assumption. I will try to get webkit working. - PR: https://git.openjdk.java.net/jfx/pull/167
Re: [Rev 01] RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
> Make the two ways of associating a ToggleButton with a ToggleGroup interact > correctly. > > This fixes https://bugs.openjdk.java.net/browse/JDK-8198402 Jesper Skov has updated the pull request incrementally with three additional commits since the last revision: - Fail instead of print message - addedToggles list is final - Leave unrelated file alone - Changes: - all: https://git.openjdk.java.net/jfx/pull/167/files - new: https://git.openjdk.java.net/jfx/pull/167/files/1386ad74..10eef05b Webrevs: - full: https://webrevs.openjdk.java.net/jfx/167/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/167/webrev.00-01 Stats: 5 lines in 3 files changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/167.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/167/head:pull/167 PR: https://git.openjdk.java.net/jfx/pull/167
