Change X11 error handler override to allow easy X11 error checking.

Change X11 error handler override to allow easy X11 error checking.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102978

[dominich, 2011-09-14 16:52:14]

As per the thread on catching non-fatal X errors before attempting to make GL
calls in an effort to trap GPU crashes earlier. I'm interested in your take on
the approach.

I'm not sure how best to integrate something like this into the
platform-agnostic gles2_cmd_decoder.cc so if you have any ideas I'm open to
them.

I considered just overriding the handlers for all of the Initialization phase
with a handler that will crash on any error, but was worried about the noise
level that might create.

[marcheu, 2011-09-14 17:02:43]

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc
File ui/gfx/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:118: 
Why not do this earlier in the function? glX functions can cause X errors just
like other X functions.

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:133: 
Why not do this later in the function? There are more X calls and errors to
catch? Unless you want to insert more sync points of course.

[Ami GONE FROM CHROMIUM, 2011-09-14 17:11:13]

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc#newcode59
ui/base/x/x11_util.cc:59: XErrorHandler error_handler_ = NULL;
file-statics don't get trailing underscores (here and below).
(will require renaming either these or the args to the new ctor below)

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc#newcode847
ui/base/x/x11_util.cc:847: ui::SetX11ErrorHandlers(error_handler,
io_error_handler);
ui:: here and below unnecessary?

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc#newcode847
ui/base/x/x11_util.cc:847: ui::SetX11ErrorHandlers(error_handler,
io_error_handler);
I expected sync calls here and in the dtor.

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc
File ui/gfx/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:51: ui::LogErrorEventDescription(d, *e);
not legit per NOTE @ x11_util_internal.h:46.

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:132: CHECK(!g_has_x_error_);
If this is all you care about, why not make the default handler maintain the
last-reported XErrorEvent and add an accessor for it?

[dominich, 2011-09-14 17:15:57]

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc
File ui/gfx/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:132: CHECK(!g_has_x_error_);
On 2011/09/14 17:11:13, Ami Fischman wrote:
> If this is all you care about, why not make the default handler maintain the
> last-reported XErrorEvent and add an accessor for it?

There are two sets of default handlers, one of which is set at the browser
level. Relying on the default handler not having been overridden is fragile.

[Ami GONE FROM CHROMIUM, 2011-09-14 17:20:50]

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc
File ui/gfx/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:132: CHECK(!g_has_x_error_);
On 2011/09/14 17:15:57, dominich wrote:
> On 2011/09/14 17:11:13, Ami Fischman wrote:
> > If this is all you care about, why not make the default handler maintain the
> > last-reported XErrorEvent and add an accessor for it?
> 
> There are two sets of default handlers, one of which is set at the browser
> level. Relying on the default handler not having been overridden is fragile.

An alternative to depending on a specific handler is having the
SetX11ErrorHandlers() machinery always execute a base handler (such as tracking
the last error) and then dispatch the last-set handler.  Just throwing ideas out
there.

The idiom of a scoped error handler seems nice to me, but the fact that even in
its first use you don't want to use it as such makes me suspicious that this
isn't an optimal setup.

[dominich, 2011-09-14 17:27:03]

On 2011/09/14 17:20:50, Ami Fischman wrote:
> http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc
> File ui/gfx/gl/gl_context_glx.cc (right):
> 
>
http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
> ui/gfx/gl/gl_context_glx.cc:132: CHECK(!g_has_x_error_);
> On 2011/09/14 17:15:57, dominich wrote:
> > On 2011/09/14 17:11:13, Ami Fischman wrote:
> > > If this is all you care about, why not make the default handler maintain
the
> > > last-reported XErrorEvent and add an accessor for it?
> > 
> > There are two sets of default handlers, one of which is set at the browser
> > level. Relying on the default handler not having been overridden is fragile.
> 
> An alternative to depending on a specific handler is having the
> SetX11ErrorHandlers() machinery always execute a base handler (such as
tracking
> the last error) and then dispatch the last-set handler.  Just throwing ideas
out
> there.

I like this. I'll see what that looks like.

> 
> The idiom of a scoped error handler seems nice to me, but the fact that even
in
> its first use you don't want to use it as such makes me suspicious that this
> isn't an optimal setup.

