[PATCH] xserver/Xext/sync.c: Fix wrong bracket values when startOver = FALSE (Bug 27023)

[David James]

Currently, the SyncComputeBracketValues function in
xserver/Xext/sync.c calculates bracket values incorrectly when
startOver = FALSE. I have attached a patch to fix this.

Reproduction recipe:
  1. Setup a positive transition alarm for when idle time exceeds 2000ms
  2. Setup a positive transition alarm for when idle time exceeds 5000ms
  3. Become idle, and wait for the alarms to trigger.

Expected result:
  - When idle time exceeds 2000ms, the first alarm triggers.
  - When idle time exceeds 5000ms, the second alarm triggers.

Actual result:
  - When idle time exceeds 2000ms, the first alarm triggers.
  - When idle time exceeds 5000ms, the second alarm does not trigger.

Analysis:

 1) When you first setup the alarms, the SyncComputeBracketValues
function is called with startOver = TRUE. In this case, the bracket
values are calculated correctly, and the member variable
"bracket_greater" on the idle time counter is set to 2000.
 2) When the first alarm triggers, the SyncComputeBracketValues
function is called with startOver = FALSE, from SyncChangeCounter. In
this case, the second alarm should be set up, and the member variable
"bracket_greater" on the idle time counter should be set to 5000.
However, because the test value (5000) is greater than the previous
value of bracket_greater (2000), this alarm is not enabled.

Solution:

To fix this problem, I updated the SyncChangeCounter function to call
SyncComputeBracketValues with startOver = TRUE. This causes
"bracket_greater" to be reset and the alarm to be triggered correctly.
After doing this, the startOver variable is always set to TRUE, so I
also cleaned up the code and eliminated this parameter altogether.

Patch is attached. See also Bug 27023
<http://bugs.freedesktop.org/show_bug.cgi?id=27023>

Cheers,

David
-------------- next part --------------
From ed5b62184ca07005afc63522cd518ae754d71c9b Mon Sep 17 00:00:00 2001
From: David James <davidjames at google.com>
Date: Thu, 11 Mar 2010 10:36:37 -0800
Subject: [PATCH] Fix wrong bracket values when startOver = FALSE.
 Currently, SyncComputeBracketValues reuses old values of bracket_greater
 and bracket_less when startOver = FALSE. This can result in incorrect bracket
 values. To fix this issue, the startOver parameter is removed, and we do not
 reuse old values of bracket_greater and bracket_less.

X.Org Bug 27023 <http://bugs.freedesktop.org/show_bug.cgi?id=27023>
---
 Xext/sync.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/Xext/sync.c b/Xext/sync.c
index ce65314..2015fc1 100644
--- a/Xext/sync.c
+++ b/Xext/sync.c
@@ -94,7 +94,7 @@ static SyncCounter **SysCounterList = NULL;
 #define XSyncCAAllTrigger \
     (XSyncCACounter | XSyncCAValueType | XSyncCAValue | XSyncCATestType)
 
-static void SyncComputeBracketValues(SyncCounter *, Bool);
+static void SyncComputeBracketValues(SyncCounter *);
 
 static void SyncInitServerTime(void);
 
@@ -167,7 +167,7 @@ SyncDeleteTriggerFromCounter(SyncTrigger *pTrigger)
     }
 
     if (IsSystemCounter(pTrigger->pCounter))
-	SyncComputeBracketValues(pTrigger->pCounter, /*startOver*/ TRUE);
+	SyncComputeBracketValues(pTrigger->pCounter);
 }
 
 
@@ -194,7 +194,7 @@ SyncAddTriggerToCounter(SyncTrigger *pTrigger)
     pTrigger->pCounter->pTriglist = pCur;
 
     if (IsSystemCounter(pTrigger->pCounter))
-	SyncComputeBracketValues(pTrigger->pCounter, /*startOver*/ TRUE);
+	SyncComputeBracketValues(pTrigger->pCounter);
 
     return Success;
 }
@@ -351,7 +351,7 @@ SyncInitTrigger(ClientPtr client, SyncTrigger *pTrigger, XSyncCounter counter,
     }
     else if (IsSystemCounter(pCounter))
     {
-	SyncComputeBracketValues(pCounter, /*startOver*/ TRUE);
+	SyncComputeBracketValues(pCounter);
     }
 
     return Success;
@@ -646,7 +646,7 @@ SyncChangeCounter(SyncCounter *pCounter, CARD64 newval)
 
     if (IsSystemCounter(pCounter))
     {
-	SyncComputeBracketValues(pCounter, /* startOver */ FALSE);
+	SyncComputeBracketValues(pCounter);
     }
 }
 
@@ -913,7 +913,7 @@ SyncDestroySystemCounter(pointer pSysCounter)
 }
 
 static void
-SyncComputeBracketValues(SyncCounter *pCounter, Bool startOver)
+SyncComputeBracketValues(SyncCounter *pCounter)
 {
     SyncTriggerList *pCur;
     SyncTrigger *pTrigger;
@@ -930,11 +930,8 @@ SyncComputeBracketValues(SyncCounter *pCounter, Bool startOver)
     if (ct == XSyncCounterNeverChanges)
 	return;
 
-    if (startOver)
-    {
-	XSyncMaxValue(&psci->bracket_greater);
-	XSyncMinValue(&psci->bracket_less);
-    }
+    XSyncMaxValue(&psci->bracket_greater);
+    XSyncMinValue(&psci->bracket_less);
 
     for (pCur = pCounter->pTriglist; pCur; pCur = pCur->next)
     {
-- 
1.7.0.1

Reproduction recipe:
  1. Setup a positive transition alarm for when idle time exceeds 2000ms
  2. Setup a positive transition alarm for when idle time exceeds 5000ms
  3. Become idle, and wait for the alarms to trigger.

Expected result:
  - When idle time exceeds 2000ms, the first alarm triggers.
  - When idle time exceeds 5000ms, the second alarm triggers.

Actual result:
  - When idle time exceeds 2000ms, the first alarm triggers.
  - When idle time exceeds 5000ms, the second alarm does not trigger.

Analysis:

 1) When you first setup the alarms, the SyncComputeBracketValues function is
called with startOver = TRUE. In this case, the bracket values are calculated
correctly, and the member variable "bracket_greater" on the idle time counter
is set to 2000.
 2) When the first alarm triggers, the SyncComputeBracketValues function is
called with startOver = FALSE, from SyncChangeCounter. In this case, the second
alarm should be set up, and the member variable "bracket_greater" on the idle
time counter should be set to 5000. However, because the test value (5000) is
greater than the previous value of bracket_greater (2000), this alarm is not
enabled.

Solution:

To fix this problem, I updated the SyncChangeCounter function to call
SyncComputeBracketValues with startOver = TRUE. This causes "bracket_greater"
to be reset and the alarm to be triggered correctly. After doing this, the
startOver variable is always set to TRUE, so I also cleaned up the code and
eliminated this parameter altogether.