DCOM (was Re: Fix CoMarshalInterThreadInterfaceInStream)

[Robert Shearman]

On Sat, 2004-05-29 at 00:44, Mike Hearn wrote:
> Of course calling CoMarshalInterface in this case doesn't actually do
> much, but if it helps InstallShield OK.

Actually, it does. It appears as though the interface marshaling
functions are all compatible with each other on Windows (even though
MSDN says that they aren't and then says they use the same code as the
others). Presumably InstallShield was quite understandably passing the
IStream from on the two functions that were fixed into one of the other
(supposedly compatible) functions and then that function failed because
it wasn't the right format.
Even on Wine, those other functions, such as CoMarshalInterface do
actually attempt to get a marshaller or just use the standard one and
> If you are interested in this
> stuff I'd say trying to merge the TransGaming patch would be a good
> start.

I assume you mean this:
http://www.winehq.com/hypermail/wine-devel/2003/07/att-0179/01-ole.diff__cha...

I've had a look at what is in there. The way I interpret it, there are a
couple of sections:
o Auto-generated marshaling code -- this is fine to be included in Wine
and could probably be done so at any point
o Converting COM (ole32) to using RPC -- as you can see by the comments
in the code, this isn't really done through the correct interfaces yet
(and this is stuff that you only want implement once) and hence I'm not
confident that the related stuff is done correctly. There are useful
bits, but I doubt I will use all of that code.
o Rewriting the Type Library marshaller -- this isn't much better than
the code we have at the moment. Again, this isn't done through the
correct interfaces - I would prefer to use the stubless RPC to do the
same, but it would probably mean writing half of MIDL into RPCRT4.
o Miscellaneous header and bug fixes -- I'll check whether they are
included in Wine already and check whether they are correct or
workarounds for something in the patch
> This includes a lot of what you've done lately such as proper
> apartments, proper inter-thread marshalling and so on. Otherwise the
> codebase drift is going to get even worse :(

I would prefer it if we could all work on the same codebase, but as you
know the licensing restrictions largely prevent that. I hope (wish?)
that Transgaming will use more and more LGPL modules from Wine and only
develop separately the ones that are core to their business. They have
been kind of informally doing that by submitting their RPC / OLE patches
back to Wine, but they are working from the ReWind codebase, not Wine
and they are not in sync.

Rob

P.S. I am currently putting together a plan for tackling RPC / DCOM and
will present it to wine-devel soon.

[Mike Hearn]

On Tue, 2004-06-01 at 16:56 +0100, Robert Shearman wrote:
> Actually, it does. It appears as though the interface marshaling
> functions are all compatible with each other on Windows (even though
> MSDN says that they aren't and then says they use the same code as the
> others). Presumably InstallShield was quite understandably passing the
> IStream from on the two functions that were fixed into one of the other
> (supposedly compatible) functions and then that function failed because
> it wasn't the right format.
> Even on Wine, those other functions, such as CoMarshalInterface do
> actually attempt to get a marshaller or just use the standard one and

and ... ?

You mean it correctly does inter-thread marshalling? Last time I checked
WineHQ didn't have any code to do this properly, we just handed raw
pointers around. This is what Oves patch adds.

The current code for one uses pipes for everything rather than going via
RPC, even in the inter-thread case. COM_AptWndProc should be processing
RPC messages.
> I assume you mean this:
> http://www.winehq.com/hypermail/wine-devel/2003/07/att-0179/01-ole.diff__cha...

http://www.winehq.org/hypermail/wine-patches/2003/12/att-0134/01-world-of-dc...

That one is somewhat merged with winehq but yes it's the same patch
pretty much.
> I've had a look at what is in there. The way I interpret it, there are a
> couple of sections:
> o Auto-generated marshaling code -- this is fine to be included in Wine
> and could probably be done so at any point

Last time I checked Alexandre saw using MIDL code in Wine as a temporary
hack, he may require us to get widl up to speed to produce this code.
> o Converting COM (ole32) to using RPC -- as you can see by the comments
> in the code, this isn't really done through the correct interfaces yet
> (and this is stuff that you only want implement once) and hence I'm not
> confident that the related stuff is done correctly. There are useful
> bits, but I doubt I will use all of that code.

It's not so much converting as adding the necessary code. Remember that
when this patch was written:

- there were no apartments
- there was no typelib marshaller

In particular I think our proxy/stub code currently isn't up to doing
inter-thread marshalling.
> o Rewriting the Type Library marshaller -- this isn't much better than
> the code we have at the moment. Again, this isn't done through the
> correct interfaces - I would prefer to use the stubless RPC to do the
> same, but it would probably mean writing half of MIDL into RPCRT4.

Remember when Ove wrote this patch the WineHQ typelib marshaller either
wasn't written, or wasn't X11 licensed, I don't remember which.

And yeah, iirc the Windows typelib marshaller generates format strings.
In fact isn't there an ITypeLib::GetMOPs method or somesuch?

The main reason I'm unhappy about the idea of reimplementing what's in
the TransGaming patch is that this patch works, I have merged it with
WineHQ and used it with some fairly complex scenarios (think lots of
inter-thread IDispatch marshalling happening simultaneously). We still
need to write test cases, but the complexity of DCOM is staggering and
the more code we re-use the more time we have for documenting it,
writing tests, filling out the details and so on.

Still, I look forward to seeing your plan for this.

thanks -mike

[Robert Shearman]

On Tue, 2004-06-01 at 17:27, Mike Hearn wrote:
> On Tue, 2004-06-01 at 16:56 +0100, Robert Shearman wrote:
> > Even on Wine, those other functions, such as CoMarshalInterface do
> > actually attempt to get a marshaller or just use the standard one and
>
> and ... ?
>
> You mean it correctly does inter-thread marshalling? Last time I checked
> WineHQ didn't have any code to do this properly, we just handed raw
> pointers around. This is what Oves patch adds.

No, we actually do proper marshalling. This is either via the generated
*_p.c files or via the type library marshaller. In the *_p.c case, the
proxies are generated in dlls/rpcrt4/cproxy.c and the stubs in cstub.c
The last bit of pointer passing was the bit I just removed.
> The current code for one uses pipes for everything rather than going via
> RPC, even in the inter-thread case. COM_AptWndProc should be processing
> RPC messages.

Yes, that is correct.
> > I've had a look at what is in there. The way I interpret it, there are a
> > couple of sections:
> > o Auto-generated marshaling code -- this is fine to be included in Wine
> > and could probably be done so at any point
>
> Last time I checked Alexandre saw using MIDL code in Wine as a temporary
> hack, he may require us to get widl up to speed to produce this code.

Yes, there is a lot of work to do.
> > o Converting COM (ole32) to using RPC -- as you can see by the comments
> > in the code, this isn't really done through the correct interfaces yet
> > (and this is stuff that you only want implement once) and hence I'm not
> > confident that the related stuff is done correctly. There are useful
> > bits, but I doubt I will use all of that code.
>
> It's not so much converting as adding the necessary code.

I meant converting as in converting it from using its own named pipes
(and IRpc* interfaces) to using proper RPC ones. Obviously there would
need to be extra code to handle this (and also a lot of code removed).
> In particular I think our proxy/stub code currently isn't up to doing
> inter-thread marshalling.

What makes you think that?
> And yeah, iirc the Windows typelib marshaller generates format strings.
> In fact isn't there an ITypeLib::GetMOPs method or somesuch?

Yes, you're right. I had initially thought that it was all done in
RPCRT4, but it looks like it is spread out across both libraries.
> The main reason I'm unhappy about the idea of reimplementing what's in
> the TransGaming patch is that this patch works, I have merged it with
> WineHQ and used it with some fairly complex scenarios (think lots of
> inter-thread IDispatch marshalling happening simultaneously).

I'll submit a patch containing the autogenerated stuff since we need
that anyway. Like I said, the proxy code isn't 100% correct. It may be
fine for Crossover or for Wine temporarily however.
> We still
> need to write test cases, but the complexity of DCOM is staggering and
> the more code we re-use the more time we have for documenting it,
> writing tests, filling out the details and so on.

I don't understand a lot of the apartment, OXID and OID code that is in
the patch so I would have a hard time documenting it.
I have a fairly large RPC test suite, but I have yet to really delve
into the DCOM side of things.

Rob

[Mike Hearn]

On Tue, 2004-06-01 at 19:26 +0100, Robert Shearman wrote:
> No, we actually do proper marshalling. This is either via the generated
> *_p.c files or via the type library marshaller. In the *_p.c case, the
> proxies are generated in dlls/rpcrt4/cproxy.c and the stubs in cstub.c
> The last bit of pointer passing was the bit I just removed.

Hm, OK, I took my eye off the ball. I hadn't even seen the cproxy/cstub
files.
> I meant converting as in converting it from using its own named pipes
> (and IRpc* interfaces) to using proper RPC ones. Obviously there would
> need to be extra code to handle this (and also a lot of code removed).

Ah right, I'm with you now.
> > In particular I think our proxy/stub code currently isn't up to doing
> > inter-thread marshalling.
>
> What makes you think that?

Well, last time I tried it, it didn't work :) Since then I've kept half
an eye on wine-patches for RPC/DCOM related patches and have seen a few,
but I must have totally missed some big ones. Either that, or there was
a little one that makes it all go via the pipes/server which I also
missed.
> Yes, you're right. I had initially thought that it was all done in
> RPCRT4, but it looks like it is spread out across both libraries.

There is an article somewhere that explains how it works. Basically the
ITypeInfo (sorry not ITypeLib) objects understand how to convert their
type data into marshal ops/format strings (which term do you prefer?),
which are fed to the RPC runtime which then in turn converts them to
calls to the Ndr APIs. I remember you implementing the last bit (was it
merged?) so for 100% correctness we'd need to rewrite the typelib
marshaller to work the way Windows does.

To be frank I doubt anything depends on that. The custom marshalling it
uses is probably fine for now. One of my goals for CodeWeavers is to
eliminate our dependency on DCOM95.EXE, and obviously that means fixing
whatever bugs block apps.
> I'll submit a patch containing the autogenerated stuff since we need
> that anyway. Like I said, the proxy code isn't 100% correct. It may be
> fine for Crossover or for Wine temporarily however.

Could you elaborate on how it's not correct? If you mean things like the
lack of IRemoteActivation then yeah, I agree, though I think for now
nailing what we have the floor with test cases and getting InstallShield
solid is more important.
> I don't understand a lot of the apartment, OXID and OID code that is in
> the patch so I would have a hard time documenting it.
> I have a fairly large RPC test suite, but I have yet to really delve
> into the DCOM side of things.

Cool, will you be submitting the RPC tests anytime soon?

As for the apartment/OXID/OID stuff, I feel I have a basic understanding
of it. The DCOM RFC makes for informative reading, but it's rather
heavyweight. One of the things I want to do is document this in an
easier to read form in the Wine developers guide, which can also point
to the relevant pieces of code in Wine.

thanks -mike

[Robert Shearman]

On Tue, 2004-06-01 at 19:42, Mike Hearn wrote:
> > Yes, you're right. I had initially thought that it was all done in
> > RPCRT4, but it looks like it is spread out across both libraries.
>
> There is an article somewhere that explains how it works. Basically the
> ITypeInfo (sorry not ITypeLib) objects understand how to convert their
> type data into marshal ops/format strings (which term do you prefer?),
> which are fed to the RPC runtime which then in turn converts them to
> calls to the Ndr APIs. I remember you implementing the last bit (was it
> merged?)

I haven't really implemented the Ndr APIs properly yet, but I have
written an interpreter outside of Wine that can parse every type. It's
just a question of outputting the right information on the wire. The
test framework could be ideal for this type of situation. The big patch
I had was implementing the stubless interpreter (NdrClientCall2).
> so for 100% correctness we'd need to rewrite the typelib
> marshaller to work the way Windows does.
>
> To be frank I doubt anything depends on that. The custom marshalling it
> uses is probably fine for now.

That depends on what your definition of "fine" is. I think I've hit a
bug in it that causes InstallShield to fail. In the long run I think it
would be best to use as much RPC code as possible so that we only have
to fix bugs in one place.
> One of my goals for CodeWeavers is to
> eliminate our dependency on DCOM95.EXE, and obviously that means fixing
> whatever bugs block apps.
> > I'll submit a patch containing the autogenerated stuff since we need
> > that anyway. Like I said, the proxy code isn't 100% correct. It may be
> > fine for Crossover or for Wine temporarily however.
>
> Could you elaborate on how it's not correct? If you mean things like the
> lack of IRemoteActivation then yeah, I agree, though I think for now
> nailing what we have the floor with test cases and getting InstallShield
> solid is more important.

IRemoteActivation is one, but there are too many other hacks. If it
solves a bug that stops InstallShield working, then fine, we should
merge the code.
> > I don't understand a lot of the apartment, OXID and OID code that is in
> > the patch so I would have a hard time documenting it.
> > I have a fairly large RPC test suite, but I have yet to really delve
> > into the DCOM side of things.
>
> Cool, will you be submitting the RPC tests anytime soon?

Unfortunately not. Due to the different calling conventions with
structures, the tests don't work when compiled as ELF.
> As for the apartment/OXID/OID stuff, I feel I have a basic understanding
> of it. The DCOM RFC makes for informative reading, but it's rather
> heavyweight. One of the things I want to do is document this in an
> easier to read form in the Wine developers guide, which can also point
> to the relevant pieces of code in Wine.

Ok, I had forgotten about the DCOM RFC. Maybe reading that will clear
some things up for me.

Rob