It felt like the right approach given the number of early exit points and
wanting to reset the handlers, but if I can just avoid having to override the
handlers in the first place that's even better.

[dominich, 2011-09-14 19:55:06]

I have introduced a CHECK_X_ERROR macro but I can't find a good place to put it
so it's accessible to the places that need it and has access to the things it
needs. Perhaps it needs its own header...

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc#newcode59
ui/base/x/x11_util.cc:59: XErrorHandler error_handler_ = NULL;
On 2011/09/14 17:11:13, Ami Fischman wrote:
> file-statics don't get trailing underscores (here and below).
> (will require renaming either these or the args to the new ctor below)

Done.

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc#newcode847
ui/base/x/x11_util.cc:847: ui::SetX11ErrorHandlers(error_handler,
io_error_handler);
On 2011/09/14 17:11:13, Ami Fischman wrote:
> ui:: here and below unnecessary?

Done.

http://codereview.chromium.org/7889040/diff/1/ui/base/x/x11_util.cc#newcode847
ui/base/x/x11_util.cc:847: ui::SetX11ErrorHandlers(error_handler,
io_error_handler);
On 2011/09/14 17:11:13, Ami Fischman wrote:
> I expected sync calls here and in the dtor.

Done.

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc
File ui/gfx/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:51: ui::LogErrorEventDescription(d, *e);
On 2011/09/14 17:11:13, Ami Fischman wrote:
> not legit per NOTE @ x11_util_internal.h:46.

Done.

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:118: 
On 2011/09/14 17:02:43, marcheu wrote:
> Why not do this earlier in the function? glX functions can cause X errors just
> like other X functions.

Done.

http://codereview.chromium.org/7889040/diff/1/ui/gfx/gl/gl_context_glx.cc#new...
ui/gfx/gl/gl_context_glx.cc:133: 
On 2011/09/14 17:02:43, marcheu wrote:
> Why not do this later in the function? There are more X calls and errors to
> catch? Unless you want to insert more sync points of course.

Done.

[Ami GONE FROM CHROMIUM, 2011-09-14 23:23:11]

http://codereview.chromium.org/7889040/diff/5003/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7889040/diff/5003/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:50: #define CHECK_X_ERROR() do {
\
Why not put this in the x11_util_internal.h header?

http://codereview.chromium.org/7889040/diff/5003/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:51:
CHECK(!ui::GetLastX11Error(gfx::GLSurfaceGLX::GetDisplay())); \
Can this be simplified by calling XGetDisplay() (inside GetLastX11Error())?

http://codereview.chromium.org/7889040/diff/5003/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:51:
CHECK(!ui::GetLastX11Error(gfx::GLSurfaceGLX::GetDisplay())); \
In fact since this is the only use, you might reduce the API surface by only
offering a ui::CheckFailOnX11Error() method instead of the general getter.

http://codereview.chromium.org/7889040/diff/5003/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/5003/ui/base/x/x11_util.cc#newcode75
ui/base/x/x11_util.cc:75: last_error_event = e;
(this is the comment referenced from the .h comment)
Is there any reason to believe |e| will continue to point to valid data after
this returns?
I imagined there'd be a memcpy here into an error struct that this file owned,
and that the getter would further memcpy a copy for each caller.

http://codereview.chromium.org/7889040/diff/5003/ui/base/x/x11_util_internal.h
File ui/base/x/x11_util_internal.h (right):

http://codereview.chromium.org/7889040/diff/5003/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:55: // all requests have been received and
processed by the X server.
Document semantics of NULL return.

http://codereview.chromium.org/7889040/diff/5003/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:55: // all requests have been received and
processed by the X server.
If you really returned a pointer here you'd want you to doco the ownership
semantics of it (but see .cc commentary).

http://codereview.chromium.org/7889040/diff/5003/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:56: UI_EXPORT XErrorEvent*
GetLastX11Error(Display* display);
s/Get/GetAndClear/

http://codereview.chromium.org/7889040/diff/5003/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:56: UI_EXPORT XErrorEvent*
GetLastX11Error(Display* display);
I wonder about the threading semantics here.  Do you not need to restrict
callers to the thread dealing w/ xlib?
(xlib is thread-unsafe)

