javafxports / openjdk-jfx

The openjfx repo has moved to:
https://github.com/openjdk/jfx
GNU General Public License v2.0
1.01k stars 146 forks source link

java.lang.ArrayIndexOutOfBoundsException: -1 #69

Open pupeno opened 6 years ago

pupeno commented 6 years ago

In my exception tracker, I got this error report:

java.lang.ArrayIndexOutOfBoundsException: -1
    at java.util.ArrayList.elementData(ArrayList.java:422)
    at java.util.ArrayList.get(ArrayList.java:435)
    at com.sun.javafx.collections.ObservableListWrapper.get(ObservableListWrapper.java:89)
    at com.sun.javafx.collections.VetoableListDecorator.get(VetoableListDecorator.java:306)
    at javafx.scene.Parent.updateCachedBounds(Parent.java:1591)
    at javafx.scene.Parent.recomputeBounds(Parent.java:1535)
    at javafx.scene.Parent.impl_computeGeomBounds(Parent.java:1388)
    at javafx.scene.layout.Region.impl_computeGeomBounds(Region.java:3078)
    at javafx.scene.Node.updateGeomBounds(Node.java:3577)
    at javafx.scene.Node.getGeomBounds(Node.java:3530)
    at javafx.scene.Node.getLocalBounds(Node.java:3478)
    at javafx.scene.Node.updateTxBounds(Node.java:3641)
    at javafx.scene.Node.getTransformedBounds(Node.java:3424)
    at javafx.scene.Node.updateBounds(Node.java:559)
    at javafx.scene.Parent.updateBounds(Parent.java:1719)
    at javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2404)
    at com.sun.javafx.tk.Toolkit.lambda$runPulse$29(Toolkit.java:398)
    at java.security.AccessController.doPrivileged(AccessController.java)
    at com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:397)
    at com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:424)
    at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:518)
    at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:498)
    at com.sun.javafx.tk.quantum.QuantumToolkit.pulseFromQueue(QuantumToolkit.java:491)
    at com.sun.javafx.tk.quantum.QuantumToolkit.lambda$runToolkit$403(QuantumToolkit.java:319)
    at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
    at com.sun.glass.ui.win.WinApplication._runLoop(WinApplication.java)
    at com.sun.glass.ui.win.WinApplication.lambda$null$147(WinApplication.java:177)
    at java.lang.Thread.run(Thread.java:748)

At no point, this touches my application, which is why I think it might be a bug. It might be that my application is creating the wrong state and that's why this is happening.

At my current version of JavaFX, the method updateCachedBounds looks like this:

