[PATCH 1/5] user32/tests: Add more tests for windows destroyed by thread exit.

[Jacek Caban]

Signed-off-by: Jacek Caban 
---
  dlls/user32/tests/msg.c | 33 +++++++++++++++++++++++++++++++++
  1 file changed, 33 insertions(+)

[Rémi Bernon]

On 2/1/22 15:12, Jacek Caban wrote:
> Signed-off-by: Jacek Caban
>
> dlls/user32/tests/msg.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)

Hi Jacek!

I've been trying to fix this for quite some time as I believe we have a 
race condition in thread destroy and that causes some spurious 
user32:msg test failures already.

I wrote a few more tests (attached) before to check Windows behavior and 
they don't pass with your changes.

I also had several variations of the fix, and I'm attaching the last one 
for reference, although I understand PATCH 3 and 4 which are changing 
the parent pointer semantics on the wineserver side is a bit risky.

From the errors returned by window functions it doesn't look completely 
off though (some functions fail in the same way for desktop window and 
for orphaned windows, both not having a parent).

Note that my fix then doesn't pass your WM_NCDESTROY parent test either.

Cheers,

[Jacek Caban]

Hi Rémi,

On 2/1/22 15:52, Rémi Bernon wrote:
> On 2/1/22 15:12, Jacek Caban wrote:
> > Signed-off-by: Jacek Caban
> >
> > dlls/user32/tests/msg.c | 33 +++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
>
> Hi Jacek!
>
> I've been trying to fix this for quite some time as I believe we have 
> a race condition in thread destroy and that causes some spurious 
> user32:msg test failures already.
>
> I wrote a few more tests (attached) before to check Windows behavior 
> and they don't pass with your changes.
>
> I also had several variations of the fix, and I'm attaching the last 
> one for reference, although I understand PATCH 3 and 4 which are 
> changing the parent pointer semantics on the wineserver side is a bit 
> risky.
>
> From the errors returned by window functions it doesn't look 
> completely off though (some functions fail in the same way for desktop 
> window and for orphaned windows, both not having a parent).
>
> Note that my fix then doesn't pass your WM_NCDESTROY parent test either.

Thanks, those patches look interesting. I tested your tests from the 
first two patches. Most of failures are not strictly related to 
destroying window on thread exit, they fail the same way when normal 
DestroyWindow() is used. I attached a patch that confirms that with 
tests and has a quick fix for that. I still need to look at todo_wine 
around IsWindow(child3) test.

Thanks,

Jacek

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=106759

Your paranoid android.

=== build (build log) ===

error: patch failed: dlls/user32/tests/msg.c:9035
error: patch failed: dlls/user32/tests/win.c:839
error: patch failed: server/window.c:568
Task: Patch failed to apply

=== debian11 (build log) ===

error: patch failed: dlls/user32/tests/msg.c:9035
error: patch failed: dlls/user32/tests/win.c:839
error: patch failed: server/window.c:568
Task: Patch failed to apply

=== debian11 (build log) ===

error: patch failed: dlls/user32/tests/msg.c:9035
error: patch failed: dlls/user32/tests/win.c:839
error: patch failed: server/window.c:568
Task: Patch failed to apply

[Jacek Caban]

Hi Rémi,

On 2/1/22 21:59, Jacek Caban wrote:
> Hi Rémi,
>
> On 2/1/22 15:52, Rémi Bernon wrote:
> > On 2/1/22 15:12, Jacek Caban wrote:
> > > Signed-off-by: Jacek Caban
> > >
> > > dlls/user32/tests/msg.c | 33 +++++++++++++++++++++++++++++++++
> > >   1 file changed, 33 insertions(+)
> >
> > Hi Jacek!
> >
> > I've been trying to fix this for quite some time as I believe we have 
> > a race condition in thread destroy and that causes some spurious 
> > user32:msg test failures already.
> >
> > I wrote a few more tests (attached) before to check Windows behavior 
> > and they don't pass with your changes.
> >
> > I also had several variations of the fix, and I'm attaching the last 
> > one for reference, although I understand PATCH 3 and 4 which are 
> > changing the parent pointer semantics on the wineserver side is a bit 
> > risky.
> >
> > From the errors returned by window functions it doesn't look 
> > completely off though (some functions fail in the same way for 
> > desktop window and for orphaned windows, both not having a parent).
> >
> > Note that my fix then doesn't pass your WM_NCDESTROY parent test either.
>
> Thanks, those patches look interesting. I tested your tests from the 
> first two patches. Most of failures are not strictly related to 
> destroying window on thread exit, they fail the same way when normal 
> DestroyWindow() is used. I attached a patch that confirms that with 
> tests and has a quick fix for that. I still need to look at todo_wine 
> around IsWindow(child3) test.

The remaining todo_wine is addressed by your patch 6 and 7, which still 
works fine. I left it out for now. Also more testing showed that things 
like MoveWindow() should still work. We implement it on top of 
SetWindowPos() (which is supposed to fail), so we'd need to change that 
a bit to make SetWindowPos fail on orphaned windows.

Also, looking closer at this, I think that we really should keep parent 
object around until its children are properly destroyed. I submitted a 
new series implementing it.

Thanks,

Jacek

[Rémi Bernon]

On 2/8/22 12:39, Jacek Caban wrote:
> Hi Rémi,
>
> On 2/1/22 21:59, Jacek Caban wrote:
> > Hi Rémi,
> >
> > On 2/1/22 15:52, Rémi Bernon wrote:
> > > On 2/1/22 15:12, Jacek Caban wrote:
> > > > Signed-off-by: Jacek Caban
> > > >
> > > > dlls/user32/tests/msg.c | 33 +++++++++++++++++++++++++++++++++
> > > >   1 file changed, 33 insertions(+)
> > >
> > > Hi Jacek!
> > >
> > > I've been trying to fix this for quite some time as I believe we have 
> > > a race condition in thread destroy and that causes some spurious 
> > > user32:msg test failures already.
> > >
> > > I wrote a few more tests (attached) before to check Windows behavior 
> > > and they don't pass with your changes.
> > >
> > > I also had several variations of the fix, and I'm attaching the last 
> > > one for reference, although I understand PATCH 3 and 4 which are 
> > > changing the parent pointer semantics on the wineserver side is a bit 
> > > risky.
> > >
> > > From the errors returned by window functions it doesn't look 
> > > completely off though (some functions fail in the same way for 
> > > desktop window and for orphaned windows, both not having a parent).
> > >
> > > Note that my fix then doesn't pass your WM_NCDESTROY parent test either.
> >
> > Thanks, those patches look interesting. I tested your tests from the 
> > first two patches. Most of failures are not strictly related to 
> > destroying window on thread exit, they fail the same way when normal 
> > DestroyWindow() is used. I attached a patch that confirms that with 
> > tests and has a quick fix for that. I still need to look at todo_wine 
> > around IsWindow(child3) test.
>
> The remaining todo_wine is addressed by your patch 6 and 7, which still 
> works fine. I left it out for now. Also more testing showed that things 
> like MoveWindow() should still work. We implement it on top of 
> SetWindowPos() (which is supposed to fail), so we'd need to change that 
> a bit to make SetWindowPos fail on orphaned windows.
>
> Also, looking closer at this, I think that we really should keep parent 
> object around until its children are properly destroyed. I submitted a 
> new series implementing it.

I saw that, didn't look very close (or at your tree, yet) but I trust it 
does the right thing, thanks for taking over!

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=106735

Your paranoid android.

=== w7u_2qxl (32 bit report) ===

user32:
win.c:1500: Test failed: Expected color 0xc0c0c0, got 0xffffffff.