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

[Andy Goryachev]

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

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