Bug 24719 – QTMovieWinTimer logic inversion

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

[Eric Carlson]

The Win32 timer used by QTMovieWin has a logic error that causes it to always use SetTimer(), even when the callback interval is below USER_TIMER_MINIMUM. A side effect of this is that the movie timer is sometimes blocked for significant amounts of time because (low priority) WM_TIMER messages are not processed when the thread's message queue has any higher priority messages, and WebCore/Win's timer uses (high priority) PostMessage for low interval timers.

[Eric Carlson]

rdar://6704282

[Eric Carlson]

Created attachment 28789 [details]
proposed patch

[Adam Roben (:aroben)]

Comment on attachment 28789 [details]
proposed patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 41862)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> [phone]  Eric Carlson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        <rdar://problem/6704282>
> +        https://bugs.webkit.org/show_bug.cgi?id=24719
> +        QTMovieWinTimer logic inversion
> +
> +        Fix logic inversion in the Win32 timer used by QTMovieWin that caused it to always
> +        use SetTimer, even when the intervals was below USER_TIMER_MINIMUM. A side effect of
> +        this is that the movie timer is sometimes blocked for significant amounts of time 
> +        because WM_TIMER messages are not processed when the thread's message queue has any
> +        higher priority messages, and WebCore/Win's timer uses PostMessage for low interval
> +        timers.

I'm not sure from this comment whether the movie timer is now blocked due to your change, or whether it was getting blocked before your change and no longer will.

> +        Not possible to make a test for this because it is so timeing dependant.

Typo: timeing

> @@ -51,7 +51,9 @@ static LRESULT CALLBACK TimerWindowWndPr
>          processingCustomTimerMessage = true;
>          sharedTimerFiredFunction();
>          processingCustomTimerMessage = false;
> -    } else
> +    } else if (message == WM_TIMER) {
> +        sharedTimerFiredFunction();
> +	} else

Should we call KillTimer here? WebCore does that. If we don't do that, WM_TIMER will continue being sent to this window forever.

The braces around the body of the else if case should be removed. It looks like you have a tab on the last modified line here.

> @@ -99,23 +96,20 @@ void setSharedTimerFireDelay(double inte
>              intervalInMS = (unsigned)interval;
>      }
>  
> -    if (timerID) {
> -        KillTimer(0, timerID);
> -        timerID = 0;
> -    }

Why don't we need this KillTimer call anymore?

>      // We don't allow nested PostMessages, since the custom messages will effectively starve
>      // painting and user input. (Win32 has a tri-level queue with application messages > 
>      // user input > WM_PAINT/WM_TIMER.)
>      // In addition, if the queue contains input events that have been there since the last call to
>      // GetQueueStatus, PeekMessage or GetMessage we favor timers.

> -    if (intervalInMS < USER_TIMER_MINIMUM && processingCustomTimerMessage &&
> -        !LOWORD(::GetQueueStatus(QS_ALLINPUT))) {
> +    if (intervalInMS < USER_TIMER_MINIMUM 
> +		&& !processingCustomTimerMessage 
> +        && !LOWORD(::GetQueueStatus(QS_ALLINPUT))) {

You have some tabs here.

> -    } else
> -        timerID = SetTimer(0, 0, intervalInMS, timerFired);
> +	} else {
> +        timerID = SetTimer(timerWindowHandle, timerFiredMessage, intervalInMS, 0);
> +	}

It seems a little strange to use timerFiredMessage as the ID of the timer.

You have some tabs here. The braces around the body of the else should be removed.

r- for now until you convince me we really don't need to call KillTimer ;-)

[Eric Carlson]

Created attachment 28798 [details]
updated patch

Fixed the problems aroben noted and a few more.

[Adam Roben (:aroben)]

Comment on attachment 28798 [details]
updated patch

> +    } else if (message == WM_TIMER && wParam == timerID) {
> +        KillTimer(hWnd, timerID);
> +        timerID = 0;
> +        sharedTimerFiredFunction();
>      } else

Should we call stopSharedTimer here instead of duplicating its code?

It might be worth calling out that the bug fix here was the addition of the ! operator, since the other changes in the patch kind of obscure that fact.

r=me

[Eric Carlson]

Committed revision 41908.