Downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1054294#c7

Can you please check this one? It could be GTK3 only also.

Comment on attachment 8373387 [details] [diff] [review] flash-plugin-destroy.patch gtk_window_destroyed() merely sets a pointer to null, so wrapping it in an error trap doesn't make much sense. This may work because it is happening after an async GDK _trap_pop_ignore and effectively turning it into a sync _trap_pop. The core issue I think is described in https://bugzilla.redhat.com/show_bug.cgi?id=1054294#c18 GDK uses _pop_ignore frequently and so sprinkling workarounds like this around the code would be a never-ending battle. I thought about Gecko using g_log_add_handlers and catch SIGTRAP to cause g_error to continue to run, so it can call into GDK's X error handler to find out whether GDK would ignore the error, but this feels very precarious. Catching SIGTRAP is not so nice in a multithreaded app, and it depends on GDK built with G_ENABLE_DEBUG, and those are not the only problems. I think the best work around for Gecko would be to use g_log_add_handlers instead of the X11 error handler, and filter GDK's "X Window System error" messages to get the codes. We'd lose the ability to print the resource id.

This indeed fixes/works-around the crash I mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=624422#c48

Karl, what do you think the GDK's error handler should do? Just print the message, call crash reporter or so?

Something like this one?

Comment on attachment 8426265 [details] [diff] [review] handler patch (In reply to Martin Stránský from comment #5) > Something like this one? Yes, although I suspect the log domain is "Gdk", and the serial, request_code, and minor_code are not useful without the translation provided in nsX11ErrorHandler. Can this share the code in nsX11ErrorHandler to provide similar info?

An updated one.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=e5479000c3cf

Comment on attachment 8430800 [details] [diff] [review] patch, v.2 A couple of things to take into account: GdkErrorHandler will also need to handle other GDK errors that are not "X Window System error". Perhaps g_log_default_handler() is useful, but that won't abort the program for us. NS_RUNTIMEABORT() would add its string to crashreporter notes. I'd like to keep the MOZ_X_SYNC functionality. >-#include "nsX11ErrorHandler.h" >- > #include "prenv.h" > #include "nsXULAppAPI.h" > #include "nsExceptionHandler.h" > #include "nsDebug.h" > > #include "mozilla/X11Util.h" > #include <X11/Xlib.h> > >+#include "nsX11ErrorHandler.h" It shouldn't be necessary to move the include. It is common for .cpp to include its corresponding .h first. Each header should be self sufficient. Types can be forward declared. e.g. typedef struct XErrorEvent;

Thanks, there's an updated one.

Comment on attachment 8431549 [details] [diff] [review] patch v.3 Can you add to comment please linking to https://bugzilla.gnome.org/show_bug.cgi?id=629608#c8 please to explain why this X error handling is necessary? Can XRE_mainStartup in nsAppRunner call XRE_InstallX11ErrorHandler() please, instead of being another place to choose the appropriate handler? >+++ b/toolkit/xre/nsEmbedFunctions.cpp >@@ -12,16 +12,21 @@ > > #include "nsXULAppAPI.h" > > #include <stdlib.h> > #if defined(MOZ_WIDGET_GTK) > #include <glib.h> > #endif > >+#ifdef MOZ_X11 >+#include "mozilla/X11Util.h" >+#include <X11/Xlib.h> >+#endif These includes shouldn't be necessary. >+++ b/toolkit/xre/nsGDKErrorHandler.cpp >@@ -0,0 +1,73 @@ >+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include <gtk/gtk.h> >+ >+#include "nsX11ErrorHandler.h" >+#include "nsGDKErrorHandler.h" Include nsGDKErrorHandler.h (the header corresponding to the .cpp file) before any other headers. >+#include "nsXULAppAPI.h" >+#include "nsExceptionHandler.h" >+#include "mozilla/X11Util.h" Are all these headers used? >+#if defined(MOZ_WIDGET_GTK) >+void InstallGdkErrorHandler(); >+#endif MOZ_WIDGET_GTK == 3 >+ nsAutoCString buffer(message); nsDependentCString. No need to make a copy. >+ * (Details: serial XXXX error_code XXXX request_code XXXX (core protocol) minor_code XXXX) I don't see "(core protocol)", in 3.10.7 at least. >+ nsAutoCString serialString("(Details: serial "); nsLiteralCString or NS_NAMED_LITERAL_CSTRING. Similarly elsewhere. >+ int32_t start = buffer.Find(serialString) + serialString.Length(); Can failure in the string comparisons fall back to the NS_RUNTIMEABORT() path, please? That provides some safety against changes to the GDK strings. Find() is declared within "#if MOZ_STRING_WITH_OBSOLETE_API" but I'm not sure that FindInReadable() is easier to use, so I personally don't mind using Find() if it is simpler. >+ } >+ else { Mozilla style is to have this on one line, for new files. >+ int32_t end = buffer.FindChar(' ', start); >+ event.serial = atoi(nsDependentCSubstring(buffer, start, end).Data()); nsDependentCSubstring(buffer, start, end).Data() can be simplified to "buffer.BeginReading() + start". Using strtol() instead of atoi() will return a pointer to the character after the numerals, which would be tidier than having a separate FindChar() to search for the end. >+ nsAutoCString errorCodeString("error_code "); >+ start = buffer.Find(errorCodeString, end) + errorCodeString.Length(); We know what characters should be immediately after the number, so this need not search, but can merely compare with " error_code " using something like StringBeginsWith(Substring(endptr, buffer.EndReading()), errorCodeString)

Thanks, there's an updated one. >+#include "mozilla/X11Util.h" > Are all these headers used? X11Util.h are needed here. > >+ * (Details: serial XXXX error_code XXXX request_code XXXX > > (core protocol) minor_code XXXX) > I don't see "(core protocol)", in 3.10.7 at least. It's in gtk3-3.10.9. It's generated by _gdk_x11_decode_request_code().

Comment on attachment 8439194 [details] [diff] [review] patch v.4 >+++ b/toolkit/xre/nsGDKErrorHandler.cpp >@@ -0,0 +1,95 @@ >+/* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include <gtk/gtk.h> >+#include <errno.h> >+ >+#include "nsGDKErrorHandler.h" Include nsGDKErrorHandler.h (the header corresponding to the .cpp file) before any other headers. >+ if(errno) Mozilla style is a space after control structure keywords such as "if". Similarly elsewhere. >+++ b/toolkit/xre/nsX11ErrorHandler.h >@@ -1,8 +1,11 @@ > /* -*- Mode: C++; tab-width: 40; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #ifdef MOZ_X11 >+#include <X11/Xlib.h> >+ > void InstallX11ErrorHandler(); >+extern "C" int X11Error(Display *display, XErrorEvent *event); Move the Xlib.h include to the .cpp file, please, and use forward declarations. e.g. struct XErrorEvent; Can you surround InstallX11ErrorHandler(), here and in the .cpp, with MOZ_WIDGET_GTK == 2, please? That makes it clear that this is not used with GTK3 and prevents accidental use. (In reply to Martin Stránský from comment #12) > X11Util.h are needed here. For what? Would X11/Xlib.h be sufficient?

Comment on attachment 8439194 [details] [diff] [review] patch v.4 >- InstallX11ErrorHandler(); >+ XRE_InstallX11ErrorHandler(); Can you remove the "nsX11ErrorHandler.h" include in nsAppRunner.cpp, please?

This should fix it. XErrorEvent can't be forward declared because it's defined as unnamed struct. It's a bug in X lib headers.

Comment on attachment 8439894 [details] [diff] [review] patch v.5 (In reply to Martin Stránský from comment #15) > XErrorEvent can't be forward declared because it's > defined as unnamed struct. Ah, thanks. Sorry that I didn't understand that. >+ g_log_set_handler("Gdk", >+ (GLogLevelFlags)(G_LOG_LEVEL_ERROR | G_LOG_FLAG_FATAL | G_LOG_FLAG_RECURSION), >+ GdkErrorHandler, >+ NULL); s/NULL/nullptr/ >diff --git a/toolkit/xre/nsX11ErrorHandler.cpp b/toolkit/xre/nsX11ErrorHandler.cpp > XSynchronize(display, True); > } > } >+#endif >+ Please remove the extra blank line at the end of the file.

Thanks! Try build: https://tbpl.mozilla.org/?tree=Try&rev=831a1e2ae079