RFR: 8301302: Platform preferences API [v39]

[Michael Strauß]

On Tue, 5 Dec 2023 23:32:41 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Michael Strauß has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   check return value of JNI functions
>
> modules/javafx.graphics/src/main/native-glass/mac/GlassMacros.h line 178:
> 
>> 176: 
>> 177: #define GLASS_CHECK_NONNULL_EXCEPTIONALLY_RETURN(ENV, EXPR)                        \
>> 178:     GLASS_CHECK_EXCEPTION(ENV);                                                    \
> 
> Should this call `GLASS_CHECK_EXCEPTIONALLY_RETURN` instead? It doesn't matter for the current places where it is used, but it might be more consistent.

Yes.

> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 159:
> 
>> 157:     memset(wstr, 0, wstr_len * sizeof(WCHAR));
>> 158:     MultiByteToWideChar(CP_UTF8, 0, str, -1, wstr, wstr_len);
>> 159:     WindowsCreateString(wstr, wstr_len - 1, &hstr_);
> 
> This could use some error checking. At a minimum, `wstr` should be checked for null and skip the rest if it is. I think you also should check `wstr_len == 0`, since it is will return 0 on error. You might also consider whether you need to check the return value of `WindowsCreateString`. You would need to make sure `hstr_ = nullptr` if there are any errors.

I added the checks, but not for `WindowsCreateString`, since it is specified to set `hstr_` to null in case of failure.

> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 165:
> 
>> 163: hstring::~hstring()
>> 164: {
>> 165:     WindowsDeleteString(hstr_);
> 
> After adding the check for a failure to allocate the string, a null check is needed here (unless `WindowsDeleteString` specifies that it does nothing if the string is null).

`WindowsDeleteString` does nothing when null is passed into it, but I added a null check anyway so people don't stumble upon it and wonder.

> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 182:
> 
>> 180:     size_t len = strlen(message);
>> 181:     char* msg = new char[len + 1];
>> 182:     strcpy_s(msg, len + 1, message);
> 
> You should check for null before copying.

Done.

> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 199:
> 
>> 197:     char* result = new char[message_length + error_length];
>> 198:     WideCharToMultiByte(CP_ACP, 0, error, -1, result + message_length, error_length, NULL, FALSE);
>> 199:     memcpy_s(result, message_length, message, message_length);
> 
> Same comment about error checking as in `hstring`.

Done.

> modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 208:
> 
>> 206: RoException::~RoException()
>> 207: {
>> 208:     delete[] message_;
> 
> Check `message_` for null (meaning you will need to initialize it to null in case of error).

I added a null check, and also initialize the field to null in the constructor.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416495255
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416486073
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416494229
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416494466
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416494701
PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416495125