RFR: 8342870: Errors related to unused code on Windows after 8339120 in accessibility [v5]

Tue Jan 21 21:23:44 UTC 2025

On Tue, 7 Jan 2025 09:56:11 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> After 8339120, gcc began catching many different instances of unused code in the Windows specific codebase. Some of these seem to be bugs. I've taken the effort to mark out all the relevant globals and locals that trigger the unused warnings and addressed all of them by commenting out the code as appropriate. I am confident that in many cases this simplistic approach of commenting out code does not fix the underlying issue, and the warning actually found a bug that should be fixed. In these instances, I will be aiming to fix these bugs with help from reviewers, so I recommend anyone reviewing who knows more about the code than I do to see whether there is indeed a bug that needs fixing in a different way than what I did
>> 
>> build.log on release configuration:
>> 
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp: In function 'int regEnable()':
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:334:14: warning: unused variable 'freeData' [-Wunused-variable]
>>   334 |         bool freeData = false;
>>       |              ^~~~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:326:11: warning: unused variable 'retval' [-Wunused-variable]
>>   326 |     DWORD retval = -1;
>>       |           ^~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp: In function 'int regDeleteValue(HKEY, LPCWSTR)':
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:379:14: warning: unused variable 'freeData' [-Wunused-variable]
>>   379 |         bool freeData = false;
>>       |              ^~~~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:367:11: warning: unused variable 'retval' [-Wunused-variable]
>>   367 |     DWORD retval = -1;
>>       |           ^~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/bridge/AccessBridgeCalls.c: In function 'shutdownAccessBridge':
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win3...
>
> Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'openjdk:master' into accessibility
>  - Cast to void in AccessBridgeCalls.c
>  - static_cast to void in jaccessinspector.cpp
>  - Formatting changes in AccessBridgeEventHandler.cpp
>  - Merge branch 'master' into accessibility
>  - Remove now unused result
>  - Merge branch 'master' into accessibility
>  - 8342870

I haven't looked at all the files, yet I'm posting my comments right away for discussion.

src/jdk.accessibility/windows/native/bridge/AccessBridgeCalls.c line 230:

> 228:             if (result != TRUE) {
> 229:                 error = GetLastError();
> 230:             }

The comparison above should be `if (result == 0)` instead of `TRUE`, because [`FreeLibrary`](https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-freelibrary) returns non-zero if the function succeeds, which implies the return value could be different from 1 (`TRUE`).

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp line 334:

> 332:         TCHAR dataBuffer[DEFAULT_ALLOC];
> 333:         TCHAR *data = dataBuffer;
> 334:      // bool freeData = false;

This should be set to `true` if `data` is reallocated, otherwise both `regEnable` and `regDeleteValue` leak `data`.

https://github.com/openjdk/jdk/blob/4a9fba615da0dfa6646ecb9fd9d929f74fe6875e/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp#L336-L341

In fact, the above statement should probably be removed — the following code uses only `dataBuffer` which does not contain any data if the buffer was dynamically allocated and assigned to `data`.

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp line 364:

> 362: }
> 363: 
> 364: int regDeleteValue(HKEY hFamilyKey, LPCWSTR lpSubKey)

There's one more memory leak:

https://github.com/openjdk/jdk/blob/4a9fba615da0dfa6646ecb9fd9d929f74fe6875e/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp#L352

The `delete[]` operator must be called on `newStr`.

It's present in both `regEnable` and `regDeleteValue`.

src/jdk.accessibility/windows/native/jaccessinspector/jaccessinspector.cpp line 1212:

> 1210:     }
> 1211: 
> 1212:     static_cast<void>(lastError);

This falls into category that's useful for debugging only. The value of `lastError` is never used to report an error, yet one can use a debugger to inspect the value.

There seems to be no way to report or log the error.

Isn't it better to add a flag to suppress the warning? Casting `lastError` to `void` is cryptic. A comment would definitely be helpful here.

src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp line 261:

> 259:         GetClientRect(hWnd, &rcClient);
> 260:      // hwndEdit =
> 261:                    CreateWindow("Edit",

The edit control is created visible and it's resized to cover the entire client area.

The edit control is used… the text is updated in the [`displayAndLog` function](https://github.com/openjdk/jdk/blob/c38417a86e27f047715cfd9a98770387d994a512/src/jdk.accessibility/windows/native/toolscommon/AccessInfo.cpp#L60).

Since the edit control is a child window, it's automatically destroyed when the parent window is destroyed.

Therefore, there's no reason to store the handle in a variable, and I suggest removing the `hwndEdit` variable.

Moreover, the comment for `hwndEdit`—`// handle of tree-view control`—is confusing because it doesn't store a handle to a TreeView control.

src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp line 561:

> 559:         tvis.item = tvi;
> 560: 
> 561:         /* HTREEITEM treeNodeItem = */ TreeView_InsertItem(treeWnd, &tvis);

Since it's unused, I'd rather remove the variable altogether.

Does the handle have any other usage except for checking whether an error occurred?

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

PR Review: https://git.openjdk.org/jdk/pull/21656#pullrequestreview-2564668085
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1924375318
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923750005
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923752261
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923774492
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923839914
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923849086