private boolean updateCachedBounds(final List<Node> dirtyNodes,
                                       int remainingDirtyNodes) {
        // fast path for untransformed bounds calculation
        if (cachedBounds.isEmpty()) {
            createCachedBounds(dirtyNodes);
            return true;
        }

        int invalidEdges = 0;

        if ((left == null) || left.boundsChanged) {
            invalidEdges |= LEFT_INVALID;
        }
        if ((top == null) || top.boundsChanged) {
            invalidEdges |= TOP_INVALID;
        }
        if ((near == null) || near.boundsChanged) {
            invalidEdges |= NEAR_INVALID;
        }
        if ((right == null) || right.boundsChanged) {
            invalidEdges |= RIGHT_INVALID;
        }
        if ((bottom == null) || bottom.boundsChanged) {
            invalidEdges |= BOTTOM_INVALID;
        }
        if ((far == null) || far.boundsChanged) {
            invalidEdges |= FAR_INVALID;
        }

        // These indicate the bounds of the Group as computed by this
        // function
        float minX = cachedBounds.getMinX();
        float minY = cachedBounds.getMinY();
        float minZ = cachedBounds.getMinZ();
        float maxX = cachedBounds.getMaxX();
        float maxY = cachedBounds.getMaxY();
        float maxZ = cachedBounds.getMaxZ();

        // this checks the newly added nodes first, so if dirtyNodes is the
        // whole children list, we can end early
        for (int i = dirtyNodes.size() - 1; remainingDirtyNodes > 0; --i) {
            final Node node = dirtyNodes.get(i);
            if (node.boundsChanged) {
                // assert node.isVisible();
                node.boundsChanged = false;
                --remainingDirtyNodes;
                tmp = getChildTransformedBounds(node, BaseTransform.IDENTITY_TRANSFORM, tmp);
                if (!tmp.isEmpty()) {
                    float tmpx = tmp.getMinX();
                    float tmpy = tmp.getMinY();
                    float tmpz = tmp.getMinZ();
                    float tmpx2 = tmp.getMaxX();
                    float tmpy2 = tmp.getMaxY();
                    float tmpz2 = tmp.getMaxZ();

                    // If this node forms an edge, then we will set it to be the
                    // node for this edge and update the min/max values
                    if (tmpx <= minX) {
                        minX = tmpx;
                        left = node;
                        invalidEdges &= ~LEFT_INVALID;
                    }
                    if (tmpy <= minY) {
                        minY = tmpy;
                        top = node;
                        invalidEdges &= ~TOP_INVALID;
                    }
                    if (tmpz <= minZ) {
                        minZ = tmpz;
                        near = node;
                        invalidEdges &= ~NEAR_INVALID;
                    }
                    if (tmpx2 >= maxX) {
                        maxX = tmpx2;
                        right = node;
                        invalidEdges &= ~RIGHT_INVALID;
                    }
                    if (tmpy2 >= maxY) {
                        maxY = tmpy2;
                        bottom = node;
                        invalidEdges &= ~BOTTOM_INVALID;
                    }
                    if (tmpz2 >= maxZ) {
                        maxZ = tmpz2;
                        far = node;
                        invalidEdges &= ~FAR_INVALID;
                    }
                }
            }
        }

        if (invalidEdges != 0) {
            // failed to validate some edges
            return false;
        }

        cachedBounds = cachedBounds.deriveWithNewBounds(minX, minY, minZ,
                                                        maxX, maxY, maxZ);
        return true;
    }

And line 1591 is:

final Node node = dirtyNodes.get(i);

I guess dirty nodes were modified by another thread?

kevinrushforth commented 6 years ago

I guess dirty nodes were modified by another thread?

Possibly, in which case it would be an application bug. Nodes that are "live" (attached to a scene of a Window that is showing) must only be modified on the JavaFX Application Thread.

pupeno commented 6 years ago

Doesn't JavaFX prevent you from doing UI actions outside the JavaFX thread?

kevinrushforth commented 6 years ago

The thread is only checked for Window and Scene operations. Modifying scene graph nodes does not check the thread, but is documented (in the class documentation for Node) as needing to be done on the JavaFX app thread.

pupeno commented 6 years ago

What should I be looking for? My application never builds a scene by itself, all the UI is in FXML files.

kevinrushforth commented 6 years ago

Do you have any bindings where the source of the binding might be operated on by another thread?

If you can provide a test program, then we could also take a look at it.

pupeno commented 6 years ago

I can't provide a test program. I can give you a copy of my app, but it only happened once on hours of execution. I don't know how to reproduce it. I did go through all my source code making sure changes to bindings were happening in the JavaFX thread, but obviously it's impossible to be sure.

kevinrushforth commented 6 years ago

I see. In that case this will be hard to diagnose.

Another approach would be for you to instrument a local build of JavaFX and add a thread check in the place(s) that the dirtyNodes list is modified.

salimvanak commented 6 years ago

I have an application that has the same problem, with the a very similar stack trace. The code below (which I found somewhere in the java bug database) gives the same exception with an almost identical stack trace to the one above.

I also have a very dirty fix for this problem in my application. I can share it if it would be helpful.

package test;