[dominich, 2011-09-20 22:58:22]

I think this is a much cleaner implementation. Let me know your thoughts.

[Ami GONE FROM CHROMIUM, 2011-09-20 23:45:51]

I like this version a lot better!

http://codereview.chromium.org/7889040/diff/7001/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7889040/diff/7001/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:45: #include
"ui/base/x/x11_util.h"
drop?

http://codereview.chromium.org/7889040/diff/7001/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:47: #define CHECK_X_ERROR()
ui::CheckFailOnX11Error()
This makes me sad but I don't have a uniformly better solution.

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcode73
ui/base/x/x11_util.cc:73: XErrorEvent* last_error_event = NULL;
Why not simply in-line?
static XErrorEvent last_error_event = { 0, NULL, 0, 0, 0, 0, 0 };
and you can check last_error_event.display==NULL to see if it's been set?

That makes BaseX11ErrorHandler simpler: no need to new() anything, and no need
to commentate about leaking (b/c it's a file-static; it's *supposed* to leak!).

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:105: std::string BuildX11ErrorString(Display* dpy, const
XErrorEvent& error_event) {
|dpy| is unnecessary since you can use |error_event.display|, no?

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:772: XSync(display, False);
The Xlib calls you make here and from BuildX11ErrorString() tell me that this
function must be called on the (only) thread talking to Xlib.  What
enforces/documents this?

(besides xlib's thread-unsafety, there's the fact that the mechanism used to
store/retrieve the last error isn't thread-safe; so access from multiple threads
is likely a bug)

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util_internal.h
File ui/base/x/x11_util_internal.h (right):

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:53: // CHECK thas no X11 errors have been
reported. Uses XSync to ensure that all
s/thas/that/

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:56: void CheckFailOnX11Error();
s/On/OnReported/?
(the current name makes me think it is future-facing)

http://codereview.chromium.org/7889040/diff/7001/ui/gfx/gl/gl_context_glx.cc
File ui/gfx/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/7889040/diff/7001/ui/gfx/gl/gl_context_glx.cc#...
ui/gfx/gl/gl_context_glx.cc:14: #include "ui/base/x/x11_util.h"
Why this file?

[Ami GONE FROM CHROMIUM, 2011-09-20 23:46:17]

BTW the CL description is now way out of date :)

[dominich, 2011-09-21 19:47:30]

I had to put back the #undef for symbols that are shared between gtest and X11,
but I've put them in the files where it's a problem rather than somewhere deeper
in the include stack to fix some of the errors.

http://codereview.chromium.org/7889040/diff/7001/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/7889040/diff/7001/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:45: #include
"ui/base/x/x11_util.h"
On 2011/09/20 23:45:51, Ami Fischman wrote:
> drop?

Done.

http://codereview.chromium.org/7889040/diff/7001/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder.cc:47: #define CHECK_X_ERROR()
ui::CheckFailOnX11Error()
On 2011/09/20 23:45:51, Ami Fischman wrote:
> This makes me sad but I don't have a uniformly better solution.

I agree. I considered putting this in a more obvious place (x11_util.h) but then
would need somewhere for the do-nothing version, and nowhere sprung to mind.

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcode73
ui/base/x/x11_util.cc:73: XErrorEvent* last_error_event = NULL;
On 2011/09/20 23:45:51, Ami Fischman wrote:
> Why not simply in-line?
> static XErrorEvent last_error_event = { 0, NULL, 0, 0, 0, 0, 0 };
> and you can check last_error_event.display==NULL to see if it's been set?
> 
> That makes BaseX11ErrorHandler simpler: no need to new() anything, and no need
> to commentate about leaking (b/c it's a file-static; it's *supposed* to
leak!).

