Bug 24047 – Need to simplify nested if's in WorkerRunLoop::runInMode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Levin]

As part of this, it would be nice to also remove duplicate code in MessageQueue and add a method waitForMessageFilteredWithTimeout.

[David Levin]

Created attachment 27829 [details]
Proposed fix.

[David Levin]

This doesn't fire timers in nested loops but as I get to the point of testing that, this change should make it very easy to change that behavior as needed.

[Dmitry Titov]

As discussed on irc, the overflow check in ThreadingWin and ThreadingQt that protects from double overflow could be merged with the subsequent check that protects from int overlflow so there is only one check, something like:
// Overflow protection
if (absoluteTime > currentTime + static_cast<double>(INT_MAX) / 1000.0) {
    wait(mutex);
    return true;
}

ThreadingPthread and ThreadingGtk are fine as is since they don't put milliseconds in an int.

[David Levin]

Created attachment 27840 [details]
Proposed fix.

Addressed dimich's feedback.

[David Levin]

The changelog shows up in the middle of other changes.  I'll move it to the top before checking in.

[Alexey Proskuryakov]

Comment on attachment 27840 [details]
Proposed fix.

> +    inline MessageQueueWaitResult MessageQueue<DataType>::waitForMessageFilteredWithTimeout(DataType& result, Predicate& predicate, double absoluteTime)
>      {
> -        MutexLocker lock(m_mutex);
> +        bool allowTimeOut = absoluteTime != infiniteTime();

Inconsistent naming here: "timeout" is a word, so it should be allowTimeout or allowToTimeOut.

>          bool timedOut = false;

This one is fine.

> +        while (!m_killed  && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end())

There are two spaces before &&.

> +            timedOut = !m_condition.timedWait(m_mutex, absoluteTime) && allowTimeOut;

This looks suspicious. Can timedWait() return false for other reasons? If it can, is the problem limited to cases where allowTimeOut is false?

> +    // Time is too far in the future (and would overflow int)  - wait forever.
> +    if (absoluteTime - currentTime > static_cast<double>(INT_MAX) / 1000.0) {
> +        wait(mutex);
> +        return true;
> +    }
>  
> +    double intervalMilliseconds = (absoluteTime - currentTime) * 1000.0;

Isn't this subject to rounding errors? I don't see what guarantees that intervalMilliseconds won't overflow when converted to unsigned long.

> +    double absoluteTime = predicate.isDefaultMode() && m_sharedTimer->isActive() ? m_sharedTimer->fireTime() : MessageQueue<RefPtr<Task> >::infiniteTime();

I think that parentheses around the ?: predicate would help readability.

r=me, but please consider the comments.

[David Levin]

Commited in r41156.