Re: [PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Alexandre Julliard]

Miklós Máté  writes:
> @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
>  /***********************************************************************
> -
>          create_gl_drawable
>
> */
> -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
>  {
>      gl->drawable = 0;
>  if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())  /* top-level window */
>  {
> -
>    struct x11drv_win_data *data = get_win_data( hwnd );
> -
>    if (data)
>    {
>        gl->type = DC_GL_WINDOW;
>        gl->window = create_client_window( data, gl->visual );
>        if (gl->window)
> -
>        {
>            gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> -
>        release_win_data( data );
> -
>            XSync( gdi_display, False );

Do you really need all the extra XSync calls?  We try to avoid server
round-trips as much as possible.

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Miklós Máté]

On 04/04/17 12:11, Alexandre Julliard wrote:
> Miklós Máté  writes:
> > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> >   /***********************************************************************
> > -
> >          create_gl_drawable
> >
> > */
> > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> >   {
> >       gl->drawable = 0;
> >   if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())  /* top-level window */
> >   {
> > -
> >    struct x11drv_win_data *data = get_win_data( hwnd );
> > -
> >     if (data)
> >     {
> >         gl->type = DC_GL_WINDOW;
> >         gl->window = create_client_window( data, gl->visual );
> >         if (gl->window)
> > -
> >        {
> >             gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > -
> >        release_win_data( data );
> > -
> >            XSync( gdi_display, False );
>
> Do you really need all the extra XSync calls?  We try to avoid server
> round-trips as much as possible.

Yes. We must ensure that glXCreateWindow() and glXDestroyWindow() refer 
to a valid X window, so the command queue of data->display and 
gdi_display have to be in sync around those calls.

MM

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Alexandre Julliard]

Miklós Máté  writes:
> On 04/04/17 12:11, Alexandre Julliard wrote:
> > Miklós Máté  writes:
> > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > >   /***********************************************************************
> > > -
> > >          create_gl_drawable
> > >
> > > */
> > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > >   {
> > >       gl->drawable = 0;
> > >         if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > /* top-level window */
> > >       {
> > > -
> > >    struct x11drv_win_data *data = get_win_data( hwnd );
> > > -
> > >     if (data)
> > >     {
> > >         gl->type = DC_GL_WINDOW;
> > >         gl->window = create_client_window( data, gl->visual );
> > >         if (gl->window)
> > > -
> > >        {
> > >             gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > -
> > >        release_win_data( data );
> > > -
> > >            XSync( gdi_display, False );
> >
> > Do you really need all the extra XSync calls?  We try to avoid server
> > round-trips as much as possible.
>
> Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> refer to a valid X window, so the command queue of data->display and
> gdi_display have to be in sync around those calls.

If the goal is that the X window is valid, I'd have expected an XSync on
the thread display. Why is the XSync on gdi_display needed here? What is
the scenario that fails?

I'm not questioning that some XSyncs are necessary (and we already have
a few), but I want to make sure we don't add more than needed.

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Miklós Máté]

On 04/04/17 16:06, Alexandre Julliard wrote:
> Miklós Máté  writes:
> > On 04/04/17 12:11, Alexandre Julliard wrote:
> > > Miklós Máté  writes:
> > > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > > >    /***********************************************************************
> > > >     *              create_gl_drawable
> > > >     */
> > > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > > >    {
> > > >        gl->drawable = 0;
> > > >          if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > > /* top-level window */
> > > >        {
> > > > -
> > > >    struct x11drv_win_data *data = get_win_data( hwnd );
> > > > -
> > > >      if (data)
> > > >      {
> > > >          gl->type = DC_GL_WINDOW;
> > > >          gl->window = create_client_window( data, gl->visual );
> > > >          if (gl->window)
> > > > -
> > > >        {
> > > >              gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > > -
> > > >        release_win_data( data );
> > > > -
> > > >            XSync( gdi_display, False );
> > >
> > > Do you really need all the extra XSync calls?  We try to avoid server
> > > round-trips as much as possible.
> >
> > Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> > refer to a valid X window, so the command queue of data->display and
> > gdi_display have to be in sync around those calls.
>
> If the goal is that the X window is valid, I'd have expected an XSync on
> the thread display. Why is the XSync on gdi_display needed here? What is
> the scenario that fails?
>
> I'm not questioning that some XSyncs are necessary (and we already have
> a few), but I want to make sure we don't add more than needed.

This one prevents crash in the following scenario:

1. create_gl_drawable()

2. X11DRV_ThreadDetach() calls XCloseDisplay( data->display );

3. the Xserver processes the glXCreateWindow in the command queue of 
gdi_display -> X window is not valid

This happens sometimes during the ddraw and d3d9 tests.

MM

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Alexandre Julliard]

Miklós Máté  writes:
> On 04/04/17 16:06, Alexandre Julliard wrote:
> > Miklós Máté  writes:
> > > On 04/04/17 12:11, Alexandre Julliard wrote:
> > > > Miklós Máté  writes:
> > > > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > > > >    /***********************************************************************
> > > > >     *              create_gl_drawable
> > > > >     */
> > > > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > > > >    {
> > > > >        gl->drawable = 0;
> > > > >          if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > > > /* top-level window */
> > > > >        {
> > > > > -
> > > > >    struct x11drv_win_data *data = get_win_data( hwnd );
> > > > > -
> > > > >      if (data)
> > > > >      {
> > > > >          gl->type = DC_GL_WINDOW;
> > > > >          gl->window = create_client_window( data, gl->visual );
> > > > >          if (gl->window)
> > > > > -
> > > > >        {
> > > > >              gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > > > -
> > > > >        release_win_data( data );
> > > > > -
> > > > >            XSync( gdi_display, False );
> > > >
> > > > Do you really need all the extra XSync calls?  We try to avoid server
> > > > round-trips as much as possible.
> > >
> > > Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> > > refer to a valid X window, so the command queue of data->display and
> > > gdi_display have to be in sync around those calls.
> >
> > If the goal is that the X window is valid, I'd have expected an XSync on
> > the thread display. Why is the XSync on gdi_display needed here? What is
> > the scenario that fails?
> >
> > I'm not questioning that some XSyncs are necessary (and we already have
> > a few), but I want to make sure we don't add more than needed.
>
> This one prevents crash in the following scenario:
> -
>
> create_gl_drawable()
> -
>
> X11DRV_ThreadDetach() calls XCloseDisplay( data->display );
> -
>
> the Xserver processes the glXCreateWindow in the command queue of
>
> gdi_display -> X window is not valid
>
> This happens sometimes during the ddraw and d3d9 tests.

Shouldn't this be happening even without your other changes then?

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Miklós Máté]

On 05/04/17 19:52, Alexandre Julliard wrote:
> Miklós Máté  writes:
> > On 04/04/17 16:06, Alexandre Julliard wrote:
> > > Miklós Máté  writes:
> > > > On 04/04/17 12:11, Alexandre Julliard wrote:
> > > > > Miklós Máté  writes:
> > > > > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > > > > >     /***********************************************************************
> > > > > >      *              create_gl_drawable
> > > > > >      */
> > > > > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > > > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > > > > >     {
> > > > > >         gl->drawable = 0;
> > > > > >           if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > > > > /* top-level window */
> > > > > >         {
> > > > > > -
> > > > > >    struct x11drv_win_data *data = get_win_data( hwnd );
> > > > > > -
> > > > > >       if (data)
> > > > > >       {
> > > > > >           gl->type = DC_GL_WINDOW;
> > > > > >           gl->window = create_client_window( data, gl->visual );
> > > > > >           if (gl->window)
> > > > > > -
> > > > > >        {
> > > > > >               gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > > > > -
> > > > > >        release_win_data( data );
> > > > > > -
> > > > > >            XSync( gdi_display, False );
> > > > >
> > > > > Do you really need all the extra XSync calls?  We try to avoid server
> > > > > round-trips as much as possible.
> > > >
> > > > Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> > > > refer to a valid X window, so the command queue of data->display and
> > > > gdi_display have to be in sync around those calls.
> > >
> > > If the goal is that the X window is valid, I'd have expected an XSync on
> > > the thread display. Why is the XSync on gdi_display needed here? What is
> > > the scenario that fails?
> > >
> > > I'm not questioning that some XSyncs are necessary (and we already have
> > > a few), but I want to make sure we don't add more than needed.
> >
> > This one prevents crash in the following scenario:
> > -
> >
> > create_gl_drawable()
> > -
> >
> > X11DRV_ThreadDetach() calls XCloseDisplay( data->display );
> > -
> >
> > the Xserver processes the glXCreateWindow in the command queue of
> >
> > gdi_display -> X window is not valid
> >
> > This happens sometimes during the ddraw and d3d9 tests.
>
> Shouldn't this be happening even without your other changes then?

Well, according to my notes from half a year ago the ddraw test of 
wine-csmt definitely crashes without this, but now I can't reproduce it. 
I don't know how likely the above scenario is in the real world.

MM