Bug 114696 - Wrong use of REALTIME clock causes pages to fail loading
Summary: Wrong use of REALTIME clock causes pages to fail loading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-16 12:49 PDT by Anderson Fraga
Modified: 2014-02-06 10:04 PST (History)
13 users (show)

See Also:


Attachments
Webkit patch for invalid uses of REALTIME clock (7.06 KB, patch)
2013-04-17 08:52 PDT, Anderson Fraga
no flags Details | Formatted Diff | Diff
Webkit patch for invalid uses of REALTIME clock (6.85 KB, patch)
2013-04-17 09:08 PDT, Anderson Fraga
no flags Details | Formatted Diff | Diff
Webkit patch for invalid uses of REALTIME clock (6.84 KB, patch)
2013-04-17 09:15 PDT, Anderson Fraga
ap: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Revised patch (6.05 KB, patch)
2013-04-18 12:56 PDT, Anderson Fraga
no flags Details | Formatted Diff | Diff
Patch (7.90 KB, patch)
2013-04-19 07:53 PDT, Anderson Fraga
no flags Details | Formatted Diff | Diff
Webkit patch for invalid uses of REALTIME clock (6.99 KB, patch)
2013-04-19 09:38 PDT, Anderson Fraga
no flags Details | Formatted Diff | Diff
Webkit patch for invalid uses of REALTIME clock when computing deltas (7.00 KB, patch)
2013-04-19 10:12 PDT, Anderson Fraga
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anderson Fraga 2013-04-16 12:49:10 PDT
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.
Comment 1 Anderson Fraga 2013-04-17 08:51:28 PDT
+CC jmason@blackberry.com
Comment 2 Anderson Fraga 2013-04-17 08:52:57 PDT
Created attachment 198567 [details]
Webkit patch for invalid uses of REALTIME clock
Comment 3 Joe Mason 2013-04-17 09:02:30 PDT
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.
Comment 4 Anderson Fraga 2013-04-17 09:08:45 PDT
Created attachment 198579 [details]
Webkit patch for invalid uses of REALTIME clock

revised version of patch
Comment 5 Anderson Fraga 2013-04-17 09:15:18 PDT
Created attachment 198580 [details]
Webkit patch for invalid uses of REALTIME clock

revised version of patch with review flags set :)
Comment 6 Alexey Proskuryakov 2013-04-18 10:57:43 PDT
Comment on attachment 198580 [details]
Webkit patch for invalid uses of REALTIME clock

Nice catch!
Comment 7 WebKit Commit Bot 2013-04-18 10:58:29 PDT
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 8 Alexey Proskuryakov 2013-04-18 11:02:15 PDT
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?
Comment 9 Anderson Fraga 2013-04-18 11:41:51 PDT
(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.
Comment 10 Joe Mason 2013-04-18 12:02:41 PDT
(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.
Comment 11 Anderson Fraga 2013-04-18 12:26:59 PDT
Good catch. I see it now. I will remove this change from the patch.
Comment 12 Anderson Fraga 2013-04-18 12:56:57 PDT
Created attachment 198754 [details]
Revised patch
Comment 13 Joe Mason 2013-04-18 13:21:44 PDT
I just filed https://bugs.webkit.org/show_bug.cgi?id=114826 to fix timedWait and its callers.
Comment 14 Benjamin Poulain 2013-04-18 14:03:24 PDT
Comment on attachment 198754 [details]
Revised patch

The changelog needs _a lot_ more explanations.
Comment 15 Alexey Proskuryakov 2013-04-18 20:57:29 PDT
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.
Comment 16 Arvid Nilsson 2013-04-18 21:21:42 PDT
(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" :)
Comment 17 Arvid Nilsson 2013-04-18 21:25:41 PDT
(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".
Comment 18 Anderson Fraga 2013-04-19 07:53:49 PDT
Created attachment 198855 [details]
Patch
Comment 19 Anderson Fraga 2013-04-19 07:58:36 PDT
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.
Comment 20 Anderson Fraga 2013-04-19 09:38:13 PDT
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.
Comment 21 Anderson Fraga 2013-04-19 10:12:01 PDT
Created attachment 198895 [details]
Webkit patch for invalid uses of REALTIME clock when computing deltas

Edited changeLogs for better description
Comment 22 Benjamin Poulain 2013-04-19 11:00:18 PDT
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.
Comment 23 Anderson Fraga 2013-04-19 11:08:57 PDT
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.
Comment 24 Benjamin Poulain 2013-04-19 12:49:52 PDT
(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 25 Anders Carlsson 2014-02-06 10:00:47 PST
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.