JDK-8089563 : Javascript Timing Events stop work on system clock changes at past
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: 8u45,9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2015-05-11
  • Updated: 2016-09-27
  • Resolved: 2016-09-27
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
8u112Fixed 9Fixed
Related Reports
Duplicate :  
Relates :  
Relates :  
Javascript timing events(setInterval/setTimeout) stop work when system clock set a date in the past.

To reproduce:
- Open this page http://www.w3schools.com/js/tryit.asp?filename=tryjs_setinterval2 in a webview
- Change system date with a date in the past

At this point timer will stop work as each other timing events
This was fixed, Accidentally reset the state.

changeset: 5dec31642959 user: asrivastava date: Tue Jun 28 15:17:09 2016 +0530 Reviewed-by: arajkumar, kcr, mbilla URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/5dec31642959

lgtm. +1

Please review : http://cr.openjdk.java.net/~asrivastava/8089563/webrev.03/ Tested with JDK9 32 bit also.

We don't normally put URLs to code changesets in our source code, so you can remove the github URL comment line (no need for a new webrev if that is the only change). +1

The logic to calculate the monotonicallyIncreasingTime() is basically taken from open source glib library function "g_get_monotonic_time()". In my first patch I put a reference for the same.Which uses guint64, guint32 data types. I missed to use correct data types. Instead of unsigned data types I used signed. While debugging I found that the "if (ticks32 - ticksAs32Bit <= INT_MAX)" always returns true. Will send a modified patch.

Yes..This condition if (ticks32 - ticksAs32Bit <= INT_MAX) always returns true as per the logic in this case.. I think the only way this condition can sometimes return false is if we store the return values of GetTickCount64() and timeGetTime() into unsigned ints. May be ankit can explain in detail..

Maybe I'm mnissing something. It seems to me that by converting the 64-bit GetTickCount() value to a 32-bit integer (to compare it with the result of timeGetTime), you risk having that value truncated. I don't think the logic would work if that were to happen. Can you explain your solution more clearly? Also, I question the validity of the following test: if (ticks32 - ticksAs32Bit <= INT_MAX) since both values are signed 32-bit integers, that test will always return true. I note that the GetTickCount64() and timeGetTime functions both return unsigned ints, so I'm puzzled by the conversion to signed ints.

lgtm +1

Incorporated Arun's comment: http://cr.openjdk.java.net/~asrivastava/8089563/webrev.02/

Incorporated Arun's review comment: http://cr.openjdk.java.net/~asrivastava/8089563/webrev.01/

+#include <winbase.h> We are already including "windows.h" that internally includes winbase.h. So it should work without winbase.h. + int32_t ticks_as_32bit =(int32_t)ticks; Leave a space before and after =. Use camel caps variable naming style. i.e. ticksAs32Bit.

+ wchar_t* wString=new wchar_t[4096]; + MultiByteToWideChar(CP_ACP, 0, charArray, -1, wString, 4096); + return wString; Who is taking the ownership of memory referenced by "wString"? Looks like you are leaking it. + kernel32 = GetModuleHandle (convertCharArrayToLPCWSTR("KERNEL32.DLL")); You could use GetModuleHandleW instead of GetModuleHandle like below, it avoids conversion from char* to wchar_t*. HMODULE kernel32 = ::GetModuleHandleW(L"Kernel32.dll"); Why can't you directly call "GetTickCount64()", winbase.h has declaration for the same. -#elif PLATFORM(JAVA) && OS(LINUX) +#elif PLATFORM(JAVA) +#if OS(WINDOWS) #elif PLATFORM(JAVA) && OS(LINUX) // Linux implementation #elif PLATFORM(JAVA) && OS(WINDOWS) // Windows implementation Please indent properly according to the style guidelines.

Review Comments: 1.I think creation of new function convertCharArrayToLPCWSTR() is not needed. You can do MultiByteToWideChar(CP_ACP, 0, charArray, -1, wString, 4096); in monotonicallyIncreasingTime() itself so that new array need not be passed from convertCharArrayToLPCWSTR. 2. Indentation errors : if (kernel32 != NULL){ -> space between ( and { else{ -> space between else and { int32_t ticks_as_32bit =(int32_t)ticks; -> space between = and (int32_t) +#else -> Empty line before #else // glib libraray -> library spelling check 3. If and else - need to keep { and } if (ticks32 - ticks_as_32bit <= INT_MAX) ticks += ticks32 - ticks_as_32bit; else ticks -= ticks_as_32bit - ticks32;

http://cr.openjdk.java.net/~asrivastava/8089563/webrev.00/ Please review. monotonicallyIncreasingTime() implemented taking reference from glib library. Tested on Win64 timer works fine.

Given the comments and evaluation during the review, I think it makes sense to fix both at the same time. Probably best to use this bug, since it has been open for the longest time. If it turns out that the Windows fix will be more involved, then go ahead and proceed with just the Linux fix separately.

Timer works fine on Mac platform. Tested on OS X Yosemite 10.10.4 JDK-- java version "9-ea" Java(TM) SE Runtime Environment (build 9-ea+109-2016-03-09-180602.javare.4620) Java HotSpot(TM) 64-Bit Server VM (build 9-ea+109-2016-03-09-180602.javare.4620, mixed mode) And java version "1.9.0-ea" Java(TM) SE Runtime Environment (build 1.9.0-ea-b94) Java HotSpot(TM) 64-Bit Server VM (build 1.9.0-ea-b94, mixed mode) Opened a new issue for linux platform JDK-8157559. Will keep on working for Windows platform.

Issue resolved on Linux 64 platform, will test on Win and Mac and provide the webrev. monotonicallyIncreasingTime implementaion was using gettimeofday procedure which can not handle clock variation. Now using std::chrono::steady_clock::now() to get the correct time.

Debugging to understand the flow

I can't suggest any workaround.

I can reproduce it. In com.sun.webkit.Timer.notifyTick() a "fireTime" value is checked to fit in a range [0 - System.currentTimeMillis()] and only then a native WebKit timer callback is called. I can only suggest that the system time change doesn't affect WebKit and it continues to set a "fireTime" relative to the initial time. So that the value stops to fit in the range. When I set system time to normal, the timer wakes up.

This is not a P1 bug. Lowering to P3 (although if there is a workaround then it is a P4).