[Bug 20711] New: Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Summary: Flatout2 demo crashes on exit
           Product: Wine
           Version: 1.1.32
          Platform: PC
               URL: http://www.gamershell.com/download_16702.shtml
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: -unknown
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]

Created an attachment (id=24746)
 --> (http://bugs.winehq.org/attachment.cgi?id=24746)
Console diff between crashing and non-crashing (reverted) wine-1.1.33

Flatout2 demo crashes on exit and this is caused by Markus Stockhausen's
commit...

1. Please consider KEYWORDS: +REGRESSION, +DOWNLOAD

2. Did a regression test between 1.1.31 and 1.1.32:

70ae1ba4b552be732a5f627f62ec3521c4234988 is first bad commit
commit 70ae1ba4b552be732a5f627f62ec3521c4234988
Author: Markus Stockhausen 
Date:   Wed Oct 14 19:47:45 2009 +0200

dinput8: Ensure balance of CoInitialize/CoUninitialize.

:040000 040000 dc187bed5354bf26855c8698f49f66b8c93aa5ce
3ff8bf3680dd13dde00639352dbd026f94db1861 M      dlls

3. No other bug report suffers from this commit.

4. Revert of this patch on top of wine-1.1.33 makes that problem goes away.

5. I'm not able to add author ([e-mail]) to CC!

--private keyword: bisected

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Jeff Zaroyko  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |download, regression

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Dan Kegel  changed:

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

--- Comment #1 from Dan Kegel   2009-11-15 12:16:55 ---
patch sent by rob, 
http://www.winehq.org/pipermail/wine-patches/2009-November/081407.html

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #2 from Wylda   2009-11-17 02:44:01 ---
(In reply to comment #1)
> patch sent by rob, 
> http://www.winehq.org/pipermail/wine-patches/2009-November/081407.html

Hi Dan, thank you for letting me now.

Unfortunately the patch did NOT fix the problem in FlatOut2 nor in
FlatOut2_demo. I assume that the mentioned patch is in current git
(wine-1.1.33-88-gd51b4e1) as:

http://source.winehq.org/git/wine.git/?a=commit;h=1f3a14e7676c298f5c2de648da...

and current git crash like was mentioned in comment #0. And my programming
skill (slightly behind "Hello world!") tells me, that it really could not fix
that.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #3 from Rob Shearman   2009-11-17 16:06:04 ---
Sorry, the bug that patch was fixed was 15755. I'm not sure how I managed to
get the bug id wrong.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Vincent Povirk  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|-unknown                    |directx-dinput

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #4 from Wylda   2009-12-12 02:59:29 ---

Still not fixed in wine-1.1.34-309-g9352509.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #5 from Wylda   2009-12-26 16:34:24 ---

Still not fixed in wine-1.1.35-180-g20a50f3.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Lauri Niskanen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1

--- Comment #6 from Lauri Niskanen   2010-03-21 01:49:11 ---
*** This bug has been confirmed by popular vote. ***

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #7 from Wylda   2010-03-21 02:31:04 ---

This bisected regression is still not fixed in wine-1.1.41. Reverting the
commit on top of 1.1.41 makes that problem go away. Can anyone add the author
to CC, please?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #8 from Dan Kegel   2010-03-21 07:43:33 ---
No, I don't think he has a bugzilla account.  But I will forward this to him.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Markus  changed:

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

--- Comment #9 from Markus   2010-03-21 15:32:10 ---
Hi there.

Thanks Dan for your mail. The patch was applied because auf bug
http://bugs.winehq.org/show_bug.cgi?id=10970

At the moment I have no Wine development environment to test this out. But to
make it short. The patch was intended to fix the following buggy behaviour in
Crazy Taxi Demo:

- Application called CoInitialize 
- Application called DirectInput8Create
- DirectInput8Create called CoInitialize and got error code RPC_E_CHANGED_MODE
- The subsequent call to CoUnitialize reverted CoInitialize of application

When thinking about it the best way should be to change the code in the
following way. This would only call CoUninitialize if the previous
CoInitiailize was successful.

...
/*  ensure balance of calls */
if(hrCo == S_OK)
  CoUninitialize();
...

Hopefully someone can give it a try. The solution should be tested with both
applications.

Best regards and thanks in advance.

Markus

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #10 from Markus   2010-03-21 16:17:20 ---
Hm,

please ignore the solution of my last answer. To be more specific: In my
initial Crazy Taxi error analysis I thought it was very strange for
DirectInput8Create to call a function that could already have been called by
the application before. So the idea of the fix was to call the UnInitialize
function only if Initialize really succeded (was called by us). Now we have a
dilemma in DirectInput8Create:

1. According to Microsoft documentation: ... A thread must call CoUninitialize
once for each successful call it has made to the CoInitialize or CoInitializeEx
function, including any call that returns S_FALSE ...

2. Crazy Taxi: CoUninitialize must not be called if CoInitialize returned
RPC_E_CHANGED_MODE. Otherwise we "close" the applications Initialize call. This
conforms to the documentation

3. Flatout Demo: CoUnintialize must be called although CoInitialize returned
something else than S_OK or S_FALSE.

Two questions arise:

1. What was the result of CoInitialize?

2. The application depends on our internal UnInitialize call. Do we have other
unbalanced calls of Init/CoUnInit in that app?

Best regards.

Markus

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #11 from Markus   2010-03-23 15:59:48 ---
Finally had some time to narrow down the cause of the error.

With the patch CoUninitialize is called es expected at the end of the
application and not at the beginning during initialization of dinput8.dll. A
call leads to a subcall of apartment_freeunusedlibraries(). In this function
the system wants to unload dinput8.dll and crashes in the following line.

if (entry->dll->DllCanUnloadNow && (entry->dll->DllCanUnloadNow() == S_OK))

DllCanUnloadNow of dinput8 for some reason points to nowhere.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #12 from Markus   2010-03-25 07:23:56 ---
To make it short.

the application produces the following call sequence:

- LoadLibrary(dinput8.dll)
  - First reference to this library
- DirectInput8Create()
  - This will nail a second reference to dinput8.dll due to CoCreateInstance
  - compobj will store a separated link to dinput8.DllCanUnloadNow()

- game runs.

- FreeLibrary(Handle_To_Dinput8)
  - destroy first reference to dinput8.dll
- FreeLibrary(Handle_To_Dinput8)
  - This will destroy the last reference to dinput8.dll 
  - dinput8.dll will be unloaded
- CoUninitialize
  - Wants to do its own cleanup of libraries.
  - Therefore calls stored link of dinput8.DllCanUnloadNow()

This fails as the library is already unloaded from memory.

Best regards.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #13 from Henri Verbeet   2010-03-25 07:34:25 ---
(In reply to comment #12)
> - FreeLibrary(Handle_To_Dinput8)
> - destroy first reference to dinput8.dll
> - FreeLibrary(Handle_To_Dinput8)
> - This will destroy the last reference to dinput8.dll

Where does that second FreeLibrary() come from?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #14 from Markus   2010-03-25 07:40:24 ---
Sorry for not pointing that out. Its the application itself - not Wine. I
checked both call stacks. Seems to be time for some comparison tests with
native ole32.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #15 from Markus   2010-03-25 10:16:08 ---
No luck with native ole32. It does not allow wine dinput8.dll to call
CoInitialize.

err:dinput:DirectInput8Create CoCreateInstance failed with hr = -2147024776;
Try running wineprefixcreate to fix it.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #16 from Wylda   2010-03-28 08:35:46 ---

Hi Markus, any progres/patch which i could possibly test?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #17 from Markus   2010-03-28 11:47:16 ---
Hi Wylda,

at the moment I'm quite out of wine development (nothing made for half a year).
But the only solution I see is to implement a pessimistic cleanup solution in
compobj.c to fix an application bug. This would need something like this.

Create a new reference to the loaded (or maybe in between freed library).
Create a new link to the DllCanUnloadNow function of it and call it afterwards.
Additionally we must call a second FreeLibrary() in the same function.

static void apartment_freeunusedlibraries()
  ...
  HANDLE hLibrary; 
  ...
  hLibrary = LoadLibraryExW(entry->dll->library_name, 0,
                            LOAD_WITH_ALTERED_SEARCH_PATH);
  if (hLibrary)
  {
    entry->dll->DllCanUnloadNow = 
      (void *)GetProcAddress(hLibrary, "DllCanUnloadNow");
  }
  if (entry->dll->DllCanUnloadNow && (entry->dll->DllCanUnloadNow() == S_OK)) 
  {
  ...  
  else if (entry->unload_time)
            entry->unload_time = 0;

FreeLibrary(hLibrary);
  ...

This is only an idea. Maybe someone with deeper insight into compobj.c can give
a statement if that can be a valid solution. In between you could have a test.
A quick and dirty implementation and compilation fixed the error. But I made no
other tests.

Best regards.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #18 from Henri Verbeet   2010-03-28 12:38:15 ---
Can't you just use GetModuleHandle() to check if the library is still loaded?
The important thing is to find out how this is supposed to behave though.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #19 from Markus   2010-03-28 14:46:42 ---
I don't think that GetModuleHandle will work because the library in doubt
(dinput8.dll) is not loaded at the moment we issue the request. At the moment I
only rely on MS documentation:

... The module must have been loaded by the calling process. ... The
GetModuleHandle function returns a handle to a mapped module without
incrementing its reference count ...

So how could we access a library without having a single reference?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #20 from Henri Verbeet   2010-03-28 15:03:08 ---
(In reply to comment #19)
> I don't think that GetModuleHandle will work because the library in doubt
> (dinput8.dll) is not loaded at the moment we issue the request. At the moment I
> only rely on MS documentation:
>
> ... The module must have been loaded by the calling process. ... The
> GetModuleHandle function returns a handle to a mapped module without
> incrementing its reference count ...
>
> So how could we access a library without having a single reference?

Do we really need to unload the library if it isn't loaded anymore?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #21 from Markus   2010-03-29 01:13:18 ---
> Do we really need to unload the library if it isn't loaded anymore?

Finally got the idea. Nevertheless using GetModuleHandle leaves a bad taste. We
cannot rely on the library existence between the two calls in a multithreading
environment. To be sure one needs to call GetModuleHandleEx using the increment
counter. So we once again have the FreeLibrary Call.

Do you think this would be a possible solution?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #22 from Henri Verbeet   2010-03-29 06:36:49 ---
(In reply to comment #21)
> Finally got the idea. Nevertheless using GetModuleHandle leaves a bad taste. We
> cannot rely on the library existence between the two calls in a multithreading
> environment. To be sure one needs to call GetModuleHandleEx using the increment
> counter. So we once again have the FreeLibrary Call.

Well yeah, but we're already keeping a reference to the module, and that didn't
stop the application from freeing it either.
> Do you think this would be a possible solution?

Sure, needs tests though.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #23 from Markus   2010-03-30 08:52:19 ---
Another try with GetModuleHandleEx. I checked it with balanced an unbalanced
calls to LoadLibrary/FreeLibrary. In the normal and the error case it should
work as expected.

@Wylda: Maybe you could test the following patch in compobj.c. Sorry for not
having a git-diff so changes only marked with an asterisk

static void apartment_freeunusedlibraries(struct apartment *apt, DWORD delay)
{
    struct apartment_loaded_dll *entry, *next;
*   BOOL res;

...

*   res = GetModuleHandleExW(0, entry->dll->library_name,
&entry->dll->library);
*   if (   res 
        && entry->dll->DllCanUnloadNow 
        && (entry->dll->DllCanUnloadNow() == S_OK))
    {

...

}
    else if (entry->unload_time)
            entry->unload_time = 0;

*   if (res) FreeLibrary(entry->dll->library);

@Henri: Do we need tastcases for that?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #24 from Henri Verbeet   2010-03-30 09:10:03 ---
Doesn't that still try to call FreeLibrary() from apartment_release() ->
COMPOBJ_DllList_ReleaseRef() though?

(In reply to comment #23)
> @Henri: Do we need tastcases for that?

You'll probably need to show at least that calling CoUnintialize when the
library is already unloaded isn't supposed to just crash.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #25 from Markus   2010-03-30 09:22:40 ---
> Doesn't that still try to call FreeLibrary() from apartment_release() ->
> COMPOBJ_DllList_ReleaseRef() though?

GetModuleHandleExW(0, ... increases the reference counter and ensures that the
library is still loaded. So we can survive another call to FreeLibrary until
DllCanUnloadNow is called. In this case we have to call FreeLibrary twice. One
can be done in the original position.

Thanks to your comment I have seen another issue. COMPOBJ_DllList_ReleaseRef()
releases the memory of entry. so I need a separate variable for that.
Correction should be as follows:

static void apartment_freeunusedlibraries(struct apartment *apt, DWORD delay)
{
    struct apartment_loaded_dll *entry, *next;
*   HANDLE hLibrary;
*   BOOL res;

...

*   res = GetModuleHandleExW(0, entry->dll->library_name, &hLibrary);
*   if (   res 
        && entry->dll->DllCanUnloadNow 
        && (entry->dll->DllCanUnloadNow() == S_OK))
    {

...

}
    else if (entry->unload_time)
            entry->unload_time = 0;

*   if (res) FreeLibrary(hLibrary);

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #26 from Henri Verbeet   2010-03-30 09:30:34 ---
(In reply to comment #25)
> > Doesn't that still try to call FreeLibrary() from apartment_release() ->
> > COMPOBJ_DllList_ReleaseRef() though?
>
> GetModuleHandleExW(0, ... increases the reference counter and ensures that the
> library is still loaded. So we can survive another call to FreeLibrary until
> DllCanUnloadNow is called. In this case we have to call FreeLibrary twice. One
> can be done in the original position.

Yes, but that's only if the library was still loaded. If it's already unloaded
we're passing an invalid handle to FreeLibrary().

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #27 from Markus   2010-03-30 09:41:26 ---
> Yes, but that's only if the library was still loaded. If it's already unloaded
> we're passing an invalid handle to FreeLibrary().

If it was already unloaded before, the result of GetHandle will be FALSE
according to my tests.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #28 from Henri Verbeet   2010-03-30 09:57:33 ---
My bad, didn't notice the "free_entry" parameter to
COMPOBJ_DllList_ReleaseRef() being FALSE.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #29 from Wylda   2010-06-19 22:38:54 ---

Still present in wine-1.2-rc4. Unfortunately i'm not a programmer, so i'm not
able to melt your pseudo code(?) into a workable patch.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #30 from joaopa   2010-06-19 23:12:03 ---
Created an attachment (id=28999)
 --> (http://bugs.winehq.org/attachment.cgi?id=28999)
Patch for testing

Here is a diff git of Markus' patch. I had to change HANDLE to HMODULE to make
it compile. I don't know if this change is correct though...

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

joaopa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28999|1                           |0
           is patch|                            |
  Attachment #28999|0                           |1
        is obsolete|                            |

--- Comment #31 from joaopa   2010-06-19 23:14:52 ---
(From update of attachment 28999)
oops. the wrong one.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #32 from joaopa   2010-06-19 23:15:40 ---
Created an attachment (id=29000)
 --> (http://bugs.winehq.org/attachment.cgi?id=29000)
Patch for testing

The good one now.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Wylda  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #33 from Wylda   2010-06-19 23:44:01 ---
> Patch for testing
>
> The good one now.

This one is really good. Crash on exit gone :-) Because both full and demo were
affected, i test them both. No more crashes when patch applied.

Could you or Marcus pass the patch further? Thank you!

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

--- Comment #34 from joaopa   2010-06-20 01:58:19 ---
Created an attachment (id=29007)
 --> (http://bugs.winehq.org/attachment.cgi?id=29007)
Test for Henri's concern

Would this patch be acceptable to deal with Henri's concern about 
"You'll probably need to show at least that calling CoUnintialize when the
library is already unloaded isn't supposed to just crash." ?

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

GyB  changed:

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

--- Comment #35 from GyB  2011-07-23 00:25:47 CDT ---
Flatout 2 demo exits cleanly in Wine-1.3.25 (as well as in 1.3.24).

Fixed by
http://source.winehq.org/git/wine.git/commit/cf073c8ad43f1f918726062b1e17edd...

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Dan Kegel  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #36 from Dan Kegel  2011-07-23 10:37:24 CDT ---
Reported fixed.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Alexandre Julliard  changed:

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

--- Comment #37 from Alexandre Julliard  2011-08-05 12:39:02 CDT ---
Closing bugs fixed in 1.3.26.

[Bug 20711] Flatout2 demo crashes on exit

http://bugs.winehq.org/show_bug.cgi?id=20711

Wylda  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |cf073c8ad43f1f918726062b1e1
                   |                            |7edd0fe7efe60
    Regression SHA1|                            |70ae1ba4b552be732a5f627f62e
                   |                            |c3521c4234988