Re: [PATCH] msscript: Initial support for hosting script engines

[Jacek Caban]

Hi Nikolay,

On 14.07.2016 16:35, Nikolay Sivov wrote:
> +static HRESULT init_script_host(const CLSID *clsid, ScriptHost **ret)
> +{
> - IObjectSafety *objsafety;
> - ScriptHost *host;
> - HRESULT hr;
> -
> - *ret = NULL;
> -
> - host = heap_alloc(sizeof(*host));
> - if (!host)
> -
>    return E_OUTOFMEMORY;
> -
> - host->IActiveScriptSite_iface.lpVtbl = &ActiveScriptSiteVtbl;
> - host->ref = 1;
> - host->script = NULL;
> - host->parse = NULL;
> - host->clsid = *clsid;
> -
> - hr = CoCreateInstance(&host->clsid, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
> -
>        &IID_IActiveScript, (void**)&host->script);
> - if (FAILED(hr)) {
> -
>    WARN("Failed to create an instance for %s, %#x\n", debugstr_guid(clsid), hr);
> -
>    goto failed;
> - }
> -
> - hr = IActiveScript_QueryInterface(host->script, &IID_IObjectSafety, (void**)&objsafety);
> - if (FAILED(hr)) {
> -
>    FIXME("Could not get IObjectSafety, %#x\n", hr);
> -
>    goto failed;
> - }
> -
> - hr = IObjectSafety_SetInterfaceSafetyOptions(objsafety, &IID_IActiveScriptParse, INTERFACESAFE_FOR_UNTRUSTED_DATA, 0);
> - if (FAILED(hr))
> -
>    FIXME("SetInterfaceSafetyOptions failed, %#x\n", hr);

This failure is important, please handle it well.
> - IObjectSafety_Release(objsafety);
> -
> - hr = IActiveScript_SetScriptSite(host->script, &host->IActiveScriptSite_iface);
> - if (FAILED(hr)) {
> -
>    WARN("SetScriptSite failed, %#x\n", hr);
> -
>    goto failed;
> - }
> -
> - hr = IActiveScript_QueryInterface(host->script, &IID_IActiveScriptParse, (void**)&host->parse);
> - if (FAILED(hr)) {
> -
>    WARN("Failed to get IActiveScriptParse, %#x\n", hr);
> -
>    goto failed;
> - }

This and...
> - hr = IActiveScriptParse_InitNew(host->parse);
> - if (FAILED(hr)) {
> -
>    WARN("InitNew failed, %#x\n", hr);
> -
>    goto failed;
> - }

...this failure will cause ref leak. You have already set script site,
so engine holds a reference to ScriptHost, but you only release one
reference on failure. Using release_script_engine on failure should fix
that.
> -
> - *ret = host;
> - return S_OK;
> -
>
> +failed:
> - IActiveScriptSite_Release(&host->IActiveScriptSite_iface);
> - return hr;
>
> +}
> -
>
> static HRESULT WINAPI ScriptControl_QueryInterface(IScriptControl *iface, REFIID riid, void **ppv)
>  {
>      ScriptControl *This = impl_from_IScriptControl(iface);
> @@ -257,6 +498,10 @@ static ULONG WINAPI ScriptControl_Release(IScriptControl *iface)
>      if(!ref) {
>          if (This->site)
>              IOleClientSite_Release(This->site);
> -
>    if (This->host) {
> -
>        release_script_engine(This->host);
> -
>        IActiveScriptSite_Release(&This->host->IActiveScriptSite_iface);
> -
>    }
>    heap_free(This);
> }
>
> @@ -321,15 +566,47 @@ static HRESULT WINAPI ScriptControl_Invoke(IScriptControl *iface, DISPID dispIdM
>  static HRESULT WINAPI ScriptControl_get_Language(IScriptControl *iface, BSTR *p)
>  {
>      ScriptControl *This = impl_from_IScriptControl(iface);
> - FIXME("(%p)->(%p)\n", This, p);
> - return E_NOTIMPL;
> - LPOLESTR progidW;
> - HRESULT hr;
> -
> - TRACE("(%p)->(%p)\n", This, p);
> -
> - if (!p)
> -
>    return E_POINTER;
> -
> - *p = NULL;
> -
> - if (!This->host)
> -
>    return S_OK;
> -
> - hr = ProgIDFromCLSID(&This->host->clsid, &progidW);
> - if (FAILED(hr))
> -
>    return hr;
> -
> - *p = SysAllocStringLen(progidW, lstrlenW(progidW));

How about SysAllocString instead?
> - CoTaskMemFree(progidW);
> - return *p ? S_OK : E_OUTOFMEMORY;
>
> }
>
> static HRESULT WINAPI ScriptControl_put_Language(IScriptControl *iface, BSTR language)
>  {
>      ScriptControl *This = impl_from_IScriptControl(iface);
> - FIXME("(%p)->(%s)\n", This, debugstr_w(language));
> - return E_NOTIMPL;
> - CLSID clsid;
> -
> - TRACE("(%p)->(%s)\n", This, debugstr_w(language));
> -
> - if (language && FAILED(CLSIDFromProgID(language, &clsid)))
> -
>    return CTL_E_INVALIDPROPERTYVALUE;
> -
> - if (This->host) {
> -
>    IActiveScriptSite_Release(&This->host->IActiveScriptSite_iface);
> -
>    This->host = NULL;
> - }

This will leave zombie script host and engine. You should probably use
release_script_engine here as well.
> -
> - if (!language)
> -
>    return S_OK;
> -
> - return init_script_host(&clsid, &This->host);
>
> }
>
> static HRESULT WINAPI ScriptControl_get_State(IScriptControl *iface, ScriptControlStates *p)
> @@ -1379,6 +1656,7 @@ static HRESULT WINAPI ScriptControl_CreateInstance(IClassFactory *iface, IUnknow
>      script_control->ref = 1;
>      script_control->site = NULL;
>      script_control->cp_list = NULL;
> -
>
> script_control->host = NULL;
>
> ConnectionPoint_Init(&script_control->cp_scsource, script_control, &DIID_DScriptControlSource);
> ConnectionPoint_Init(&script_control->cp_propnotif, script_control, &IID_IPropertyNotifySink);

Thanks,
Jacek