[Bug 48732] New: How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

Bug ID: 48732
           Summary: How to Survive crashes on start (fullscreen mode)
           Product: Wine
           Version: 5.0-rc6
          Hardware: x86
               URL: https://store.steampowered.com/app/250400/How_to_Survi
                    ve/
                OS: Linux
            Status: NEW
          Keywords: regression
          Severity: normal
          Priority: P2
         Component: quartz
          Assignee: [e-mail]
          Reporter: [e-mail]
                CC: [e-mail]
   Regression SHA1: 54d82d80d738cddb95f925189b8937e37a1a85c9
      Distribution: ---

Created attachment 66615
  --> https://bugs.winehq.org/attachment.cgi?id=66615
terminal output

The game crashes after showing the first logo screen when run in fullscreen
mode.
The crash can be workaround by selecting windowed mode in the configuration
menu.

Reverting the following commit also gets rid of the crash:

commit 54d82d80d738cddb95f925189b8937e37a1a85c9
strmbase: Detach the window from the parent before destroying it.
This fixes a regression introduced by 3b5198c8283d891095612c1001edb5e5788d6059.

Tested in wine-5.3-181-geb63713f60.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

Zebediah Figura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Zebediah Figura  ---
Created attachment 66617
  --> https://bugs.winehq.org/attachment.cgi?id=66617
don't unload quartz

I can't reproduce this. Does the attached patch help?

If not, can you please attach a log with
WINEDEBUG=+seh,+ole,+quartz,+strmbase,+loaddll?

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #2 from Béla Gyebrószki  ---
Created attachment 66618
  --> https://bugs.winehq.org/attachment.cgi?id=66618
+seh,+ole,+quartz,+strmbase,+loaddll log

