Closed Bug 601267 Opened 10 years ago Closed 10 years ago

xpcshell tests fail in qt builds, work in gtk ones

Categories

(Core :: IPC, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: azakai, Assigned: dougt)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
tracking-fennec: --- → ?
This came from bug 594960
Blocks: 594960
alone, what was the error message again?  Are we just asserting here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsX11ErrorHandler.cpp#201
(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: nobody → doug.turner
tracking-fennec: ? → 2.0+
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #481894 - Flags: review?(jones.chris.g)
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)
> 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.
Attached patch patch v.2Splinter Review
Attachment #481894 - Attachment is obsolete: true
Attachment #481935 - Flags: review?(jones.chris.g)
Attachment #481935 - Flags: review?(jones.chris.g) → review+
Depends on: 603046
http://hg.mozilla.org/mozilla-central/rev/b12cd9bd4cae
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.