re: BSTR caching

[Dan Kegel]

Hi Markus,
Wine development is driven by test cases checked in to the
Wine source tree.  Please extend the test cases in
dlls/oleaut32/tests to verify that oleaut32 is caching BSTRs
properly, and make sure that test passes on Windows
and is marked todo_wine.
Once we have a test case like that checked into the Wine
source tree, we'll be in better shape to accept a patch
that fixes bug 12460.
Thanks!

Help needed: BSTR cache testing

[Markus Stockhausen]

Am Freitag, den 09.10.2009, 00:27 -0700 schrieb Dan Kegel:
> Hi Markus,
> Wine development is driven by test cases checked in to the
> Wine source tree.  Please extend the test cases in
> dlls/oleaut32/tests to verify that oleaut32 is caching BSTRs
> properly, and make sure that test passes on Windows
> and is marked todo_wine.
> Once we have a test case like that checked into the Wine
> source tree, we'll be in better shape to accept a patch
> that fixes bug 12460.
> Thanks!

Already sent. Take a look here:
http://www.winehq.org/pipermail/wine-devel/2009-October/078957.html

So I will ask again with this mail if someone can verify if the test
above succeeds in various Windows versions. Just to make sure that it
only fails in Wine. If I get positive feedback I will send it do
wine-patches.

Thanks in advance.

Markus

Help needed: BSTR cache testing

[Paul Vriens]

On 10/09/2009 09:38 AM, Markus Stockhausen wrote:
> Am Freitag, den 09.10.2009, 00:27 -0700 schrieb Dan Kegel:
> > Hi Markus,
> > Wine development is driven by test cases checked in to the
> > Wine source tree.  Please extend the test cases in
> > dlls/oleaut32/tests to verify that oleaut32 is caching BSTRs
> > properly, and make sure that test passes on Windows
> > and is marked todo_wine.
> > Once we have a test case like that checked into the Wine
> > source tree, we'll be in better shape to accept a patch
> > that fixes bug 12460.
> > Thanks!
>
> Already sent. Take a look here:
> http://www.winehq.org/pipermail/wine-devel/2009-October/078957.html
>
> So I will ask again with this mail if someone can verify if the test
> above succeeds in various Windows versions. Just to make sure that it
> only fails in Wine. If I get positive feedback I will send it do
> wine-patches.
>
> Thanks in advance.
>
> Markus

Hi Markus,

Test results:

Win98SE : no failures
WinME : no failures
NT4 : no failures
W2K : no failures
XP Prof : no failures
W2K3 : no failures
Vista Ult SP2 : no failures

Help needed: BSTR cache testing

[Markus Stockhausen]

Am Freitag, den 09.10.2009, 09:56 +0200 schrieb Paul Vriens:
> On 10/09/2009 09:38 AM, Markus Stockhausen wrote:
> > Am Freitag, den 09.10.2009, 00:27 -0700 schrieb Dan Kegel:
> > > Hi Markus,
> > > Wine development is driven by test cases checked in to the
> > > Wine source tree.  Please extend the test cases in
> > > dlls/oleaut32/tests to verify that oleaut32 is caching BSTRs
> > > properly, and make sure that test passes on Windows
> > > and is marked todo_wine.
> > > Once we have a test case like that checked into the Wine
> > > source tree, we'll be in better shape to accept a patch
> > > that fixes bug 12460.
> > > Thanks!
> >
> > Already sent. Take a look here:
> > http://www.winehq.org/pipermail/wine-devel/2009-October/078957.html
> >
> > So I will ask again with this mail if someone can verify if the test
> > above succeeds in various Windows versions. Just to make sure that it
> > only fails in Wine. If I get positive feedback I will send it do
> > wine-patches.
> >
> > Thanks in advance.
> >
> > Markus
>
> Hi Markus,
>
> Test results:
>
> Win98SE : no failures
> WinME : no failures
> NT4 : no failures
> W2K : no failures
> XP Prof : no failures
> W2K3 : no failures
> Vista Ult SP2 : no failures

