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

[Andy Goryachev]

On Mon, 22 Jul 2024 20:46:08 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:
> 
>   Add lines I forgot to commit

lgtm, with a couple of very minor comments.

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

> 31: import javafx.css.CssParser;
> 32: import javafx.css.CssParser.ParseError;
> 33: import javafx.css.CssParser.ParseError.PropertySetError;

very minor: I would have used `ParseError.PropertySetError` in the code instead of static import, removes the guesswork when looking at the code.

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

> 697: 
> 698:         // Note: on Windows, the message is using inconsistent line endings (sometimes Windows, sometimes Linux)
> 699:         // so I've stripped it.

would looking for substrings be a better solution?  for example:


contains("Caught java.lang.IllegalArgumentException: Loop detected in *.root{") &&
contains("while resolving '-fx-base'' while calculating value for '-fx-background-color' from rule '*.pane' in stylesheet"

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

Marked as reviewed by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1505#pullrequestreview-2192657068
PR Review Comment: https://git.openjdk.org/jfx/pull/1505#discussion_r1687212293
PR Review Comment: https://git.openjdk.org/jfx/pull/1505#discussion_r1687210456