[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [Rev 01] RFR: 8236840: Memory leak when switching ButtonSkin
From:       Jeanette Winzenburg <fastegal () openjdk ! java ! net>
Date:       2020-03-30 10:38:23
Message-ID: Hw6GQAEgH9kqtcQgh4YiRf_kDFcMu8Y03hP03picn54=.9815b589-0cec-4ad8-a4f6-8d8f01e97571 () github ! com
[Download RAW message or body]

On Mon, 30 Mar 2020 07:21:38 GMT, Ambarish Rapte <arapte@openjdk.org> wrote:

> > modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java \
> > line 178: 
> > > 177:     @Override public void dispose() {
> > > 178:         getSkinnable().sceneProperty().removeListener(weakChangeListener);
> > > 179:         super.dispose();
> > 
> > +1! Actually, looks like that manual remove is really needed - otherwise we get \
> > an NPE when removing the default button from its parent after the skin has been \
> > switched: @Test
> > public void testDefaultButtonSwitchSkinAndRemove() {
> > Button button = new Button();
> > button.setDefaultButton(true);
> > Group root = new Group(button);
> > Scene scene = new Scene(root);
> > Stage stage = new Stage();
> > stage.setScene(scene);
> > stage.show();
> > 
> > button.setSkin(new ButtonSkin1(button));
> > root.getChildren().remove(button);
> > }
> > 
> > Note: to see this NPE as failing test (vs. its printout on sysout), we need to \
> > re-wire the uncaughtExceptionHandler, see ComboBoxTest setup/cleanup for an \
> > example.
> 
> Thanks for the test case, I did minor changes to it and included in the next \
> commit. The NPE can occur even without `button.setDefaultButton(true);`.
> Please take a look

good catch! You are right, without explicit removal of the listener, the NPE happens \
always.

Actually, I had been sloppy and got distracted by the NPE from my actual goal which \
was to dig into Kevin's "not cleaning up after itself" and .. finally found a \
(concededly extreme corner-case :) where that's happening: when setting the skin to \
null. Two failing tests:

    @Test
    public void testDefaultButtonNullSkinReleased() {
        Button button = new Button();
        button.setDefaultButton(true);
        Group root = new Group(button);
        Scene scene = new Scene(root);
        Stage stage = new Stage();
        stage.setScene(scene);
        stage.show();
        WeakReference<ButtonSkin> defSkinRef = new \
WeakReference<>((ButtonSkin)button.getSkin());  button.setSkin(null);
        
        attemptGC(defSkinRef);
        assertNull("skin must be gc'ed", defSkinRef.get());
    }
    
    @Test
    public void testDefaultButtonNullSkinAcceleratorRemoved() {
        Button button = new Button();
        button.setDefaultButton(true);
        Group root = new Group(button);
        Scene scene = new Scene(root);
        Stage stage = new Stage();
        stage.setScene(scene);
        stage.show();
        KeyCodeCombination key = new KeyCodeCombination(KeyCode.ENTER);

        assertNotNull(scene.getAccelerators().get(key));
        button.setSkin(null);
        assertNull(scene.getAccelerators().get(key));
        
    }

An explicitly cleanup in dispose makes them pass:

    @Override
    public void dispose() {
        setDefaultButton(false);
        setCancelButton(false);
        getSkinnable().sceneProperty().removeListener(weakChangeListener);

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

PR: https://git.openjdk.java.net/jfx/pull/147


[prev in list] [next in list] [prev in thread] [next in thread]