I wasn't sure if that was allowed, given that theoretically XErrorEvent could
become non-POD in the future. It is much cleaner though and it should be
relatively obvious from a compile error should the definition of XErrorEvent
change.

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:105: std::string BuildX11ErrorString(Display* dpy, const
XErrorEvent& error_event) {
On 2011/09/20 23:45:51, Ami Fischman wrote:
> |dpy| is unnecessary since you can use |error_event.display|, no?

Yes, but adding a DCHECK that these are the same. I don't think they ever can be
different, but I'm not 100% certain.

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:772: XSync(display, False);
On 2011/09/20 23:45:51, Ami Fischman wrote:
> The Xlib calls you make here and from BuildX11ErrorString() tell me that this
> function must be called on the (only) thread talking to Xlib.  What
> enforces/documents this?

Nothing enforces it, though it is documented in the x11_util_internal.h for
LogErrorEventDescription. This is called from the error handler but through a
task posted to the message loop.

We could do the same for storing and retrieving the last error, but it would
require call-sites to wait for that task to complete before continuing which
will complicate the initialization code significantly. Another option is to
determine which thread it can be safely called on and DCHECK that it's being
called from that thread only. This is more fragile but is used in other places
in the code-base for similar reasons.

I think the second option is the best option, even though it is fragile as it is
less disruptive to the call-sites.


> 
> (besides xlib's thread-unsafety, there's the fact that the mechanism used to
> store/retrieve the last error isn't thread-safe; so access from multiple
threads
> is likely a bug)

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util_internal.h
File ui/base/x/x11_util_internal.h (right):

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:53: // CHECK thas no X11 errors have been
reported. Uses XSync to ensure that all
On 2011/09/20 23:45:51, Ami Fischman wrote:
> s/thas/that/

Done.

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util_internal....
ui/base/x/x11_util_internal.h:56: void CheckFailOnX11Error();
On 2011/09/20 23:45:51, Ami Fischman wrote:
> s/On/OnReported/?
> (the current name makes me think it is future-facing)

Done.

http://codereview.chromium.org/7889040/diff/7001/ui/gfx/gl/gl_context_glx.cc
File ui/gfx/gl/gl_context_glx.cc (right):

http://codereview.chromium.org/7889040/diff/7001/ui/gfx/gl/gl_context_glx.cc#...
ui/gfx/gl/gl_context_glx.cc:14: #include "ui/base/x/x11_util.h"
On 2011/09/20 23:45:51, Ami Fischman wrote:
> Why this file?

Done.

[sadrul, 2011-09-21 21:50:34]

Hi! Just a note that came up in a review in another CR:
http://codereview.chromium.org/7981030/

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:770: }
Can this return a bool depending on whether there was an error or not? This
could perhaps then replace the gdk_error_trap_push/gdk_error_trap_pop calls in a
few places in this file?

[dominich, 2011-09-21 22:05:16]

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:770: }
On 2011/09/21 21:50:34, sadrul wrote:
> Can this return a bool depending on whether there was an error or not? This
> could perhaps then replace the gdk_error_trap_push/gdk_error_trap_pop calls in
a
> few places in this file?

It could, but there's a good chance this will have to check for an error
asynchronously due to Xlib not being thread-safe. In that case returning a value
would be useless.

One thing we might have to do is have a callback in the calling code that is
called with the success/failure value.

(insert xzibit-meme here).

[Ami GONE FROM CHROMIUM, 2011-09-21 22:21:42]

LGTM modulo nits

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:772: XSync(display, False);
On 2011/09/21 19:47:30, dominich wrote:
> On 2011/09/20 23:45:51, Ami Fischman wrote:
> > The Xlib calls you make here and from BuildX11ErrorString() tell me that
this
> > function must be called on the (only) thread talking to Xlib.  What
> > enforces/documents this?
> 
> Nothing enforces it, though it is documented in the x11_util_internal.h for
> LogErrorEventDescription.  This is called from the error handler but through a
> task posted to the message loop.

