RFR: 8336389: Infinite loop occurs while resolving lookups [v2]

[Kevin Rushforth]

On Mon, 15 Jul 2024 13:30:13 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Fixes an infinite loop that can occur while resolving lookups.
>> 
>> There were 2 bugs:
>> - A `contains` check was done on some value X, but then a value Y was added to the set used to track duplicates
>> - The `Set` to track duplicates was cleared in some recursive calls, meaning that the caller (which may be processing multiple values in a loop) would end up with an empty set, losing track of what was visited so far
>> 
>> Also removed a redundant `null` check (an NPE would have occurred before it could reach that code).
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix formatting and spelling

The fix looks good.

The test will verify that it no longer goes into an infinite loop, although it now mixes JUnit 5 and JUnit 4. Is it possible to also check that a parsing error is reported in that case, using `CssParser::errorsProperty`?

modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java line 43:

> 41: import static org.junit.Assert.assertFalse;
> 42: import static org.junit.Assert.assertTrue;
> 43: import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;

We have a general "best practice" to not mix JUnit 4 and JUnit 5 calls in the same test class.

modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java line 686:

> 684:         root.getChildren().addAll(a);
> 685: 
> 686:         assertDoesNotThrow(() -> stage.show());  // This should not result in a StackOverflowError

Since this is still a JUnit 4 test, perhaps replace this with a try/catch and call `Assert.fail` in the `catch` block?

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

PR Review: https://git.openjdk.org/jfx/pull/1505#pullrequestreview-2180914588
PR Review Comment: https://git.openjdk.org/jfx/pull/1505#discussion_r1679817453
PR Review Comment: https://git.openjdk.org/jfx/pull/1505#discussion_r1679820286