Bug 114696 – Wrong use of REALTIME clock causes pages to fail loading

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Anderson Fraga]

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.

[Anderson Fraga]

+CC [e-mail]

[Anderson Fraga]

Created attachment 198567 [details]
Webkit patch for invalid uses of REALTIME clock

[Joe Mason]

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.

[Anderson Fraga]

Created attachment 198579 [details]
Webkit patch for invalid uses of REALTIME clock

revised version of patch

[Anderson Fraga]

Created attachment 198580 [details]
Webkit patch for invalid uses of REALTIME clock

revised version of patch with review flags set :)

[Alexey Proskuryakov]

Comment on attachment 198580 [details]
Webkit patch for invalid uses of REALTIME clock

Nice catch!

[WebKit Commit Bot]

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

[Alexey Proskuryakov]

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?

[Anderson Fraga]

(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.

[Joe Mason]

(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.

[Anderson Fraga]

Good catch. I see it now. I will remove this change from the patch.

[Anderson Fraga]

Created attachment 198754 [details]
Revised patch

[Joe Mason]

I just filed https://bugs.webkit.org/show_bug.cgi?id=114826 to fix timedWait and its callers.

[Benjamin Poulain]

Comment on attachment 198754 [details]
Revised patch

The changelog needs _a lot_ more explanations.

[Alexey Proskuryakov]

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.

[Arvid Nilsson]

(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" :)

[Arvid Nilsson]

(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".

[Anderson Fraga]

Created attachment 198855 [details]
Patch

[Anderson Fraga]

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.

[Anderson Fraga]

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.

[Anderson Fraga]

Created attachment 198895 [details]
Webkit patch for invalid uses of REALTIME clock when computing deltas

Edited changeLogs for better description

[Benjamin Poulain]

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.

[Anderson Fraga]

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.

[Benjamin Poulain]

(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.

[Anders Carlsson]

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.