I think you're talking about out-of-line execution (which is fine).
My point was about thread-safety; none of this is safe to call from any thread
other than the Xlib thread.  Maybe that's obvious from the "ui" namespace?  IDK.

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
File gpu/command_buffer/common/gl_mock.h (right):

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
gpu/command_buffer/common/gl_mock.h:12: #if defined(USE_X11)
Ditto comment on id_manager_unittest.cc.

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right):

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:10: #if
defined(USE_X11)
Ditto comment on id_manager_unittest.cc.

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
File gpu/command_buffer/service/id_manager_unittest.cc (right):

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
gpu/command_buffer/service/id_manager_unittest.cc:7: // These are defined by
both gtest and Xlib so we need to undefine them before
An alternative is to #include gtest early, as in 
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...
Saves you an #if-defined and the explicit enumeration of conflicts.

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:766: if (last_error_event.display) {
Reversing the test lets you early-return and de-indent the rest of the function.

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:859: DCHECK(dpy == error_event.display)
DCHECK_EQ work?

[dominich, 2011-09-21 23:01:30]

On 2011/09/21 22:21:42, Ami Fischman wrote:
> LGTM modulo nits
> 
> http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc
> File ui/base/x/x11_util.cc (right):
> 
>
http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
> ui/base/x/x11_util.cc:772: XSync(display, False);
> On 2011/09/21 19:47:30, dominich wrote:
> > On 2011/09/20 23:45:51, Ami Fischman wrote:
> > > The Xlib calls you make here and from BuildX11ErrorString() tell me that
> this
> > > function must be called on the (only) thread talking to Xlib.  What
> > > enforces/documents this?
> > 
> > Nothing enforces it, though it is documented in the x11_util_internal.h for
> > LogErrorEventDescription.  This is called from the error handler but through
a
> > task posted to the message loop.
> 
> I think you're talking about out-of-line execution (which is fine).
> My point was about thread-safety; none of this is safe to call from any thread
> other than the Xlib thread.  Maybe that's obvious from the "ui" namespace? 
IDK.

I understand that. I'm not sure if there's a way to enforce that it's being
called on the right thread. I can presume that the error handlers are called
correctly, but would prefer to DCHECK if I can. The bigger issue is the Check
call itself which is either thread unsafe or needs to be made async. I prefer to
DCHECK there as well if I can.

> 
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
> File gpu/command_buffer/common/gl_mock.h (right):
> 
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
> gpu/command_buffer/common/gl_mock.h:12: #if defined(USE_X11)
> Ditto comment on id_manager_unittest.cc.
> 
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
> File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right):
> 
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
> gpu/command_buffer/service/framebuffer_manager_unittest.cc:10: #if
> defined(USE_X11)
> Ditto comment on id_manager_unittest.cc.
> 
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
> File gpu/command_buffer/service/id_manager_unittest.cc (right):
> 
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
> gpu/command_buffer/service/id_manager_unittest.cc:7: // These are defined by
> both gtest and Xlib so we need to undefine them before
> An alternative is to #include gtest early, as in 
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...
> Saves you an #if-defined and the explicit enumeration of conflicts.
> 
> http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
> File ui/base/x/x11_util.cc (right):
> 
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> ui/base/x/x11_util.cc:766: if (last_error_event.display) {
> Reversing the test lets you early-return and de-indent the rest of the
function.
> 
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> ui/base/x/x11_util.cc:859: DCHECK(dpy == error_event.display)
> DCHECK_EQ work?

[sadrul, 2011-09-22 15:22:49]

On 2011/09/21 23:01:30, dominich wrote:
> On 2011/09/21 22:21:42, Ami Fischman wrote:
> > LGTM modulo nits
> > 
> > http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc
> > File ui/base/x/x11_util.cc (right):
> > 
> >
>
http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
> > ui/base/x/x11_util.cc:772: XSync(display, False);
> > On 2011/09/21 19:47:30, dominich wrote:
> > > On 2011/09/20 23:45:51, Ami Fischman wrote:
> > > > The Xlib calls you make here and from BuildX11ErrorString() tell me that
> > this
> > > > function must be called on the (only) thread talking to Xlib.  What
> > > > enforces/documents this?
> > > 
> > > Nothing enforces it, though it is documented in the x11_util_internal.h
for
> > > LogErrorEventDescription.  This is called from the error handler but
through
> a
> > > task posted to the message loop.
> > 
> > I think you're talking about out-of-line execution (which is fine).
> > My point was about thread-safety; none of this is safe to call from any
thread
> > other than the Xlib thread.  Maybe that's obvious from the "ui" namespace? 
> IDK.
> 
> I understand that. I'm not sure if there's a way to enforce that it's being
> called on the right thread. I can presume that the error handlers are called
> correctly, but would prefer to DCHECK if I can. The bigger issue is the Check
> call itself which is either thread unsafe or needs to be made async. I prefer
to
> DCHECK there as well if I can.

Maybe you want DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));?

