601267 - xpcshell tests fail in qt builds, work in gtk ones

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alon Zakai (:azakai)]

In ContentChild::Init, the X11 error handler is set up. Different code is run in GTK or Qt builds on Fennec. The Qt path fails with

  ASSERTION: No X display: 'display'

GTK builds run fine.

Comment 1

[Alon Zakai (:azakai)]

Attachment: Example test that fails in Qt with no X11 display

Comment 2

[Oleg Romashin (:romaxa)]

This came from bug 594960

Comment 3

[Doug Turner (:dougt)]

alone, what was the error message again?  Are we just asserting here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsX11ErrorHandler.cpp#201

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is a known symptom of bug 598884.

Comment 5

[Alon Zakai (:azakai)]

(In reply to comment #3)
> alone, what was the error message again?  Are we just asserting here:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsX11ErrorHandler.cpp#201

Yes, that is what we hit.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 7

[Doug Turner (:dougt)]

ignore   @@ -68,16 +72,19 @@, and the extra ws in @@ -183,16 +190,22 @@

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 481894 [details] [diff] [review]
patch v.1

(In reply to comment #7)
> ignore   @@ -68,16 +72,19 @@, and the extra ws in @@ -183,16 +190,22 @@

That makes for unhappy reviewer.
>diff --git a/gfx/src/X11Util.h b/gfx/src/X11Util.h
>--- a/gfx/src/X11Util.h
>+++ b/gfx/src/X11Util.h
>@@ -58,21 +58,24 @@
> namespace mozilla {
> 
> /**
>  * Return the default X Display created and used by the UI toolkit.
>  */
> inline Display*
> DefaultXDisplay()
> {
>+  Display* display = NULL;
> #if defined(MOZ_WIDGET_GTK2)
>-  return GDK_DISPLAY();
>+  display = GDK_DISPLAY();
> #elif defined(MOZ_WIDGET_QT)
>-  return QX11Info::display();
>+  display = QX11Info::display();
> #endif
>+  NS_ASSERTION(display, "No X display");
>+  return display;

I don't understand this change.  Clients should be allowed to check |if (DefaultXDisplay())| without triggering an assertion.

Comment 9

[Doug Turner (:dougt)]

> I don't understand this change.  Clients should be allowed to check |if
(DefaultXDisplay())| without triggering an assertion.

Why would you ever have a null display other than in an error condition?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Headless environment.  Either way, code that assumes DefaultXDisplay() will crash anyway, this assertion doesn't help anyone, just adds more code.

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 12

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/b12cd9bd4cae