Bug 103000 - XSync regression: unity settings daemon crash (= whole Ubuntu desktop in crash-restart cycle)
Summary: XSync regression: unity settings daemon crash (= whole Ubuntu desktop in cras...
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: git
Hardware: Other All
: high major
Assignee: Adam Jackson
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-26 15:52 UTC by Eero Tamminen
Modified: 2018-01-26 14:09 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Client X errors and backtraces (3.80 KB, text/plain)
2017-09-26 15:52 UTC, Eero Tamminen
no flags Details
This should fix the remaining regression (1003 bytes, patch)
2018-01-08 18:01 UTC, David Weinehall
no flags Details | Splinter Review

Description Eero Tamminen 2017-09-26 15:52:00 UTC
Created attachment 134493 [details]
Client X errors and backtraces

Setup:
- Ubuntu 17.04 with Unity
- Latest X server built from git:
  git://anongit.freedesktop.org/git/xorg/xserver

Use-case:
- Boot

Expected output:
- Unity comes up like with earlier X server versions

Actual outcome:
- Desktop dies because unity-settings-daemon dies to X error

Notes:
- Backtraces attached, showing that issues is with X sync functionality
- Haven't seen this on machines with Ubuntu 16.04, but that could be just luck


Initial bisecting showed the issue to start between commits de3b618691 and d770f92932.

Manual review of the changes, showed few likely errors in following commit:
--------------------------------------------------
commit e0f872207aa203adb85e825c311ed50fe3a3af60
Author:     Eric Anholt <eric@anholt.net>
AuthorDate: Mon Sep 18 17:34:32 2017 -0700
Commit:     Adam Jackson <ajax@redhat.com>
CommitDate: Wed Sep 20 13:19:27 2017 -0400

    sync: Convert from "CARD64" to int64_t.
    
    The extension was using the name CARD64 to represent 64-bit values,
    with a #define from CARD64 to XSyncValue, a struct with a pair of
    32-bit values representing a signed 64-bit value.  This interfered
    with protocol headers using CARD64 to try to actually store a
    uint64_t.  Now that stdint.h exists, let's just use that here,
    instead.
    
    v2: Fix alarm delta changes.
    v3: Do the potentially overflowing math as uint and convert to int
        afterward, out of C spec paranoia.
    
    Signed-off-by: Eric Anholt <eric@anholt.net>
    Reviewed-by: Keith Packard <keithp@keithp.com>
--------------------------------------------------

At least following changes seem to be obviously wrong:
--------------------------------------------------
diff --git a/Xext/sync.c b/Xext/sync.c
...
@@ -655,16 +652,16 @@ SyncAwaitTriggerFired(SyncTrigger * pTrigger)
...
-            diffgreater = XSyncValueGreaterThan(diff, pAwait->event_threshold);
-            diffequal = XSyncValueEqual(diff, pAwait->event_threshold);
+            diffgreater = diff >= pAwait->event_threshold;
+            diffequal = diff == pAwait->event_threshold;
...
@@ -862,16 +859,13 @@ SyncChangeAlarmAttributes(ClientPtr client, SyncAlarm * pAlarm, Mask mask,
...
-             && XSyncValueGreaterThan(pAlarm->delta, zero))
+             && pAlarm->delta >= 0)
--------------------------------------------------
Comment 2 Eero Tamminen 2017-09-28 09:55:20 UTC
Yes, fixing just the latter mistyped transformation is enough to get Unity working again.  Shouldn't the other one be fixed too?
Comment 3 Eero Tamminen 2017-09-28 10:58:02 UTC
(In reply to Eero Tamminen from comment #0)
> - Haven't seen this on machines with Ubuntu 16.04, but that could be just
> luck

Fonts were rendering badly in terminal between e0f872207aa and 8060196a3e, now they again render fine in 16.04.  -> I guess XSync messed in other ways with unity-settings-daemon on 16.04.
Comment 4 Eero Tamminen 2017-10-03 11:28:10 UTC
(In reply to Eero Tamminen from comment #3)
> (In reply to Eero Tamminen from comment #0)
> > - Haven't seen this on machines with Ubuntu 16.04, but that could be just
> > luck
> 
> Fonts were rendering badly in terminal between e0f872207aa and 8060196a3e,
> now they again render fine in 16.04.  -> I guess XSync messed in other ways
> with unity-settings-daemon on 16.04.

Titlebar colors are still not right in Unity.  That also regressed with the e0f872207aa203adb85e825c311ed50fe3a3af60 X version.

When the other indicated conversion issue in the commit is going to be fixed?
Comment 5 David Weinehall 2018-01-08 18:01:51 UTC
Created attachment 136620 [details] [review]
This should fix the remaining regression

The attached patch should fix the second incorrect GreaterThan to >= bug.
Comment 6 David Weinehall 2018-01-26 14:09:57 UTC
The fix for this has now been merged. Resolving as fixed.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.