Skip to content

Commit

Permalink
Bug 1685375 - Remove ScopedXErrorHandler. r=jgilbert
Browse files Browse the repository at this point in the history 
Avoid relying on X11 errors to detect failures where alternative means would
suffice (i.e. checking results for failure or MakeCurrent failures). All other
users of ScopedXErrorHandler outside of GLContextProviderGLX use it only to
ignore errors rather than actually check the error result. Given those concerns,
we also change the default X11 error handler to merely ignore errors rather than
abort, such that X11 calls in Gecko no longer require an error trap by default.
This also avoids contention with other libraries that may temporarily override
the error handler such as GDK or Cairo since Gecko will never touch the handler
after startup.

Differential Revision: https://phabricator.services.mozilla.com/D147247
  • Loading branch information
@lsalzman
lsalzman committed May 26, 2022
1 parent 4e71c54 commit 2d5de17
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 161 deletions.
54 changes: 24 additions & 30 deletions gfx/gl/GLContextProviderGLX.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,11 @@ bool GLXLibrary::SupportsVideoSync(Display* aDisplay) {
}

static int (*sOldErrorHandler)(Display*, XErrorEvent*);
ScopedXErrorHandler::ErrorEvent sErrorEvent;
static XErrorEvent sErrorEvent = {};

static int GLXErrorHandler(Display* display, XErrorEvent* ev) {
if (!sErrorEvent.mError.error_code) {
sErrorEvent.mError = *ev;
if (!sErrorEvent.error_code) {
sErrorEvent = *ev;
}
return 0;
}
Expand All @@ -264,21 +265,23 @@ GLXLibrary::WrapperScope::~WrapperScope() {
if (mDisplay) {
FinishX(mDisplay);
}
if (sErrorEvent.mError.error_code) {
if (sErrorEvent.error_code) {
char buffer[100] = {};
if (mDisplay) {
XGetErrorText(mDisplay, sErrorEvent.mError.error_code, buffer,
sizeof(buffer));
XGetErrorText(mDisplay, sErrorEvent.error_code, buffer, sizeof(buffer));
} else {
SprintfLiteral(buffer, "%d", sErrorEvent.mError.error_code);
SprintfLiteral(buffer, "%d", sErrorEvent.error_code);
}
printf_stderr("X ERROR after %s: %s (%i) - Request: %i.%i, Serial: %lu",
mFuncName, buffer, sErrorEvent.mError.error_code,
sErrorEvent.mError.request_code,
sErrorEvent.mError.minor_code, sErrorEvent.mError.serial);
mFuncName, buffer, sErrorEvent.error_code,
sErrorEvent.request_code, sErrorEvent.minor_code,
sErrorEvent.serial);
MOZ_ASSERT_UNREACHABLE("AfterGLXCall sErrorEvent");
}
XSetErrorHandler(sOldErrorHandler);
const auto was = XSetErrorHandler(sOldErrorHandler);
if (was != GLXErrorHandler) {
NS_WARNING("Concurrent XSetErrorHandlers");
}
}
}