Fine, thanks for that.

I will sent a final version of the testcase.

Help needed: BSTR cache testing

[Dan Kegel]

Markus Stockhausen  wrote:
> > Please extend the test cases in
> > dlls/oleaut32/tests to verify that oleaut32 is caching BSTRs
> > properly, and make sure that test passes on Windows
> > and is marked todo_wine.
>
> Already sent. Take a look here:
> http://www.winehq.org/pipermail/wine-devel/2009-October/078957.html

Three problems:
You didn't mark the test todo_wine.
The name of the test is cryptic, don't you want to call
it something like test_bstr_caching()?
This change should not be included in the patch:
-  test_VarImp();
+  test_VarImp();

Other notes:
You might want to also verify the contents of the string rather
than the length, and do it right after the free as well.
Also, how did you decide on the size of sz99?
> So I will ask again with this mail if someone can verify if the test
> above succeeds in various Windows versions. Just to make sure that it
> only fails in Wine. If I get positive feedback I will send it do
> wine-patches.

Passes on vista.
Have you tested it with 'winetricks dcom98'?
Passing those two should suffice.
- Dan

Help needed: BSTR cache testing

[Markus Stockhausen]

Am Freitag, den 09.10.2009, 01:05 -0700 schrieb Dan Kegel:
> Also, how did you decide on the size of sz99?

I do not want to explain this in detail, as it is the result of looking
at internal behaviour (something that is not liked very much as I
understand now).

But to make a long story short: This will ensure that oleaut will
allocate new memory instead of reusing its caches.

Markus

Help needed: BSTR cache testing

[Paul Vriens]

On 10/09/2009 10:12 AM, Markus Stockhausen wrote:
> Am Freitag, den 09.10.2009, 01:05 -0700 schrieb Dan Kegel:
> > Also, how did you decide on the size of sz99?
>
> I do not want to explain this in detail, as it is the result of looking
> at internal behaviour (something that is not liked very much as I
> understand now).
>
> But to make a long story short: This will ensure that oleaut will
> allocate new memory instead of reusing its caches.
>
> Markus

Hi Markus,

Isn't there a way that you can change the tests to show this number (in 
some kind of loop by creating a larger second string on the go)?

Help needed: BSTR cache testing

[Markus Stockhausen]

Am Freitag, den 09.10.2009, 13:43 +0200 schrieb Paul Vriens:
> Hi Markus,
>
> Isn't there a way that you can change the tests to show this number (in 
> some kind of loop by creating a larger second string on the go)?

It simply boils down to this one and only testcase. A SysFreeString will
always preserve the contents of the heap and will put the address into
some caching freelist.

It does not matter how these freelists are organized because of two
reasons:

- The application does not know the state of oleaut freelists. Earlier
calls could have filled them up to some state.
- This leads to the second situation that the application cannot
anticipate. Will it get a new memory area or something from a freelist
when doing a SysAllocString.

Reading bugzilla i can see that errors due to missing BSTR caching are
ancient WINE companions. But only because of faulty applications. In my
opinion there are three ways to go:

- close all associated bugs and blame the developers
- take some weeks and try to explore the caching behaviour of oleaut
- implement only that bit of caching that is required for the bugs

My sent in testcase is the basis for the third solution and as my code
proposal shows I'm willing to spend some time to get it fixed.

Best regards.

Markus

Help needed: BSTR cache testing

[Paul Vriens]

