• Type: Bug
• Component: javafx
• Sub-Component: web
• Affected Version: 8u112,9

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

• Submitted: 2016-07-28
• Updated: 2020-01-31
• Resolved: 2016-08-11

JDK 8	JDK 9
8u131Fixed	9Fixed

changeset: 54223585e09c user: arajkumar date: Thu Aug 11 10:47:44 2016 +0530 Reviewed-by: kcr, mbilla URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/54223585e09c 11-08-2016 +1 10-08-2016 Looks great now. All my testing runs as expected. +1 10-08-2016 Indeed it was a careless mistake, sorry about that. Fixed in the new webrev: http://cr.openjdk.java.net/~arajkumar/8162715/webrev.05 10-08-2016 The changes to the assertion testing and fixing the loop look good, but you are still adding 20 msec in the call method: + stat.firedTime = System.currentTimeMillis() + 20; You should store the unmodified time here. 10-08-2016 Addressed Kevin's review comments: http://cr.openjdk.java.net/~arajkumar/8162715/webrev.04 09-08-2016 Added 20ms offset while testing fire time. http://cr.openjdk.java.net/~arajkumar/8162715/webrev.03 06-08-2016 Thanks Kevin. Actually I created a simple program to test this case. Below are the samples taken from the test, FAILED: java.time.delta:349.0, js.time.delta:350.0, expected:350.0 FAILED: java.time.delta:99.0, js.time.delta:100.0, expected:100.0 FAILED: java.time.delta:549.0, js.time.delta:549.0, expected:550.0 FAILED: java.time.delta:399.0, js.time.delta:400.0, expected:400.0 FAILED: java.time.delta:49.0, js.time.delta:49.0, expected:50.0 FAILED: java.time.delta:149.0, js.time.delta:150.0, expected:150.0 FAILED: java.time.delta:199.0, js.time.delta:200.0, expected:200.0 FAILED: java.time.delta:549.0, js.time.delta:550.0, expected:550.0 FAILED: java.time.delta:599.0, js.time.delta:599.0, expected:600.0 FAILED: java.time.delta:199.0, js.time.delta:200.0, expected:200.0 (* java.time.delta => uses System.currentTimeMillis(), js.time.delta uses engine.executeScript("Date.now()" to measure elapsed time) Most the time the difference is 1ms, as you mentioned. It looks like a floating point / integer conversion issues in the underlying functions. I will add 20 ms offset while testing for the failure case. 05-08-2016 It would be cleaner to add the 20 msec in the comparison for two reasons. First, if there is a failure you want to print out the actual time value received (not an adjusted time value). Second, it is more consistent with what you do in the <= case (and avoids comparing in that case with a value that is 20 msec larger than actual. You have introduced a control flow bug in this version. You changed the loop to <= INTERVAL_COUNT which means you will produce INTERVAL_COUNT+1 callbacks, but the countdown latch is only waiting for INTERVAL_COUNT. 05-08-2016 Btw, as a (much) faster way to run just that one test over and over, I was using this: gradle -PBUILD_SDK_FOR_TEST=false -PTEST_SDK=build/sdk :web:cleanTest :web:test --tests test.javafx.scene.web.MiscellaneousTest 04-08-2016 @Kevin: Thanks for the details. Timer shouldn't fire early. I tried couple of times here, I couldn't see the issue. I will take a look. 04-08-2016 The test looks good now. I also verified that running it on a build without the fix for JDK-8161258 fails consistently with a good error message. I ran the test several times in a row on a two different Windows machines and saw intermittent failures. Here are two of the failures (the first is from Win 10 and the second from a somewhat slower Win7 machine): test.javafx.scene.web.MiscellaneousTest > testDOMTimer FAILED java.lang.AssertionError: firedTime:1470332081972 createdTime:1470332081673 interval:300 at org.junit.Assert.fail(Assert.java:91) at org.junit.Assert.assertTrue(Assert.java:43) at test.javafx.scene.web.MiscellaneousTest.testDOMTimer(MiscellaneousTest.java:200) [actual delta time = 299] test.javafx.scene.web.MiscellaneousTest > testDOMTimer FAILED java.lang.AssertionError: firedTime:1470332066895 createdTime:1470332066651 interval:250 at org.junit.Assert.fail(Assert.java:91) at org.junit.Assert.assertTrue(Assert.java:43) at test.javafx.scene.web.MiscellaneousTest.testDOMTimer(MiscellaneousTest.java:200) [actual delta time = 244] This suggests that the accuracy of the timer on Windows isn't good enough to do a strictly >= comparison. You should consider testing against a value of 20 msec less than the expected interval. The worst case I saw was off by 9 msec, and most of the failures were in the 1-5 msec range. 04-08-2016 @Arun: I meant that the test should interrupt the JUnit test thread, which is waiting in CountDownLatch.await, but the way you have coded it seems like a fine approach. I will review it shortly. 04-08-2016 Thanks Kevin. Thread.interrupt() is not working as expected because it needs a safe point to interrupt a Thread. In our case, TimerCallback.call will be called from native, it never goes into the safe point. Currently I have stored all data into a "Stat[]" and validating at the end. http://cr.openjdk.java.net/~arajkumar/8162715/webrev.02 04-08-2016 1. The assertTrue statements in the call method of the TimerCallback class will not be propagated to the testDOMTimer method. I verified this by running the test against a Windows build without the fix for JDK-8161258. The assertion failures were not printed and not noticed by JUnit. The only reason that the test failed with an error is that the assertion failures caused the latch.CountDown() to be skipped so the test timed out after 30 seconds, but with no other error message. In order to address this, you will want some mechanism to save the assertion error in an instance variable, and then have the test thread check. In order to do this, you either need to use the variant of await with a timeout (and pass in a timeout of, say, 5 seconds) or you need to interrupt the test thread (which will throw InterruptedException from await()). The latter is probably cleaner. You could pass the thread to be interrupted into the constructor of TimerCallback. Something like this might work: public void call(long startTime, long interval) { try { // Timer should not fire too early assertTrue((System.currentTimeMillis() - startTime) >= interval); // Timer should not be too late. Since it is not a real time system, // we can't expect the timer to be fire at exactly on the requested // time, give a 1000 ms extra time. assertTrue((System.currentTimeMillis() - startTime) <= (interval + 1000)); latch.countDown(); catch (Error e) { err = e testThread.interrupt(); } } Then the test method would check for and throw the error in Interrupted exception like this: try { timer.latch.await(); } catch (InterruptedException e) { if (err != null) { throw err; } else { throw new AssertionError(e); } } 2. There are unrelated white space changes to the testDOMObjectThreadOwnership() method that should be reverted. 03-08-2016 lgtm +1 02-08-2016 ok..i Just checked other test cases in the same file (like below), which are throwing exception.. @Test public void testRT22458() throws Exception { @Test public void testRT30835() throws Exception { 02-08-2016 >> 3. Since you are throwing AssertionError in testDOMTimer(), testDOMTimer() should throw exeception? >> @Test(timeout = 10000) public void testDOMTimer() throws Exception { Since it is a junit test method, adding throws clause doesn't add value. 02-08-2016 Below correction is not present in webrev.01 ? 3. Since you are throwing AssertionError in testDOMTimer(), testDOMTimer() should throw exeception? @Test(timeout = 10000) public void testDOMTimer() throws Exception { 02-08-2016 Incorporated Murali's review comments: http://cr.openjdk.java.net/~arajkumar/8162715/webrev.01 02-08-2016 I tested this fix in windows 7, without JDK-8161258 and test case failed (pasted below) and test case passed with the fix JDK-8161258 test.javafx.scene.web.MiscellaneousTest > testDOMTimer FAILED java.lang.Exception: test timed out after 10000 milliseconds I executed test case ~10 times and i did not observe the 2nd test case failing or flaky. If you are not sure about 2nd case, im ok to remove this. Few Minor Comments: 1. To create WebEngine, already a function defined createWebEngine(). Can you use this API to create webengine instead of WebEngine webEngine = new WebEngine(); 2. There should be a space between JSObject and webEngine. JSObject window = (JSObject) webEngine.executeScript("window"); 3. Since you are throwing AssertionError in testDOMTimer(), testDOMTimer() should throw exeception? @Test(timeout = 10000) public void testDOMTimer() throws Exception { 4. You can add null check for window. JSObject window = (JSObject)webEngine.executeScript("window"); assertNotNull(window); 5. Also you can make window as final. final JSObject window = (JSObject) webEngine.executeScript("window"); 01-08-2016 http://cr.openjdk.java.net/~arajkumar/8162715/webrev.00 Tested on Windows and Linux. I don't have OSX machine to verify the test. + // Timer should not fire too early + assertTrue((System.currentTimeMillis() - startTime) >= interval); + // Timer should not be too late. Since it is not a real time system, + // we can't expect the timer to be fire at exactly on the requested + // time, give a 1000 ms extra time. + assertTrue((System.currentTimeMillis() - startTime) + <= (interval + 1000)); Two kinds of expectations are tested here, 1. Should not fire too early 2. Should not fire too late #1 is easy to test, but #2 will be tricky since it is not executed on real time operating system. Added additional 1 sec offset for the case #2, but even it could fail. If all agrees, I can remove the #2 since it going to be flaky. 30-07-2016 This is an excellent plan given that the regression has already led to two independent bug reports: JDK-8162621 and JDK-8162549 28-07-2016