Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 20:47:45 GMT, Alexey Ivanov wrote: > You're right. The old icon handles in `hOldIcon` and `hOldIconSm` will be > leaked here if `CreateIconFromRaster` throws an exception. I've submitted [JDK-8282862](https://bugs.openjdk.java.net/browse/JDK-8282862): AwtWindow::SetIconData leaks old icon handles if an exception is detected - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 15:54:45 GMT, Zhengyu Gu wrote: >> Please review this small patch that fixes a potential memory leak that >> exception return fails to release allocated `cacheDirs` >> >> Test: >> >> - [x] jdk_desktop on Linux x86_64 > > Zhengyu Gu has updated the pull request incrementally with one additional > commit since the last revision: > > mrserb and aivanov-jdk's comments Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 15:54:45 GMT, Zhengyu Gu wrote: >> Please review this small patch that fixes a potential memory leak that >> exception return fails to release allocated `cacheDirs` >> >> Test: >> >> - [x] jdk_desktop on Linux x86_64 > > Zhengyu Gu has updated the pull request incrementally with one additional > commit since the last revision: > > mrserb and aivanov-jdk's comments Still looks good to me. - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 21:12:33 GMT, Zhengyu Gu wrote: >>> I think `NewStringUTF()` can throw OOM and also return `NULL`, just which >>> one you pick. >> >> Yes. >> >> But we're discussing whether a check for exception is necessary after >> `(*env)->SetObjectArrayElement`. > > `SetObjectArrayElement()` may throw `ArrayIndexOutOfBoundsException` and > `ArrayStoreException`, but I don't see it is possible here. That's exactly my point. Therefore the check for exception after `SetObjectArrayElement` is unnecessary. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 21:05:13 GMT, Alexey Ivanov wrote: >> I think `NewStringUTF()` can throw OOM and also return `NULL`, just which >> one you pick. > >> I think `NewStringUTF()` can throw OOM and also return `NULL`, just which >> one you pick. > > Yes. > > But we're discussing whether a check for exception is necessary after > `(*env)->SetObjectArrayElement`. `SetObjectArrayElement()` may throw `ArrayIndexOutOfBoundsException` and `ArrayStoreException`, but I don't see it is possible here. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 21:02:04 GMT, Zhengyu Gu wrote: > I think `NewStringUTF()` can throw OOM and also return `NULL`, just which one > you pick. Yes. But we're discussing whether a check for exception is necessary after `(*env)->SetObjectArrayElement`. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 20:57:27 GMT, Alexey Ivanov wrote: >>> ??? You want to check and cleanup if NewStringUTF fails. You only want to >>> call SetObjectElementArray if NewStringUTF succeeds. >> >> If the SetObjectArrayElement will throw an exception we will call the >> NewStringUTF while the exception is raised which is a kind of "jni" issue. >> If such an exception is possible we should check and cleanup. > > A quick search for `SetObjectArrayElement` in `java.desktop` module shows the > call to `SetObjectArrayElement` is rarely followed by exception check. > > What kind of exception can `SetObjectArrayElement` raise if the index is > within the range and the stored element is of correct type? I think `NewStringUTF()` can throw OOM and also return `NULL`, just which one you pick. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 20:12:29 GMT, Sergey Bylokhov wrote: >> Can `SetObjectElementArray` raise an exception? >> The index is within the array length and we store a string. I assume >> `cacheDirArray` is a string array. > >> ??? You want to check and cleanup if NewStringUTF fails. You only want to >> call SetObjectElementArray if NewStringUTF succeeds. > > If the SetObjectArrayElement will throw an exception we will call the > NewStringUTF while the exception is raised which is a kind of "jni" issue. If > such an exception is possible we should check and cleanup. A quick search for `SetObjectArrayElement` in `java.desktop` module shows the call to `SetObjectArrayElement` is rarely followed by exception check. What kind of exception can `SetObjectArrayElement` raise if the index is within the range and the stored element is of correct type? - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 15:54:45 GMT, Zhengyu Gu wrote: >> Please review this small patch that fixes a potential memory leak that >> exception return fails to release allocated `cacheDirs` >> >> Test: >> >> - [x] jdk_desktop on Linux x86_64 > > Zhengyu Gu has updated the pull request incrementally with one additional > commit since the last revision: > > mrserb and aivanov-jdk's comments > > > I did a quick grep, there are a few suspicious spots, e.g. > > > https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711, > > > I have yet take a close look. > > > > > > It doesn't leak the icon handle here, it's assigned to the member of the > > object (probably `NULL`). Yet it's inconsistent: if an exception is > > expected from `CreateIconFromRaster` on the line above, why isn't it > > checked after `CreateIconFromRaster` is called on the following line? > > I am not familiar with this code. What I see is that, it assigns `m_hIcon` > and `m_hIconSm` to local variables, then reset them to `NULL`, once exception > returns, are references to icon and small icon handles lost? and it will > destroy icons as L2727 - 2732 do. But I may miss something. You're right. The old icon handles in `hOldIcon` and `hOldIconSm` will be leaked here if `CreateIconFromRaster` throws an exception. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 13:37:44 GMT, Alexey Ivanov wrote: >> ??? You want to check and cleanup if NewStringUTF fails. You only want to >> call SetObjectElementArray if NewStringUTF succeeds. > > Can `SetObjectElementArray` raise an exception? > The index is within the array length and we store a string. I assume > `cacheDirArray` is a string array. > ??? You want to check and cleanup if NewStringUTF fails. You only want to > call SetObjectElementArray if NewStringUTF succeeds. If the SetObjectArrayElement will throw an exception we will call the NewStringUTF while the exception is raised which is a kind of "jni" issue. If such an exception is possible we should check and cleanup. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 15:58:57 GMT, Alexey Ivanov wrote: > > I did a quick grep, there are a few suspicious spots, e.g. > > [https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711](url), > > I have yet take a close look. > > It doesn't leak the icon handle here, it's assigned to the member of the > object (probably `NULL`). Yet it's inconsistent: if an exception is expected > from `CreateIconFromRaster` on the line above, why isn't it checked after > `CreateIconFromRaster` is called on the following line? I am not familiar with this code. What I see is that, it assigns `m_hIcon` and `m_hIconSm` to local variables, then reset them to `NULL`, once exception returns, are references to icon and small icon handles lost? and it will destroy icons as L2727 - 2732 do. But I may miss something. - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
On Tue, 8 Mar 2022 15:54:45 GMT, Zhengyu Gu wrote: >> Please review this small patch that fixes a potential memory leak that >> exception return fails to release allocated `cacheDirs` >> >> Test: >> >> - [x] jdk_desktop on Linux x86_64 > > Zhengyu Gu has updated the pull request incrementally with one additional > commit since the last revision: > > mrserb and aivanov-jdk's comments > I did a quick grep, there are a few suspicious spots, e.g. > [https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711](url), > I have yet take a close look. It doesn't leak the icon handle here, it's assigned to the member of the object (probably `NULL`). Yet it's inconsistent: if an exception is expected from `CreateIconFromRaster` on the line above, why isn't it checked after `CreateIconFromRaster` is called on the following line? - PR: https://git.openjdk.java.net/jdk/pull/7691
Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]
> Please review this small patch that fixes a potential memory leak that > exception return fails to release allocated `cacheDirs` > > Test: > > - [x] jdk_desktop on Linux x86_64 Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision: mrserb and aivanov-jdk's comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7691/files - new: https://git.openjdk.java.net/jdk/pull/7691/files/0ff07ddd..c4a3ec87 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7691=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7691=00-01 Stats: 23 lines in 2 files changed: 2 ins; 18 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7691/head:pull/7691 PR: https://git.openjdk.java.net/jdk/pull/7691
