[PATCH 1/4] quartz: Factor out more of AdviseTime() and AdvisePeriodic().

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/systemclock.c | 74 ++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 47 deletions(-)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index 24d8651aaba..cbe5f395258 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -169,8 +169,29 @@ static DWORD WINAPI SystemClockAdviseThread(void *param)
     }
 }

-static void notify_thread(struct system_clock *clock)
+static HRESULT add_sink(struct system_clock *clock, DWORD_PTR handle,
+        REFERENCE_TIME due_time, REFERENCE_TIME period, DWORD_PTR *cookie)
 {
+    struct advise_sink *sink;
+
+    if (!handle)
+        return E_INVALIDARG;
+
+    if (!cookie)
+        return E_POINTER;
+
+    if (!(sink = heap_alloc_zero(sizeof(*sink))))
+        return E_OUTOFMEMORY;
+
+    sink->handle = (HANDLE)handle;
+    sink->due_time = due_time;
+    sink->period = period;
+    sink->cookie = InterlockedIncrement(&cookie_counter);
+
+    EnterCriticalSection(&clock->cs);
+    list_add_tail(&clock->sinks, &sink->entry);
+    LeaveCriticalSection(&clock->cs);
+
     if (!InterlockedCompareExchange(&clock->thread_created, TRUE, FALSE))
     {
         clock->notify_event = CreateEventW(NULL, FALSE, FALSE, NULL);
@@ -178,6 +199,9 @@ static void notify_thread(struct system_clock *clock)
         clock->thread = CreateThread(NULL, 0, SystemClockAdviseThread, clock, 0, NULL);
     }
     SetEvent(clock->notify_event);
+
+    *cookie = sink->cookie;
+    return S_OK;
 }

static HRESULT WINAPI SystemClockImpl_QueryInterface(IReferenceClock *iface, REFIID iid, void **out)
@@ -225,72 +249,28 @@ static HRESULT WINAPI SystemClockImpl_AdviseTime(IReferenceClock *iface,
         REFERENCE_TIME base, REFERENCE_TIME offset, HEVENT event, DWORD_PTR *cookie)
 {
     struct system_clock *clock = impl_from_IReferenceClock(iface);
-    struct advise_sink *sink;

TRACE("clock %p, base %s, offset %s, event %#lx, cookie %p.\n",
             clock, debugstr_time(base), debugstr_time(offset), event, cookie);

-    if (!event)
-        return E_INVALIDARG;
-
     if (base + offset <= 0)
         return E_INVALIDARG;

-    if (!cookie)
-        return E_POINTER;
-
-    if (!(sink = heap_alloc_zero(sizeof(*sink))))
-        return E_OUTOFMEMORY;
-
-    sink->handle = (HANDLE)event;
-    sink->due_time = base + offset;
-    sink->period = 0;
-    sink->cookie = InterlockedIncrement(&cookie_counter);
-
-    EnterCriticalSection(&clock->cs);
-    list_add_tail(&clock->sinks, &sink->entry);
-    LeaveCriticalSection(&clock->cs);
-
-    notify_thread(clock);
-
-    *cookie = sink->cookie;
-    return S_OK;
+    return add_sink(clock, event, base + offset, 0, cookie);
 }

static HRESULT WINAPI SystemClockImpl_AdvisePeriodic(IReferenceClock* iface,
         REFERENCE_TIME start, REFERENCE_TIME period, HSEMAPHORE semaphore, DWORD_PTR *cookie)
 {
     struct system_clock *clock = impl_from_IReferenceClock(iface);
-    struct advise_sink *sink;

TRACE("clock %p, start %s, period %s, semaphore %#lx, cookie %p.\n",
             clock, debugstr_time(start), debugstr_time(period), semaphore, cookie);

-    if (!semaphore)
-        return E_INVALIDARG;
-
     if (start <= 0 || period <= 0)
         return E_INVALIDARG;

-    if (!cookie)
-        return E_POINTER;
-
-    if (!(sink = heap_alloc_zero(sizeof(*sink))))
-        return E_OUTOFMEMORY;
-
-    sink->handle = (HANDLE)semaphore;
-    sink->due_time = start;
-    sink->period = period;
-    sink->cookie = InterlockedIncrement(&cookie_counter);
-
-    EnterCriticalSection(&clock->cs);
-    list_add_tail(&clock->sinks, &sink->entry);
-    LeaveCriticalSection(&clock->cs);
-
-    notify_thread(clock);
-
-    *cookie = sink->cookie;
-    return S_OK;
+    return add_sink(clock, semaphore, start, period, cookie);
 }