> 
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
> > File gpu/command_buffer/common/gl_mock.h (right):
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
> > gpu/command_buffer/common/gl_mock.h:12: #if defined(USE_X11)
> > Ditto comment on id_manager_unittest.cc.
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
> > File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
> > gpu/command_buffer/service/framebuffer_manager_unittest.cc:10: #if
> > defined(USE_X11)
> > Ditto comment on id_manager_unittest.cc.
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
> > File gpu/command_buffer/service/id_manager_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
> > gpu/command_buffer/service/id_manager_unittest.cc:7: // These are defined by
> > both gtest and Xlib so we need to undefine them before
> > An alternative is to #include gtest early, as in 
> >
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...
> > Saves you an #if-defined and the explicit enumeration of conflicts.
> > 
> > http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
> > File ui/base/x/x11_util.cc (right):
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> > ui/base/x/x11_util.cc:766: if (last_error_event.display) {
> > Reversing the test lets you early-return and de-indent the rest of the
> function.
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> > ui/base/x/x11_util.cc:859: DCHECK(dpy == error_event.display)
> > DCHECK_EQ work?

[dominich, 2011-09-22 15:52:48]

On 2011/09/22 15:22:49, sadrul wrote:
> On 2011/09/21 23:01:30, dominich wrote:
> > On 2011/09/21 22:21:42, Ami Fischman wrote:
> > > LGTM modulo nits
> > > 
> > > http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc
> > > File ui/base/x/x11_util.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/7001/ui/base/x/x11_util.cc#newcod...
> > > ui/base/x/x11_util.cc:772: XSync(display, False);
> > > On 2011/09/21 19:47:30, dominich wrote:
> > > > On 2011/09/20 23:45:51, Ami Fischman wrote:
> > > > > The Xlib calls you make here and from BuildX11ErrorString() tell me
that
> > > this
> > > > > function must be called on the (only) thread talking to Xlib.  What
> > > > > enforces/documents this?
> > > > 
> > > > Nothing enforces it, though it is documented in the x11_util_internal.h
> for
> > > > LogErrorEventDescription.  This is called from the error handler but
> through
> > a
> > > > task posted to the message loop.
> > > 
> > > I think you're talking about out-of-line execution (which is fine).
> > > My point was about thread-safety; none of this is safe to call from any
> thread
> > > other than the Xlib thread.  Maybe that's obvious from the "ui" namespace?

> > IDK.
> > 
> > I understand that. I'm not sure if there's a way to enforce that it's being
> > called on the right thread. I can presume that the error handlers are called
> > correctly, but would prefer to DCHECK if I can. The bigger issue is the
Check
> > call itself which is either thread unsafe or needs to be made async. I
prefer
> to
> > DCHECK there as well if I can.
> 
> Maybe you want DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));?

content/ depends on ui/base. I'm not sure that having ui/ depend on content/ is
a good idea.

> 
> > 
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
> > > File gpu/command_buffer/common/gl_mock.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
> > > gpu/command_buffer/common/gl_mock.h:12: #if defined(USE_X11)
> > > Ditto comment on id_manager_unittest.cc.
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
> > > File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
> > > gpu/command_buffer/service/framebuffer_manager_unittest.cc:10: #if
> > > defined(USE_X11)
> > > Ditto comment on id_manager_unittest.cc.
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
> > > File gpu/command_buffer/service/id_manager_unittest.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
> > > gpu/command_buffer/service/id_manager_unittest.cc:7: // These are defined
by
> > > both gtest and Xlib so we need to undefine them before
> > > An alternative is to #include gtest early, as in 
> > >
> >
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...
> > > Saves you an #if-defined and the explicit enumeration of conflicts.
> > > 
> > > http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
> > > File ui/base/x/x11_util.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> > > ui/base/x/x11_util.cc:766: if (last_error_event.display) {
> > > Reversing the test lets you early-return and de-indent the rest of the
> > function.
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> > > ui/base/x/x11_util.cc:859: DCHECK(dpy == error_event.display)
> > > DCHECK_EQ work?

[sadrul, 2011-09-22 16:19:37]

[snip]
> > > 
> > > I understand that. I'm not sure if there's a way to enforce that it's
being
> > > called on the right thread. I can presume that the error handlers are
called
> > > correctly, but would prefer to DCHECK if I can. The bigger issue is the
> Check
> > > call itself which is either thread unsafe or needs to be made async. I
> prefer
> > to
> > > DCHECK there as well if I can.
> > 
> > Maybe you want DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));?
> 
> content/ depends on ui/base. I'm not sure that having ui/ depend on content/
is
> a good idea.

