Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8205092: Added dirty check for viewOrderChildren #353

Merged
merged 4 commits into from Jan 31, 2019

Conversation

@DeanWookey
Copy link
Contributor

DeanWookey commented Jan 21, 2019

If the scene graph is modified while handling a mouse event, viewOrderChildren can contain nodes which are no longer in the scene graph, causing an NPE in PickResultChooser.processOffer. This is fairly rare, and only affects parent nodes with children that have a view order set. testPickingChildNodeWithDirtyViewOrder replicates these conditions and results in the same NPE as reported in the bug report.

This solution doesn't unset the dirty bits because I'm unsure of the consequences. In the worst case the viewOrderChildren will be recalculated one extra time because of this.

An alternative solution is to keep viewOrderChildren in sync with the children list, however that would mean sorting it whenever the view order property of a child is changed (the child calls markViewOrderChildrenDirty of its parent when this happens). That could happen many times. Updating the list as it's needed seems like a better solution to me.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Jan 23, 2019

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Jan 23, 2019

I'll take a look.

Copy link
Collaborator

kevinrushforth left a comment

Both the fix and the test look good to me. I agree that it is best to not clear the dirty bit after checking it in getOrderedChildren. A similar pattern is used in doUpdatePeer.

I have a few minor code style comments in the test (a missing space in a few places), but otherwise, this looks ready to go.

This fix should have a second reviewer.

@@ -741,6 +743,101 @@ public void sceneListenerCanAddChild() {
assertSame(scene, ParentShim.getChildren(child).get(3).getScene());
}

@Test
public void testPickingChildNode(){

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 25, 2019

Collaborator

Minor: add a space before {

Scene scene = new Scene(g);
stage.setScene(scene);
stage.show();
ParentShim.getChildren(g).addAll(rect1,rect2);

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 25, 2019

Collaborator

Minor: add space after the comma.

}

@Test
public void testPickingChildNodeWithViewOrderSet(){

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 25, 2019

Collaborator

Minor: add space before {

Scene scene = new Scene(g);
stage.setScene(scene);
stage.show();
ParentShim.getChildren(g).addAll(rect1,rect2);

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 25, 2019

Collaborator

Minor: add space after comma

}

@Test
public void testPickingChildNodeWithDirtyViewOrder(){

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 25, 2019

Collaborator

Minor: add space before {

Scene scene = new Scene(g);
stage.setScene(scene);
stage.show();
ParentShim.getChildren(g).addAll(rect1,rect2);

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 25, 2019

Collaborator

Minor: add space after comma.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Jan 25, 2019

I tagged @aghaisas as a second reviewer.

@DeanWookey

This comment has been minimized.

Copy link
Contributor Author

DeanWookey commented Jan 28, 2019

I made the requested changes and fixed a spelling error in computeViewOrderChildrenAndUpdatePeer as it seemed like a good opportunity.

Copy link
Collaborator

kevinrushforth left a comment

Looks good.

@kevinrushforth kevinrushforth dismissed their stale review Jan 30, 2019

I took a closer look at the function being called and this is not MT safe. I'll explain is more detail.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Jan 30, 2019

I missed this when initially reviewing it, and only saw it when I was double-checking something related to the updated list of children in the peer.

The problem with the fix as currently proposed is that the Parent::computeViewOrderChidrenAndUpdatePeer method combines combines the "compute ordered children" operation with the "update the ordered children in the peer" operation. The latter must only be called from the Scene.ScenePulseListener::pulse while holding the render lock (all of the doUpdatePeermethods in the nodes are only ever called with the lock held, so the existing usage is fine). Performing the "update peer" operation at any other time is not thread-safe.

We will need an updated fix that splits "compute ordered children" and "update the ordered children in the peer" into two parts, with only "compute ordered children" being called from getOrderedChildren. One way to do this would be to rename the existing method to computeViewOrderChildren and add a boolean updatePeer argument which would be called with true from doUpdatePeer and false from getOrderedChildren. Or you could just split it into two methods and have doUpdatePeer call both, while getOrderedChildred would only call the compute method.

@@ -292,6 +292,10 @@ private void computeViewOrderChidrenAndUpdatePeer() {
// Call this method if children view order is needed for picking.
// The returned list should be treated as read only.
private List<Node> getOrderedChildren() {
if (isDirty(DirtyBits.PARENT_CHILDREN_VIEW_ORDER)) {
//Fix for JDK-8205092
computeViewOrderChildrenAndUpdatePeer();

This comment has been minimized.

Copy link
@kevinrushforth

kevinrushforth Jan 30, 2019

Collaborator

As mentioned in my earlier comment, this needs to be changed as it is not thread-safe.

This comment has been minimized.

Copy link
@DeanWookey

DeanWookey Jan 30, 2019

Author Contributor

Thanks for the detailed explanation. I went with the second option, although I didn't make a second method to update the peer's view order since it was a one-liner.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Jan 30, 2019

I'll take a closer look and run some tests, but at a quick glance, the latest changes look fine.

@DeanWookey

This comment has been minimized.

Copy link
Contributor Author

DeanWookey commented Jan 30, 2019

To avoid double (or perhaps more in extreme cases) computing the viewOrderChildren when this error would occur, we could store an additional flag in the markViewOrderChildrenDirty() method to store a separate dirty flag for the node's viewOrderChildren list only. computeViewOrderChildren() could check and set that variable.

Like I mentioned before, I'm not sure that optimisation is worth it, but it could be done relatively easily if you think it's worth it.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Jan 30, 2019

The performance enhancement you mention could be done -- something similar is done for transforms in node (see the transformDirty flag) -- not sure it's worth the effort in this case, but if you want to file a follow-on bug for this we could consider it.

Copy link
Collaborator

kevinrushforth left a comment

Looks good. I'll push it tomorrow if @aghaisas has no further comments on your latest change.

@kevinrushforth kevinrushforth merged commit 12ae657 into javafxports:develop Jan 31, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.