import javafx.application.Application;
import javafx.beans.binding.DoubleBinding;
import javafx.scene.Group;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.scene.shape.Line;
import javafx.scene.text.Text;
import javafx.stage.Stage;

public class TestApp extends Application { 
    public static void main(String args[]) { 
        launch(); 
    } 

    public void start(Stage primaryStage){ 
        Text t = new Text("x"); 
        HBox h = new HBox(t); 
        Group g = new Group(h); 

        Line l = new Line(); 
        DoubleBinding innerWidth = javafx.beans.binding.Bindings.createDoubleBinding( 
                () -> g.layoutBoundsProperty().get().getWidth(), 
                g.layoutBoundsProperty()); 
        l.endXProperty().bind(innerWidth); 

        VBox v = new VBox(l, g); 
        v.boundsInLocalProperty().addListener((a,b,c) -> {}); 

        Group g2 = new Group(v); 
        g2.layoutBoundsProperty().addListener((a,b,c) -> {}); 

        h.getChildren().remove(t); 

        //Platform.exit(); 
    } 
} 
kevinrushforth commented 6 years ago

Thanks for the test case. If the above code is causing the AIOOBE then it isn't a threading problem. As for a fix...we would need to understand the problem first and make sure that any fix does not introduce other problems. Or did you mean that you have an application-level workaround?

If you this this is a bug in JavaFX, and would like to contribute, please read the CONTRIBUTING guidelines.

salimvanak commented 6 years ago

I have just run the test case JDK10 and it still produces a similar stack trace.

java 10.0.1 2018-04-17
Java(TM) SE Runtime Environment 18.3 (build 10.0.1+10)
Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10.0.1+10, mixed mode)

What I have is an application-level workaround.

The workaround is basically:

This stops the application from hanging when ever this inconsistency happens.

pupeno commented 6 years ago

I couldn't reproduce it, but this error happened a few more times in my codebase (it appears on my exception tracker). It is more common than I expected. I'm going to add this workaround to see if it changes anything (for better or worse).

pupeno commented 6 years ago

@salimvanak would you mind sharing the code for the workaround?

salimvanak commented 6 years ago
/**
* Empties the dirtyChildren list and set the dirtyChildrenCount to ZERO
*/
public static void recomputeBounds(Parent p) {
        try {
        log.error("Checking {} id: {}", p.getClass().getName(), p.getId());
        Field dirtyChildren = Parent.class.getDeclaredField("dirtyChildren");
        dirtyChildren.setAccessible(true);
        dirtyChildren.set(p, null);
        Field dirtyChildrenCount = Parent.class.getDeclaredField("dirtyChildrenCount");
        dirtyChildrenCount.setAccessible(true);
        dirtyChildrenCount.set(p, 0);
    } 
    catch (Exception e) {
        log.error("While recalculating bounds",e);
    }
}

The dirtynodes.txt file contains the extras that are needed to detect when the exception is thrown and how the scene graph is walked. The scene graph walk is not complete, for example, the function misses out graphic nodes similar to those in Labeled and TabPane .

dirtynodes.txt

@kevinrushforth I can try to find some time to look at this, if you think that would help? I have never contributed to such a large project before so may need some guidance.

pupeno commented 6 years ago

@salimvanak: thank you so much for sharing that code. It looks like you put a lot of effort in coming up with the workaround.

I modified your uncaughtException to not need main pre-defined:

@Override
public void uncaughtException(Thread t, Throwable e) {
    System.out.printf("Unhandled Exception: %s%n", e);
    if (e instanceof ArrayIndexOutOfBoundsException) {
        boolean isUpdateCachedBoundsBug = Arrays.stream(e.getStackTrace())
                .anyMatch(s -> s.getClassName().startsWith("javafx.scene.Parent") && "updateCachedBounds".equals(s.getMethodName()));
        if (isUpdateCachedBoundsBug) {
            System.out.printf("Cleaning dirty nodes on the whole scene graph.%n");
            Platform.runLater(() -> { // make sure it s the UI thread
                for (Stage stage : StageHelper.getStages()) {
                    traverseChildren(stage.getScene().getRoot(), r -> { // walk the scene graph
                        if (r instanceof Parent) recomputeBounds((Parent) r);
                        return true;
                    });
                }

            });
        } else {
            System.out.printf("Detected an AIOBE that is not an updateCachedBounds bug: %s%n", e);
        }
    } else {
        System.out.printf("Unhandled Exception: %s%n", e);
    }
}