(In reply to Zebediah Figura from comment #1)
> Created attachment 66617 [details]
> don't unload quartz
>
> I can't reproduce this. Does the attached patch help?

No, it doesn't.
> If not, can you please attach a log with
> WINEDEBUG=+seh,+ole,+quartz,+strmbase,+loaddll?

I attached the log as you requested.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #3 from Gabriel Ivăncescu  ---
I haven't been able to reproduce it either, but from your log it seems it
crashes in a different thread immediately after put_Owner.

However, while mine didn't crash, this is the surrounding log to put_Owner:

0035:trace:quartz:FilterGraph2_RemoveFilter Disconnect 2: 00000001
0035:trace:strmbase:filter_inner_Release 17A9F688 decreasing refcount to 2.
0035:trace:strmbase:IEnumPinsImpl_Next (17AA04B0)->(1, 00EFACC4, 00000000)
0035:trace:strmbase:IEnumPinsImpl_Release (17AA04B0)->(): new ref = 0
0035:trace:strmbase:filter_inner_Release 17A9F688 decreasing refcount to 1.
0035:trace:strmbase:filter_JoinFilterGraph (17A9F688)->(00000000, (null))
0035:trace:strmbase:filter_SetSyncSource (17A9F688)->(00000000)
0035:trace:strmbase:filter_inner_Release 17A9F688 decreasing refcount to 0.
0035:trace:strmbase:BaseControlWindowImpl_put_Owner window 17A9F988, owner 0.
0035:trace:strmbase:filter_inner_Release 17A9F688 decreasing refcount to
4294967295.
0035:trace:strmbase:sink_Disconnect pin 17A9F7D4.
0035:trace:strmbase:SeekInner_Release (17A9E268)->(): new ref = 0
0035:trace:quartz:Inner_Release (17A9E1C8)->(): new ref = 0

Note the refcount decrease to 4294967295 (underflow to -1), that's clearly a
refcount bug somewhere and is probably the reason for the crash
(use-after-free).

For extra info: the app has no prior owner on the window (it's NULL) and the
window's thread ID is the same as the thread from where put_Owner is called
from.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #4 from Gabriel Ivăncescu  ---
Created attachment 66627
  --> https://bugs.winehq.org/attachment.cgi?id=66627
Test workaround

Does this patch help? It should avoid the call if the window has no owner to
begin with.

The issue is that this shouldn't be needed, as there's nothing in the call, so
I don't know if it's suitable for upstream, but that's up to Zeb. Personally
I'm fine with it, if it helps.

I don't know if it's a game bug that can't handle a redundant call to SetParent
for example, or if there's a deeper issue with Wine.

The refcount issue I mentioned earlier seems harmless. For some reason, it is
caused by:

if (filter->presenter)
        IVMRImagePresenter9_Release(filter->presenter);

in quartz/vmr9.c vmr_destroy function, which decreases the refcount after it
was already zero. However, this doesn't really seem to do anything, since on
the filter side, it checks if (!refcount) and otherwise does nothing, so it
seems harmless and likely not the culprit.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #5 from Zebediah Figura  ---
(In reply to Gabriel Ivăncescu from comment #4)
> Created attachment 66627 [details]
> Test workaround
>
> Does this patch help? It should avoid the call if the window has no owner to
> begin with.
>
> The issue is that this shouldn't be needed, as there's nothing in the call,
> so I don't know if it's suitable for upstream, but that's up to Zeb.
> Personally I'm fine with it, if it helps.
>
> I don't know if it's a game bug that can't handle a redundant call to
> SetParent for example, or if there's a deeper issue with Wine.

That doesn't seem right, and I'd be surprised if it helps. If the window is
parentless it shouldn't result in any messages getting sent to the application.
> The refcount issue I mentioned earlier seems harmless. For some reason, it
> is caused by:
> if (filter->presenter)
>     IVMRImagePresenter9_Release(filter->presenter);
>
> in quartz/vmr9.c vmr_destroy function, which decreases the refcount after it
> was already zero. However, this doesn't really seem to do anything, since on
> the filter side, it checks if (!refcount) and otherwise does nothing, so it
> seems harmless and likely not the culprit.

That doesn't seem right either; the allocator/presenter is a separate object
with its own refcount. Are you sure that's where the dereference is coming
from?

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #6 from Gabriel Ivăncescu  ---
(In reply to Zebediah Figura from comment #5)
> (In reply to Gabriel Ivăncescu from comment #4)
> > Created attachment 66627 [details]
> > Test workaround
> >
> > Does this patch help? It should avoid the call if the window has no owner to
> > begin with.
> >
> > The issue is that this shouldn't be needed, as there's nothing in the call,
> > so I don't know if it's suitable for upstream, but that's up to Zeb.
> > Personally I'm fine with it, if it helps.
> >
> > I don't know if it's a game bug that can't handle a redundant call to
> > SetParent for example, or if there's a deeper issue with Wine.
>
> That doesn't seem right, and I'd be surprised if it helps. If the window is
> parentless it shouldn't result in any messages getting sent to the
> application.

Right, I was thinking the app does something weird like hook into SetParent. Or
perhaps the call exposes a race condition of a deeper issue, which is a bigger
concern. (this would just "hide" it)
> > The refcount issue I mentioned earlier seems harmless. For some reason, it
> > is caused by:
> > if (filter->presenter)
> >     IVMRImagePresenter9_Release(filter->presenter);
> >
> > in quartz/vmr9.c vmr_destroy function, which decreases the refcount after it
> > was already zero. However, this doesn't really seem to do anything, since on
> > the filter side, it checks if (!refcount) and otherwise does nothing, so it
> > seems harmless and likely not the culprit.
>
> That doesn't seem right either; the allocator/presenter is a separate object
> with its own refcount. Are you sure that's where the dereference is coming
> from?

I'm as puzzled as you are, but yes, that's where it happened for some reason; I
added traces between each call to verify this. It could be an application bug,
though (which is also silently ignored on Windows). I'm curious, do you not get
this behavior when you try it? (I mean the refcount thing)

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #7 from Béla Gyebrószki  ---
(In reply to Gabriel Ivăncescu from comment #4)
> Created attachment 66627 [details]
> Test workaround
>
> Does this patch help? It should avoid the call if the window has no owner to
> begin with.

FWIW, the patch fixes the crash for me.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #8 from Zebediah Figura  ---
(In reply to Gabriel Ivăncescu from comment #6)
> Right, I was thinking the app does something weird like hook into SetParent.
> Or perhaps the call exposes a race condition of a deeper issue, which is a
> bigger concern. (this would just "hide" it)

That seems very unlikely, but if the patch really helps, I don't even know what
to think. I'd really like to figure out *why* the patch helps before accepting
it, honestly.
> I'm as puzzled as you are, but yes, that's where it happened for some
> reason; I added traces between each call to verify this. It could be an
> application bug, though (which is also silently ignored on Windows). I'm
> curious, do you not get this behavior when you try it? (I mean the refcount
> thing)

Oh, I didn't notice the first time, but apparently the application uses its own
presenter. I guess it must release a reference to the filter when its presenter
is destroyed. Native seems to handle that well enough, and I guess we do too.

I can reproduce the extra free, but I can't reproduce a crash.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #9 from Gabriel Ivăncescu  ---
(In reply to Zebediah Figura from comment #8)
> (In reply to Gabriel Ivăncescu from comment #6)
> > Right, I was thinking the app does something weird like hook into SetParent.
> > Or perhaps the call exposes a race condition of a deeper issue, which is a
> > bigger concern. (this would just "hide" it)
>
> That seems very unlikely, but if the patch really helps, I don't even know
> what to think. I'd really like to figure out *why* the patch helps before
> accepting it, honestly.

Maybe it's due to SetWindowLongPtr rather than SetParent itself. Possible
causes I can think of that might be related:

* SetWindowLongPtr does a wineserver call.
* It sends WM_STYLECHANGING message.
* It calls a winex11 driver function, X11DRV_SetWindowStyle.

I'm speculating at this point, because I can't reproduce the crash either. But
I suspect the last one. It probably has to do with a specific Window Manager or
similar (perhaps that's why it also applies only to full-screen).

Regardless, should I send it to the mailing list for upstream inclusion? Since
it does help with the crash.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #10 from Zebediah Figura  ---
(In reply to Gabriel Ivăncescu from comment #9)
> (In reply to Zebediah Figura from comment #8)
> > (In reply to Gabriel Ivăncescu from comment #6)
> > > Right, I was thinking the app does something weird like hook into SetParent.
> > > Or perhaps the call exposes a race condition of a deeper issue, which is a
> > > bigger concern. (this would just "hide" it)
> >
> > That seems very unlikely, but if the patch really helps, I don't even know
> > what to think. I'd really like to figure out *why* the patch helps before
> > accepting it, honestly.
>
> Maybe it's due to SetWindowLongPtr rather than SetParent itself. Possible
> causes I can think of that might be related:
> - SetWindowLongPtr does a wineserver call.
> - It sends WM_STYLECHANGING message.
> - It calls a winex11 driver function, X11DRV_SetWindowStyle.
>
> I'm speculating at this point, because I can't reproduce the crash either.
> But I suspect the last one. It probably has to do with a specific Window
> Manager or similar (perhaps that's why it also applies only to full-screen).

I don't see any of this as an explanation. Why would any of these cause a crash
in application code?
> Regardless, should I send it to the mailing list for upstream inclusion?
> Since it does help with the crash.

I'm not particularly happy about accepting it without knowing why it helps,
given that it's not obviously a good idea otherwise.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #11 from Gabriel Ivăncescu  ---
Created attachment 66642
  --> https://bugs.winehq.org/attachment.cgi?id=66642
Don't unparent the window, but clear the child style.

Can you please test this patch and tell me if it also solves the crash?

I've done more tests how Windows operates here. It does not actually unparent
the window, but only clear the child style, regardless of whether it had a
parent before or not.

SetParent does actually send some messages: WM_WINDOWPOSCHANGING and
WM_WINDOWPOSCHANGED, since it hides then shows the window. I don't know if
those are the cause of this, or others. There's a lot of messages sent on Wine
that aren't on Windows, because of it:

WM_WINDOWPOSCHANGING
WM_WINDOWPOSCHANGED
WM_ACTIVATEAPP
WM_NCACTIVATE
WM_ACTIVATE
WM_SETFOCUS
WM_KILLFOCUS

On Windows 7, when releasing, it's only a custom message and the
WM_STYLECHANGING/WM_STYLECHANGED that are sent, along the destroy messages of
course.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #12 from Béla Gyebrószki  ---
(In reply to Gabriel Ivăncescu from comment #11)
> Created attachment 66642 [details]
> Don't unparent the window, but clear the child style.
>
> Can you please test this patch and tell me if it also solves the crash?

I confirm that this patch also fixes the problem for me.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #13 from Zebediah Figura  ---
I don't really want to sit here and block this bug from being fixed, but I
still don't like this solution. The test snoops deeper into implementation
details than is really justified, since the game doesn't do any hooking or
subclassing. Skipping a call to SetParent() isn't really any better than
skipping both calls. I'd rather figure out why the program is crashing for
Béla; depending on the cause, one of these patches may even be acceptable,
but...

Béla, are you using any native components or overrides?

What window manager are you using?

Can you please attach a log with
WINEDEBUG=+seh,+timestamp,+quartz,+strmbase,+win,+message,+msg,+hook?

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #14 from Béla Gyebrószki  ---
Created attachment 66647
  --> https://bugs.winehq.org/attachment.cgi?id=66647
+seh,+timestamp,+quartz,+strmbase,+win,+message,+msg,+hook (uncompressed 22 MB)

(In reply to Zebediah Figura from comment #13)
> Béla, are you using any native components or overrides?

I installed native d3dx9 and d3dcompiler_43.dll via winetricks. Apart from
that, it is a clean wineprefix I created specifically for this game to test.
The crash happens with built-in d3dx9* though. I didn't try native quartz et
al. yet.
> What window manager are you using?

I'm on Arch Linux and using XFCE 4.14 as the window manager (compositor is
disabled).
I compiled Wine from source without any particular CFLAGS added, but
--without-mingw switch was specified.

I hope the attached log is helpful and I really don't want to distract you from
more important tasks.
I much appreciate both of you for taking so much time and effort to investigate
this silly bug so far. If you have the slightest doubt that it is a bug in Wine
itself, feel free to close it as invalid. The game is playable in windowed mode
anyway.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #15 from Gabriel Ivăncescu  ---
Actually, even if we ignore this bug's peculiarities, I still think I will end
up sending the patch to the mailing list (perhaps in a slightly different
form).

Zeb, here's my reasoning as to why, perhaps this might persuade you.

First, it's obviously more correct in terms of what Windows does; even if it's
an implementation detail, it looks some applications depend on it, so we should
IMO implement it as close as possible.

Secondly, when closing Media Player Classic while playing a video, there's a
short flashing/flicker window on the top-left of the screen, which appears
because of SetParent, which also seems to cause some fast focus changes. This
patch fixes that as well, and so is a smoother experience overall.

I think that makes it worth it.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #16 from Zebediah Figura  ---
(In reply to Gabriel Ivăncescu from comment #15)
> First, it's obviously more correct in terms of what Windows does; even if
> it's an implementation detail, it looks some applications depend on it, so
> we should IMO implement it as close as possible.

Clearly the application doesn't depend on this exact code (if it's not using a
hook or subclassing or anything), so it must depend on some distant side
effect. The only question is whether that side effect can be achieved in a more
reasonable manner, which is why I really want to know what it is.
> Secondly, when closing Media Player Classic while playing a video, there's a
> short flashing/flicker window on the top-left of the screen, which appears
> because of SetParent, which also seems to cause some fast focus changes.
> This patch fixes that as well, and so is a smoother experience overall.

Is it really correct that SetParent() on a non-visible window should cause any
focus changes? That seems surprising to me. If it's not correct, I think that's
what should be fixed instead.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #17 from Gabriel Ivăncescu  ---
(In reply to Zebediah Figura from comment #16)
> Is it really correct that SetParent() on a non-visible window should cause
> any focus changes? That seems surprising to me. If it's not correct, I think
> that's what should be fixed instead.

I was under the impression that the window itself is visible, at least in MPC
(not this bug). Then, when it gets unparented, it gets moved to top-left before
it gets destroyed, because its parent is now the desktop window, hence the
flickering.

Its position may be 0,0 but that's relative to the parent, of course. When the
parent changes, so does its position. And because it becomes a top-level
window, focus changes, probably due to some X11 event.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #18 from Zebediah Figura  ---
(In reply to Gabriel Ivăncescu from comment #17)
> (In reply to Zebediah Figura from comment #16)
> > Is it really correct that SetParent() on a non-visible window should cause
> > any focus changes? That seems surprising to me. If it's not correct, I think
> > that's what should be fixed instead.
>
> I was under the impression that the window itself is visible, at least in
> MPC (not this bug). Then, when it gets unparented, it gets moved to top-left
> before it gets destroyed, because its parent is now the desktop window,
> hence the flickering.
>
> Its position may be 0,0 but that's relative to the parent, of course. When
> the parent changes, so does its position. And because it becomes a top-level
> window, focus changes, probably due to some X11 event.

Eh, I missed that you said Media Player Classic.

If I'm reading SetParent() correctly, shouldn't it keep the window at the same
position on the screen?

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #19 from Gabriel Ivăncescu  ---
> From a glance, yeah, I think so as well. Not sure why that flickering happens.

But I honestly doubt SetParent itself is wrong, it might be a Window Manager
thing or bug (sometimes new popup windows also get positioned wrongly, not only
from Wine, either). I use Compiz with XFCE.

Still this patch would help with that, so I think it's worth it, since it's
less intrusive and more correct, anyway. Just for curiosity, do you find the
testcase I wrote goes too much into implementation detail? Or the actual change
itself? I mainly added it so it doesn't regress later if someone wonders why
it's done the way it is.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

--- Comment #20 from Zebediah Figura  ---
(In reply to Gabriel Ivăncescu from comment #19)
> From a glance, yeah, I think so as well. Not sure why that flickering
> happens. But I honestly doubt SetParent itself is wrong, it might be a
> Window Manager thing or bug (sometimes new popup windows also get positioned
> wrongly, not only from Wine, either). I use Compiz with XFCE.

I wouldn't be so quick to assume that SetParent() is entirely correct.
> Still this patch would help with that, so I think it's worth it, since it's
> less intrusive and more correct, anyway. Just for curiosity, do you find the
> testcase I wrote goes too much into implementation detail? Or the actual
> change itself? I mainly added it so it doesn't regress later if someone
> wonders why it's done the way it is.

The test delves too deep into implementation details, yes.

The change is maybe fine, but should have some form of justification, and "it
breaks without the patch" isn't really enough. It isn't obvious to me this
isn't user32's fault. If it's not, I'd want to see at least a comment
describing what SetParent() does and why we can't have that.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

Béla Gyebrószki  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
      Fixed by SHA1|                            |5c8903a0a9fcef947d958c93a7f
                   |                            |a05f9faa3bcb8
             Status|NEW                         |RESOLVED

--- Comment #21 from Béla Gyebrószki  ---
Commit 5c8903a0a9fcef947d958c93a7fa05f9faa3bcb8
fixes the problem for me. 
Thank you both for your help.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #22 from Alexandre Julliard  ---
Closing bugs fixed in 5.5.

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

Michael Stefaniuc  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |5.0.x

[Bug 48732] How to Survive crashes on start (fullscreen mode)

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=48732

Michael Stefaniuc  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|5.0.x                       |---

--- Comment #23 from Michael Stefaniuc  ---
Removing the 5.0.x milestone from bug fixes included in 5.0.2.