Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-09 Thread Alexey Ivanov
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-09 Thread Alexey Ivanov
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Thomas Stuefe
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Alexey Ivanov
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`. > >

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Zhengyu Gu
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Alexey Ivanov
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:

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Zhengyu Gu
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Alexey Ivanov
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Alexey Ivanov
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Sergey Bylokhov
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Zhengyu Gu
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Alexey Ivanov
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

Re: RFR: 8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() [v2]

2022-03-08 Thread Zhengyu Gu
> 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: