968196 - [gtk3] flash-plugin throws ABORT: X_UnmapWindow: BadWindow when plugin window is closed

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

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

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: flash-plugin-destroy.patch

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

Comment 2

[Karl Tomlinson (:karlt)]

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.

Comment 3

[Emilio Pozuelo Monfort]

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

Comment 4

[Martin Stránský [:stransky] (ni? me)]

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

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: handler patch

Something like this one?

Comment 6

[Karl Tomlinson (:karlt)]

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?

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch, v.2

An updated one.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

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

Comment 9

[Karl Tomlinson (:karlt)]

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;

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.3

Thanks, there's an updated one.

Comment 11

[Karl Tomlinson (:karlt)]

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)

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.4

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 13

[Karl Tomlinson (:karlt)]

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 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8439194 [details] [diff] [review]
patch v.4

>-  InstallX11ErrorHandler();
>+  XRE_InstallX11ErrorHandler();

Can you remove the "nsX11ErrorHandler.h" include in nsAppRunner.cpp, please?

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.5

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 16

[Karl Tomlinson (:karlt)]

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.

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v5 for check-in

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9263e3668ab9

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9263e3668ab9