static HRESULT WINAPI SystemClockImpl_Unadvise(IReferenceClock *iface, DWORD_PTR cookie)

[PATCH 2/4] quartz: Use a condition variable to wake up the system clock advise thread.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/systemclock.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index cbe5f395258..597373a4112 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -42,10 +42,11 @@ struct system_clock
     IUnknown *outer_unk;
     LONG refcount;

-    BOOL thread_created;
-    HANDLE thread, notify_event, stop_event;
+    BOOL thread_created, thread_stopped;
+    HANDLE thread;
     REFERENCE_TIME last_time;
     CRITICAL_SECTION cs;
+    CONDITION_VARIABLE cv;

struct list sinks;
 };
@@ -96,11 +97,12 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface)
     {
         if (clock->thread)
         {
-            SetEvent(clock->stop_event);
+            EnterCriticalSection(&clock->cs);
+            clock->thread_stopped = TRUE;
+            LeaveCriticalSection(&clock->cs);
+            WakeConditionVariable(&clock->cv);
             WaitForSingleObject(clock->thread, INFINITE);
             CloseHandle(clock->thread);
-            CloseHandle(clock->notify_event);
-            CloseHandle(clock->stop_event);
         }
         clock->cs.DebugInfo->Spare[0] = 0;
         DeleteCriticalSection(&clock->cs);
@@ -128,7 +130,6 @@ static DWORD WINAPI SystemClockAdviseThread(void *param)
     struct system_clock *clock = param;
     struct advise_sink *sink, *cursor;
     REFERENCE_TIME current_time;
-    HANDLE handles[2] = {clock->stop_event, clock->notify_event};

TRACE("Starting advise thread for clock %p.\n", clock);

@@ -162,10 +163,13 @@ static DWORD WINAPI SystemClockAdviseThread(void *param)
             timeout = min(timeout, (sink->due_time - current_time) / 10000);
         }

-        LeaveCriticalSection(&clock->cs);
-
-        if (WaitForMultipleObjects(2, handles, FALSE, timeout) == 0)
+        SleepConditionVariableCS(&clock->cv, &clock->cs, timeout);
+        if (clock->thread_stopped)
+        {
+            LeaveCriticalSection(&clock->cs);
             return 0;
+        }
+        LeaveCriticalSection(&clock->cs);
     }
 }