Expand Down Expand Up @@ -338,24 +341,17 @@ already_AddRefed<GLContextGLX> GLContextGLX::CreateGLContext(

const auto CreateWithAttribs =
[&](const std::vector<int>& attribs) -> RefPtr<GLContextGLX> {
OffMainThreadScopedXErrorHandler handler;

auto terminated = attribs;
terminated.push_back(0);

// X Errors can happen even if this context creation returns non-null, and
// we should not try to use such contexts. (Errors may come from the
// distant server, or something)
const auto glxContext = glx.fCreateContextAttribs(
*display, cfg, nullptr, X11True, terminated.data());
if (!glxContext) return nullptr;
const RefPtr<GLContextGLX> ret =
new GLContextGLX(desc, display, drawable, glxContext, deleteDrawable,
isDoubleBuffered, pixmap);
if (handler.SyncAndGetError(*display)) return nullptr;

if (!ret->Init()) return nullptr;
if (handler.SyncAndGetError(*display)) return nullptr;

return ret;
};
Expand Down Expand Up @@ -439,13 +435,12 @@ GLContextGLX::~GLContextGLX() {
}

// see bug 659842 comment 76
#ifdef DEBUG
bool success =
#endif
mGLX->fMakeCurrent(*mDisplay, X11None, nullptr);
MOZ_ASSERT(success,
"glXMakeCurrent failed to release GL context before we call "
"glXDestroyContext!");
bool success = mGLX->fMakeCurrent(*mDisplay, X11None, nullptr);
if (!success) {
NS_WARNING(
"glXMakeCurrent failed to release GL context before we call "
"glXDestroyContext!");
}

mGLX->fDestroyContext(*mDisplay, mContext);

Expand Down Expand Up @@ -476,7 +471,9 @@ bool GLContextGLX::MakeCurrentImpl() const {
}

const bool succeeded = mGLX->fMakeCurrent(*mDisplay, mDrawable, mContext);
NS_ASSERTION(succeeded, "Failed to make GL context current!");
if (!succeeded) {
NS_WARNING("Failed to make GL context current!");
}

if (!IsOffscreen() && mGLX->SupportsSwapControl()) {
// Many GLX implementations default to blocking until the next
Expand Down Expand Up @@ -868,14 +865,12 @@ static already_AddRefed<GLContextGLX> CreateOffscreenPixmapContext(
int depth;
FindVisualAndDepth(*display, visid, &visual, &depth);

OffMainThreadScopedXErrorHandler xErrorHandler;
bool error = false;

gfx::IntSize dummySize(16, 16);
RefPtr<gfxXlibSurface> surface = gfxXlibSurface::Create(
display, DefaultScreenOfDisplay(display->get()), visual, dummySize);
if (surface->CairoStatus() != 0) {
mozilla::Unused << xErrorHandler.SyncAndGetError(*display);
return nullptr;
}

Expand All @@ -888,8 +883,7 @@ static already_AddRefed<GLContextGLX> CreateOffscreenPixmapContext(
error = true;
}

bool serverError = xErrorHandler.SyncAndGetError(*display);
if (error || serverError) return nullptr;
if (error) return nullptr;

auto fullDesc = GLContextDesc{desc};
fullDesc.isOffscreen = true;
Expand Down
39 changes: 0 additions & 39 deletions gfx/src/X11Util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,43 +39,4 @@ void FinishX(Display* aDisplay) {
XSync(aDisplay, X11False);
}

ScopedXErrorHandler::ErrorEvent* ScopedXErrorHandler::sXErrorPtr;

int ScopedXErrorHandler::ErrorHandler(Display*, XErrorEvent* ev) {
// only record the error if no error was previously recorded.
// this means that in case of multiple errors, it's the first error that we
// report.
if (!sXErrorPtr->mError.error_code) sXErrorPtr->mError = *ev;
return 0;
}

ScopedXErrorHandler::ScopedXErrorHandler(bool aAllowOffMainThread) {
if (!aAllowOffMainThread) {
// Off main thread usage is not safe in general, but OMTC GL layers uses
// this with the main thread blocked, which makes it safe.
NS_WARNING_ASSERTION(
NS_IsMainThread(),
"ScopedXErrorHandler being called off main thread, may cause issues");
}
// let sXErrorPtr point to this object's mXError object, but don't reset this
// mXError object! think of the case of nested ScopedXErrorHandler's.
mOldXErrorPtr = sXErrorPtr;
sXErrorPtr = &mXError;
mOldErrorHandler = XSetErrorHandler(ErrorHandler);
}

ScopedXErrorHandler::~ScopedXErrorHandler() {
sXErrorPtr = mOldXErrorPtr;
XSetErrorHandler(mOldErrorHandler);
}

bool ScopedXErrorHandler::SyncAndGetError(Display* dpy, XErrorEvent* ev) {
FinishX(dpy);

bool retval = mXError.mError.error_code != 0;
if (ev) *ev = mXError.mError;
mXError = ErrorEvent(); // reset
return retval;
}

} // namespace mozilla
69 changes: 0 additions & 69 deletions gfx/src/X11Util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,75 +70,6 @@ struct ScopedXFreePtrTraits {
};
SCOPED_TEMPLATE(ScopedXFree, ScopedXFreePtrTraits)

/**
* On construction, set a graceful X error handler that doesn't crash the
* application and records X errors. On destruction, restore the X error handler
* to what it was before construction.
*
* The SyncAndGetError() method allows to know whether a X error occurred,
* optionally allows to get the full XErrorEvent, and resets the recorded X
* error state so that a single X error will be reported only once.
*
* Nesting is correctly handled: multiple nested ScopedXErrorHandler's don't
* interfere with each other's state. However, if SyncAndGetError is not called
* on the nested ScopedXErrorHandler, then any X errors caused by X calls made
* while the nested ScopedXErrorHandler was in place may then be caught by the
* other ScopedXErrorHandler. This is just a result of X being asynchronous and
* us not doing any implicit syncing: the only method in this class what causes
* syncing is SyncAndGetError().
*
* This class is not thread-safe at all. It is assumed that only one thread is
* using any ScopedXErrorHandler's. Given that it's not used on Mac, it should
* be easy to make it thread-safe by using thread-local storage with __thread.
*/
class ScopedXErrorHandler {
public:
// trivial wrapper around XErrorEvent, just adding ctor initializing by zero.
struct ErrorEvent {
XErrorEvent mError;

ErrorEvent() { memset(this, 0, sizeof(ErrorEvent)); }
};

private:
// this ScopedXErrorHandler's ErrorEvent object
ErrorEvent mXError;

// static pointer for use by the error handler
static ErrorEvent* sXErrorPtr;

// what to restore sXErrorPtr to on destruction
ErrorEvent* mOldXErrorPtr;

// what to restore the error handler to on destruction
int (*mOldErrorHandler)(Display*, XErrorEvent*);

public:
static int ErrorHandler(Display*, XErrorEvent* ev);

/**
* @param aAllowOffMainThread whether to warn if used off main thread
*/
explicit ScopedXErrorHandler(bool aAllowOffMainThread = false);

~ScopedXErrorHandler();

/** \returns true if a X error occurred since the last time this method was
* called on this ScopedXErrorHandler object, or since the creation of this
* ScopedXErrorHandler object if this method was never called on it.
*
* \param ev this optional parameter, if set, will be filled with the
* XErrorEvent object. If multiple errors occurred, the first one will be
* returned.
*/
bool SyncAndGetError(Display* dpy, XErrorEvent* ev = nullptr);
};

class OffMainThreadScopedXErrorHandler : public ScopedXErrorHandler {
public:
OffMainThreadScopedXErrorHandler() : ScopedXErrorHandler(true) {}
};

} // namespace mozilla

#endif // mozilla_X11Util_h
11 changes: 9 additions & 2 deletions toolkit/xre/nsEmbedFunctions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
# include "chrome/common/mach_ipc_mac.h"
# include "gfxPlatformMac.h"
#endif
#include "nsX11ErrorHandler.h"
#include "nsGDKErrorHandler.h"
#include "base/at_exit.h"
#include "base/message_loop.h"
Expand Down Expand Up @@ -962,9 +963,15 @@ bool XRE_ShutdownTestShell() {
void XRE_InstallX11ErrorHandler() {
# ifdef MOZ_WIDGET_GTK
InstallGdkErrorHandler();
# else
InstallX11ErrorHandler();
# endif

// Ensure our X11 error handler overrides the default GDK error handler such
// that errors are ignored by default. GDK will install its own error handler
// temporarily when pushing error traps internally as needed. This avoids us
// otherwise having to frequently override the error handler merely to trap
// errors in multiple places that would otherwise contend with GDK or other
// libraries that might also override the handler.
InstallX11ErrorHandler();
}
#endif

Expand Down
11 changes: 2 additions & 9 deletions toolkit/xre/nsX11ErrorHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,6 @@ int X11Error(Display* display, XErrorEvent* event) {
}
}

switch (XRE_GetProcessType()) {
case GeckoProcessType_Default:
case GeckoProcessType_Content:
CrashReporter::AppendAppNotesToCrashReport(notes);
break;
default:; // crash report notes not supported.
}

#ifdef DEBUG
// The resource id is unlikely to be useful in a crash report without
// context of other ids, but add it to the debug console output.
Expand All @@ -130,7 +122,8 @@ int X11Error(Display* display, XErrorEvent* event) {
# endif
#endif

MOZ_CRASH_UNSAFE(notes.get());
NS_WARNING(notes.get());
return 0;
}
}

Expand Down
19 changes: 7 additions & 12 deletions widget/gtk/nsWindow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2586,18 +2586,13 @@ static bool GetWindowManagerName(GdkWindow* gdk_window, nsACString& wmName) {
if (!property || !req_type) {
return false;
}
{
// Suppress fatal errors for a missing window.
ScopedXErrorHandler handler;
result =
XGetWindowProperty(xdisplay, wmWindow, property,
0L, // offset
INT32_MAX, // length
false, // delete
req_type, &actual_type_return, &actual_format_return,
&nitems_return, &bytes_after_return, &prop_return);
}

result =
XGetWindowProperty(xdisplay, wmWindow, property,
0L, // offset
INT32_MAX, // length
false, // delete
req_type, &actual_type_return, &actual_format_return,
&nitems_return, &bytes_after_return, &prop_return);
if (result != Success || bytes_after_return != 0) {
return false;
}
Expand Down

0 comments on commit 2d5de17

Please sign in to comment.