Ah, good point. Perhaps DCHECK_EQ(MessageLoop::TYPE_UI ,
MessageLoop::current()->type()); would work?

[dominich, 2011-09-22 17:00:15]

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
File gpu/command_buffer/common/gl_mock.h (right):

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/common/gl...
gpu/command_buffer/common/gl_mock.h:12: #if defined(USE_X11)
On 2011/09/21 22:21:42, Ami Fischman wrote:
> Ditto comment on id_manager_unittest.cc.

Done.

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
File gpu/command_buffer/service/framebuffer_manager_unittest.cc (right):

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/f...
gpu/command_buffer/service/framebuffer_manager_unittest.cc:10: #if
defined(USE_X11)
On 2011/09/21 22:21:42, Ami Fischman wrote:
> Ditto comment on id_manager_unittest.cc.

Done.

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
File gpu/command_buffer/service/id_manager_unittest.cc (right):

http://codereview.chromium.org/7889040/diff/9004/gpu/command_buffer/service/i...
gpu/command_buffer/service/id_manager_unittest.cc:7: // These are defined by
both gtest and Xlib so we need to undefine them before
On 2011/09/21 22:21:42, Ami Fischman wrote:
> An alternative is to #include gtest early, as in 
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...
> Saves you an #if-defined and the explicit enumeration of conflicts.

Done.

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:766: if (last_error_event.display) {
On 2011/09/21 22:21:42, Ami Fischman wrote:
> Reversing the test lets you early-return and de-indent the rest of the
function.

Done.

[Ami GONE FROM CHROMIUM, 2011-09-22 17:17:50]

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:773: LOG(FATAL) << BuildX11ErrorString(last_error_event);
once you FATAL the rest of this method doesn't matter so much... ;)

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util_internal.h
File ui/base/x/x11_util_internal.h (right):

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util_internal...
ui/base/x/x11_util_internal.h:56: // has been reported.
Wait, what?
The comment & function name still say check.

[dominich, 2011-09-22 17:46:18]

On 2011/09/21 21:50:34, sadrul wrote:
> Hi! Just a note that came up in a review in another CR:
> http://codereview.chromium.org/7981030/
> 
> http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
> File ui/base/x/x11_util.cc (right):
> 
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> ui/base/x/x11_util.cc:770: }
> Can this return a bool depending on whether there was an error or not? This
> could perhaps then replace the gdk_error_trap_push/gdk_error_trap_pop calls in
a
> few places in this file?

I did put in the return, but it's much cleaner to LOG(FATAL) instead and have a
void return. I still think it can replace the gdk_error_trap_* calls as instead
of popping and checking the return, and calling XSync explicitly, you can just
call the Check function.

[sadrul, 2011-09-22 17:49:11]

On 2011/09/22 17:46:18, dominich wrote:
> On 2011/09/21 21:50:34, sadrul wrote:
> > Hi! Just a note that came up in a review in another CR:
> > http://codereview.chromium.org/7981030/
> > 
> > http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
> > File ui/base/x/x11_util.cc (right):
> > 
> >
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> > ui/base/x/x11_util.cc:770: }
> > Can this return a bool depending on whether there was an error or not? This
> > could perhaps then replace the gdk_error_trap_push/gdk_error_trap_pop calls
in
> a
> > few places in this file?
> 
> I did put in the return, but it's much cleaner to LOG(FATAL) instead and have
a
> void return. I still think it can replace the gdk_error_trap_* calls as
instead
> of popping and checking the return, and calling XSync explicitly, you can just
> call the Check function.

Hm. Does LOG(FATAL) cause the process to terminate? If that is the case, do you
think it'd be possible to have non-fatal error reporting? If we were to replace
gdk_error_trap_ calls with this, then I think that'll be necessary.

[sadrul, 2011-09-22 17:51:44]

