On Mon, 22 Mar 2021 14:35:58 GMT, Florian Kirmaier <[email protected]>
wrote:
>> Below is a fix that I tried, It fixes the issue. The system test passed with
>> this change.
>> I was suggesting a solution like this where we can know exactly when to
>> `null` the reference. The change is not extensively tested though. (For
>> example, test what happens if we remove a node and add it back, OR remove a
>> node and add it to a different scenegraph)
>> However, in this case using `WeakReference` approach seems harmless. Using
>> `WeakReference` for Listeners is not clean and may cause issues, but a
>> `WeakReference` to Node should not cause any harm.
>>
>> I would recommend earlier way to explicitly release references/resources
>> when it is possible. That way we get to have the control instead of gc.
>>
>>
>> diff --git
>> a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
>> b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
>> index fd02c0c1ad..471d0071b5 100644
>> --- a/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
>> +++ b/modules/javafx.graphics/src/main/java/javafx/scene/CssStyleHelper.java
>> @@ -36,6 +36,7 @@ import java.util.Set;
>>
>> import javafx.beans.property.ObjectProperty;
>> import javafx.beans.property.SimpleObjectProperty;
>> +import javafx.beans.value.ChangeListener;
>> import javafx.beans.value.WritableValue;
>> import com.sun.javafx.css.CascadingStyle;
>> import javafx.css.CssMetaData;
>> @@ -82,6 +83,14 @@ final class CssStyleHelper {
>> this.triggerStates = new PseudoClassState();
>> }
>>
>> + ChangeListener<Scene> sceneChangeListener;
>> +
>> + static void dispose(CssStyleHelper styleHelper, Node node) {
>> + styleHelper.resetToInitialValues(node);
>> + styleHelper.firstStyleableAncestor = null;
>> +
>> node.sceneProperty().removeListener(styleHelper.sceneChangeListener);
>> + }
>> +
>> /**
>> * Creates a new StyleHelper.
>> */
>> @@ -158,7 +167,7 @@ final class CssStyleHelper {
>> // If this node had a style helper, then reset properties
>> to their initial value
>> // since the node won't have a style helper after this call
>> if (node.styleHelper != null) {
>> - node.styleHelper.resetToInitialValues(node);
>> + dispose(node.styleHelper, node);
>> }
>>
>> //
>> @@ -181,8 +190,14 @@ final class CssStyleHelper {
>> // If this node had a style helper, then reset properties to their
>> initial value
>> // since the style map might now be different
>> if (node.styleHelper != null) {
>> - node.styleHelper.resetToInitialValues(node);
>> + dispose(node.styleHelper, node);
>> }
>> + helper.sceneChangeListener = (ov, oldScene, newScene) -> {
>> + if (newScene == null) {
>> + helper.firstStyleableAncestor = null;
>> + }
>> + };
>> + node.sceneProperty().addListener(helper.sceneChangeListener);
>> return helper;
>> }
>
> I rewrote now the test. The initialization and teardown of JavaFX are now
> separated from the actual test. Now also none of the variables which is used
> in the test, are accessible from outside the test, and vise versa.
>
> Should I switch the fix to your solution, or should I keep mine which is
> based on WeakReferences? As you've mentioned, WeakReference should be fine
> here too.
> As I've mentioned, doing something for every node, whose scene is set to
> null, might change the complexity of removing a node from O(1) to
> O({size-of-tree}). I think it's also quite important to support fast
> removing/adding subtrees.
I think the fix based on `WeakReference` is fine in this case, for the reasons
discussed (it is a back link to an ancestor node) and given the performance
concerns. I'd like Ambarish to comment as well.
-------------
PR: https://git.openjdk.java.net/jfx/pull/424