Unfortunately, I couldn't test it. I took the crashing example you posted earlier and I tried adding a default exception handler but it's never called. I even ended up doing this:

public class TestApp extends Application {
    public static void main(String args[]) {
        ErrorHandler errorHandler = new ErrorHandler();
        Thread.setDefaultUncaughtExceptionHandler(errorHandler);
        Thread.currentThread().setUncaughtExceptionHandler(errorHandler);
        launch();
    }

    public void start(Stage primaryStage) {
        ErrorHandler errorHandler = new ErrorHandler();
        Thread.setDefaultUncaughtExceptionHandler(errorHandler);
        Thread.currentThread().setUncaughtExceptionHandler(errorHandler);
        Platform.runLater(() -> {
            Thread.currentThread().setUncaughtExceptionHandler(errorHandler);

            Text t = new Text("x");
            HBox h = new HBox(t);
            Group g = new Group(h);

            Line l = new Line();
            DoubleBinding innerWidth = javafx.beans.binding.Bindings.createDoubleBinding(
                    () -> g.layoutBoundsProperty().get().getWidth(),
                    g.layoutBoundsProperty());
            l.endXProperty().bind(innerWidth);

            VBox v = new VBox(l, g);
            v.boundsInLocalProperty().addListener((a, b, c) -> {});

            Group g2 = new Group(v);
            g2.layoutBoundsProperty().addListener((a, b, c) -> {});

            h.getChildren().remove(t);

            //Platform.exit();
        });
    }
}

and it still wouldn't call the the exception handler. Any ideas why?

Also, in you traverseChildren method there's a variable called depth that is never defined. I haven't looked into that bit yet.

salimvanak commented 6 years ago

@pupeno Its is because the static methods in the binding class all (helpfully) catch any exceptions and log them, therefore the would be no "uncaught exception". Below is the corrected test case:

public class TestApp extends Application {
    public static void main(String args[]) {
        Thread.setDefaultUncaughtExceptionHandler(TestApp::handleException);
        launch();
    }

    public static void handleException(Thread t, Throwable e) {
        System.out.println("Unhandled Exception: " + e);
        if (e instanceof IndexOutOfBoundsException || e instanceof ArrayIndexOutOfBoundsException) {
            boolean isUpdateCachedBoundsBug = Arrays.stream(e.getStackTrace()).anyMatch(
                    s -> s.getClassName().startsWith("javafx.scene.Parent") && "updateCachedBounds".equals(s.getMethodName()));

            if (isUpdateCachedBoundsBug) {
                System.out.println("Detected an AIOBE or IOBE from the updateCachedBounds bug:");
                e.printStackTrace();
            } else {
                System.out.println("Detected an AIOBE or IOBE that is not an updateCachedBounds bug: " + e);
            }
        } else {
            System.out.println("Unhandled Exception: " + e);
        }
    }

    public void start(Stage primaryStage) {
        Text t = new Text("x");
        HBox h = new HBox(t);
        Group g = new Group(h);
        Line l = new Line();
        DoubleBinding innerWidth = new DoubleBinding() {
            {
                bind(g.layoutBoundsProperty());
            }

            @Override
            protected double computeValue() {
                return g.layoutBoundsProperty().get().getWidth();
            }

            @Override
            public void dispose() {
                super.unbind(getDependencies());
            }

            @Override
            public ObservableList<?> getDependencies() {
                return FXCollections.singletonObservableList(g.layoutBoundsProperty());
            }
        };

        l.endXProperty().bind(innerWidth);

        VBox v = new VBox(l, g);
        v.boundsInLocalProperty().addListener((a, b, c) -> {
        });

        Group g2 = new Group(v);
        g2.layoutBoundsProperty().addListener((a, b, c) -> {
        });

        h.getChildren().remove(t);
        // Platform.exit();
    }
}