@@ -194,11 +198,9 @@ static HRESULT add_sink(struct system_clock *clock, DWORD_PTR handle,

if (!InterlockedCompareExchange(&clock->thread_created, TRUE, FALSE))
     {
-        clock->notify_event = CreateEventW(NULL, FALSE, FALSE, NULL);
-        clock->stop_event = CreateEventW(NULL, TRUE, FALSE, NULL);
         clock->thread = CreateThread(NULL, 0, SystemClockAdviseThread, clock, 0, NULL);
     }
-    SetEvent(clock->notify_event);
+    WakeConditionVariable(&clock->cv);

*cookie = sink->cookie;
     return S_OK;

[PATCH 3/4] quartz/tests: Check that system clock measurements are in sequence instead of comparing absolute time.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/tests/systemclock.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/dlls/quartz/tests/systemclock.c b/dlls/quartz/tests/systemclock.c
index f13e89abedb..fa9464429f0 100644
--- a/dlls/quartz/tests/systemclock.c
+++ b/dlls/quartz/tests/systemclock.c
@@ -24,12 +24,6 @@

static ULONGLONG (WINAPI *pGetTickCount64)(void);

-static BOOL compare_time(REFERENCE_TIME x, REFERENCE_TIME y, unsigned int max_diff)
-{
-    REFERENCE_TIME diff = x > y ? x - y : y - x;
-    return diff <= max_diff;
-}
-
 static IReferenceClock *create_system_clock(void)
 {
     IReferenceClock *clock = NULL;
@@ -172,7 +166,7 @@ static void test_aggregation(void)
 static void test_get_time(void)
 {
     IReferenceClock *clock = create_system_clock();
-    REFERENCE_TIME time1, time2;
+    REFERENCE_TIME time1, ticks, time2;
     HRESULT hr;
     ULONG ref;

@@ -180,18 +174,21 @@ static void test_get_time(void)
     ok(hr == E_POINTER, "Got hr %#x.\n", hr);

hr = IReferenceClock_GetTime(clock, &time1);
-    if (pGetTickCount64)
-        time2 = pGetTickCount64() * 10000;
-    else
-        time2 = (REFERENCE_TIME)GetTickCount() * 10000;
     ok(hr == S_OK, "Got hr %#x.\n", hr);
     ok(time1 % 10000 == 0, "Expected no less than 1ms coarseness, but got time %s.\n",
             wine_dbgstr_longlong(time1));
-    ok(compare_time(time1, time2, 20 * 10000), "Expected about %s, got %s.\n",
-            wine_dbgstr_longlong(time2), wine_dbgstr_longlong(time1));
+
+    if (pGetTickCount64)
+        ticks = pGetTickCount64() * 10000;
+    else
+        ticks = (REFERENCE_TIME)GetTickCount() * 10000;

hr = IReferenceClock_GetTime(clock, &time2);
     ok(hr == (time2 == time1 ? S_FALSE : S_OK), "Got hr %#x.\n", hr);
+    ok(time2 % 10000 == 0, "Expected no less than 1ms coarseness, but got time %s.\n",
+            wine_dbgstr_longlong(time1));
+
+    ok(time2 >= ticks && ticks >= time1, "Got timestamps %I64d, %I64d, %I64d.\n", time1, ticks, time2);

Sleep(100);
     hr = IReferenceClock_GetTime(clock, &time2);

[PATCH 3/4] quartz/tests: Check that system clock measurements are in sequence instead of comparing absolute time.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=94259

Your paranoid android.

=== w8 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 786340000, 786400000, 786340000.

=== w8adm (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 579450000, 579530000, 579450000.

=== w864 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 34279625900000, 377877009530000, 34279625900000.

=== w1064v1507 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 507560000, 507650000, 507560000.

=== w1064v1809 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 660270000, 660310000, 660270000.

=== w1064 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 601610000, 601560000, 601610000.

=== w1064_tsign (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 882550000, 882500000, 882550000.

=== w10pro64 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 706740000, 706710000, 706740000.

=== w2008s64 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 796420000, 796400000, 796420000.

=== w864 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 34279621680000, 377877005310000, 34279621680000.

=== w1064v1507 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 499270000, 499370000, 499270000.

=== w1064v1809 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 660550000, 660620000, 660550000.

=== w1064 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 589700000, 589680000, 589700000.

=== w1064_2qxl (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 696720000, 696710000, 696720000.

=== w1064_tsign (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 885080000, 885000000, 885080000.

=== w10pro64 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 705210000, 705310000, 705210000.

=== w10pro64_ar (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 2709150000, 2709060000, 2709150000.

=== w10pro64_he (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 593630000, 593590000, 593630000.

=== w10pro64_ja (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 593560000, 593590000, 593560000.

=== w10pro64_zh_CN (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 571660000, 571560000, 571660000.

[PATCH 3/4] quartz/tests: Check that system clock measurements are in sequence instead of comparing absolute time.

[Francois Gouget]

On Mon, 19 Jul 2021, Marvin wrote:
[...]
> === w1064v1809 (32 bit report) ===
>
> quartz:
> systemclock.c:191: Test failed: Got timestamps 660270000, 660310000, 660270000.
>
> === w1064 (32 bit report) ===
>
> quartz:
> systemclock.c:191: Test failed: Got timestamps 601610000, 601560000, 601610000.

It looks like this does fail on about every Windows version, VM or not:

https://test.winehq.org/data/patterns.html#quartz:systemclock

I guess you're already looking into it?

[PATCH 3/4] quartz/tests: Check that system clock measurements are in sequence instead of comparing absolute time.

[Zebediah Figura (she/her)]

On 7/22/21 1:59 PM, Francois Gouget wrote:
> On Mon, 19 Jul 2021, Marvin wrote:
> [...]
> > === w1064v1809 (32 bit report) ===
> >
> > quartz:
> > systemclock.c:191: Test failed: Got timestamps 660270000, 660310000, 660270000.
> >
> > === w1064 (32 bit report) ===
> >
> > quartz:
> > systemclock.c:191: Test failed: Got timestamps 601610000, 601560000, 601610000.
>
> It looks like this does fail on about every Windows version, VM or not:
>
> https://test.winehq.org/data/patterns.html#quartz:systemclock
>
> I guess you're already looking into it?

Yep, it's already on my list.

[PATCH 4/4] quartz: Free outstanding advise requests when destroying a system clock.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/quartz/systemclock.c       |  8 ++++++++
 dlls/quartz/tests/systemclock.c | 12 ++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c
index 597373a4112..73a923a7b99 100644
--- a/dlls/quartz/systemclock.c
+++ b/dlls/quartz/systemclock.c
@@ -90,6 +90,7 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface)
 {
     struct system_clock *clock = impl_from_IUnknown(iface);
     ULONG refcount = InterlockedDecrement(&clock->refcount);
+    struct advise_sink *sink, *cursor;

TRACE("%p decreasing refcount to %u.\n", clock, refcount);

@@ -104,6 +105,13 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface)
             WaitForSingleObject(clock->thread, INFINITE);
             CloseHandle(clock->thread);
         }
+
+        LIST_FOR_EACH_ENTRY_SAFE(sink, cursor, &clock->sinks, struct advise_sink, entry)
+        {
+            list_remove(&sink->entry);
+            heap_free(sink);
+        }
+
         clock->cs.DebugInfo->Spare[0] = 0;
         DeleteCriticalSection(&clock->cs);
         heap_free(clock);
diff --git a/dlls/quartz/tests/systemclock.c b/dlls/quartz/tests/systemclock.c
index fa9464429f0..be5472ed6e1 100644
--- a/dlls/quartz/tests/systemclock.c
+++ b/dlls/quartz/tests/systemclock.c
@@ -272,11 +272,19 @@ static void test_advise(void)
     ok(hr == S_OK, "Got hr %#x.\n", hr);
     ok(WaitForSingleObject(semaphore, 200) == WAIT_TIMEOUT, "Semaphore should not be signaled.\n");

-    CloseHandle(event);
-    CloseHandle(semaphore);
+    ResetEvent(event);
+    hr = IReferenceClock_GetTime(clock, &current);
+    ok(SUCCEEDED(hr), "Got hr %#x.\n", hr);
+    hr = IReferenceClock_AdviseTime(clock, current, 500 * 10000, (HEVENT)event, &cookie);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);

ref = IReferenceClock_Release(clock);
     ok(!ref, "Got outstanding refcount %d.\n", ref);
+
+    ok(WaitForSingleObject(event, 0) == WAIT_TIMEOUT, "Event should not be signaled.\n");
+
+    CloseHandle(event);
+    CloseHandle(semaphore);
 }

START_TEST(systemclock)

[PATCH 4/4] quartz: Free outstanding advise requests when destroying a system clock.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=94260

Your paranoid android.

=== w2008s64 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 798440000, 798430000, 798440000.

=== w7u_adm (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 653790000, 653800000, 653790000.

=== w8 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 780350000, 780310000, 780350000.

=== w8adm (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 570120000, 570150000, 570120000.

=== w864 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 34279626520000, 377877010150000, 34279626520000.

=== w1064v1507 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 504310000, 504370000, 504310000.

=== w1064v1809 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 658580000, 658590000, 658580000.

=== w1064_tsign (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 883390000, 883280000, 883390000.

=== w10pro64 (32 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 706480000, 706560000, 706480000.

=== wvistau64 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 2354820000, 2354830000, 2354820000.

=== w2008s64 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 813740000, 813750000, 813740000.

=== w864 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 34279621070000, 377877004680000, 34279621070000.

=== w1064v1507 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 497640000, 497650000, 497640000.

=== w1064v1809 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 659310000, 659210000, 659310000.

=== w1064 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 588670000, 588590000, 588670000.

=== w1064_2qxl (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 692870000, 692960000, 692870000.

=== w1064_tsign (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 877680000, 877650000, 877680000.

=== w10pro64 (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 703150000, 703120000, 703150000.

=== w10pro64_ar (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 652760000, 652650000, 652760000.

=== w10pro64_he (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 597950000, 597960000, 597950000.

=== w10pro64_ja (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 592350000, 592340000, 592350000.

=== w10pro64_zh_CN (64 bit report) ===

quartz:
systemclock.c:191: Test failed: Got timestamps 572390000, 572340000, 572390000.