On 10/09/2009 02:07 PM, Markus Stockhausen wrote:
> Am Freitag, den 09.10.2009, 13:43 +0200 schrieb Paul Vriens:
> > Hi Markus,
> >
> > Isn't there a way that you can change the tests to show this number (in
> > some kind of loop by creating a larger second string on the go)?
>
> It simply boils down to this one and only testcase. A SysFreeString will
> always preserve the contents of the heap and will put the address into
> some caching freelist.
>
> It does not matter how these freelists are organized because of two
> reasons:
> - The application does not know the state of oleaut freelists. Earlier
>
> calls could have filled them up to some state.
> - This leads to the second situation that the application cannot
>
> anticipate. Will it get a new memory area or something from a freelist
> when doing a SysAllocString.
>
> Reading bugzilla i can see that errors due to missing BSTR caching are
> ancient WINE companions. But only because of faulty applications. In my
> opinion there are three ways to go:
> - close all associated bugs and blame the developers
> - take some weeks and try to explore the caching behaviour of oleaut
> - implement only that bit of caching that is required for the bugs
>
> My sent in testcase is the basis for the third solution and as my code
> proposal shows I'm willing to spend some time to get it fixed.
>
> Best regards.
>
> Markus

Hi Markus,

My question about a modified test case came from your statement:

"I do not want to explain this in detail, as it is the result of looking
at internal behaviour (something that is not liked very much as I
understand now)."

If that sz99 (or now sz128) came from "looking at internal behaviour", 
I'm not sure if that would raise some eyebrows.

Help needed: BSTR cache testing

[Markus Stockhausen]

Am Freitag, den 09.10.2009, 14:15 +0200 schrieb Paul Vriens:
> If that sz99 (or now sz128) came from "looking at internal behaviour", 
> I'm not sure if that would raise some eyebrows.

As I said "looking at internal behaviour" are debugging messages in the
IMalloc routines of ifs.c. Simply something you cannot catch when
writing testcases only.

Help needed: BSTR cache testing

[Roderick Colenbrander]

On Fri, Oct 9, 2009 at 2:25 PM, Markus Stockhausen
 wrote:
> Am Freitag, den 09.10.2009, 14:15 +0200 schrieb Paul Vriens:
> > If that sz99 (or now sz128) came from "looking at internal behaviour",
> > I'm not sure if that would raise some eyebrows.
>
> As I said "looking at internal behaviour" are debugging messages in the
> IMalloc routines of ifs.c. Simply something you cannot catch when
> writing testcases only.

Blackbox reverse engineering is feeding test input to a function and
looking at the output. A trace of calls made by the native MS dll
while looking under Wine isn't blackbox. It is considered about the
same as looking at disassembled Microsoft code.

I'm not sure how to proceed from this though.

Roderick

Help needed: BSTR cache testing

[Michael Stefaniuc]

Roderick Colenbrander wrote:
> On Fri, Oct 9, 2009 at 2:25 PM, Markus Stockhausen
>  wrote:
> > Am Freitag, den 09.10.2009, 14:15 +0200 schrieb Paul Vriens:
> > > If that sz99 (or now sz128) came from "looking at internal behaviour",
> > > I'm not sure if that would raise some eyebrows.
> >
> > As I said "looking at internal behaviour" are debugging messages in the
> > IMalloc routines of ifs.c. Simply something you cannot catch when
> > writing testcases only.
>
> Blackbox reverse engineering is feeding test input to a function and
> looking at the output. A trace of calls made by the native MS dll
> while looking under Wine isn't blackbox. It is considered about the
> same as looking at disassembled Microsoft code.
>
> I'm not sure how to proceed from this though.

He probably can still write the test case; I really doubt that Alexandre
will accept a patch to fix the issue. Somebody else would have to do it
based on Markus' description.

bye
    michael

Help needed: BSTR cache testing

[Alexandre Julliard]

Michael Stefaniuc  writes:
> Roderick Colenbrander wrote:
> > Blackbox reverse engineering is feeding test input to a function and
> > looking at the output. A trace of calls made by the native MS dll
> > while looking under Wine isn't blackbox. It is considered about the
> > same as looking at disassembled Microsoft code.
> >
> > I'm not sure how to proceed from this though.
>
> He probably can still write the test case; I really doubt that Alexandre
> will accept a patch to fix the issue. Somebody else would have to do it
> based on Markus' description.

If the test case is based on observing internal behavior that's not
acceptable either. Even if someone else fixes it, the test would force
the fixer to replicate internal details.

