[Bug 48732] New: How to Survive crashes on start (fullscreen mode)
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: wine-bugs(a)winehq.org Reporter: gyebro69(a)gmail.com CC: gabrielopcode(a)gmail.com 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 Zebediah Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12(a)gmail.com --- Comment #1 from Zebediah Figura <z.figura12(a)gmail.com> --- 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? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #2 from Béla Gyebrószki <gyebro69(a)gmail.com> --- 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #3 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #4 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #5 from Zebediah Figura <z.figura12(a)gmail.com> --- (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? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #6 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- (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) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #7 from Béla Gyebrószki <gyebro69(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #8 from Zebediah Figura <z.figura12(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #9 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #10 from Zebediah Figura <z.figura12(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #11 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #12 from Béla Gyebrószki <gyebro69(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #13 from Zebediah Figura <z.figura12(a)gmail.com> --- 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? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #14 from Béla Gyebrószki <gyebro69(a)gmail.com> --- 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #15 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #16 from Zebediah Figura <z.figura12(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #17 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #18 from Zebediah Figura <z.figura12(a)gmail.com> --- (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? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #19 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 --- Comment #20 from Zebediah Figura <z.figura12(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 Béla Gyebrószki <gyebro69(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Fixed by SHA1| |5c8903a0a9fcef947d958c93a7f | |a05f9faa3bcb8 Status|NEW |RESOLVED --- Comment #21 from Béla Gyebrószki <gyebro69(a)gmail.com> --- Commit 5c8903a0a9fcef947d958c93a7fa05f9faa3bcb8 fixes the problem for me. Thank you both for your help. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #22 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 5.5. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |5.0.x -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=48732 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|5.0.x |--- --- Comment #23 from Michael Stefaniuc <mstefani(a)winehq.org> --- Removing the 5.0.x milestone from bug fixes included in 5.0.2. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla