Re: [shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.

[Rob Shearman]

2009/1/1 Michael Karcher :
> diff --git a/include/shobjidl.idl b/include/shobjidl.idl
> index a647798..c36f4ce 100644
> --- a/include/shobjidl.idl
> +++ b/include/shobjidl.idl
> @@ -452,6 +452,9 @@ interface IShellView : IOleWindow
>          [in] LPFNSVADDPROPSHEETPAGE pfn,
>          [in] LPARAM lparam);
> - [call_as(AddPropertySheetPages)]
> - HRESULT RemoteAddPropertySheetPages();
> - HRESULT SaveViewState();
>    HRESULT SelectItem(
>        [in] LPCITEMIDLIST pidlItem,
>
> @@ -582,6 +585,9 @@ cpp_quote("#endif")
>          [in] LPARAM lParam,
>          [in] LRESULT *pret);
> - [call_as(SendControlMsg)]
> - HRESULT RemoteSendControlMsg();
> - HRESULT QueryActiveShellView( [out] IShellView **ppshv );
>    HRESULT OnViewWindowActive( [in] IShellView *pshv );
>
> @@ -590,6 +596,8 @@ cpp_quote("#endif")
>          [in] LPTBBUTTONSB lpButtons,
>          [in] UINT nButtons,
>          [in] UINT uFlags);
> - [call_as(SetToolbarItems)]
> - HRESULT RemoteSetToolbarItems();

These shouldn't be necessary. What is it that you're trying to fix?
> }
>
> @@ -1441,9 +1449,12 @@ interface IAutoCompleteDropDown : IUnknown
>  {
>  cpp_quote("#define ACDD_VISIBLE   0x0001")
> - HRESULT GetDropDownStatus(
> - [local] HRESULT GetDropDownStatus(
>     [out] DWORD *pdwFlags,
> -
>    [out, string] LPWSTR *ppwszString);
> -
>    [out] LPWSTR *ppwszString);
> -
> - [call_as(GetDropDownStatus)]
> - HRESULT RemoteGetDropDownStatus();

This isn't correct. IAutoCompleteDropDown::GetDropDownStatus should be
remoted as is.
>  HRESULT ResetEnumerator();
>
> }

[shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.

[Michael Karcher]

Am Donnerstag, den 01.01.2009, 19:09 +0000 schrieb Rob Shearman:
> 2009/1/1 Michael Karcher :
> > - [call_as(AddPropertySheetPages)]
> > - HRESULT RemoteAddPropertySheetPages();

[...]
> > - [call_as(SendControlMsg)]
> > - HRESULT RemoteSendControlMsg();
> > -

[...]
> > - [call_as(SetToolbarItems)]
> > - HRESULT RemoteSetToolbarItems();
>
> These shouldn't be necessary. What is it that you're trying to fix?

Widl outputting vtable slot number mismatch errors when a local function
is present without a remote function being present also. But if I might
take a guess from my last experiences here: This is a widl bug that
should be fixed...
> > - HRESULT GetDropDownStatus(
> > - [local] HRESULT GetDropDownStatus(
> >     [out] DWORD *pdwFlags,
> > -
> >    [out, string] LPWSTR *ppwszString);
> > -
> >    [out] LPWSTR *ppwszString);
> > -
> > - [call_as(GetDropDownStatus)]
> > - HRESULT RemoteGetDropDownStatus();
>
> This isn't correct. IAutoCompleteDropDown::GetDropDownStatus should be
> remoted as is.

How does that work? ppwszString is a NULL-pointer-terminated array of
strings. Does the [string] flag work for pointer arrays too? MSDN
doesn't say so.

Regards,
  Michael Karcher

[shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.

[Rob Shearman]

2009/1/1 Michael Karcher :
> Am Donnerstag, den 01.01.2009, 19:09 +0000 schrieb Rob Shearman:
> > 2009/1/1 Michael Karcher :
> > > - [call_as(AddPropertySheetPages)]
> > > - HRESULT RemoteAddPropertySheetPages();
>
> [...]
> > > - [call_as(SendControlMsg)]
> > > - HRESULT RemoteSendControlMsg();
> > > -
>
> [...]
> > > - [call_as(SetToolbarItems)]
> > > - HRESULT RemoteSetToolbarItems();
> >
> > These shouldn't be necessary. What is it that you're trying to fix?
>
> Widl outputting vtable slot number mismatch errors when a local function
> is present without a remote function being present also. But if I might
> take a guess from my last experiences here: This is a widl bug that
> should be fixed...

I'm afraid so.
> > > - HRESULT GetDropDownStatus(
> > > - [local] HRESULT GetDropDownStatus(
> > >     [out] DWORD *pdwFlags,
> > > -
> > >    [out, string] LPWSTR *ppwszString);
> > > -
> > >    [out] LPWSTR *ppwszString);
> > > -
> > > - [call_as(GetDropDownStatus)]
> > > - HRESULT RemoteGetDropDownStatus();
> >
> > This isn't correct. IAutoCompleteDropDown::GetDropDownStatus should be
> > remoted as is.
>
> How does that work? ppwszString is a NULL-pointer-terminated array of
> strings.

MSDN says that this function actually just returns one string, not a
NULL-pointer-terminated array of strings, so this should just work.
> Does the [string] flag work for pointer arrays too? MSDN
> doesn't say so.

No, the [string] attribute in this case in redundant and it should
apply to the first pointer in the parameter. I'm guessing by the
change that you had to make that widl doesn't handle this too well...

[shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.

[Michael Karcher]

Am Donnerstag, den 01.01.2009, 20:43 +0000 schrieb Rob Shearman:
> > > > - HRESULT GetDropDownStatus(
> > > > - [local] HRESULT GetDropDownStatus(
> > > >     [out] DWORD *pdwFlags,
> > > > -
> > > >    [out, string] LPWSTR *ppwszString);
> > > > -
> > > >    [out] LPWSTR *ppwszString);
> > > > -
> > > > - [call_as(GetDropDownStatus)]
> > > > - HRESULT RemoteGetDropDownStatus();
> > >
> > > This isn't correct. IAutoCompleteDropDown::GetDropDownStatus should be
> > > remoted as is.
> >
> > How does that work? ppwszString is a NULL-pointer-terminated array of
> > strings.
>
> MSDN says that this function actually just returns one string, not a
> NULL-pointer-terminated array of strings, so this should just work.

Oh, thanks for opening my eyes!

And, thanks again for taking your time to review my patches. Your work
is really appreciated!
> No, the [string] attribute in this case in redundant and it should
> apply to the first pointer in the parameter.

Now, that makes sense.
> I'm guessing by the
> change that you had to make that widl doesn't handle this too well...

Seems so. I don't want to hurt anyone, but I somehow get the impression
that the quality of widl is somewhere around works-for-me.

Regards,
  Michael Karcher

[shell32 5/6] include: Add remotable stubs for non-default-marshallable shobjidl calls.

[Rob Shearman]

2009/1/1 Michael Karcher :
> Am Donnerstag, den 01.01.2009, 20:43 +0000 schrieb Rob Shearman:
> > No, the [string] attribute in this case in redundant and it should
> > apply to the first pointer in the parameter.
>
> Now, that makes sense.
> > I'm guessing by the
> > change that you had to make that widl doesn't handle this too well...
>
> Seems so. I don't want to hurt anyone, but I somehow get the impression
> that the quality of widl is somewhere around works-for-me.

The trouble in this case was a corner case that was only tested by
some bad IDL. Corner cases are never easy to test for and the best way
of fixing bugs is to eliminate them as much as possible. To that end I
have some patches queued up that add more of an API to access the
parse tree to try to make the generator code more robust. In
particular, the C generator code could probably do with a function
like type_get_ndr_type that deals with the nuances of detecting
strings, user-types, context handles, interface pointers, etc and that
would likely have fixed several of the issues you reported.

In terms of testing, I want to find and fix these issues before users
(i.e. other developers, like you) find them. To that end, I think a
good test may be to run "widl -p" on as many of the files in include/
as possible and fix/log any issues that come up, and then to compile
each file. There should be no errors in either case, since errors
should be reported up front (i.e. even for just generating a header
file or a syntax check). I also plan to write a fuzzer for widl
someday - if anyone knows of a free fuzzer framework out there
somewhere then that would be useful.