BSTR & Heap - a restart

[Markus Stockhausen]

Am Freitag, den 09.10.2009, 15:44 +0200 schrieb Alexandre Julliard:
> If the test case is based on observing internal behavior that's not
> acceptable either. Even if someone else fixes it, the test would force
> the fixer to replicate internal details.

Hi,

I can see all of your concerns and the message of what not to do is
understood. If my enthusiasm to reveal the reason for the error got me
too far I'm sorry and this is my official excuse. I'll try to rewind any
knowledge to the point where it all started.

And that was here:

The original goal of my journey was to understand why BSTR memory
structures constantly survived a SysFree cycle in Windows while they got
overwritten for some case in Wine. From what I read in bugzilla the
argumentation always explained, that this comes from heap management.
The tip to solve this with native oleaut, further links to MSDN
documentation about SetOaNoCache() and the KB article
http://support.microsoft.com/kb/937360 led the way that heap was not
freed upon calling SysFreeString.

With this bit of knowledge I ask if the following testcase will be
valid. It should not care about any code inside the "black box":

...
  for (i=10;i<100;i++) {
    s1 = SysAllocStringLen(sz128,i);
    SysFreeString(s1);

HeapAlloc(GetProcessHeap(),0,10);

s2 = SysAllocStringLen(sz128,i);
    SysFreeString(s1);
    ok(s1==s2, "got a new memory address\n");
  }
  ...

The only development goal this testcase has, is to request a function
that provides the same memory address in two subsequent Alloc/Dealloc
calls with the same string size. Noone should be forced to implement
something that goes anywhere close to native implementation.

A start maybe.

Help needed: BSTR cache testing

[Markus Stockhausen]

Am Freitag, den 09.10.2009, 14:15 +0200 schrieb Paul Vriens:
> If that sz99 (or now sz128) came from "looking at internal behaviour", 
> I'm not sure if that would raise some eyebrows.

... and I simply allocate a "very large" string to ensure that no reuse
of the just freed small memory portion can occur.

Markus

Help needed: BSTR cache testing

[Paul Vriens]

On 10/09/2009 02:15 PM, Paul Vriens wrote:
> On 10/09/2009 02:07 PM, Markus Stockhausen wrote:
> > Am Freitag, den 09.10.2009, 13:43 +0200 schrieb Paul Vriens:
> > > Hi Markus,
> > >
> > > Isn't there a way that you can change the tests to show this number (in
> > > some kind of loop by creating a larger second string on the go)?
> >
> > It simply boils down to this one and only testcase. A SysFreeString will
> > always preserve the contents of the heap and will put the address into
> > some caching freelist.
> >
> > It does not matter how these freelists are organized because of two
> > reasons:
> > - The application does not know the state of oleaut freelists. Earlier
> >
> > calls could have filled them up to some state.
> > - This leads to the second situation that the application cannot
> >
> > anticipate. Will it get a new memory area or something from a freelist
> > when doing a SysAllocString.
> >
> > Reading bugzilla i can see that errors due to missing BSTR caching are
> > ancient WINE companions. But only because of faulty applications. In my
> > opinion there are three ways to go:
> > - close all associated bugs and blame the developers
> > - take some weeks and try to explore the caching behaviour of oleaut
> > - implement only that bit of caching that is required for the bugs
> >
> > My sent in testcase is the basis for the third solution and as my code
> > proposal shows I'm willing to spend some time to get it fixed.
> >
> > Best regards.
> >
> > Markus
>
> Hi Markus,
>
> My question about a modified test case came from your statement:
>
> "I do not want to explain this in detail, as it is the result of looking
> at internal behaviour (something that is not liked very much as I
> understand now)."
>
> If that sz99 (or now sz128) came from "looking at internal behaviour",
> I'm not sure if that would raise some eyebrows.

Ok, what about something like the attached testcase?

This one succeeds on W2K3 (didn't test on other boxes yet).

Does it prove anything?