Closed Bug 968196 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Plug-ins, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(1 file, 6 obsolete files)

Attached patch flash-plugin-destroy.patch (obsolete) — Splinter Review
Can you please check this one? It could be GTK3 only also.
Attachment #8373387 - Flags: review?(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.
Attachment #8373387 - Flags: review?(karlt) → review-
This indeed fixes/works-around the crash I mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=624422#c48
Severity: normal → critical
Severity: critical → normal
Karl, what do you think the GDK's error handler should do? Just print the message, call crash reporter or so?
Flags: needinfo?(karlt)
Attached patch handler patch (obsolete) — Splinter Review
Something like this one?
Attachment #8426265 - Flags: feedback?(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?
Attachment #8426265 - Flags: feedback?(karlt) → feedback+
Flags: needinfo?(karlt)
Attached patch patch, v.2 (obsolete) — Splinter Review
An updated one.
Attachment #8373387 - Attachment is obsolete: true
Attachment #8426265 - Attachment is obsolete: true
Attachment #8430800 - Flags: review?(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;
Attachment #8430800 - Flags: review?(karlt) → review-
Attached patch patch v.3 (obsolete) — Splinter Review
Thanks, there's an updated one.
Attachment #8430800 - Attachment is obsolete: true
Attachment #8431549 - Flags: review?(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)
Attachment #8431549 - Flags: review?(karlt) → review-
Attached patch patch v.4 (obsolete) — Splinter Review
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().
Attachment #8431549 - Attachment is obsolete: true
Attachment #8439194 - Flags: review?(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?
Attachment #8439194 - Flags: review?(karlt) → review-
Comment on attachment 8439194 [details] [diff] [review]
patch v.4

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

Can you remove the "nsX11ErrorHandler.h" include in nsAppRunner.cpp, please?
Attached patch patch v.5 (obsolete) — Splinter Review
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.
Attachment #8439194 - Attachment is obsolete: true
Attachment #8439894 - Flags: review?(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.
Attachment #8439894 - Flags: review?(karlt) → review+
Keywords: checkin-needed
Attachment #8439894 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9263e3668ab9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Summary: flash-plugin throws ABORT: X_UnmapWindow: BadWindow when plugin window is closed → [gtk3] flash-plugin throws ABORT: X_UnmapWindow: BadWindow when plugin window is closed
You need to log in before you can comment on or make changes to this bug.