[PATCH] wmp/tests: Stop crash when setting a media option.
Fixes a regression of 9a89db9897eec5b3e0e7e6e5e33f7261f503f765 Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/wmp/tests/oleobj.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/wmp/tests/oleobj.c b/dlls/wmp/tests/oleobj.c index e4c2294436..58ba6081b1 100644 --- a/dlls/wmp/tests/oleobj.c +++ b/dlls/wmp/tests/oleobj.c @@ -970,10 +970,13 @@ static void test_wmp_ifaces(IOleObject *oleobj) ok(0 == lstrcmpW(url, filename), "%s != %s\n", wine_dbgstr_w(url), wine_dbgstr_w(filename)); SysFreeString(url); + /* Keep a reference since we are using the same media object, stops a crash (w2003std, wvistau64) */ + IWMPMedia_AddRef(media); SET_EXPECT(GetContainer); hres = IWMPPlayer4_put_currentMedia(player4, media); ok(hres == S_OK, "put_currentMedia failed: %08x\n", hres); todo_wine CHECK_CALLED_OR_BROKEN(GetContainer); + IWMPMedia_Release(media); IWMPMedia_Release(media); -- 2.16.2
Hi Alistair, diff --git a/dlls/wmp/tests/oleobj.c b/dlls/wmp/tests/oleobj.c index e4c2294436..58ba6081b1 100644 --- a/dlls/wmp/tests/oleobj.c +++ b/dlls/wmp/tests/oleobj.c @@ -970,10 +970,13 @@ static void test_wmp_ifaces(IOleObject *oleobj) ok(0 == lstrcmpW(url, filename), "%s != %s\n", wine_dbgstr_w(url), wine_dbgstr_w(filename)); SysFreeString(url); + /* Keep a reference since we are using the same media object, stops a crash (w2003std, wvistau64) */ + IWMPMedia_AddRef(media); SET_EXPECT(GetContainer); hres = IWMPPlayer4_put_currentMedia(player4, media); ok(hres == S_OK, "put_currentMedia failed: %08x\n", hres); todo_wine CHECK_CALLED_OR_BROKEN(GetContainer); + IWMPMedia_Release(media); IWMPMedia_Release(media); Note that we already hold reference to media interface, so this should not be needed. If the patch helps, it means that we have a problem with ref counting somewhere else or Windows implementation is somehow broken. Given that the problem is visible only on one platform, I suspect the later. Maybe we should skip WMP tests on this platform? Thanks, Jacek
Hi, While running your changed tests on Windows, 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=37245 Your paranoid android. === w2003std (32 bit oleobj) === The task timed out
On 29/03/18 08:46, Jacek Caban wrote:
Hi Alistair,
diff --git a/dlls/wmp/tests/oleobj.c b/dlls/wmp/tests/oleobj.c index e4c2294436..58ba6081b1 100644 --- a/dlls/wmp/tests/oleobj.c +++ b/dlls/wmp/tests/oleobj.c @@ -970,10 +970,13 @@ static void test_wmp_ifaces(IOleObject *oleobj) ok(0 == lstrcmpW(url, filename), "%s != %s\n", wine_dbgstr_w(url), wine_dbgstr_w(filename)); SysFreeString(url);
+ /* Keep a reference since we are using the same media object, stops a crash (w2003std, wvistau64) */ + IWMPMedia_AddRef(media); SET_EXPECT(GetContainer); hres = IWMPPlayer4_put_currentMedia(player4, media); ok(hres == S_OK, "put_currentMedia failed: %08x\n", hres); todo_wine CHECK_CALLED_OR_BROKEN(GetContainer); + IWMPMedia_Release(media);
IWMPMedia_Release(media);
Note that we already hold reference to media interface, so this should not be needed. If the patch helps, it means that we have a problem with ref counting somewhere else or Windows implementation is somehow broken. Given that the problem is visible only on one platform, I suspect the later. Maybe we should skip WMP tests on this platform?
Thanks, Jacek
For what it's worth, it seems to me that it'd be more desirable to work around a Windows bug rather than skip the test entirely.
On 03/29/2018 04:36 PM, Zebediah Figura wrote:
For what it's worth, it seems to me that it'd be more desirable to work around a Windows bug rather than skip the test entirely.
I'm not sure I agree. If we suspect that it messes ref counting, you can't be sure that the workaround will make it behave correctly, all you know is that it happens to avoid a crash. It's likely that the library may be in a wrong state under the hood. If we hit another crash on this platform, we will need to invest more time on investigating it for questionable benefit. Jacek
On 29/03/18 10:16, Jacek Caban wrote:
On 03/29/2018 04:36 PM, Zebediah Figura wrote:
For what it's worth, it seems to me that it'd be more desirable to work around a Windows bug rather than skip the test entirely.
I'm not sure I agree. If we suspect that it messes ref counting, you can't be sure that the workaround will make it behave correctly, all you know is that it happens to avoid a crash. It's likely that the library may be in a wrong state under the hood. If we hit another crash on this platform, we will need to invest more time on investigating it for questionable benefit.
Jacek
I see; that's a fair concern.
participants (4)
-
Alistair Leslie-Hughes -
Jacek Caban -
Marvin -
Zebediah Figura