xpcshell tests fail in qt builds, work in gtk ones

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: azakai, Assigned: dougt)

Tracking

unspecified
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
tracking-fennec: --- → ?
(Reporter)

Comment 1

8 years ago
Created attachment 480266 [details]
Example test that fails in Qt with no X11 display
This came from bug 594960
Blocks: 594960
(Assignee)

Comment 3

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

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

Comment 5

8 years ago
(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.
(Assignee)

Updated

8 years ago
Assignee: nobody → doug.turner
tracking-fennec: ? → 2.0+
(Assignee)

Comment 6

8 years ago
Created attachment 481894 [details] [diff] [review]
patch v.1
Attachment #481894 - Flags: review?(jones.chris.g)
(Assignee)

Comment 7

8 years ago
ignore   @@ -68,16 +72,19 @@, and the extra ws in @@ -183,16 +190,22 @@
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.
Attachment #481894 - Flags: review?(jones.chris.g)
(Assignee)

Comment 9

8 years ago
> 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?
Headless environment.  Either way, code that assumes DefaultXDisplay() will crash anyway, this assertion doesn't help anyone, just adds more code.
(Assignee)

Comment 11

8 years ago
Created attachment 481935 [details] [diff] [review]
patch v.2
Attachment #481894 - Attachment is obsolete: true
Attachment #481935 - Flags: review?(jones.chris.g)
Attachment #481935 - Flags: review?(jones.chris.g) → review+
Depends on: 603046
(Assignee)

Comment 12

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b12cd9bd4cae
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.