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