dirtynodes.txt

micheljung commented 6 years ago

I had the same issue and it was a threading problem. To figure out why, I set a conditional breakpoint to where dirtyNodes is set, which suspended when not in the JavaFX application thread. I fixed all such cases and the IOOBE was gone.

salimvanak commented 6 years ago

I don't see a threading issue in the test case. Yet it still produces the exception.

yangwuan55 commented 5 years ago

I have same problem,but I don't know where the problem is. Why doesn't the framework fix this?

yuforia commented 4 years ago

I'm not yet sure whether there is a bug in the toolkit, but regardless it would always be great if debugging could be made easier. I managed to catch an IndexOutOfBoundsException in the debugger and here are a few thoughts.

How about some some assertions?

When the exception happens, dirtyChildrenCount is 9 while size of dirtyChildren is only 8. This isn't supposed to happen, right? If that's the case, could more checks be added to detect inconsistencies and throw exceptions earlier? It will also be helpful even if checks are only performed in debug mode.

count9size8

Ending this loop when the end of the list is reached?

Could this for loop in function updateCachedBounds be simplified, so that it obviously never throws IndexOutOfBoundsException?

https://github.com/javafxports/openjdk-jfx/blob/6eabc8c84f698c04548395826a8bb738087666b5/modules/javafx.graphics/src/main/java/javafx/scene/Parent.java#L1700-L1704

In its current form, it's not easy to understand when the loop will stop: i gets decremented in each iteration, but the condition checks remainingDirtyNodes, which only gets updated if node.boundsChanged is true. To understand this part, I guess intimate knowledge of Parent, Node, and the calculation and caching of bounds would be needed.

On the other hand, I think what the for loop does is just iterating through the list dirtyNodes, possibly ending early if going through the whole list is unnecessary. It should be functionally equivalent to a forEach or a for in statement in some languages, which can be stopped early, but will always stop after reaching the end of a list, and the possibility of out-of-bounds errors caused by manual indexing can be avoided.

If this for loop were to be modified to address the issue, how about adding i >= 0 to its condition, in addition to remainingDirtyNodes > 0? If i has gone out of bounds, it just means we've gone through the entire list, no harm done.

kevinrushforth commented 4 years ago

This GitHub issue tracker is not actively tracked or managed. Please file a bug at bugreport.java.com.

If you are interested in contributing a fix for this issue, please see CONTRIBUTING.md in the official openjdk/jfx GutHub repo.

bytecod3r commented 3 years ago

@pupeno, did you finally find any fix for this?

pupeno commented 3 years ago

No. I just workaround it with:

    if (e instanceof ArrayIndexOutOfBoundsException &&

isUpdateCachedBoundsBug(e)) { report(new RuntimeException("Detected and corrected ArrayIndexOutOfBoundsException due to cached bounds.")); Platform.runLater(() -> { // make sure it s the UI thread for (Window window : Window.getWindows()) { traverseChildren(window.getScene().getRoot(), r -> { // walk the scene graph if (r instanceof Parent) recomputeBounds((Parent) r); return true; }); } }); }

On Wed, 9 Sep 2020 at 03:24, Bytecod3r notifications@github.com wrote:

@pupeno https://github.com/pupeno, did you finally find any fix for this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/javafxports/openjdk-jfx/issues/69#issuecomment-689258103, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACB2MWBIDTXZZCBLI5L4LSE3RPNANCNFSM4E3NJXXQ .

-- J. Pablo Fernández pupeno@pupeno.com (http://pupeno.com)