On 2011/09/22 17:49:11, sadrul wrote:
> On 2011/09/22 17:46:18, dominich wrote:
> > On 2011/09/21 21:50:34, sadrul wrote:
> > > Hi! Just a note that came up in a review in another CR:
> > > http://codereview.chromium.org/7981030/
> > > 
> > > http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
> > > File ui/base/x/x11_util.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> > > ui/base/x/x11_util.cc:770: }
> > > Can this return a bool depending on whether there was an error or not?
This
> > > could perhaps then replace the gdk_error_trap_push/gdk_error_trap_pop
calls
> in
> > a
> > > few places in this file?
> > 
> > I did put in the return, but it's much cleaner to LOG(FATAL) instead and
have
> a
> > void return. I still think it can replace the gdk_error_trap_* calls as
> instead
> > of popping and checking the return, and calling XSync explicitly, you can
just
> > call the Check function.
> 
> Hm. Does LOG(FATAL) cause the process to terminate? If that is the case, do
you
> think it'd be possible to have non-fatal error reporting? If we were to
replace
> gdk_error_trap_ calls with this, then I think that'll be necessary.

If that sounds OK, I can make that non-fatal change in my CL
(http://codereview.chromium.org/7981030/), i.e. I do not want to block this CL
for this change.

[dominich, 2011-09-22 17:53:04]

On 2011/09/22 17:49:11, sadrul wrote:
> On 2011/09/22 17:46:18, dominich wrote:
> > On 2011/09/21 21:50:34, sadrul wrote:
> > > Hi! Just a note that came up in a review in another CR:
> > > http://codereview.chromium.org/7981030/
> > > 
> > > http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc
> > > File ui/base/x/x11_util.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7889040/diff/9004/ui/base/x/x11_util.cc#newcod...
> > > ui/base/x/x11_util.cc:770: }
> > > Can this return a bool depending on whether there was an error or not?
This
> > > could perhaps then replace the gdk_error_trap_push/gdk_error_trap_pop
calls
> in
> > a
> > > few places in this file?
> > 
> > I did put in the return, but it's much cleaner to LOG(FATAL) instead and
have
> a
> > void return. I still think it can replace the gdk_error_trap_* calls as
> instead
> > of popping and checking the return, and calling XSync explicitly, you can
just
> > call the Check function.
> 
> Hm. Does LOG(FATAL) cause the process to terminate? If that is the case, do
you
> think it'd be possible to have non-fatal error reporting? If we were to
replace
> gdk_error_trap_ calls with this, then I think that'll be necessary.

Yes it does. Currently we LOG(ERROR) on X11 errors and then fail hard with a
crash next time an X11 function is called. The point of this CL is to introduce
a process termination sooner and with more information.

I'd argue for replacing the current gdk_error reporting with fatal errors in
some cases.

[dominich, 2011-09-22 18:24:11]

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:773: LOG(FATAL) << BuildX11ErrorString(last_error_event);
On 2011/09/22 17:17:50, Ami Fischman wrote:
> once you FATAL the rest of this method doesn't matter so much... ;)

There might one day be a build that disables FATAL. You never know ;)

Cleaned this up.

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util_internal.h
File ui/base/x/x11_util_internal.h (right):

http://codereview.chromium.org/7889040/diff/17001/ui/base/x/x11_util_internal...
ui/base/x/x11_util_internal.h:56: // has been reported.
On 2011/09/22 17:17:50, Ami Fischman wrote:
> Wait, what?
> The comment & function name still say check.

Changed the function name and updated the comment. I removed the return value
too as I think it confuses things rather than adds flexibility.

[Ami GONE FROM CHROMIUM, 2011-09-22 18:26:34]

LGTM

[marcheu, 2011-09-23 19:49:34]

You have my LGTM for what it's worth (I'm not a Chrome committer).

[sadrul, 2011-09-23 19:51:22]

I chimed in for the comment. No need for an LGTM from me ;)

[Ken Russell (switch to Gerrit), 2011-09-26 22:12:13]

LGTM

[commit-bot: I haz the power, 2011-09-27 16:44:37]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/7889040/29001

[commit-bot: I haz the power, 2011-09-27 18:27:00]

Change committed as 102978