• Type: Bug
• Component: javafx
• Sub-Component: graphics
• Affected Version: 8

• Priority: P3
• Status: Resolved
• Resolution: Fixed

• Submitted: 2012-08-01
• Updated: 2015-06-17
• Resolved: 2013-11-25

JDK 8
8Fixed

http://hg.openjdk.java.net/openjfx/8/graphics/rt/rev/ea99d38f2c90 25-11-2013 Tested-with: Ran all existing unit tests, and all other snapshot test programs (toys) with instrumented code to detect the MT issue. I tested this on Windows, Mac, and Linux. Unit Tests: This cannot be turned into a unit test since it requires intrusive instrumentation in the runtime. Instead, the way to test this is to verify no regressions in shapshot functionality. 25-11-2013 The latest version of the fix looks fine to me. Thanks. 25-11-2013 Updated version (with just the above change). http://cr.openjdk.java.net/~kcr/RT-23840.02/ 25-11-2013 Btw, here is the delta diff between v1 and v2: diff --git a/modules/graphics/src/main/java/javafx/scene/Scene.java b/modules/gr aphics/src/main/java/javafx/scene/Scene.java --- a/modules/graphics/src/main/java/javafx/scene/Scene.java +++ b/modules/graphics/src/main/java/javafx/scene/Scene.java @@ -1153,9 +1153,9 @@ if (!paused) { getRoot().updateBounds(); if (impl_peer != null) { - try { impl_peer.waitForRenderingToComplete(); impl_peer.waitForSynchronization(); + try { // Run the synchronizer while holding the render lock scenePulseListener.synchronizeSceneNodes(); } finally { impl_peer.releaseSynchronization(false); 25-11-2013 #1 - Agreed. I will make that change (Note that I copied the code from the pulse logic, which has the same problem, so I will file a new P4 issue for that). #2 - The existing call to synchronizeSceneNodes() is still there in the "else" block, so there is no change in behavior. 25-11-2013 Hi Kevin, Two observations: 1. The lock/finally/unlock pattern seems to be misused, in that the following: 1156 try { 1157 impl_peer.waitForRenderingToComplete(); 1158 impl_peer.waitForSynchronization(); 1159 // Run the synchronizer while holding the render lock 1160 scenePulseListener.synchronizeSceneNodes(); 1161 } finally { 1162 impl_peer.releaseSynchronization(false); 1163 } is going to fail should the waitForRenderingToComplete() throw an exception for some reason. This will happen because waitForSynchronization() won't be invoked, and hence the RenderLock won't be acquired. So the finally {releaseSynchronization(false)} will attempt to release a lock that isn't held. I'd suggest to follow the pattern exactly, as in: lock(); try { //... } finally { unlock(); } This code doesn't cause a confusion because it is a common pattern, and it is guaranteed to work as expected in all cases. 2. Scene.doCSSLayoutSyncForSnapshot() didn't require the impl_peer to be non-null previously, and now the synchronizeSceneNodes() action will only be performed if the peer exists. Could this cause problems for people trying to take snapshots of scenes that don't have a peer yet? 25-11-2013 Please review the following: http://cr.openjdk.java.net/~kcr/RT-23840/ Reviewers: felipe, anthony, snorthov The fix is to do the same thing in snapshot synchronization as is done in the pulse for regular synchronization. Namely, wait for rendering to be complete (this happens later on anyway when the snapshot is actually taken, so we just do it a little earlier now), and then grab the render lock before synchronizing the scene params. We then release the lock. Since the existing method to release the lock also updates the scene state, I added a boolean to the method which to indicate whether to do this. All existing calls to this method now pass "true" whereas snapshot passes "false". I've tested as many snapshot programs as I could find on Windows. I'll test on Mac and Linux on Monday. 23-11-2013 I verfied that snapshot is not MT-safe with respect to synchronizing the nodes in a scene prior to taking the snapshot. I instrumented the code as follows to prove this: * Added a 500 msec sleep in PresentingPainter just before starting rendering; I then verified that a subsequent snapshot will cause new node params to be synced down to the NG render graph while the rendering is in progress. This can lead to rendering with inconsistent state causing bad rendering or exceptions. * Added an assertion check in synchronizeSceneNodes() to verify that the renderLock is held by the FX thread. In the case of snapshot, it isn't (thus leading the problem noted above) I am in the process of testing a simple fix. 23-11-2013