Re: [Approved] RFR: 8211308: Support HTTP/2 in WebView
On Wed, 16 Oct 2019 17:57:58 GMT, Arunprasad Rajkumar wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 1832c2db: Incorporate fixes provided by @kcr > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/14/files > - new: https://git.openjdk.java.net/jfx/pull/14/files/1798a661..1832c2db > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/14/webrev.01 > - incr: https://webrevs.openjdk.java.net/jfx/14/webrev.00-01 > > Stats: 13 lines in 2 files changed: 5 ins; 0 del; 8 mod > Patch: https://git.openjdk.java.net/jfx/pull/14.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/14/head:pull/14 Looks good to me. Approved by ghb (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/14
Re: RFR: 8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute
On Wed, 20 Nov 2019 15:04:07 GMT, Kevin Rushforth wrote: > On Wed, 20 Nov 2019 07:05:40 GMT, Arun Joseph wrote: > >> Issue: Native part of WebView throws a DOMException and then, continues >> executing the rest of the function assuming that value is present. This >> causes the JVM to crash when retrieving the value. >> >> Fix: Return from the function if exception was raised (code is similar to >> exception handling in >> [WebKitLegacy/java/DOM/JavaTreeWalker.cpp](https://github.com/openjdk/jfx/blob/master/modules/javafx.web/src/main/native/Source/WebKitLegacy/java/DOM/JavaTreeWalker.cpp)) >> >> This fix also needs to be applied to all function calls in >> [WebKitLegacy/java/DOM](https://github.com/openjdk/jfx/tree/master/modules/javafx.web/src/main/native/Source/WebKitLegacy/java/DOM) >> functions which raises DOMError similar to createAttributeImpl(). >> >> >> >> Commits: >> - acc52780: 8233747: JVM crash in >> com.sun.webkit.dom.DocumentImpl.createAttribute >> >> Changes: https://git.openjdk.java.net/jfx/pull/47/files >> Webrev: https://webrevs.openjdk.java.net/jfx/47/webrev.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8233747 >> Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod >> Patch: https://git.openjdk.java.net/jfx/pull/47.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/47/head:pull/47 > > The proposed fix seems more like a workaround to me. There are dozens of very > similar calls to `raiseOnDOMError` in this and other files, so I would think > a more general solution is needed. For calls to `raiseOnDOMError()` with argument of type `ExceptionOr>`, the returned value is again passed through `WTF::getPtr()`. This doesn't modify the value returned, but removing it will require changing about 40 function calls. PR: https://git.openjdk.java.net/jfx/pull/47
Re: [Rev 01] RFR: 8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute
The pull request has been updated with additional changes. Added commits: - 2bd56c11: Modified only raiseOnDOMError Changes: - all: https://git.openjdk.java.net/jfx/pull/47/files - new: https://git.openjdk.java.net/jfx/pull/47/files/acc52780..2bd56c11 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/47/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/47/webrev.00-01 Issue: https://bugs.openjdk.java.net/browse/JDK-8233747 Stats: 32 lines in 2 files changed: 24 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/47.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/47/head:pull/47 PR: https://git.openjdk.java.net/jfx/pull/47
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 11:58:18 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.
>
>
>
> Commits:
> - 7af932b7: 8212034: Memory leaks in jpegLoader.c in error case
>
> Changes: https://git.openjdk.java.net/jfx/pull/54/files
> Webrev: https://webrevs.openjdk.java.net/jfx/54/webrev.00
> Issue: https://bugs.openjdk.java.net/browse/JDK-8212034
> Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 mod
> Patch: https://git.openjdk.java.net/jfx/pull/54.diff
> Fetch: git fetch https://git.openjdk.java.net/jfx pull/54/head:pull/54
modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1625:
> 1624:
> 1625: JSAMPROW scanline_ptr = (JSAMPROW) malloc(bytes_per_row * sizeof
> (JSAMPLE));
> 1626: if (scanline_ptr == NULL) {
You can remove quite a few calls to `free` if you move the memory allocation
for `scanline_ptr` just [before it's
usage](https://github.com/openjdk/jfx/blob/7af932b7f5215949776ec79fb2a5484c521b21a1/modules/javafx.graphics/src/main/native-iio/jpegloader.c#L1690).
Also free it as soon as you are done with it.
PR: https://git.openjdk.java.net/jfx/pull/54
Re: [Approved] RFR: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
On Fri, 22 Nov 2019 21:13:05 GMT, Kevin Rushforth wrote: > Now that we have switched to using gradle 6 we can switch to using JDK 13 as > the boot JDK for JavaFX 14 builds. The latest JDK 13 update release is JDK > 13.0.1. > > This will not change the minimum JDK version needed to build or run JavaFX, > which remains at 11. We will continue to generate class files using `-source > 11 -target 11`. > > > > Commits: > - 741414f1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK > > Changes: https://git.openjdk.java.net/jfx/pull/51/files > Webrev: https://webrevs.openjdk.java.net/jfx/51/webrev.00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8232064 > Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod > Patch: https://git.openjdk.java.net/jfx/pull/51.diff > Fetch: git fetch https://git.openjdk.java.net/jfx pull/51/head:pull/51 Approved by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/51
Re: RFR: 8212034: Potential memory leaks in jpegLoader.c in error case
On Wed, 27 Nov 2019 21:13:40 GMT, Johan Vos wrote: > On Wed, 27 Nov 2019 11:58:18 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. >> >> >> >> Commits: >> - 7af932b7: 8212034: Memory leaks in jpegLoader.c in error case >> >> Changes: https://git.openjdk.java.net/jfx/pull/54/files >> Webrev: https://webrevs.openjdk.java.net/jfx/54/webrev.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8212034 >> Stats: 62 lines in 1 file changed: 36 ins; 14 del; 12 mod >> Patch: https://git.openjdk.java.net/jfx/pull/54.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/54/head:pull/54 > > modules/javafx.graphics/src/main/native-iio/jpegloader.c line 1345: > >> 1344: free(cinfo->err); >> 1345: free(cinfo); >> 1346: ThrowByName(env, "java/io/IOException", buffer); > > jerr_mgr is also allocated via malloc, but not freed. Do you want to free > that too? Line: 1332=> jerr_mgr ptr is stored in cinfor->err `cinfo->err = jpeg_std_error(&(jerr_mgr->pub));` so free(cinfo->err) is same as free (jerr_mgr). free(cinfo->err) is used here [instead of free(jerr_mgr)] to match the free call in imageio_dispose() method. PR: https://git.openjdk.java.net/jfx/pull/54
