Created attachment 123617 [details] [review]
Possible fix

(In reply to Mike Blumenkrantz from comment #0)
> [...]
> ConnectionInfo is NULL, as InitInput is called before the connection data is
> initialized, meaning that anything trying to access it before this point
> will trigger a crash.

Huh...

I am not sure, why we'd need to call the wl_display_roundtrip() in InitInput()?

This and initializing xwl_screen->expecting_event to 0 will be done again later again in xwl_screen_init() where it seems more appropriate, and events can safely be queued up meanwhile.

Does this patch help? I haven't detected any ill effect in my light initial testing.

(In reply to Olivier Fourdan from comment #1)
> Created attachment 123617 [details] [review] [review]
> Possible fix
> 
> (In reply to Mike Blumenkrantz from comment #0)
> > [...]
> > ConnectionInfo is NULL, as InitInput is called before the connection data is
> > initialized, meaning that anything trying to access it before this point
> > will trigger a crash.
> 
> Huh...
> 
> I am not sure, why we'd need to call the wl_display_roundtrip() in
> InitInput()?
> 
> This and initializing xwl_screen->expecting_event to 0 will be done again
> later again in xwl_screen_init() where it seems more appropriate, and events
> can safely be queued up meanwhile.

I think that if we don't roundtrip here, we might end up in a situation where various places expects to be able to know things about wl_seat's etc, that'd now start to crash if we loose the race against the wl_seat global being discovered.

AFAICS InitInput() is done after InitOutput() which is where xwl_screen_init and the first roundtrips are done.

> 
> Does this patch help? I haven't detected any ill effect in my light initial
> testing.

I wonder if it'd be better to move the initialization roundtrip to after CreateConnectionBlock(), while adapting everything that is in response to the global object discovery to be able to wait until that stage. I don't know what would break, but at least I think we need to either make sure Xwayland can deal with working without any seat, or ensure we have discovered and created all the seat objects before finishing initialization.

Mind, wl_seats can come and go dynamically, and it is not guaranteed to have at least one at all times. So, working without any seat would sound like a good idea to me.

The same goes for outputs, and will likely be testable on Weston once Armin's GSoC project is near finishing.

(In reply to Jonas Ådahl from comment #2)
> [...]
> I think that if we don't roundtrip here, we might end up in a situation
> where various places expects to be able to know things about wl_seat's etc,
> that'd now start to crash if we loose the race against the wl_seat global
> being discovered.

I don't experience any crash with this patch applied, neither with mutter which spawns Xwayland at startup as it (still) relies on X, i.e. very early, nor with weston which spawns it when needed (ie with all outputs and seats set-up).
 
> AFAICS InitInput() is done after InitOutput() which is where xwl_screen_init
> and the first roundtrips are done.

You're right, of course, but the problem is not much xwl_screen_init() but the X ConnectionInfo (as reported in comment #0) that gets initialized after InitInput is called in dix.

It goes like this:

  xwl_screen_init()
  InitInput()
  ConnectionInfo initialized

So if the roundtrip in InitInput() trigger something that needs to use the X connection (as with RR in the backtrace), it will crash.

Meanwhile, ajax picked up the patch from the ML and pushed it, so it's landed in master (I did send the patch to the devel ML in parallel as this is where patches get discussed for xorg rather than bugzilla).

But I am not convinced this patch would cause more bad that good, initializing xwl_screen->expecting_event to 0 twice at two different places doesn't sound right and doing the rountrip before X connection is likely to cause breakage as well, as seen in comment #0.

Let's keep that bug opened and see if we get some more feedback (either here or on the xorg-devel ML), meanwhile please test master with different compositors and use cases and let's see if this a real problem.

(In reply to Pekka Paalanen from comment #3)
> Mind, wl_seats can come and go dynamically, and it is not guaranteed to have
> at least one at all times. So, working without any seat would sound like a
> good idea to me.
> 
> The same goes for outputs, and will likely be testable on Weston once
> Armin's GSoC project is near finishing.

Yes, probably, but I reckon outputs are a different issue than this particular crash.

(In reply to Olivier Fourdan from comment #4)
> You're right, of course, but the problem is not much xwl_screen_init() but
> the X ConnectionInfo (as reported in comment #0) that gets initialized after
> InitInput is called in dix.
> 
> It goes like this:
> 
>   xwl_screen_init()
>   InitInput()
>   ConnectionInfo initialized
> 
> So if the roundtrip in InitInput() trigger something that needs to use the X
> connection (as with RR in the backtrace), it will crash.

Humm, now that I re-read my own sentence, I realize there is still an obvious possibility of breakeage because the rountrips are done in xwl_screen_init(), so *before* ConnectionInfo is initialized

Patches have landed in master, closing.