On Mon, 15 Jul 2024 13:30:13 GMT, John Hendrikx <jhendr...@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
LGTM.
The fix does not cause SOE but instead detects the loop, outputting the
explanation to stderr:
Jul 16, 2024 10:05:52 AM javafx.scene.CssStyleHelper resolveLookups
WARNING: Loop detected in *.root{
-fx-base-fill: <Value>
<value values="3">
<Value lookup="true">
<value>-fx-base</value>
<converter>null</converter>
</Value> <Value>
<value values="2">
<Value>
<value>49.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0xffffffff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> <Value>
<value values="2">
<Value>
<value>50.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0x000000ff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> </value>
<converter>LadderConverter</converter>
</Value>
-fx-base: <Value>
<value values="3">
<Value lookup="true">
<value>-fx-base-fill</value>
<converter>null</converter>
</Value> <Value>
<value values="2">
<Value>
<value>49.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0xffffffff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> <Value>
<value values="2">
<Value>
<value>50.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0x000000ff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> </value>
<converter>LadderConverter</converter>
</Value>
} while resolving '-fx-base'
Jul 16, 2024 10:05:52 AM javafx.scene.CssStyleHelper calculateValue
WARNING: Caught java.lang.IllegalArgumentException: Loop detected in *.root{
-fx-base-fill: <Value>
<value values="3">
<Value lookup="true">
<value>-fx-base</value>
<converter>null</converter>
</Value> <Value>
<value values="2">
<Value>
<value>49.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0xffffffff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> <Value>
<value values="2">
<Value>
<value>50.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0x000000ff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> </value>
<converter>LadderConverter</converter>
</Value>
-fx-base: <Value>
<value values="3">
<Value lookup="true">
<value>-fx-base-fill</value>
<converter>null</converter>
</Value> <Value>
<value values="2">
<Value>
<value>49.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0xffffffff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> <Value>
<value values="2">
<Value>
<value>50.0%</value>
<converter>null</converter>
</Value> <Value>
<value>0x000000ff</value>
<converter>null</converter>
</Value> </value>
<converter>StopConverter</converter>
</Value> </value>
<converter>LadderConverter</converter>
</Value>
} while resolving '-fx-base'' while calculating value for
'-fx-background-color' from style '*.pane { -fx-background-color: <Value>
<value values="1">
<Value lookup="true">
<value>-fx-base</value>
<converter>null</converter>
</Value> </value>
<converter>Paint.SequenceConverter</converter>
</Value> } '
Also added a reproducer to the ticket, which goes through the code path
normally used by the application.
modules/javafx.graphics/src/test/java/test/javafx/scene/CssStyleHelperTest.java
line 691:
> 689: @Test
> 690: public void shouldDetectNestedInfiniteLoop() throws IOException {
> 691: Stylesheet stylesheet = new CssParser().parse(
`CssParser.parse(String,String)` is used only in tests, why are we using it?
Shouldn't we use `CssParser.parse(String)` instead?
Also, why do we need `CssParser.parse(String,String)` in the first place?
-------------
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1505#pullrequestreview-2180820690
PR Review Comment: https://git.openjdk.org/jfx/pull/1505#discussion_r1679759718
