There are a few places in webcore where the use of a REALTIME clock is wrong. The cases are simply measuring time from point A to point B and setting up timers. The use of a REALTIME clock in such cases will cause the measurement to be completely wrong and also cause timers to expire or not fire if the system time is changed between those events. To avoid such problems, the use of MONOTONIC time is required.
+CC jmason@blackberry.com
Created attachment 198567 [details] Webkit patch for invalid uses of REALTIME clock
Comment on attachment 198567 [details] Webkit patch for invalid uses of REALTIME clock View in context: https://bugs.webkit.org/attachment.cgi?id=198567&action=review > Source/WTF/ChangeLog:3 > + [BlackBerry] Wrong use of REALTIME clock causes pages to fail loading Since this touches cross-platform code it shouldn't have the [BlackBerry] tag in the title. > Source/WebCore/ChangeLog:10 > + This bug is very hard to reproduce. It requires multiple large webworks applications to launch and start loading. In theory this should affect page loads on any port, not just BlackBerry webworks apps. It's just a race condition that's easier to replicate when more of these measurements are in use, such as in a complex webworks app.
Created attachment 198579 [details] Webkit patch for invalid uses of REALTIME clock revised version of patch
Created attachment 198580 [details] Webkit patch for invalid uses of REALTIME clock revised version of patch with review flags set :)
Comment on attachment 198580 [details] Webkit patch for invalid uses of REALTIME clock Nice catch!
Comment on attachment 198580 [details] Webkit patch for invalid uses of REALTIME clock Rejecting attachment 198580 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 198580, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: * malformed patch at line 31: patching file Source/WebCore/dom/Document.cpp Hunk #2 succeeded at 2584 (offset -6 lines). patching file Source/WebCore/loader/CrossOriginPreflightResultCache.cpp patching file Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp patching file Source/WebCore/workers/WorkerRunLoop.cpp Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alexey Proskuryakov']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/131201
Comment on attachment 198580 [details] Webkit patch for invalid uses of REALTIME clock View in context: https://bugs.webkit.org/attachment.cgi?id=198580&action=review > Source/WebCore/workers/WorkerRunLoop.cpp:56 > > // SharedTimer interface. > virtual void setFiredFunction(void (*function)()) { m_sharedTimerFunction = function; } > - virtual void setFireInterval(double interval) { m_nextFireTime = interval + currentTime(); } > + virtual void setFireInterval(double interval) { m_nextFireTime = interval + monotonicallyIncreasingTime(); } Wait, but why is this correct?
(In reply to comment #8) > (From update of attachment 198580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=198580&action=review > > > Source/WebCore/workers/WorkerRunLoop.cpp:56 > > > > // SharedTimer interface. > > virtual void setFiredFunction(void (*function)()) { m_sharedTimerFunction = function; } > > - virtual void setFireInterval(double interval) { m_nextFireTime = interval + currentTime(); } > > + virtual void setFireInterval(double interval) { m_nextFireTime = interval + monotonicallyIncreasingTime(); } > > Wait, but why is this correct? This is setting up a timer to fire at a given time (which is currently set as interval + currentTime()). Because curretTime() is implemented with REALTIME clock, if the system time changes during the set up of the timer and before the timer expires, you will get the timer to expire before it is supposed to. Which will likely result in undesired behaviour.
(In reply to comment #8) > Wait, but why is this correct? As far as I can see, m_nextFireTime is only used to pass to waitForMessageFilteredWithTimeout in runInMode: if (waitMode == WaitForMessage) absoluteTime = (predicate.isDefaultMode() && m_sharedTimer->isActive()) ? m_sharedTimer->fireTime() : MessageQueue<Task>::infiniteTime(); MessageQueueWaitResult result; OwnPtr<WorkerRunLoop::Task> task = m_messageQueue.waitForMessageFilteredWithTimeout(result, predicate, absoluteTime); From there it's passed to PlatformCondition::timedWait... which compares it against currentTime() in the pthreads implementation. So this is not correct. It does seem like an error in PlatformCondition, though - it seems to me that it should not be affected by changes to the clock. But changing that would involve changing a lot of calling code. Glad you caught that! I thought I'd verified that each changed in the patch was only used in a direct comparison.
Good catch. I see it now. I will remove this change from the patch.
Created attachment 198754 [details] Revised patch
I just filed https://bugs.webkit.org/show_bug.cgi?id=114826 to fix timedWait and its callers.
Comment on attachment 198754 [details] Revised patch The changelog needs _a lot_ more explanations.
Ben, what kind of explanation are you thinking about? This seems like a pretty clear improvement to me, am I missing something? Anderson, do you know why this patch is not applying (EWS bubbles are purple)? Is it made against an old checkout or a branch? It would help if you could upload a patch that applies to ToT cleanly.
(In reply to comment #15) > Ben, what kind of explanation are you thinking about? This seems like a pretty clear improvement to me, am I missing something? > > Anderson, do you know why this patch is not applying (EWS bubbles are purple)? Is it made against an old checkout or a branch? It would help if you could upload a patch that applies to ToT cleanly. In our local setup at BlackBerry, check out "svn/master" branch and cherry pick your patch onto that branch before you do "webkit-patch upload" :)
(In reply to comment #14) > (From update of attachment 198754 [details]) > The changelog needs _a lot_ more explanations. If you agree that monotonic time should be used for delta time computations, which I think is a pretty uncontroversial idea, I don't think there's much more to say than "if we don't, delta calculations are going to fail if system clock changes".
Created attachment 198855 [details] Patch
Arvid, looks like some other garbage got added during the webkit-upload, in: - Tools/ChangeLog - Tools/qmake/mkspecs/features/default_pre.prf These are not part of my patch. And I am up to date with svn/master.
Created attachment 198892 [details] Webkit patch for invalid uses of REALTIME clock This patch should apply cleanly. Thanks for the help with the patch Joe Mason.
Created attachment 198895 [details] Webkit patch for invalid uses of REALTIME clock when computing deltas Edited changeLogs for better description
Comment on attachment 198895 [details] Webkit patch for invalid uses of REALTIME clock when computing deltas View in context: https://bugs.webkit.org/attachment.cgi?id=198895&action=review > Source/WebCore/ChangeLog:13 > + This bug is very hard to reproduce. It requires multiple large webworks applications to launch and start loading. > + Then to reproduce, constantly change the system time while the applications are loading. The end result without this patch > + is that some applications will fail to load and endup with an empty body. With the patch, applications load as expected even > + with many system time changes. Sorry, I should have been clearer. This is the part I want improved. It adds confusion, not explanation.
How about this: This bug is hard to reproduce as it requires the system time to be changed between the computation of time deltas. If this happens, the delta computation will off by the delta between the old system time and the new system time, which will result in undesired behaviour. One of the observed behaviour is that pages will fail to load.
(In reply to comment #23) > How about this: > > This bug is hard to reproduce as it requires the system time to be changed between the computation of time deltas. If this happens, the delta computation will off by the delta between the old system time and the new system time, which will result in undesired behaviour. One of the observed behaviour is that pages will fail to load. I would do: 1) First describe what is the bug. 2) Explain why the invalid delta caused the undesired behavior. 3) Explain how you fix it-> this can be short, the patch is fairly obvious.
Comment on attachment 198895 [details] Webkit patch for invalid uses of REALTIME clock when computing deltas This has been fixed in the relevant files, we now use std::chrono::steady_clock instead.