RFR: 7903538: NullPointerException: Cannot read the array length because "<local9>" is null

[Jonathan Gibbons]

On Tue, 5 Mar 2024 06:27:34 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review for this change which proposes to fix the issue noted in https://bugs.openjdk.org/browse/CODETOOLS-7903538?
> 
> `java.io.File.listFiles()` as per its API doc says:
> 
>> ... Returns null if this abstract pathname does not denote a directory, or if an I/O error occurs.
> 
> So return values from `File.listFiles()` require a null check. It looks like this missing null check, in the `for` loop of the code being changed here, was in fact noticed previously and an attempt to address that was made almost a decade back in https://github.com/openjdk/jtreg/commit/329d3fb3dd6ab31c7133ef95383068f2de6f7735. But I think there's an oversight in that change. It does introduce a `null` check for the return value, but it still goes on to again call `dir.listFiles()` in the `for` loop. `dir` doesn't change ever in the `for` loop nor in that `else` block, so the `for` loop should just have used the `children` variable which is guaranteed to be non-null at that point.
> 
> The commit in this PR addresses that oversight. No new tests have been included given the nature of this change. Existing tests continue to pass.

Good enough for now.  One suggestion to improve the comment.

A better solution, down the road, would be to convert the class to use NIO.

src/share/classes/com/sun/javatest/regtest/exec/ScratchDirectory.java line 211:

> 209:         boolean ok = true;
> 210:         File[] children = dir.listFiles();
> 211:         if (children == null) { // should always be not null, but File.listFiles() allows for it

I find the proposed new comment somewhat confusing. How about:

`// should always be not null, but may be null if an error occurs`

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

Marked as reviewed by jjg (Lead).

PR Review: https://git.openjdk.org/jtreg/pull/186#pullrequestreview-1947259622
PR Review Comment: https://git.openjdk.org/jtreg/pull/186#discussion_r1531014853